Fix #17904 and #19020 - #19738
Conversation
In mkSynBinding, move any attribute written with the explicit 'return:' target from the binding's prefix attributes into SynValData.SynValInfo.retInfo. This makes the syntactic placement (which the parser puts on the binding) match the semantic intent (the attribute targets the method's return value). Fixes: - dotnet#19020: [<return: X>] silently dropped on class members - dotnet#17904: false-positive AllowMultiple=false when [<X>] and [<return: X>] appear on the same member
❗ Release notes required
|
This comment has been minimized.
This comment has been minimized.
…-pattern detection After SynInfo.RotateReturnAttributes moves [<return: X>] from the binding's prefix attributes into SynValData.SynValInfo.retInfo, two downstream consumers also need updating: - TcNormalizedBinding's retAttribs computation now type-checks attrs already in valSynData's return SynArgInfo, so isStructRetTy/argAndRetAttribs work for [<return: Struct>] on partial active patterns. - ActivePatternElemsOfValRef now classifies the flag bag from ValReprInfo's result ArgReprInfo rather than scanning vref.Attribs. Fixes recursive struct active patterns (e.g. let rec (|HasOne|_|)).
With the parser-level rotation in SynInfo.RotateReturnAttributes, the binding's prefix attrs never contain [<return: X>] by the time TcNormalizedBinding runs. The partition-and-rotate dance and the valSynData patch are no-ops in every reachable case, so remove them and read return attrs directly from SynValData.SynValInfo.retInfo (where the parser put them) plus any attrs on the return type annotation.
|
@T-Gro I think the CI is stuck or it has not even started. |
Drop the module-let case from the dotnet#19020 test (module bindings were never affected). Rename tests with 'Issue NNNNN -' prefix and tighten source samples and failure messages.
There was a problem hiding this comment.
Pull request overview
This PR fixes two related attribute-handling bugs by ensuring [<return: ...>] prefix attributes on bindings are routed to the return-value metadata slot early (at syntax construction time), so downstream phases (type-checking, IL emit, FCS symbols) see them in the correct place.
Changes:
- Rotate
[<return: ...>]prefix attributes from binding attributes intoSynValInfo.retInfoduringmkSynBinding. - Update downstream consumers (notably active pattern return-kind detection and binding attribute type-checking) to read return attributes from return-info rather than
Val.Attribs. - Add component regression tests for issues #17904 and #19020, plus a release-notes entry.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/FSharp.Compiler.ComponentTests/Language/AttributeCheckingTests.fs | Adds regression tests covering return-attribute emission and duplicate detection across targets. |
| src/Compiler/SyntaxTree/SyntaxTreeOps.fs | Implements parser-stage rotation of [<return: ...>] attributes into SynValInfo.retInfo. |
| src/Compiler/Checking/NameResolution.fs | Adjusts active-pattern return-kind logic to look for return attributes in ValReprInfo return info. |
| src/Compiler/Checking/Expressions/CheckExpressions.fs | Removes typechecker-stage rotation and re-plumbs return attribute collection/typechecking. |
| docs/release-notes/.FSharp.Compiler.Service/11.0.100.md | Documents the bug fixes in release notes. |
Verifies that [<CompilationRepresentation(Instance)>] on a union-type member is not rotated to the return value.
…lizedBinding SynValData.SynValInfo.retInfo already contains both the return type annotation attribs (via InferSynReturnData in mkSynBinding) and the rotated [<return:>] prefix attribs. The previous code also pulled the annotation attribs from rtyOpt, processing them twice. Use SynValData as the single source of truth.
|
🔍 Tooling Safety Check — Affects-Compiler-Output
|
auduchinok
left a comment
There was a problem hiding this comment.
@edgarfgp Could you please add FCS tests accessing the attributes via both the member itself and the return value?
…aration Three tests verify the FCS Symbols API correctly surfaces: - [<X>] on FSharpMemberOrFunctionOrValue.Attributes only - [<return: X>] on ReturnParameter.Attributes only - Both, independently, when both targets are used on the same member Uses the marker-based Checker.getSymbolUse API and the existing HasAttribute<'T>() member against System.ComponentModel.DescriptionAttribute (AttributeTargets.All) to avoid new test-local helpers.
@auduchinok Done 72f0e53 |
* Rotate [<return: X>] attributes during binding normalization Since #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 #17904 and #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. * Add release note * Calculate Entity.PublicPath instead of storing (#20285) * 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> * Cleanup: use `FindByNameAndArity` guard in `CheckILBaseCall` to avoid exception-based control flow for inherited IL methods (#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 #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 #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> * Reject bitwise operators on char-backed enums (#11785) (#20322) * Remove always-on IndexerNotationWithoutDot language feature flag (#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> * Replace the stringified pattern-match memo key with a typed one (#20337) * Replace the stringified pattern-match memo key with a typed one The memo added in #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 #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 #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 * Add postmortem --------- Co-authored-by: Eugene Auduchinok <eugene.auduchinok@jetbrains.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-authored-by: Tomas Grosup <Tomas.Grosup@gmail.com> Co-authored-by: Andrii Chebukin <XperiAndri@Outlook.com> Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Co-authored-by: Edgar Gonzalez <edgargonzalez.info@gmail.com> Co-authored-by: Copilot <copilot@github.com> Co-authored-by: T-Gro <t-gro@users.noreply.github.com> Copilot-Session: 86c47998-fe39-4b68-8b80-c72b72fc9efb
Fixes #17904 and #19020.
Problem
Two related bugs share the same root cause —
[<return: X>]prefix attributes on a binding aren't routed to the return-value metadata slot:[<return: X>]on class members is silently dropped from IL. The attribute appears in source, but reflection on the method's return parameter finds nothing.[<X>]and[<return: X>]on the same member are incorrectly flagged as duplicates underAllowMultiple = false, because both end up inVal.Attribstogether rather than being separated by metadata target.Before
After
Genuine duplicates still error:
Solution
Move the rotation to the parser stage. In
mkSynBinding(SyntaxTreeOps.fs), anySynAttributewhoseTarget = Some "return"is moved out of the binding's prefix attribute list intoSynValData.SynValInfo.retInfo. From that point onward, every downstream consumer (type-checker, IL emit, FCS Symbols API) sees the attribute in the semantically correct slot, regardless of where the user wrote it.The discriminator is syntactic —
synAttr.Target = Some "return"— not the typedconstrainedTargetsbitmap. Attributes without areturn:prefix are never touched, so[<CompilationRepresentation(...)>]onOption.Valueis unaffected and FSharp.Core builds cleanly.