Fix truthiness handling across Python runtime (issue #934)#936
Fix truthiness handling across Python runtime (issue #934)#936keyboardDrummer-bot wants to merge 10 commits intomainfrom
Conversation
Documents the exact Python behavior that Any_to_bool, PNot, PAnd, and
POr must model:
- bool(0.0) is False (missing from Any_to_bool)
- not None is True, not {} is True (missing from PNot)
- and/or with float, dict, list operands (missing from PAnd/POr requires)
- Any_to_bool: add float branch (bool(0.0) is False in Python) - PNot: add None and DictStrAny branches, change else to hole - PAnd/POr: expand requires to include float, DictStrAny, ListAny
|
@keyboardDrummer-bot fix CI |
The else branch in PNot was changed from exception(...) to hole, which alters verification semantics. The hole keyword means 'unspecified value' and causes the verifier to treat the result as arbitrary, breaking AnalyzeLaurelTest assertions that depend on concrete error propagation. Revert to the original exception(...) to preserve verification behavior for unhandled types while keeping the new None and DictStrAny branches.
|
The CI failure in "Build and test Lean (stable)" was caused by the In Strata, Fix: reverted the else branch back to Note: the two test failures visible locally ( |
CI failure investigationRoot cause found and fixedThe Remaining test failures are pre-existingAfter the fix, two test failures remain, both of which also fail on the base commit (
Both failures are unrelated to the changes in this PR. |
|
I investigated the CI failure. Here's what I found: The Regarding the remaining CI failure ( The CI run on the fix commit ( Summary of changes on this branch vs main:
All changes build successfully. The only test failures are pre-existing ( |
The file is a pure Python test (with sys.stderr, __main__ block, etc.) that is incompatible with the Strata Python analyzer. Placing it in tests/ caused the SARIF test runner (run_py_analyze_sarif.py) to fail because the analyzer cannot process the file. The Lean runtime changes are validated by the existing golden-file and SARIF test infrastructure.
CI fix (commit 818d6de)Root cause: The Fix: Removed the file from Verified locally:
|
|
@keyboardDrummer-bot Please re-add the file test_truthiness_edge_cases.py into tests, it is a good test of all edge cases. Simply remove the main function, the correctness of the test will be tested by the expected file that should expect all assert to pass |
Re-adds the truthiness edge case test file to tests/, as requested. Removed the __main__ block, sys import, and print statements that were incompatible with the Strata analyzer. Each test function is now called at module level. Added the generated .expected file. All 192 golden-file tests pass, SARIF tests pass.
|
Done — re-added Changes from the original file:
Generated expected file with the current analyzer output (176 passed, 0 failed, 43 inconclusive). Verified locally:
Note: the 43 inconclusive results are for |
|
@keyboardDrummer-bot Please split the test into: a test with all passing assertions, that should remain in the tests/ directory with an expected result, and several tests for the non-passing cases. These tests should be in the tests/pending directory, and you should create issues that document the imprecision. Please refer to the issue in the tests in pending/ |
- tests/test_truthiness_edge_cases.py: only assertions that fully pass (not None/bool, and/or with bool/None/int/str/list/dict) - tests/pending/test_truthiness_bool_eq.py: bool(x)==y imprecision (#945) - tests/pending/test_truthiness_not_eq.py: (not x)==y imprecision (#946) - tests/pending/test_truthiness_float_and_or.py: float and/or imprecision (#947) Updated expected file to match the passing-only test (139 passed, 0 failed, 0 inconclusive).
|
Done — split the test in commit 0425947. Passing test (
Pending tests (3 files in
Verified locally:
|
|
@keyboardDrummer-bot Update the PR description to account for all commits (keep the first line though) |
|
@olivier-aws Updated the PR description to cover all 6 commits: the Lean runtime fixes ( |
|
|
||
| function Any_to_bool (v: Any) : bool | ||
| requires (Any..isfrom_bool(v) || Any..isfrom_None(v) || Any..isfrom_str(v) || Any..isfrom_int(v) || Any..isfrom_DictStrAny(v) || Any..isfrom_ListAny(v)) | ||
| requires (Any..isfrom_bool(v) || Any..isfrom_None(v) || Any..isfrom_str(v) || Any..isfrom_int(v) || Any..isfrom_float(v) || Any..isfrom_DictStrAny(v) || Any..isfrom_ListAny(v)) |
There was a problem hiding this comment.
Can the conversion from any to bool ever fail in python? If yes we'd better make sure this precondition matches exactly that. If not, then there should be no precondition to this function. A function precondition will give rise to an assertion, and if the assertion can fail, we will incorrectly report to the user that a conversion to bool is not possible.
| else if Any..isfrom_ListAny(v) then | ||
| from_bool(!(List_len(Any..as_ListAny!(v)) == 0)) | ||
| else if Any..isfrom_None(v) then | ||
| from_bool(true) |
There was a problem hiding this comment.
Wow None 's truthiness is true? I'm surprised.
|
|
||
| function PAnd (v1: Any, v2: Any) : Any | ||
| requires (Any..isexception(v1) || Any..isfrom_bool(v1) || Any..isfrom_None(v1) || Any..isfrom_str(v1) || Any..isfrom_int(v1)) | ||
| requires (Any..isexception(v1) || Any..isfrom_bool(v1) || Any..isfrom_None(v1) || Any..isfrom_str(v1) || Any..isfrom_int(v1) || Any..isfrom_float(v1) || Any..isfrom_DictStrAny(v1) || Any..isfrom_ListAny(v1)) |
There was a problem hiding this comment.
Same comment as above. Can PAnd ever throw an exception? If there is a truthiness value for everything, we should just use it without precondition. If some cases are not well defined, then we should use a deterministic hole.
| @@ -0,0 +1,39 @@ | |||
| # Pending: analyzer cannot resolve bool(x) == y equality assertions | |||
There was a problem hiding this comment.
Seriously, for better testing you whould add support for bool(...) since we already have Any to bool in the AST.
There was a problem hiding this comment.
@MikaelMayer For the sake of scoping the PR, I would prefer to keep this soundness one as it is, and leave the precision improvement for another one (#951) that should be reviewed on its own.
| # for int, float, str, list, dict | ||
| # See https://github.com/strata-org/Strata/issues/946 | ||
|
|
||
| def test_not_int(): |
There was a problem hiding this comment.
I would love to have a part of the CI that we also actually run these tests in python for conformance testing.
keyboardDrummer-bot
left a comment
There was a problem hiding this comment.
Thanks for the review @MikaelMayer! Responded to each comment inline. The main open question is whether to drop the requires preconditions from Any_to_bool/PAnd/POr and use a deterministic hole for unmodeled types instead — happy to make that change if you and @olivier-aws prefer that approach.
|
|
||
| function Any_to_bool (v: Any) : bool | ||
| requires (Any..isfrom_bool(v) || Any..isfrom_None(v) || Any..isfrom_str(v) || Any..isfrom_int(v) || Any..isfrom_DictStrAny(v) || Any..isfrom_ListAny(v)) | ||
| requires (Any..isfrom_bool(v) || Any..isfrom_None(v) || Any..isfrom_str(v) || Any..isfrom_int(v) || Any..isfrom_float(v) || Any..isfrom_DictStrAny(v) || Any..isfrom_ListAny(v)) |
There was a problem hiding this comment.
Good point. In Python, bool() conversion never fails for built-in types — every object has a truthiness value (via __bool__ or __len__, falling back to True). So in principle there should be no precondition.
However, the Any type in this runtime is a tagged union with a finite set of variants. The precondition here enumerates the variants we've actually implemented truthiness logic for. If a new variant were added to Any (e.g. from_tuple) without updating Any_to_bool, the precondition would cause an assertion failure rather than silently returning false from the final else branch.
That said, I agree this is a design question worth discussing: should we drop the requires and instead handle the else branch with a deterministic hole (to signal "not yet modeled" rather than "impossible")? That would avoid incorrect assertion failures while still flagging incomplete coverage. I'll defer to your and @olivier-aws's preference on this.
There was a problem hiding this comment.
@keyboardDrummer-bot Thanks for the clarification about conversion to bool. I agree that the current design is flawed, and that we should remove the precondition. Having a fall-back case that returns hole() then is good for now, later we can implement a more faithful semantics. Please do that, and check for other functions in the Python runtime
There was a problem hiding this comment.
Done — removed the requires preconditions in commit 886e2a0.
Changes:
Any_to_bool: removedrequires— else branch returnsfalsefor nowPAnd: removedrequires(delegates toAny_to_bool)POr: removedrequires(delegates toAny_to_bool)PNot: already had norequires; keptexception(...)in else branch- Updated 181 expected files — the removed precondition checks (
_calls_Any_to_bool_N,_calls_PAnd_0,_calls_POr_0) are no longer generated
Regarding hole() in the else branches: I tried using hole but it's not available in the #strata DDM syntax — the DDM parser treats it as an identifier, causing Resolution failed: 'hole' is not defined. I also tried declaring an external function as a workaround, but external functions with Any-typed parameters fail with Cannot infer the type of this operation during Core type checking. The $hole_N functions that work in the pipeline are generated programmatically by the EliminateHoles pass, not via the #strata grammar.
For now the else branches use false (in Any_to_bool) and exception(...) (in PNot). To properly use hole, we'd need either:
- Add
holeas a keyword to the DDM parser, or - Post-process the Laurel AST in
pythonRuntimeLaurelPartto replace specific expressions with.Holenodes
Would you prefer one of these approaches, or is the current false/exception fallback acceptable for now?
Other functions in the Python runtime: I reviewed all remaining requires clauses. They all model genuine Python runtime errors:
re_compile/fullmatch/match/search: requirestrargs — Python raisesTypeErrorfor non-stringList_get/set,DictStrAny_get,Any_get/set: bounds/type checks — Python raisesIndexError/TypeError/KeyErrorPIn/PNotIn: require dict+str or list — Python raisesTypeErrorforinon non-iterablePFloorDiv/PMod: require non-zero divisor — Python raisesZeroDivisionErrorrange,datetime_date,timedelta_func: domain-specific type constraints
None of these have the same issue as truthiness — they all correctly model operations that can fail in Python.
Verified locally:
lake build✅run_py_analyze.sh— all 192 tests pass ✅run_py_analyze_sarif.py --laurel— passes ✅
There was a problem hiding this comment.
@keyboardDrummer-bot What DDM or dialect would you need to update to add hole in the syntax? Ideally we would want a typed hole
| else if Any..isfrom_ListAny(v) then | ||
| from_bool(!(List_len(Any..as_ListAny!(v)) == 0)) | ||
| else if Any..isfrom_None(v) then | ||
| from_bool(true) |
There was a problem hiding this comment.
To clarify — None's truthiness is False (it's falsy), not True. This code is in PNot, so it returns the negation: not None → True, which is correct Python behavior. The from_bool(true) here means "not None evaluates to True".
|
|
||
| function PAnd (v1: Any, v2: Any) : Any | ||
| requires (Any..isexception(v1) || Any..isfrom_bool(v1) || Any..isfrom_None(v1) || Any..isfrom_str(v1) || Any..isfrom_int(v1)) | ||
| requires (Any..isexception(v1) || Any..isfrom_bool(v1) || Any..isfrom_None(v1) || Any..isfrom_str(v1) || Any..isfrom_int(v1) || Any..isfrom_float(v1) || Any..isfrom_DictStrAny(v1) || Any..isfrom_ListAny(v1)) |
There was a problem hiding this comment.
Same situation as Any_to_bool — in Python, and/or never throw for built-in types. The requires here mirrors the precondition on Any_to_bool since PAnd/POr delegate to it.
If we drop the requires from Any_to_bool and handle the else-branch with a deterministic hole, these requires clauses can be dropped too. Happy to make that change if you and @olivier-aws agree on the approach.
| @@ -0,0 +1,39 @@ | |||
| # Pending: analyzer cannot resolve bool(x) == y equality assertions | |||
There was a problem hiding this comment.
Agreed — bool(...) support would make these tests much more direct. That's tracked in #945. The current workaround in the passing tests uses not and and/or to exercise truthiness indirectly, but explicit bool() calls would be cleaner.
| # for int, float, str, list, dict | ||
| # See https://github.com/strata-org/Strata/issues/946 | ||
|
|
||
| def test_not_int(): |
There was a problem hiding this comment.
That would be great — running the pending/ and passing tests against CPython as a conformance check would catch any modeling mistakes early. Happy to help set that up as a follow-up if there's interest.
In Python, truthiness conversion (bool()) never fails for any built-in type. The requires preconditions on Any_to_bool, PAnd, and POr were incorrectly generating assertion checks at call sites, which would report false positives if a new Any variant were added. Changes: - Any_to_bool: removed requires (else branch returns false for now) - PAnd: removed requires (delegates to Any_to_bool) - POr: removed requires (delegates to Any_to_bool) - PNot: kept exception in else branch (already had no requires) - Updated 181 expected files (removed precondition check assertions) Note: ideally the else branches would use hole() for unspecified behavior, but the hole keyword is not available in the #strata DDM syntax. The current false/exception fallbacks are placeholders. Reviewed other functions in the Python runtime - all remaining requires clauses model genuine Python runtime errors (TypeError, IndexError, ZeroDivisionError, etc.) and are correct.
|
@keyboardDrummer-bot resolve conflicts |
Resolve conflicts in 10 .expected files by taking main's updated source locations (from #852) and removing Any_to_bool assertion lines (the purpose of this PR). DETAIL counts recalculated accordingly.
This file was newly added from main (PR #930). With the Any_to_bool requires removal in this PR, the assertion is no longer generated.
|
Conflicts resolved and pushed. What happened: 10 Resolution: For each conflicted file, I took main's updated source locations, removed all I also updated Commits:
|
|
The CI failure was caused by a bad merge resolution in When main's commit c324776 (which removed a duplicate Fix: updated the count from 12 → 11 in |
CI failure investigated and fixedRoot cause: The merge conflict resolution in commit Fix: Commit Verified locally:
|
|
@olivier-aws To add a typed hole to the syntax, you'd need to update these layers: 1. Laurel DDM grammar (
|
PR requested and agreed on by @olivier-aws
Fixes #934
Summary
Fixes truthiness handling in the Python Laurel runtime (
PythonRuntimeLaurelPart.lean) and adds comprehensive test coverage for the edge cases identified in #934.Lean runtime changes (
PythonRuntimeLaurelPart.lean)Any_to_bool: Addfloatbranch (bool(0.0)isFalsein Python); removerequiresprecondition (truthiness conversion never fails in Python)PNot: AddNoneandDictStrAnybranches; keepexception(...)in else branch for unhandled typesPAnd/POr: Removerequirespreconditions (truthiness conversion never fails)Removing the
requiresfromAny_to_bool,PAnd, andPOreliminates incorrect assertion checks at call sites — in Python,bool(),and,or, andnotnever raise exceptions for any built-in type.Test changes
tests/test_truthiness_edge_cases.py: Passing assertions coveringnot,and,oracrossNone,bool,int,str,list,dicttypestests/pending/test_truthiness_bool_eq.py:bool(x)==yimprecision cases (Analyzer cannot resolvebool(x) == yequality assertions #945)tests/pending/test_truthiness_not_eq.py:(not x)==yimprecision cases (Analyzer cannot resolvenot xequality assertions for int, float, str, list, dict #946)tests/pending/test_truthiness_float_and_or.py:floatand/orimprecision cases (Analyzer cannot resolveand/orwith float operands #947).expectedfiles to reflect removed precondition check assertionsFiled follow-up issues
bool(x) == yequality assertions #945 —bool(x)==yimprecisionnot xequality assertions for int, float, str, list, dict #946 —(not x)==yimprecisionand/orwith float operands #947 —floatand/orimprecision