Skip to content

[WIP] Fix VCG path multiplication with goto in if-then-else#519

Closed
olivier-aws wants to merge 7 commits intostrata-org:mainfrom
olivier-aws:fix/vcg-path-multiplication-goto-ite
Closed

[WIP] Fix VCG path multiplication with goto in if-then-else#519
olivier-aws wants to merge 7 commits intostrata-org:mainfrom
olivier-aws:fix/vcg-path-multiplication-goto-ite

Conversation

@olivier-aws
Copy link
Contributor

Issue #419

🤖 Note: This PR was created with assistance from an AI agent.

Summary

This PR fixes a critical bug where procedures containing goto (exit) statements inside if-then-else branches caused verification condition (VC) path multiplication. When multiple paths converged to the same exit label, each path was treated independently through ProgramEval, duplicating proof obligations for subsequent procedures.

Changes

1. Block-level path merging (fix(StatementEval): Merge block handler results)

  • Added mergeByExitLabel in the block handler to merge environments that converge to the same exit label
  • Prevents path multiplication by ensuring at most one result per distinct exit label

2. Fixed path merging logic (fix(StatementEval): Address review feedback)

  • Fixed Env.merge argument order in mergeByExitLabel (true-branch = E1, false-branch = E2)
  • Fixed condition extraction to inspect path condition structure instead of blindly using newest entry
  • Fixed repeated merge corruption by replacing foldl-based merge with exact 2-path matching
  • Moved merge logic into processIteBranches for same-label exits

3. Test coverage

  • Added ExitItePathMerge.lean with tests for:
    • Merged variable value correctness
    • Three paths converging to same exit label via nested ite

Files Changed

  • Strata/Languages/Core/StatementEval.lean (+59/-12)
  • StrataTest/Languages/Core/Examples/ExitItePathMerge.lean (+210 new)

Status

🚧 Work in Progress - Testing and review in progress
Description of changes:

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

…ultiplication

When a procedure contains an exit (goto) inside an if-then-else branch,
processIteBranches produces multiple EnvWithNext results (one per branch
path). These propagate through evalAuxGo and the block handler, which
consumes matching exits but does not merge the resulting environments.
This causes subsequent procedures in ProgramEval to be verified once per
path from the previous procedure, duplicating proof obligations.

The fix adds mergeByExitLabel in the block handler: after consuming exits,
results that converge to the same exit label are merged using Env.merge.
This ensures at most one result per distinct exit label, preventing path
multiplication through ProgramEval.

Fix VCG path multiplication with goto in if-then-else
Ref: strata-org#419
…cation with goto in if-then-else

Fix three bugs in the path merging logic:

1. Fix Env.merge argument order in mergeByExitLabel: the true-branch
   environment must be E1 and false-branch E2. The condition is now
   extracted and used to identify which environment is which, by
   detecting the negation pattern (ite cond false true).

2. Fix condition extraction: determine the ite condition by inspecting
   the path condition structure rather than blindly using the newest
   entry. The negated form identifies the false branch.

3. Fix repeated merge corruption: replace the foldl-based merge (which
   corrupted path-condition scopes on 3+ paths) with a match that only
   merges exactly 2-path groups and leaves larger groups unmerged.

Additionally, move merge logic into processIteBranches for same-label
exits where cond' is directly available, mirroring the existing
single-path merge. Unmatched paths (different exit labels) fall through
to the block-level mergeByExitLabel.

Add tests for:
- Merged variable value correctness (assert r == 1 || r == 0)
- Three paths converging to the same exit label via nested ite
@shigoel
Copy link
Contributor

shigoel commented Mar 5, 2026

Thanks, @olivier-aws! I know this is WIP, but I wonder whether merging by labels is the right thing to do especially since we don't (yet) enforce the uniqueness of labels.

@olivier-aws
Copy link
Contributor Author

Thanks, @olivier-aws! I know this is WIP, but I wonder whether merging by labels is the right thing to do especially since we don't (yet) enforce the uniqueness of labels.

We should enforce uniqueness of labels within a block at least. In that case, I believe that the proposed solution is safe. Do you think we should add a check about uniqueness of labels with a block and reject Core programs that fail this check?

Answer from kiro-cli:

The key insight is that the label-based merging is safe even without enforced label uniqueness because:

Why It's Safe

  1. Sibling paths only: mergeByExitLabel only merges paths that result from evaluating the same block's statements. All paths in the input are siblings (started from the same entry point).

  2. No cross-block merging: The function never merges paths from different blocks. Each block evaluation is independent.

  3. Same-label convergence is meaningful: If two sibling paths converge to the same exit label, they represent different execution paths through the same code that should be merged - regardless of whether other
    blocks elsewhere use the same label name.

Example

Even with duplicate labels across different blocks, the merging is safe:

lean
procedure foo() {
outer: { if (x > 0) { exit outer; } exit outer; }
// Paths merged here (both from foo's outer block)
}

