fix(lint): correlate emits with writes order-independently in missing-events-access-control - #16651
Open
gomesalexandre wants to merge 1 commit into
Open
Conversation
…-events-access-control
The mark_event correlation only matched writes already recorded when an emit
statement was visited, so an emit written BEFORE the state change it documents
could never mark that later write as evented - producing a false 'missing
event' warning on an equally valid, common ordering:
emit OwnershipTransferred(owner, newOwner);
owner = newOwner;
Fix: defer correlation. record_emit just appends (EventId, Sources) facts;
correlate_pending() sweeps all pending emits against all not-yet-evented
writes, called at scope boundaries (function end, each if-branch, each loop,
each try/catch clause) so a conditionally-reached emit can never satisfy a
write outside the scope that might not have run it. Loop and Try additionally
restore each pre-existing write's evented flag after their own
correlate_pending() call, since that mutation isn't otherwise isolated the way
the if-branch clone-and-merge already isolates it.
Along the way this also closes a related, pre-existing defect (present before
this diff too): a scope-unaware immediate correlation let an emit inside one
mutually-exclusive branch/loop/try-clause retroactively satisfy a write
reachable outside it.
Known, deliberately out-of-scope limitations (disclosed, not fixed):
- Two branches/clauses that both emit the same matching event can still be
treated as a false positive for a write after them (this file has no
event-fact intersection across branches, mirroring how merge_branches
already treats writes).
- A conditionally-evaluated call (inside &&/||/?:) is analyzed as if always
called, matching this file's pre-existing, unconditional call-inlining.
- Within a single loop body, a write that always executes followed by a
conditionally-skippable emit (via break/continue/revert) can still miss a
warning - the analyzer has no statement-reachability model at all.
- A try/catch clause that always reverts is treated the same as one that
returns for the writes-evented AND-rule (conservative false positive, not a
false negative) since the two aren't distinguished.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Lh6V2uPTUqauqq45BM7m5k
gomesalexandre
requested review from
0xrusowsky,
DaniPopes,
figtracer,
grandizzy,
mablr,
mattsse and
stevencartavia
as code owners
September 4, 2026 22:40
Contributor
✅ Changelog foundThe deterministic check will validate the changed entry. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
mark_event's correlation only matched writes already recorded when anemitstatement was visited - it walkedself.state.writesforward-only, marking matches asevented. So anemitwritten BEFORE the state change it documents could never mark that later write as evented, producing a false "missing event" warning on an equally valid, common ordering:vs. the currently-passing (because it's after the write):
Fix
Defer correlation instead of doing it immediately.
record_emitjust appends(EventId, Sources)facts to a newstate.emits. A newcorrelate_pending()sweeps all pending emits against all not-yet-evented writes, order-independently, called at scope boundaries:if-branch (before the branch state is discarded/merged),LoopSource, includingdo-while, since even a guaranteed-first-iteration canbreak/continue/revertpast the emit on that pass),try/catchclause.Each of these boundaries discards the emits recorded inside that scope before continuing past it - a conditionally-reached emit (inside one
ifbranch, one loop iteration, or onecatchclause) must never satisfy a write reachable via a DIFFERENT path that might not have run it.LoopandTryadditionally restore each pre-existing write'seventedflag after their owncorrelate_pending()call, since - unlike theif-branch case, which is already isolated by a full state clone-and-merge - that flag isn't otherwise protected from being mutated by a conditionally-reached emit.Trytreats clauses the same waymerge_branchestreats anif's two arms (each analyzed from its own isolated clone of the pre-try state, then recombined), generalized to N mutually-exclusive clauses via a newmerge_try_clauses, and is exit-aware: a clause whose body always exits (return/revert) is excluded from the taint/alias merge (mirroringmerge_branches's existing handling of an exitingif/elsearm), since code after the try never observes what an always-exiting clause did.Along the way this also closes a related, pre-existing defect that predates this diff: the old immediate, scope-unaware correlation let an emit inside one mutually-exclusive branch/loop/try-clause retroactively satisfy a write reachable only outside it. Regression fixtures for this are included and verified to already fail against the fully original, unpatched code.
Known, deliberately out-of-scope limitations (disclosed, not fixed)
merge_branchesalready handleswrites.eventedthe same way.&&/||/a ternary) is analyzed as if it were always called - this matches the file's pre-existing, unconditional call-inlining (analyze_call), not something this diff changes.break/continue/revert) can still miss a warning - the analyzer has no statement-reachability model at all, for loops or otherwise. Confirmed to exist identically in the original, unpatched code.try/catchclause that alwaysreverts is treated the same as one thatreturns for the writes-evented AND-rule (a conservative false positive - an extra, avoidable warning - never a false negative), since areverted write never actually persists but the two aren't distinguished here.All four are the FALSE-POSITIVE direction (or a pre-existing gap unaffected by this diff), never a new false negative introduced by this change - happy to split any into follow-up issues if a maintainer wants them tracked.
Testing
if, a matching event confined to oneif-branch/loop/try-clause not satisfying a write outside it, do-while+breakskipping an emit, a write before a loop/try with the matching emit inside it, cross-clause (success-emits/catch-writes) isolation, and a reverting catch not blocking credit for taint/aliases while still requiring its own event coverage for writes.forge-lintunit tests pass. Clippy clean (-D warnings).cargo fmt --checkclean on stable - nightly (used by this repo's CI) wasn't available in this environment to verify against.receipts
no runtime UI change -
forge-lint's own test suite is the receipt for this PR (static-analysis fixture corpus, not a runnable app).