Keep the projection's annotation when folding a record projection - #193
Merged
Conversation
#169) reduceObjectProp folded a record projection to the field's value verbatim, root annotation included. The Linker marks foreign accessor aliases Just Always (see Note [Foreign bindings structure emitted by the Linker]); once such an accessor is inlined into a record literal's field and the field is projected back out, the leaked Just Always became the root annotation of whatever binding received the fold, making isInlinableExpr pass unconditionally: the binding was duplicated at every use site and dropped from the uber-module bindings regardless of its use count. The fold result now carries the projection node's own annotation (new setAnn in IR.Types, the symmetric setter to getAnn), on both the LiteralObject and ObjectUpdate paths. Carrying the projection's annotation rather than resetting to noAnn matches the fall-through ObjectUpdate branch, which already keeps ann, and respects an explicit user @inline pragma placed on the projection. Demonstrated red first: three rule-level tests (leak out of a literal, leak out of an update patch, an annotated projection keeping its own annotation) plus an UberModule-level test where a twice-referenced binding whose RHS folds to an Always-annotated field must survive optimizedUberModule. All four failed before the fix and pass after it. No golden churn: the TailRecM2Shadow chain cited by the issue no longer routes through a shared binding on current main (verified by running the golden on a pre-fix build: output is identical), so the leak is latent there; the new tests pin it directly.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes an optimizer annotation leak in reduceObjectProp where folding a record projection could incorrectly propagate a field’s root annotation (notably the Linker’s Just Always foreign-accessor annotation) onto unrelated expressions/bindings, causing unintended unconditional inlining.
Changes:
- Preserve the projection node’s root annotation on the folded result when reducing
ObjectPropthroughLiteralObjectandObjectUpdate. - Add
setAnntoIR.Typesas a root-annotation setter symmetric togetAnn. - Add regression tests covering literal and update-folding cases plus an end-to-end
optimizedUberModulemanifestation.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| test/Language/PureScript/Backend/IR/Optimizer/Spec.hs | Adds regression tests ensuring reduceObjectProp does not leak Just Always and preserves the projection’s own annotation; includes an UberModule-level test for the previously observed inlining/DCE fallout. |
| lib/Language/PureScript/Backend/IR/Types.hs | Introduces setAnn to replace only the root annotation of a RawExp while leaving all nested annotations intact. |
| lib/Language/PureScript/Backend/IR/Optimizer.hs | Updates reduceObjectProp to use setAnn so folded projections retain the projection’s annotation rather than inheriting the projected field’s. |
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 #169.
The leak
reduceObjectPropfolded{ foo: v }.footovverbatim, root annotation included. The Linker marks exactly one shape withJust Always: a foreign accessor alias,ObjectProp (Just Always) (Ref foreign) (PropName name)(see Note [Foreign bindings structure emitted by the Linker]). The leak takes three hops: the inliner copies such an accessor into a record literal's field (a typeclass dictionary), something projects that field back out, and the fold hands the field'sJust Alwaysto the result. That annotation then becomes the root annotation of whatever binding received the fold, soisInlinableExprpasses unconditionally: the binding gets duplicated at every use site and dropped from the uber-module bindings, regardless of its use count.The fix
The fold result now carries the projection node's own annotation. This needed a new
setAnninIR.Types(the symmetric setter togetAnn; nothing on main could replace a root annotation without touching the rest of the tree), applied on both theLiteralObjectandObjectUpdatepaths.Why the projection's annotation and not a plain reset to
noAnn: the rewrite result occupies the projection's position in the tree, and the fall-throughObjectUpdatebranch already keepsann. A reset would also silently swallow an explicit user@inlinepragma placed on the projection. A test pins this choice: a field annotatedJust Alwaysprojected by a node annotatedJust Neverfolds to aJust Neverresult, which distinguishes the fix both from the bug (Just Always) and from the reset variant (Nothing).Tests, demonstrated red first
Three rule-level tests (leak out of a record literal, leak out of an update patch, the semantics pin above) and one UberModule-level test: a binding referenced twice whose RHS folds to an Always-annotated field must survive
optimizedUberModule. All four failed before the fix (the first two returnedJust Always) and pass after it.Goldens
No golden changed. The issue predicted that
Golden.TailRecM2Shadow.Testwould get its sharedaddbinding back, but on current main that chain no longer routes the leaked annotation through a shared binding: optimizer changes merged since the issue was filed reshaped it. I verified this directly by running the golden on a pre-fix build; its output is identical to the fixed build's. The leak is latent on the committed test programs, which is why the regression tests pin it at the rule and pipeline level instead. Theeval/golden.txtoracles are untouched.Relations
Related to #171, which concerns the same Linker
Just Alwaysannotation but a different interaction (the bareRefalias), and stays open.