Diagnose old()/new() applied to an expression (#1173) - #1180
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe change adds dedicated ChangesContract-state diagnostics
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Source
participant Parser
participant Checker
participant DiagnosticSystem
Source->>Parser: parse old() or new() usage
Parser->>DiagnosticSystem: report E030 or E031 for expression arguments
Parser->>Checker: pass valid contract syntax
Checker->>DiagnosticSystem: validate ensures-only placement
DiagnosticSystem-->>Checker: report E174 or E175
Possibly related issues
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 7 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (7 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1180 +/- ##
==========================================
- Coverage 93.78% 93.78% -0.01%
==========================================
Files 99 99
Lines 33600 33637 +37
Branches 458 458
==========================================
+ Hits 31512 31546 +34
- Misses 2075 2078 +3
Partials 13 13
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Adversarial review — verified against the diff and probed liveVerdict: correct as built. The reframing (argument shape, not clause placement) matches the grammar's reality, and the scan's conservatism points the right way at every edge I attacked:
Checked and agreed with both brief deviations: §7.9.2 is the right spec_ref ( |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@TESTING.md`:
- Around line 9-11: Reconcile the testing metrics in the TESTING.md overview by
recomputing the canonical total, passed, stress, skipped, and per-file counts so
all reported figures agree. Update every affected numeric occurrence in the
testing summary, including the total test count and 131-file aggregate, while
preserving the coverage and conformance metrics unless they are also
inconsistent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: d21694a2-33a0-4cac-8850-06eb479cef91
⛔ Files ignored due to path filters (6)
docs/SKILL.mdis excluded by!docs/**docs/index.htmlis excluded by!docs/**docs/index.mdis excluded by!docs/**docs/llms-full.txtis excluded by!docs/**docs/llms.txtis excluded by!docs/**tests/conformance/ch07_old_outside_ensures_rejected.verais excluded by!**/*.vera
📒 Files selected for processing (17)
AGENTS.mdCHANGELOG.mdCLAUDE.mdFAQ.mdREADME.mdROADMAP.mdSKILL.mdTESTING.mdspec/06-contracts.mdspec/07-effects.mdtests/conformance/manifest.jsontests/test_checker_functions.pytests/test_parser.pyvera/README.mdvera/_since.pyvera/checker/expressions.pyvera/errors.py
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
aallan/vera-bench(manual)
b579887 to
855a514
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@AGENTS.md`:
- Line 11: Correct the companion-module references in the conformance-suite
description in AGENTS.md: identify ch07_cross_module_contracts.vera with
ch07_cross_module_contracts_lib.vera, and ch08_cross_module_generic.vera with
ch08_cross_module_generic_lib.vera. Keep the surrounding suite and verification
guidance unchanged.
In `@FAQ.md`:
- Line 252: Update the test count in the FAQ entry from 8,558 to 8,635,
preserving the existing “including a 170-program conformance suite” wording.
In `@TESTING.md`:
- Around line 217-221: The documentation counts are stale across both referenced
files. In TESTING.md, add the skip-table rows for
ch07_old_outside_ensures_rejected and update the aggregate skip count to 63; in
CLAUDE.md, update the corpus count from 217 to 218. Ensure both values match the
current codebase.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 60a08468-fa16-4895-91ce-5d30f5dc3ce0
⛔ Files ignored due to path filters (6)
docs/SKILL.mdis excluded by!docs/**docs/index.htmlis excluded by!docs/**docs/index.mdis excluded by!docs/**docs/llms-full.txtis excluded by!docs/**docs/llms.txtis excluded by!docs/**tests/conformance/ch07_old_outside_ensures_rejected.verais excluded by!**/*.vera
📒 Files selected for processing (17)
AGENTS.mdCHANGELOG.mdCLAUDE.mdFAQ.mdREADME.mdROADMAP.mdSKILL.mdTESTING.mdspec/06-contracts.mdspec/07-effects.mdtests/conformance/manifest.jsontests/test_checker_functions.pytests/test_parser.pyvera/README.mdvera/_since.pyvera/checker/expressions.pyvera/errors.py
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
aallan/vera-bench(manual)
855a514 to
7dc4664
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CLAUDE.md`:
- Around line 61-63: Update the corpus-program count in AGENTS.md from 217 to
218, matching the existing count used by check_corpus_canonical.py documentation
in CLAUDE.md.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 0bf4979c-c26f-4b4b-bade-0a58e3938d29
⛔ Files ignored due to path filters (6)
docs/SKILL.mdis excluded by!docs/**docs/index.htmlis excluded by!docs/**docs/index.mdis excluded by!docs/**docs/llms-full.txtis excluded by!docs/**docs/llms.txtis excluded by!docs/**tests/conformance/ch07_old_outside_ensures_rejected.verais excluded by!**/*.vera
📒 Files selected for processing (17)
AGENTS.mdCHANGELOG.mdCLAUDE.mdFAQ.mdREADME.mdROADMAP.mdSKILL.mdTESTING.mdspec/06-contracts.mdspec/07-effects.mdtests/conformance/manifest.jsontests/test_checker_functions.pytests/test_parser.pyvera/README.mdvera/_since.pyvera/checker/expressions.pyvera/errors.py
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
aallan/vera-bench(manual)
7dc4664 to
1b54347
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@AGENTS.md`:
- Line 11: Correct the conformance-suite description in AGENTS.md so it no
longer claims only two programs contain imports. State that most programs are
self-contained while accurately acknowledging all imported-fixture exceptions,
including the additional Chapter 7, Chapter 8, and Chapter 9 cases; preserve the
existing verification-level and negative-fixture guidance.
In `@tests/test_checker_functions.py`:
- Around line 707-718: Add a companion test near
test_new_in_requires_carets_the_construct that places new(State<Int>) in a
decreases() clause, then filter for E175 and assert the diagnostic’s construct
caret location, including its expected line and column.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 84ec0945-4feb-412c-8ef8-ddebf580a568
⛔ Files ignored due to path filters (6)
docs/SKILL.mdis excluded by!docs/**docs/index.htmlis excluded by!docs/**docs/index.mdis excluded by!docs/**docs/llms-full.txtis excluded by!docs/**docs/llms.txtis excluded by!docs/**tests/conformance/ch07_old_outside_ensures_rejected.verais excluded by!**/*.vera
📒 Files selected for processing (17)
AGENTS.mdCHANGELOG.mdCLAUDE.mdFAQ.mdREADME.mdROADMAP.mdSKILL.mdTESTING.mdspec/06-contracts.mdspec/07-effects.mdtests/conformance/manifest.jsontests/test_checker_functions.pytests/test_parser.pyvera/README.mdvera/_since.pyvera/checker/expressions.pyvera/errors.py
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
aallan/vera-bench(manual)
1b54347 to
7deffbb
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@vera/checker/expressions.py`:
- Around line 1306-1316: Extract the duplicated diagnostic fix text used by
_check_old_expr and _check_new_expr into one shared constant, then pass that
constant to both diagnostic call sites. Preserve the existing wording exactly
and remove the separate inline fix strings.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: ce7eb37d-c25f-4b2c-ac70-731e1b40d76b
⛔ Files ignored due to path filters (6)
docs/SKILL.mdis excluded by!docs/**docs/index.htmlis excluded by!docs/**docs/index.mdis excluded by!docs/**docs/llms-full.txtis excluded by!docs/**docs/llms.txtis excluded by!docs/**tests/conformance/ch07_old_outside_ensures_rejected.verais excluded by!**/*.vera
📒 Files selected for processing (17)
AGENTS.mdCHANGELOG.mdCLAUDE.mdFAQ.mdREADME.mdROADMAP.mdSKILL.mdTESTING.mdspec/06-contracts.mdspec/07-effects.mdtests/conformance/manifest.jsontests/test_checker_functions.pytests/test_parser.pyvera/README.mdvera/_since.pyvera/checker/expressions.pyvera/errors.py
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
aallan/vera-bench(manual)
7deffbb to
cc57d77
Compare
`old` and `new` take an effect reference — `old(State<Int>)`, spec
§7.9.2 — so `old(@Int.0)`, the Dafny form, failed to parse with a
generic [E005] "Unexpected @ ... Expected UPPER_IDENT" and the caret on
its own argument. Nothing named `old`, nothing said what its argument
has to be, and the spec pointer went to the formal-grammar chapter.
Found by the VeraBench v0.0.18 sweep (VB-T5-009).
Add [E030] / [E031]: caret on the keyword, a message naming the
construct and both of its rules (effect-reference argument, ensures-only
placement), and a rationale giving the reason for both — Vera has no
mutable variables, so effect state is the only thing a call can change.
Raised at parse, where the failure is. Grammar liberalisation is not
available: `old_expr: "old" "(" expr ")"` stops `old(State<Int>)`
parsing at all (the `<` reads as a comparison), and carrying both
alternatives is a reduce/reduce collision between `effect_ref` and
`fn_call` on UPPER_IDENT. The detector fires only on the *first* token
of an `old(`/`new(` argument, so a failure later in the argument —
`old(State<Int> > 0)`, whose real fault is the missing `)` — still
reports [E005] rather than being blamed on `old`.
[E174] / [E175] — `old()`/`new()` in the wrong clause, which already had
dedicated diagnostics — gain the pre-state explanation the issue asks
for. `@T.result` outside `ensures` already reported [E131] correctly and
is unchanged, now with caret coverage.
Conformance: ch07_old_outside_ensures_rejected (E174). The parse-level
case has no fixture — a program that fails to parse cannot be formatted,
and every corpus program must be canonical.
Co-Authored-By: Claude <noreply@anthropic.invalid>
cc57d77 to
120bb22
Compare
CodeRabbit round on PR #1188: TESTING.md's test-level inventory had drifted across three merges — the check-level prose lists and the level-limited skip table were missing ch05_reserved_fn_name_rejected (E153, this PR), ch07_old_outside_ensures_rejected (E174, #1180), and ch09_builtin_effect_redefinition_rejected (E152, #1182), and the skip-table rows for ch05_decreases_float_rejected (E127, #1179) were absent too. Regenerated against the manifest: twenty-eight check-level programs, twenty-one negatives, 69 level-limited skips, and the table verified complete for every check-level fixture. FAQ.md's test count catches up to the live 8,724 (the doc-counts oracle does not gate that line — noted for the wave-4 hardening). Skip-changelog: documentation-inventory reconciliation only, no compiler change Co-Authored-By: Claude <noreply@anthropic.invalid>
old(@Int.0)in a contract clause reported a generic[E005]parse error with the caret on its own argument. Nothing namedold, nothing said what its argument has to be, and the spec pointer went to the formal-grammar chapter. Found by the VeraBench v0.0.18 sweep,VB-T5-009, GPT-5.6 Sol.What the issue turned out to be
The report frames this as "
old()outsideensures()". Probing first changed the diagnosis, and the fix follows the probe rather than the title.Vera's
old/newtake an effect reference —old(State<Int>), spec §7.9.2 — not a Dafny-style arbitrary expression. Soold(@Int.0)is rejected byold_expr: "old" "(" effect_ref ")"at parse time, wherever it appears. The same[E005]fires insideensures():The clause-placement rule the title describes was already implemented and already had a good diagnostic. Current
main, unchanged:requires(old(State<Int>) > 0)[E174], caret onold, namesold()andensures()decreases(old(State<Int>))[E174], caret onoldrequires(@Int.result > 0)[E131], caret on@Int.result, names it andensures()requires(old(@Int.0) > 0)[E005], caret on the@ensures(old(@Int.0) > 0)[E005], caret on the@So the real gap is the argument shape, and it is not position-dependent. That also settles what the message must lead with: telling this author "move it into
ensures()" would be actively misleading, becauseold(@Int.0)is still wrong there.Before / after
Before:
After:
Mechanism: (b), a parse-level diagnostic — because (a) is not available
Mechanism (a), parse liberally and reject in the checker, was tried first and measured, not assumed. Three grammar variants were built against real Lark:
old_expr: "old" "(" expr ")"old(State<Int>)no longer parses. The<reads as a comparison andcmp_expris non-associative, so the one form that is actually valid Vera breaks.effect_refandexpralternativesGrammarError: Reduce/Reduce collision in Terminal('RPAR')betweeneffect_ref: UPPER_IDENTandfn_call: UPPER_IDENT. Not expressible in LALR(1).effect_ref | slot_ref | result_ref[E005]s onold(@Int.0 + 1)andold(foo(())), and it would put a non-EffectRefargument intoOldExpr, which eleven downstream modules pattern-match assuming an effect reference.So (a) is a hard grammar constraint, not a matter of effort. The diagnostic is raised at parse, in the existing pattern-matching section of
diagnose_lark_error— the same place[E001],[E002], and[E008]already rewrite a Lark failure into a Vera-shaped one. No AST change, no downstream blast radius.The detector is deliberately narrow: it fires only when nothing but whitespace separates the failing token from an
old(/new(immediately to its left — the exact position where the grammar demands an effect reference. A failure later in the argument, such asold(State<Int> > 0)whose real fault is the missing), leaves a parsed token in between and falls through to[E005]rather than blamingold.keep_old(is excluded by an identifier-boundary check. BothUnexpectedTokenandUnexpectedCharactersroute through it, soold($x)is covered too.Sibling verdict:
@T.resultneeds no workresult_refis inprimary_expr, so@Int.resultis grammatical everywhere an expression is and always reached the checker.requires(@Int.result > 0)already reports[E131]with the caret on the reference and a message naming both@T.resultandensures(). Left alone, as the brief allows — with caret/column tests added so it cannot regress quietly.Also in this PR
[E174]/[E175]gain the pre-state explanation the issue asks for: arequires()ordecreases()clause is itself evaluated before the body runs, so every expression in it already observes the pre-state and the after-statenew()names does not yet exist. The fix text adds the fact that closes off the obvious retry — a precondition cannot constrain effect state at all, because contract predicates must be pure andold()/new()are the only contract forms that name state.ensures-only); §6.2.2 cross-references it, since Chapter 6 is where a reader looks for contract constructs.E030/E031in the error-code list.Codes
E030/E031, in the parse range that raises them, in a newE03x — Contract constructs (parse)sub-block. Two codes rather than one, matching the adjacentE174/E175split for the same two constructs. Registered inERROR_CODESand invera/_since.pyas0.1.9. No version bump — this rides[Unreleased].Testing
Written RED first: 15 of the new parse tests failed with the documented
[E005]/[E006]before the fix.tests/test_parser.py::TestOldNewArgumentDiagnostic— 21 tests: error code, caret line and column, message content, all five diagnostic fields;requires,ensures,decreases, and nested inside a larger expression (requires(old(@Int.0) > 0 && true)); argument variants (slot ref, result ref, arbitrary expression, call, empty, invalid character, whitespace either side of the paren, argument on a later line);new(); and four over-fire guards.tests/test_checker_functions.py::TestContractStateFormPlacement— 6 tests pinning[E174]/[E175]/[E131]caret positions inrequiresanddecreases, plusold()/new()insideensuresstill accepted.tests/conformance/ch07_old_outside_ensures_rejected.vera,expected_error: E174. The parse-level case gets no conformance fixture on purpose:scripts/check_corpus_canonical.pyrunsformat_source()over every conformance program, and a program that fails to parse cannot be formatted. The fixture header records that, and points at the parser tests.Mutation-validated. Each piece of the new machinery was broken in turn and the named tests confirmed RED: feature off; identifier-boundary guard dropped; back-scan widened to the nearest
(instead of the first token; caret moved back onto the failing token;[E174]rationale reverted. Two mutations survived the first round — one because the mutation text was ineffective, one becausetest_identifier_ending_in_old_is_not_matchedwas passing for the wrong reason (keep_old(@Int.0 > )fails at the), nowhere nearkeep_old(, so it never exercised the guard). Both were fixed; the test now useskeep_old(> 0), which does fail on the first argument token. A third, dropping the open-paren check, is unfalsifiable by construction — after theoldkeyword the grammar accepts nothing but(, so the parser always fails on the very next token — and that is recorded in a code comment rather than papered over.Gates
pytest tests/ -q -m "not stress"(8,505 passed, 92 skipped),mypy vera/clean,ruff checkclean (including--select S), conformance 170/170, examples 42/42, corpus canonical 218/218, diagnostic fields, doc counts, site assets, spec/SKILL/FAQ/README/HTML example checks, explicit encoding, limitations sync, version sync. Full pre-commit hook chain passed on commit.Closes #1173
Summary by CodeRabbit
New Features
old()andnew()usage.ensuresclauses.Documentation
Tests
old(),new()and result-reference usage, including rejected contract placement.