Add "Heap reference monotone" post condition for procedures that modify the Heap#1099
Add "Heap reference monotone" post condition for procedures that modify the Heap#1099thanhnguyen-aws wants to merge 11 commits into
Conversation
|
@keyboardDrummer-bot Please rewrite and add more details to the PR description. |
|
I've rewritten the PR description. Key improvements:
|
|
|
||
| procedure modifyContainerWithoutPermission1(c: Container, d: Container) | ||
| // ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ error: modifies clause does not hold | ||
| // ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ error: modifies clause could not be proved |
There was a problem hiding this comment.
This error message changed from "modifies clause does not hold" to "modifies clause could not be proved". The former means the SMT solver found a counterexample; the latter means it timed out or returned unknown.
This is a diagnostic quality regression caused by the additional monotonicity postcondition making the VC harder for the solver. The same regression occurs at lines 130 and 155.
Please investigate whether the solver timeout can be avoided — e.g., by increasing the timeout for these VCs, or by structuring the postcondition differently so it doesn't interfere with the solver's ability to find counterexamples for unrelated assertions.
There was a problem hiding this comment.
I acknowledge this one and have been woking with ATS to find a solution for it. It is more complicated than I think.
| @@ -0,0 +1,7 @@ | |||
| unknown location: ✅ pass - Heap reference counter monotone | |||
There was a problem hiding this comment.
The unknown location prefix confirms that the synthesized postcondition has no source location. This will be confusing for users when the monotonicity check fails — they won't know which procedure it relates to.
Also: this test has two Heap reference counter monotone results (one for ClassA@__init__ and one for __main__). In a program with many classes, this could produce a lot of noise. Consider whether the monotonicity postcondition should be suppressed from user-facing output (e.g., by marking it as an internal/infrastructure check).
Co-authored-by: Michael Tautschnig <mt@debian.org>
tautschnig
left a comment
There was a problem hiding this comment.
Earlier feedback — status:
-
⚠️ Thread #1 (diagnostic regression"does not hold" → "could not be proved"inT2_ModifiesClauses.lean:71/130/155) — still open, no response from the author. This is a real SMT solver observability regression: the monotonicity postcondition is making the solver slower on previously-fast counterexample-finding VCs, and the tests lost a tighter error-message assertion. I'd like the author to acknowledge it even if the fix is deferred; the new test expectations entrench the weaker diagnostic. -
⚠️ Thread #2 (source location + proof opportunity) — marked resolved. The procedure-name half was addressed (summary now readsHeap reference counter monotone ({proc.name.text})ind23b9b7c6), but the proof opportunity part was not. My earlier review proposed theorems forupdateField(preservesnextReference) andincrement(increases it by 1); the PR adds an axiomatic postcondition onOpaque (impl = none)andAbstractbodies that is only sound because of these unit lemmas. Since thread was resolved without them, flagging here. Proof skeleton suggestions below under "Proof coverage". -
⚠️ Thread #5 (unknown location + noise) — still open. The procedure-name suffix change ind23b9b7c6will help when it's visible, but:- Every
expected_laurel/*.expectedfile currently showsunknown location: ✅ pass - Heap reference counter monotone(no proc-name suffix). These are probably stale and need regeneration once CI is green — worth confirming whether the summary change ind23b9b7c6actually flows through to the output format before declaring the noise concern addressed. - The underlying
unknown locationis still there because the synthesizedmonoCondhassource := none. Cheap fix: attach the procedure's source location (if available) so the synthetic postcondition traces back to the procedure declaration.proc.source(if that's the accessor) orproc.name.source. - The per-procedure one-line-per-test noise concern is unaddressed. In a 20-class Python program there will be 40+ monotonicity lines. Consider a
category := .Internal/summary.mkInternalmarker that is filtered from the default display.
- Every
Proof coverage — suggested theorems for a HeapParameterizationCorrect.lean follow-up. The monotonicity axiom is only sound because of two facts about heapTransformExpr:
-- Informal: updateField preserves the reference counter.
theorem updateField_preserves_nextReference
(h : Heap) (ref : Reference) (field : String) (v : Value) :
Heap.nextReference (updateField h ref field v) = Heap.nextReference h := by
sorry -- unfold updateField; by cases on h; rfl or by definition
-- Informal: increment increases the reference counter by 1.
theorem increment_succ_nextReference (h : Heap) :
Heap.nextReference (increment h) = Heap.nextReference h + 1 := by
sorry -- by definition of increment
-- Informal: heapTransformExpr only produces heap states obtained from
-- the input heap by a finite sequence of updateField/increment calls.
-- Hence nextReference is monotone across any heapTransformExpr step.
theorem heapTransformExpr_nextReference_monotone
(ctx heapIn model expr heapOut) :
heapTransformExpr heapIn model expr = .ok (expr', heapOut) →
Heap.nextReference heapOut ≥ Heap.nextReference heapIn := by
sorry -- induction on expr; cases on StmtExpr constructors;
-- each case either preserves (updateField) or increases (increment)With those three lemmas the axiomatic postcondition on Opaque / Abstract becomes a derived fact, not a trust assumption. At minimum the first two should be straightforward rfl-ish proofs.
Additional test coverage I'd like to see before merge (independent of CI fix):
-
Negative test for the "does-not-hold" → "could-not-be-proved" regression. If solver timeout is the root cause, add a directive
-- KnownIssue: modifies-clause-unknown-resultor a@[timeout ...]attribute on the affected procs that pins the regression. Otherwise this silently becomes the expected behaviour. -
A test where monotonicity is violated by a fabricated
Opaquebody — i.e., an impl that decreasesnextReference. This should fail verification with the new postcondition. Validates that the postcondition is actually being enforced onOpaquebodies with animpl, not just axiomatically assumed. -
An
Abstract-body test. The T1 test exercisesOpaque; there's no test pinning that the postcondition is injected onAbstractbodies too (and it has to be, per thematchbranch inheapTransformProcedure). -
Regression test for the original issue #1098. The Python-level
test_heap_reference_monotone.pycovers the user-visible bug, but a minimal Laurel-level test (a := new C; b := new C; assert a != bwithout any heap-writing procedure in between, purely inside the caller) would isolate the monotonicity postcondition from any incidental logic.
Refactoring suggestions:
-
The
monoCondconstruction duplicates the heap-name-identifier idiom already used at line 273 in the same file (mkMd (.Var (.Local heapVar))). Consider a tiny helperheapRef (name : Identifier) : AstNode StmtExpr := mkMd (.Var (.Local name))local to this file, used at all the call sites. Would also have made the.Identifier→.Var (.Local)migration a one-place change. -
The axiomatic-safety comment pattern duplicated across
.Opaqueand.Abstractarms is a sign that both arms are applying the same transformation. Consider extractingprependMonoCond : List (AstNode Condition) → List (AstNode Condition)(or the metadata-preserving equivalent) and calling it in both branches. One source of truth for the monotonicity injection.
Co-authored-by: Michael Tautschnig <mt@debian.org>
Co-authored-by: Michael Tautschnig <mt@debian.org>
Issue #, if available: #1098
Problem
When two distinct class instances are allocated on the heap, the analyzer cannot prove that their heap references are different. For example:
Root cause
During the heap-parameterization pass (
HeapParameterization.lean), every procedure that writes to the heap is rewritten to accept an input heap ($heap_in) and produce an output heap ($heap). However, there was no postcondition constraining the relationship between the two heaps' internal reference counters. Without such a constraint, the verifier cannot determine that references returned by separate allocations are distinct —a1anda2could be assigned the same heap reference, makinga1.val != a2.valunprovable.Solution
This PR injects a "heap reference counter monotone" postcondition into every heap-writing procedure during the
heapTransformProcedurepass inHeapParameterization.lean.How it works
Three new expressions are constructed before the body match:
inHeapRefHeap..nextReference!($heap_in)outHeapRefHeap..nextReference!($heap)monoCondoutHeapRef >= inHeapRefConditionasserting the counter never decreases, with summary"Heap reference counter monotone"This
monoCondis prepended to the postcondition list of bothOpaqueandAbstractbody variants. The other body kinds are unaffected:Transparent— exposes its full implementation to the verifier, which can derive the monotonicity property directly from the body.External— has no heap parameters, so the postcondition does not apply.The postcondition tells the verifier that each heap-writing call advances (or at least preserves) the heap's reference counter. Since each allocation increments this counter, references obtained from different calls are guaranteed to be distinct.
Changes
Strata/Languages/Laurel/HeapParameterization.leanmonoCondand prepend it toOpaqueandAbstractpostcondition listsStrataTest/Languages/Python/tests/test_heap_reference_monotone.pyClassAinstances with different values, assertinga1.val != a2.valStrataTest/Languages/Python/run_py_analyze_sarif.pyStrataTest/Languages/Python/expected_laurel/test_heap_reference_monotone.expectedStrataTest/Languages/Python/expected_interpret/test_heap_reference_monotone.expectedexpected_laurel/*.expectedfilesTesting
test_heap_reference_monotone.py): The motivating example from Python Class objects does not have unique Heap reference #1098. TwoClassAinstances are allocated with different values, andassert a1.val != a2.valis verified successfully — the assertion now passes thanks to the monotonicity postcondition establishing thata1anda2have distinct heap references.By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.