feat: conservatively abstract unsupported Python constructs#566
feat: conservatively abstract unsupported Python constructs#566MikaelMayer wants to merge 8 commits intomainfrom
Conversation
3b6064f to
6c73095
Compare
360dfb9 to
a376cca
Compare
olivier-aws
left a comment
There was a problem hiding this comment.
As a general comment, I'm not a big fan of hiding away what we don't support. There are many risks of unsoundness with this. At the very least, we need a message in the log that the translation encountered an unsupported expression/statement and over-approximated it.
| -- Compound LHS (e.g. a[i] = v, (a, b) = v) — havoc all Name targets | ||
| -- This is a sound over-approximation: we lose the assignment but keep | ||
| -- the program structure. |
There was a problem hiding this comment.
I'm not sure this is sound in Python, due to aliasing. This Python script prints 999:
a = [1, 2, 3]
b = a
b[0] = 999
print(a[0])Here you would only havoc b, not a
There was a problem hiding this comment.
Indeed good catch. The answer there is that we should havoc not just all name targets but all names in scope.
I hadn't reviewed that part yet but I'm happy you did!
There was a problem hiding this comment.
On a second thought, if we stored dictionaries via an indirection to the heap, then no target would be havoc-ed, we would here only havoc the heap to indicate something changed, so we would still be able to prove a[0] == b[0]
What is the status of the heap in Laurel? Is it fully supported?
There was a problem hiding this comment.
AFAIK Laurel does allow arbitrary StmtExprs to occur on the LHS of an assignment, it's just that Laurel->Core only supports assignmets to identifiers https://github.com/strata-org/Strata/blob/main/Strata/Languages/Laurel/LaurelToCoreTranslator.lean#L362 (that is, Laurel supports heap-parameterized procedures + the creation of objects of composite type on the heap, but not assigning to their fields after the fact)
There was a problem hiding this comment.
(that is, Laurel supports heap-parameterized procedures + the creation of objects of composite type on the heap, but not assigning to their fields after the fact)
What? You can assign to fields whenever you like: https://github.com/strata-org/Strata/blob/main/StrataTest/Languages/Laurel/Examples/Objects/T1_MutableFields.lean#L40
I would prefer to change the field selector syntax from # to . but I ran into some technical issues. Will resolve them at some point.
There was a problem hiding this comment.
Agreed. The unsupported statement catch-all now records the abstraction in ctx.abstractedStatements and emits a warning via pyAnalyzeLaurel. Unsupported expressions also record warnings via the ExprM monad.
StrataTest/Languages/Python/expected_laurel/test_augmented_assign.expected
Outdated
Show resolved
Hide resolved
2a4ed7d to
8074972
Compare
677106f to
5da2e01
Compare
| | some _ => throw (.internalError s!"Keyword arg should be a Dict") | ||
| | none => | ||
| let expr ← translateExpr ctx expr | ||
| let (expr, _) ← (translateExpr ctx expr).run' |
There was a problem hiding this comment.
Is this dropping "warnings"?
There was a problem hiding this comment.
Yes and it should not. Thanks for spotting.
There was a problem hiding this comment.
Fixed: translateDictKWords, translateVarKwargs, translateKwargs, and translateCall now use ExprM so warnings from nested expression translations are propagated rather than dropped. translateStmt also uses translateExprW for condition/value expressions to merge warnings into ctx.abstractedStatements.
- abstractedStatements field in TranslationContext - ExprM monad for expression translation with warning accumulation - havocAllVars helper (reusable across conservative abstraction) - Unsupported binary operators → Hole + warning - Unsupported expressions → Hole + warning - Unsupported statements → havocAllVars + assert * + record abstraction - Emit abstraction warnings from pyAnalyzeLaurel pipeline
5da2e01 to
4e4238b
Compare
- translateDictKWords, translateVarKwargs, translateKwargs, translateCall now use ExprM so warnings from nested expressions are not dropped - translateStmt uses translateExprW for condition/value expressions to merge warnings into ctx.abstractedStatements - With statement expression warnings propagated via currentCtx mutation
MikaelMayer
left a comment
There was a problem hiding this comment.
The core idea of abstracting unsupported constructs to sound over-approximations instead of failing is good. The ExprM monad for accumulating warnings and the havoc-all-vars + assert-star pattern for unsupported statements are well-designed.
However, there is a bug that makes the warning mechanism non-functional for the main body: at line 1758 of PythonToLaurel.lean, pythonToLaurel' returns ctx instead of finalCtx, so all abstractedStatements accumulated during translateStmtList are lost. The pyAnalyzeLaurel caller will never see any warnings. Fix: return (program, finalCtx). I verified this compiles cleanly.
Additionally, translateAssign uses translateExprVal which silently drops expression warnings, creating an inconsistency with how translateStmt handles them.
The PR description mentions compound LHS assignment and tuple unpacking in for as abstracted constructs, but neither is implemented — both still throw unsupportedConstruct. The catch-all at the end of translateStmt only catches unmatched statement types, not sub-cases within already-matched statements like For or Assign. The description also claims test_with_statement.py was added, but it's not in the diff (it was added in PR #580).
I'd like the bug fixed and the PR description corrected before approving.
- translateCall: use translateExpr directly (already in ExprM context) - translateAssign: use translateExprW to propagate RHS/target warnings - ExprM.warn: prepend instead of append (O(1)), reverse at run' boundary - Statement catch-all: pattern-match constructors instead of repr
# Conflicts: # Strata/Languages/Python/PythonToLaurel.lean
# Conflicts: # Strata/Languages/Python/PythonToLaurel.lean
What
Instead of failing on unsupported Python constructs, translate them to sound over-approximations so the verifier can still run.
How
Hole(fresh uninterpreted value)Holea[i]=v,(a,b)=v)for(for a,b in ...)withstatementas-targets declared asHoleassert *Unsupported statements and compound LHS assignments are recorded in
TranslationContext.abstractedStatementsand emitted as warnings bypyAnalyzeLaurel.Tests
Added
test_with_statement.py.