Export every ensures emission and otherwise: emissions, not just the first statement - #90
Merged
Merged
Conversation
…first statement The module export table (and the local emitted-trigger set) registered only the leading call of each ensures block, a compatibility remnant of the old TS regex. A trigger emitted as the second statement — even when the first statement is itself another emission — was invisible to importers: every qualified reference to it drew a false allium.reference.unknownName warning and its consumers a false allium.rule.unreachableTrigger, although the emission is right there in the provider's text. Triggers emitted only through a `requires: ... otherwise:` clause were invisible the same way. Reordering cannot work around it: a block emitting two triggers can export at most one under first-statement-only rules, and the exportable slot is often pinned by data dependencies (an emission referencing a binding a let above it creates cannot move first). collect_leading_ensures_call becomes collect_ensures_emission_calls: every block item whose top-level expression is a bare PascalCase call is an emission. Value-producing calls nested in assignments, lets or arguments are still not collected. otherwise: emissions (parsed as an assignment item named 'otherwise') join the emitted set. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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.
Fixes #88.
Bug
When building the set of names a module exports (and the local emitted-trigger set), the checker registered only the first statement of each rule's
ensures:block — a compatibility remnant of the old TS regex, per the comment it carried. A trigger emitted as the second or later statement, even when the first statement is itself another emission, was invisible to importers:allium.reference.unknownNamewarning (exit 1),allium.rule.unreachableTrigger,Triggers emitted only through
requires: ... otherwise: SomeError(...)were invisible the same way (they parse as an assignment item namedotherwise, which no collector visited).Repro (fails on main, passes on this branch — transcribed into
cross_module_lifecycle.rs):Why reordering was never a workaround
A block emitting two triggers can export at most one under first-statement-only rules. And the single exportable slot is often pinned by data dependencies — an emission referencing a binding a
letabove it creates cannot move first.Fix
collect_leading_ensures_callbecomescollect_ensures_emission_calls: every block item whose top-level expression is a bare PascalCase call is an emission — first or not. Value-producing calls nested inside assignments,letbindings or arguments are still not collected, so the emission/black-box-call distinction is preserved.otherwise:values join the emitted set via a new arm incollect_emitted_trigger_from_item.The parse layer needed no change:
allium parsealready contains every emission as a structuredCallnode — the loss happened only in the resolution layer.Tests
otherwise:emissions; plus a guard that a trigger nothing emits stays unreachable)SecondEmitted/AfterAssignEmitted/ otherwise-emitted triggers draw no diagnostics; a trigger the provider never mentions still warns)Real-world impact
On a ~180-spec production corpus, the v3.5.0 → v3.5.3 upgrade surfaced 4 false
unknownNamewarnings of exactly this shape (a login spec consuming triggers its auth-OTP module emits mid-ensures), forcing per-site suppression comments. With this fix those suppressions become unnecessary.Note: this PR and the config/deferred one both append tests to
cross_module_lifecycle.rsand theanalysis.rstest module — whichever merges second needs a trivial union rebase; happy to rebase promptly.