fix(tooling): honour diag-fields-exempt for unresolvable fields in all 3 passes - #965
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
🔗 Linked repositories identifiedCodeRabbit considers these linked repositories for cross-repo context during reviews:
📝 WalkthroughWalkthroughThis PR tightens the diagnostic-fields validation script so helper-owned ChangesDiagnostic-fields gate fixes
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 7 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (7 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@tests/test_diagnostic_fields.py`:
- Around line 709-753: Add fixture coverage in test_diagnostic_fields for the
remaining rebind paths handled by _rebound_names: with ... as d, walrus d :=
..., and except ... as d. Create small source snippets that mirror
REASSIGNED_BEFORE_RETURN and REBOUND_VIA_FOR_LOOP, then assert
spec_ref_violations_in_source returns one violation and still reports the
missing §99.1 reference so regressions in the With, NamedExpr, or ExceptHandler
branches are caught.
🪄 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: 30bb3cb8-3f9d-4193-a907-255c1624476b
📒 Files selected for processing (5)
CHANGELOG.mdROADMAP.mdTESTING.mdscripts/check_diagnostic_fields.pytests/test_diagnostic_fields.py
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
aallan/vera-bench(manual)
…he helper's result _plumbing_ctors already required a genuine helper method (aallan#827's fix) whose sole own-scope Diagnostic() construction is unambiguous, but never confirmed that construction was actually the helper's output. A helper that builds its one ctor and hands it to something other than return/append — e.g. self.dispatch(d) instead of self.errors.append(d) — still had it elected as plumbing and skipped by all three passes, so a bogus spec_ref or unregistered error_code on it would ship silently. _ctor_is_reachable_as_result now requires the sole ctor be return-ed, appended, or bound to a local that is later return-ed/appended, gated after the existing single-ctor check so aallan#827's ambiguity handling is untouched. On vera/ the exempt set is unchanged: all five real helpers return or append their sole ctor directly, so the gap was latent, closed defensively. The name-based return/append match in the second half of the check has no real data-flow: a local rebound after the ctor-binding assign would still match on the name alone and be wrongly treated as reachable. Fixed conservatively via _rebound_names, which counts every name-binding form in own scope — plain and augmented assignment, for/with targets, walrus, and except-as names — not just plain Assign. A name rebound more than once by any of these forms is unreliable, so the ctor is inspected rather than exempted. Fixes aallan#956 Co-Authored-By: Claude <noreply@anthropic.invalid>
…l 3 passes The field-presence pass (check_source) appended its non-literal-severity violation and continue'd before its own opt-out lookup ran, so a marker on that exact call was never consulted even in the one pass that otherwise honours it. The spec_ref-validity and error_code-registration passes never consulted the opt-out at all. Fixed for the unresolvable (non-literal) sub-case only: a non-literal severity or spec_ref can't be checked statically and so is opt-out-able, same as a missing field. A spec_ref that resolves but cites the wrong/nonexistent section, or an error_code not in ERROR_CODES, is a content error, not a tagging gap - the opt-out never suppresses either, marker or not. On vera/ the exempt set is unaffected: the three existing real markers are all missing-field cases. main()'s remedy text now states this precisely instead of implying the opt-out waives everything it lists. Fixes aallan#955 Co-Authored-By: Claude <noreply@anthropic.invalid>
d26f2ce to
5ef1b03
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #965 +/- ##
=======================================
Coverage 93.29% 93.29%
=======================================
Files 96 96
Lines 31448 31448
Branches 456 456
=======================================
Hits 29339 29339
Misses 2096 2096
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:
|
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 `@scripts/check_diagnostic_fields.py`:
- Around line 287-342: The reachability logic in _rebound_names and
_ctor_is_reachable_as_result does not account for annotated assignments, so
AnnAssign can hide or miss rebinding of the local ctor holder. Update the
binding-site counting in _rebound_names to include ast.AnnAssign targets, and
adjust _ctor_is_reachable_as_result so a ctor bound via an annotated assignment
is treated the same as a plain Assign when checking whether the local name is
still reliably reachable. Add a regression fixture covering both the annotated
rebind case and the annotated ctor-binding case.
In `@TESTING.md`:
- Line 123: Update the `test_diagnostic_fields.py` entry in `TESTING.md` so the
`# diag-fields-exempt` opt-out summary explicitly includes the `error_code` pass
alongside severity and spec_ref. The current description understates the
behavior pinned by the tests; revise the row text to reflect that the opt-out is
honored consistently across all three passes for an unresolvable non-literal
value, while still excluding cases that resolve but are factually wrong.
🪄 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: ae8862b7-a737-4f2f-8c44-1b4e660103ea
📒 Files selected for processing (5)
CHANGELOG.mdROADMAP.mdTESTING.mdscripts/check_diagnostic_fields.pytests/test_diagnostic_fields.py
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
aallan/vera-bench(manual)
…(PR aallan#964 review) A value-carrying annotated assignment (d: object = self.template) rebinds the local without being an ast.Assign, so a bogus ctor followed by an AnnAssign rebind was still elected as plumbing and skipped by all three passes (probe-confirmed EXEMPT). Dually, d: Diagnostic = Diagnostic(...) was never recognised as the initial binding, permanently inspecting genuine plumbing. Both arms fixed: _rebound_names counts value-carrying AnnAssign targets (a bare annotation does not assign and is not counted), and the binding recognition accepts an AnnAssign whose value is the ctor. Six new fixtures: the two AnnAssign directions (RED-confirmed pre-fix, each arm mutation-killed), the bare-annotation distinction, and one pin each for the with/walrus/except rebind arms so a dropped arm ships RED (PR aallan#965 review ask, pinned here where the machinery lives). Exempt set on vera/ unchanged (the same five plumbing ctors). TESTING.md/ROADMAP counts follow (6,827 -> 6,833).
# Conflicts: # CHANGELOG.md # ROADMAP.md # TESTING.md # scripts/check_diagnostic_fields.py # tests/test_diagnostic_fields.py
|
Merged — thanks @chethanuk. That's the #955/#956 gate-honesty pair complete, and with it all four of this batch. The design call you made here — honouring the opt-out for unresolvable fields only, and explicitly rejecting the blanket-honour alternative so a one-line comment can never ship a diagnostic citing a nonexistent spec section — was exactly right, and the mutation-kill you ran against your own rejected alternative is the house discipline in miniature. What the maintainer loop did on this one: the reconcile after #964's merge (your branch was cut from a pre-fix tip of that stack, so the narrow Four PRs, four merges, three issues closed (#852, #702, #955+#956). The whole batch lands in the next release. Genuinely good run — the tracker's Stage 20 table is noticeably shorter for it. |
2 participants
Fixes #955
Depends on #956 (#964) — stacked, will show a combined diff until #956 merges. This branch is cut from the tip of `fix/issue-956-plumbing-skip-reachability`, since #955's diff assumes #956's reachability predicate already exists.
Summary
Test plan
Summary by CodeRabbit
# diag-fields-exemptis applied forDiagnostic(...)checks: it now reliably suppresses only genuinely unresolvable, non-literalseverity/spec_refissues, while still reporting incorrectspec_refcontent and unregisterederror_code.spec_refcall spans and empty opt-out reasons.