Rotate [<return: X>] attributes during binding normalization - #20356
Conversation
Since dotnet#19738 the parser moved attributes written as [<return: X>] in front of a binding out of SynBinding.attributes and into SynValInfo.retInfo. The untyped tree then reported no attributes for source that visibly has one, which every consumer of the parse tree sees: formatters, analyzers, source generators and refactoring tools. The move was also lossy. The attribute list range narrowed from the [< >] span to the attribute alone, and all return attributes were collected into a single synthesized list, so [<return: A; return: B>] and [<return: A>][<return: B>] produced identical trees. Neither can be printed back to its original form. Do the rotation in BindingNormalization.NormalizeBinding instead, the single funnel from SynBinding to NormalizedBinding and already a lowering step. Everything downstream still reads retInfo as the single source of truth, so the fixes for dotnet#17904 and dotnet#19020 are unchanged, but the parse tree again says what was written. Add parse baselines for both the common [<return: Struct>] partial active pattern and for attribute grouping, neither of which had any coverage before.
❗ Release notes requiredYou can open this PR in browser to add release notes: open in github.dev
|
|
@T-Gro ready for review. //cc @edgarfgp @auduchinok |
This comment has been minimized.
This comment has been minimized.
T-Gro
left a comment
There was a problem hiding this comment.
Could you also pls trigger the "post mortem" skill so that we generate instructions that will prevent similar breaks in the future?
* IL: derive an entity's public path from its compilation path instead of storing it entity_pubpath duplicated data the entity already had: every construction site set it to exactly the enclosing compilation path plus the entity's own logical name, as the REVIEW comment on the field had noted. Storing it cost an option, a PubPath and a fresh string[] per entity — about 100 bytes each, 2.6 MB on a 489-reference project — and the pickled form was read back into a field nothing needed. PublicPath becomes a struct over the enclosing CompilationPath and the name, so Entity.PublicPath can produce one without allocating, and the field is gone. The struct carries custom equality over the mangled path and the name only: derived equality would also compare the enclosing path's ILScopeRef and SyntaxAccess, which is stricter than the flat string[] comparison it replaces. Accessors return voption so a struct payload does not force an allocation per lookup. The pickle format is unchanged: writers derive the flat path, readers consume the index and discard it, so the pubpath table no longer builds PublicPath values. Derivation is exact at all four construction sites, and the only writes to entity_cpath and entity_logical_name after construction are in Link, which copies both from one source entity, so a derived path cannot drift from a stored one. Retained memory after ParseAndCheckProject drops 0.12-2.62 MB per project (-0.18% to -0.84%) across the measurement suite. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Release notes * Release notes * Review * Compare public paths with a function, not a struct member pubPathEq ended in a call to PublicPath.Equals, and fslibRefEq in one to EqualsFullPath. A body ending in a struct member call is not inferred to make no critical tailcalls, and that inference propagates to callers: primEntityRefEq gained a .tail prefix, and from there so did tyconRefEq, tcrefAEquiv, HasHeadType and the TypeTesters predicates. A tail-prefixed call is never inlined, which cost 12% checking a two-project graph. Comparing the access paths in a plain function keeps the inference, and builds neither a MangledPath list nor a FullPath array. PublicPath no longer carries equality at all. Its only comparison was an implicit = on a PublicPath voption in Exprs.fs, which boxed both sides; that now calls pubPathEq, so structural equality on a PublicPath is a compile error rather than a silently boxing comparison. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SSWUAYfpj82BHvzpkUCy8Z * Fantomas --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-authored-by: Tomas Grosup <Tomas.Grosup@gmail.com>
… exception-based control flow for inherited IL methods (dotnet#20272) * Fix inherited IL base-call crash and add release notes * Address review: inherit the IL method in the 20264 test, resolve via FindByNameAndArity, and keep a try/with around signature matching. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * PR review fix * Clarify IL base-call check notes, split dotnet#20264 test Release notes now specify skipping `resolveILMethodRefWithRescope` only when the method isn't on the immediate IL type, not via exception flow. The `SimpleInteropTests.fs` test for dotnet#20264 is split: one test checks inherited non-abstract IL base method calls on generics succeed, the other checks abstract base method calls on generics fail with FS1201. Test names and code are now clearer and more targeted. --------- Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…net#20319) * Flatten always-on IndexerNotationWithoutDot: remove flag and collapse enforcement sites Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Remove orphaned IndexerNotationWithoutDot diagnostic strings and regenerate xlf Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Sync xlf files: remove trans-units for FSComp keys deleted in this PR --------- Co-authored-by: Copilot <copilot@github.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: T-Gro <t-gro@users.noreply.github.com>
…et#20337) * Replace the stringified pattern-match memo key with a typed one The memo added in dotnet#20244 keyed residual match states by concatenating paths, pattern node ids and bound expressions into a string, then compared those strings. Replace it with structural keys: - PathKey / BoundExprKey / FrontierKey / MemoKey instead of string concatenation, so equality is structural rather than textual. - Record field and union case keys now carry the declaring tycon stamp, so same-named fields of different types can no longer fuse. - MemoEntry replaces the (int ref * Lazy<bool> * Lazy<_>) tuple. - The diagnostics/codegen distinction is a JoinPromotion argument rather than being inferred from warnOnIncomplete, which happened to coincide. Behaviour preserving: emitted assembly size and full IL fingerprint are identical to main for the issue dotnet#18425 repro at N=8..40. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 86c47998-fe39-4b68-8b80-c72b72fc9efb * Add release note Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 86c47998-fe39-4b68-8b80-c72b72fc9efb * Keep the memo machinery private and drop a redundant pass CompilePatternBasic is only called from CompilePattern in this file and is not in the signature file, so it and JoinPromotion can be private like the key types already are. Also name the pattern node id in FrontierKey (matching the existing ClauseNumber alias) and build the bound-expression key list in one pass instead of Map.toList followed by List.map. Map enumerates in ascending key order, so the key is unchanged; verified IL-identical. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 86c47998-fe39-4b68-8b80-c72b72fc9efb * Document the key types State the safety invariant on the path key directly (equal keys imply pathEq, so being finer only costs memo misses), and give FrontierKey and MemoEntry the same brief purpose comments the neighbouring types have. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 86c47998-fe39-4b68-8b80-c72b72fc9efb * Record what the size guard actually catches Red-green checked by disabling promotion: the N=32 input then runs for 383s and is OOM-killed rather than emitting something slightly over the bound, so the exact constant does not matter. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 86c47998-fe39-4b68-8b80-c72b72fc9efb * Drop the release note This is an internal refactoring with no observable behaviour change, and the latent over-fusion it fixes was introduced by dotnet#20244 in this same release, so no shipped compiler could exhibit it. Labelled NO_RELEASE_NOTES. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 86c47998-fe39-4b68-8b80-c72b72fc9efb --------- Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 86c47998-fe39-4b68-8b80-c72b72fc9efb
|
@T-Gro thanks, done! |
|
🔍 Tooling Safety Check — Affects-Agent-Config, Affects-Compiler-Output
|
|
@T-Gro I think this MacOS Batch1 failure is unrelated:
Let me know if I need to do something here. |

Description
Follow-up to #19738. That PR moved
[<return: X>]attributes written in front of a binding out ofSynBinding.attributesand intoSynValInfo.retInfo, and it did so inmkSynBinding, in the parser. This PR keeps both fixes from that change but performs the rotation one layer later, so the untyped tree again describes the source.Given:
the binding reports no attributes for source that visibly has one. Routing the attribute to the return-value metadata slot is right for the type checker, IL emit and the Symbols API, but
SynBindingis not one of those consumers; its contract is to say where the user wrote it.The rotation is not new. It previously lived in
TcNormalizedBindingand patched a localvalSynData, so the parse tree stayed faithful to the source. Doing the same work inmkSynBindinghands every untyped-tree consumer a tree that no longer matches what was written: formatters, analyzers, source generators, refactoring tooling.It was also lossy in two ways that cannot be recovered downstream:
[< >]span to the attribute alone, so the brackets the user typed were no longer represented anywhere.RotateReturnAttributescollected every return attribute into one synthesized list, so[<return: A; return: B>]and[<return: A>][<return: B>]produced identical trees. Neither can be printed back to its original form.Fantomas hit this while moving onto a newer FCS (fsprojects/fantomas#3400): partial active patterns marked
[<return: Struct>]were parsed and then never printed, silently deleting them from every formatted file, including 34 in Fantomas' own source.The change
Move the rotation to
BindingNormalization.NormalizeBinding, the single funnel fromSynBindingtoNormalizedBindingand already a lowering step.NormalizedBindingholds a flatSynAttribute list, soRotateReturnAttributesnow takes and returns that instead ofSynAttributes, and the list-splicing that destroyed grouping is gone with it.Everything downstream (
TcNormalizedBinding,AnalyzeAndMakeAndPublishRecursiveValue, the object-expression paths) consumesNormalizeBinding's output, soTcNormalizedBindingkeeps readingretInfoas the single source of truth and theValReprInfolookup inNameResolutionfor[<return: Struct>]active patterns is untouched.The return type annotation form (
let f x : [<return: A>] int = x) never went through this rotation; those attributes are placed inretInfobyInferSynReturnDataand are unaffected.SynBindingnow again carries the attribute, with the full[< >]range, andretInfois empty at parse time:The two grouping forms are distinguishable again:
[<return: A>][<return: A>]yields twoSynAttributeLists,[<return: A; return: A>]yields one.Fixes #: not applicable, no filed issue; this is a follow-up to PR #19738.
Checklist