fix(test): drop the shadowed can_read_body double on the slot-race request - #8577
fix(test): drop the shadowed can_read_body double on the slot-race request#8577chenmingwei23 wants to merge 1 commit into
Conversation
…quest Backend Lint fails on main: flake8 reports F811 at test/test_slot_close_recreation_race.py:131, a redefinition of can_read_body from line 119. Every open PR inherits it through its merge ref, so the gate reports whichever PR ran next rather than a defect in that PR. Two changes added the same property independently -- #8536 gave the double a body surface, then #8549 gave it a can_read_body surface -- and the class ended up declaring the property twice. Python keeps the later definition, so the first was already dead code with the second silently in effect. Removing the earlier one is behaviour-preserving rather than a choice between them, and that is measurable: _raw is b"" exactly when body is None and a JSON encoding otherwise, so bool(self._raw) and self._has_body return the same answer on every constructor path -- False for no body, True for an explicitly-passed {} and for a populated one. Checked over all three. The definition kept is the documented one, whose docstring states the contract the __init__ comment reasons about: a bodyless request must report nothing to read so read_bounded_json(..., allow_absent=True) takes its empty-object path. Verified: flake8 clean on the file, and all 42 tests in it pass.
github-actions
Bot
added
the
readiness: checking
Design Review (Fable 5) — ✅ PASSDesign-level review of Design-Verdict: PASS Removes the shadowed, already-dead earlier definition — the surviving property is unchanged at runtime, and the equivalence claim checks out against the constructor. [DESIGN-REVIEWED] 2ce2409 |
GPT 5.6 Review — ✅ no blocking findingsGPT 5.6 completed its review of This comment is updated in place on each push. Review detailsNo findings. False positive or not applicable? A repository writer can comment: |
Opus 4.8 Review — ✅ no blocking findingsReviewed Review detailsThe diff removes the shadowed No findings. [OPUS-REVIEWED] 2ce2409 Verdict parsed from the review's SHA-scoped output markers for commit False positive or not applicable? A repository writer can comment: |
|
fix is here #8583 |
github-actions
Bot
removed
the
readiness: checking
2 participants
Problem / Motivation
Backend Lint fails on main. flake8 reports:
Reproduced against main's tip in a clean worktree, so this is main's own code and not one PR's diff. Every open PR inherits it through its merge ref.
Why it matters
The gate stops naming a defect and starts naming whichever PR happened to run next. It cost one PR a red
Backend Lint & Type Checktoday whose diff contains zero Python files at all, and it will do the same to every other open PR until it clears.There is also a smaller latent problem underneath the lint finding: the class declares the same property twice, so one of the two definitions is dead code and which one is live depends on declaration order rather than on anything a reader would notice.
What changed (motivation -> approach -> change)
Two changes added the same property independently -- #8536 gave the
_Reqdouble a body surface, then #8549 gave it acan_read_bodysurface -- and the class ended up declaringcan_read_bodytwice. Python keeps the later definition, so the earlier one was already dead with the later one silently in effect.Removing the earlier one is behaviour-preserving rather than a choice between two behaviours, and that is measurable rather than asserted.
_rawisb""exactly whenbody is Noneand a JSON encoding otherwise, sobool(self._raw)(removed) andself._has_body(kept) return the same answer on every constructor path:bodyNone{}{"a": 1}The definition kept is the documented one, whose docstring states the contract the
__init__comment reasons about: a bodyless request must report nothing to read, soread_bounded_json(..., allow_absent=True)takes its empty-object path instead of raising.Tests
N/A for new tests -- this removes dead code from an existing test helper and adds no behaviour. It is verified by the existing suite and the linter, below.
Manual verification
flake8 test/test_slot_close_recreation_race.py-- clean; the F811 is gone.pytest test/test_slot_close_recreation_race.py-- 42 passed.Related Issues
Follows #8536 and #8549, which each added the property. Found while driving another PR's checks, where the same failure appeared on a diff with no Python in it.
Pattern harvest
Rule candidate:
review-promptPattern: two changes adding the same member to the same class produce a silent shadow, not a conflict. Neither PR conflicts textually, because each inserts at a different point in the class body, so git merges both cleanly and the language keeps only the last one. The result passes every behavioural test -- the surviving definition is one the authors intended -- and shows up only as a lint finding at a line neither author wrote in isolation.
The generalizable check is for the reviewer of the SECOND such PR: when a change adds a property or method to a test double or shim to satisfy a contract, grep the class for that member first. A duplicate is invisible in the diff, because the diff shows only the added block and not the one already there a dozen lines up.