procedure bar() {
outer: { if (y > 0) { exit outer; } exit outer; }
// Paths merged here (both from bar's outer block)
// Never merged with foo's paths!
}

The concern about label uniqueness would only apply if we were merging paths from different blocks, which we never do.

…ultiplication

When a procedure contains an exit (goto) inside an if-then-else branch,
processIteBranches produces multiple EnvWithNext results (one per branch
path). These propagate through evalAuxGo and the block handler, which
consumes matching exits but does not merge the resulting environments.
This causes subsequent procedures in ProgramEval to be verified once per
path from the previous procedure, duplicating proof obligations.

The fix adds mergeByExitLabel in the block handler: after consuming exits,
results that converge to the same exit label are merged using Env.merge.
This ensures at most one result per distinct exit label, preventing path
multiplication through ProgramEval.

Fix VCG path multiplication with goto in if-then-else
Ref: strata-org#419
…cation with goto in if-then-else

Fix three bugs in the path merging logic:

1. Fix Env.merge argument order in mergeByExitLabel: the true-branch
   environment must be E1 and false-branch E2. The condition is now
   extracted and used to identify which environment is which, by
   detecting the negation pattern (ite cond false true).

2. Fix condition extraction: determine the ite condition by inspecting
   the path condition structure rather than blindly using the newest
   entry. The negated form identifies the false branch.

3. Fix repeated merge corruption: replace the foldl-based merge (which
   corrupted path-condition scopes on 3+ paths) with a match that only
   merges exactly 2-path groups and leaves larger groups unmerged.

Additionally, move merge logic into processIteBranches for same-label
exits where cond' is directly available, mirroring the existing
single-path merge. Unmatched paths (different exit labels) fall through
to the block-level mergeByExitLabel.

Add tests for:
- Merged variable value correctness (assert r == 1 || r == 0)
- Three paths converging to the same exit label via nested ite
Address review feedback on PR strata-org#519 regarding label uniqueness concerns.

The mergeByExitLabel function is safe even without enforced label
uniqueness because it only merges sibling paths (paths that result
from evaluating the same block's statements). Label uniqueness across
different blocks is irrelevant since we never merge paths from
different block evaluations.

Added comprehensive documentation explaining:
- Why block-level merging is safe
- How the two-level merging strategy works
- Why this merging is essential to prevent path multiplication

Changes:
- Enhanced mergeByExitLabel documentation with safety rationale
- Added clarifying comment in block handler
- Created LABEL_MERGING_SAFETY.md with detailed explanation

Ref: strata-org#519 (comment)
The VCG path multiplication fix in 37bcd26 correctly prevents duplicate
verification conditions. Update test expectations to reflect that only one
error is now produced instead of the previous duplicate.

Fixes test failure in CI for issue strata-org#419
…vier-aws/Strata into fix/vcg-path-multiplication-goto-ite
@olivier-aws olivier-aws closed this Mar 5, 2026
@olivier-aws
Copy link
Contributor Author

Closing due to git merge issues. Will open a new one

@olivier-aws olivier-aws reopened this Mar 5, 2026
@olivier-aws olivier-aws closed this Mar 5, 2026
@shigoel
Copy link
Contributor

shigoel commented Mar 5, 2026

We should enforce uniqueness of labels within a block at least. In that case, I believe that the proposed solution is safe. Do you think we should add a check about uniqueness of labels with a block and reject Core programs that fail this check?

I think we should, yes -- that's entirely reasonable to me.

olivier-aws added a commit that referenced this pull request Mar 5, 2026
Address review feedback on PR #519 regarding label uniqueness concerns.

The mergeByExitLabel function is safe even without enforced label
uniqueness because it only merges sibling paths (paths that result
from evaluating the same block's statements). Label uniqueness across
different blocks is irrelevant since we never merge paths from
different block evaluations.

Added comprehensive documentation explaining:
- Why block-level merging is safe
- How the two-level merging strategy works
- Why this merging is essential to prevent path multiplication

Changes:
- Enhanced mergeByExitLabel documentation with safety rationale
- Added clarifying comment in block handler
- Created LABEL_MERGING_SAFETY.md with detailed explanation

Ref: #519 (comment)
olivier-aws added a commit to olivier-aws/Strata that referenced this pull request Mar 10, 2026
Address review feedback on PR strata-org#519 regarding label uniqueness concerns.

The mergeByExitLabel function is safe even without enforced label
uniqueness because it only merges sibling paths (paths that result
from evaluating the same block's statements). Label uniqueness across
different blocks is irrelevant since we never merge paths from
different block evaluations.

Added comprehensive documentation explaining:
- Why block-level merging is safe
- How the two-level merging strategy works
- Why this merging is essential to prevent path multiplication

Changes:
- Enhanced mergeByExitLabel documentation with safety rationale
- Added clarifying comment in block handler
- Created LABEL_MERGING_SAFETY.md with detailed explanation

Ref: strata-org#519 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants