Fix Python->Laurel: Havoc callee or Heap after unmodeled InstanceCall#978
Conversation
|
@thanhnguyen-aws Can you please add some tests here? |
I added an expected test. |
shigoel
left a comment
There was a problem hiding this comment.
Review Summary
The fix is logically sound — when an unmodeled instance method call (e.g., xs.some_unmodeled_call(3)) is a black box, the receiver should be havocked since the method could mutate it. The implementation is correct for the standalone expression case.
Soundness gap in translateAssign
The PR only adds callee havocking in the .Expr _ value case (standalone expression statements). But y = xs.some_unmodeled_call(3) goes through translateAssign where the RHS is an unmodeled Call → Hole. The Hole case (lines 1132–1160) havocs maybe_except but does not havoc the callee. This is the same soundness gap the PR intends to fix — it's just not covered in the assignment case.
Test coverage
Only one test for the happy path. Consider adding:
y = xs.method()(assignment form — exercises thetranslateAssignpath)a.b.method()(chained attribute — verifies correct base extraction)- Non-variable base like
get_list().method()(should NOT havoc)
No performance concerns — the membership check pattern matches the rest of the file.
MikaelMayer
left a comment
There was a problem hiding this comment.
I haven't read the PR but it seems that if a method name is not recognized, we should just havoc the heap and that's it. The receiver is just a special case.
shigoel
left a comment
There was a problem hiding this comment.
@thanhnguyen-aws Could you please address @MikaelMayer's comment re. the design here?
I added Heap havocking for cases when inputs of the unmodeled function call contains Class type. Note that |
MikaelMayer
left a comment
There was a problem hiding this comment.
Super nice tests. You might consider updating the PR description and title.
shigoel
left a comment
There was a problem hiding this comment.
Review from Claude Code — 1 soundness concern, 1 invariant suggestion, 1 minor style nit.
shigoel
left a comment
There was a problem hiding this comment.
LGTM — soundness gap for composite receivers is fixed, invariant is documented, tests cover the key cases. Two minor style nits (missing space in let inputExprs:=, Prod.fst vs destructuring) but nothing blocking.
|
@thanhnguyen-aws Please update the PR description. |
…anceCall (strata-org#978)" This reverts commit 71f840a.
- Resolve two merge conflict blocks in PythonToLaurel.lean left by the revert of PR strata-org#978: use the Variable-based API (StmtExpr.Var/.Local, stmtExprToVar) with the pre-strata-org#978 havoc logic (exceptHavoc/holeExceptHavoc) - Wrap maybeExceptVar with stmtExprToVar since Assign targets now take AstNode Variable instead of StmtExprMd - Mark test_multi_service.py and test_required_with_optional.py as .success in AnalyzeLaurelTest since the revert of strata-org#978 fixed the $heap resolution errors
|
I think the right design here needs a Laurel change. When calling an unmodeled thing, we should call a Laurel procedure like this: The |
|
Asked a bot to create a separate PR for adding |
Revert PR 978: strata-org#978 By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.
Revert PR 978: strata-org#978 By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.
Fixes: #977
Problem
When the Python→Laurel translator encounters an unmodeled instance call (e.g.,
xs.some_method()), it previously only havockedmaybe_except. It did not account for the fact that the unmodeled call could mutate the receiver or heap-allocated fields, leading to unsound verification results where the verifier assumed variables retained their pre-call values.For example, after
xs.some_unmodeled_call(), the verifier would still provexs == [1, 2]— incorrectly, since the call could have mutatedxs.Fix
Introduces
mkHavocStmtsForUnmodeledCall, a centralized helper that computes the correct set of havoc statements for any unmodeled call. The helper is used in bothtranslateAssign(fory = x.method()) andtranslateStmt(for barex.method()calls), replacing the previous inlineholeExceptHavoclogic.For an unmodeled call, the helper emits:
maybe_except(the call could throw)$heap(the method could mutate heap-allocated fields)By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.