Skip to content

Add EmittedIL tests proving [<CompiledName>] works by design on extension overloads (#19604)#19737

Open
T-Gro wants to merge 1 commit into
mainfrom
fix/19604-compiledname
Open

Add EmittedIL tests proving [<CompiledName>] works by design on extension overloads (#19604)#19737
T-Gro wants to merge 1 commit into
mainfrom
fix/19604-compiledname

Conversation

@T-Gro
Copy link
Copy Markdown
Member

@T-Gro T-Gro commented May 14, 2026

Summary

Adds EmittedIL baseline tests demonstrating that [<CompiledName>] applied to one extension overload correctly renames only that overload in IL output. The unannotated overload retains its default compiled name (TypeName.MethodName).

This is correct by-design behavior — only the single overload carrying the attribute should be renamed. No product change is needed.

Test cases

  • CompiledNameAttribute06.fs: C#-style extension ([<Extension>]) with one overload annotated [<CompiledName "UseCosmosDb">]. IL shows the annotated overload becomes UseCosmosDb while the unannotated one stays Builder.UseCosmosDb.
  • CompiledNameAttribute07.fs: F#-style extension (type augmentation) with [<CompiledName "Renamed">] on one overload. IL shows the annotated overload becomes Renamed while the other stays Builder.UseDb.

Context

Per reviewer feedback on this PR (#19737 comment by @auduchinok), the existing behavior is by design. The original product change (a new warning) has been reverted. These tests codify the expected IL output as regression baselines.

Closes #19604

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 14, 2026

✅ No release notes required

@T-Gro T-Gro added AI-Auto-Resolve-CI Opt-in: LabelOps triages CI failures on this PR every 3h AI-Auto-Resolve-Conflicts Opt-in: LabelOps merges main into this PR and resolves conflicts every 3h labels May 14, 2026
@T-Gro T-Gro requested a review from abonie May 14, 2026 14:18
@T-Gro T-Gro enabled auto-merge (squash) May 14, 2026 14:18
@github-actions github-actions Bot added the AI-Tooling-Check-Bypassed Tooling check: non-fork PR, not diff-analyzed label May 14, 2026
@github-actions
Copy link
Copy Markdown
Contributor

🔍 Tooling Safety Check — Bypassed (non-fork)

(scanned-sha)abddf12aab2a6aab67b72f91fbc74da76228262f(/scanned-sha)

Generated by PR Tooling Safety Check · ● 1.6M ·

@github-actions github-actions Bot mentioned this pull request May 14, 2026
@auduchinok
Copy link
Copy Markdown
Member

auduchinok commented May 15, 2026

I'd say that the existing behavior is by design. We don't do anything like this for any other members. Why should it be done for extensions only?

Since it's adding new warnings to existing valid F# code, it may break build when warnings are turned into errors. If we want to proceed with this change, I think it should go through a language suggestion and should be guarded by a language version.

Copy link
Copy Markdown
Member Author

@T-Gro T-Gro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review: Warn on inconsistent [] across extension overloads

Overall this is a solid, well-scoped fix for #19604. The implementation is clean, the comment is helpful, the test coverage is good. A few observations:


1. Warning message is misleading for the "different compiled name values" case

The message says:

"Some, but not all, overloads of the extension member '%s' have a [] attribute..."

But the check also fires when all overloads have the attribute with different values (e.g. UseDb_A and UseDb_B). In that case the message is factually wrong — all overloads do have the attribute.

Consider either:

  • Splitting into two distinct warnings (missing vs. differing values), or
  • Rewording to something like: "The overloads of extension member '%s' have inconsistent [] usage — either some lack the attribute, or they specify different names. This can leave callers unable to resolve overloads under a single name."

2. Potential cross-module false positives

The check runs at the file level using allValsOfModDef implFileContents which collects vals from all nested modules. Extension members are grouped by (MemberApparentEntity.Stamp, LogicalName).

If two different modules in the same file both define extension overloads for the same type with the same logical name, they'll be grouped together and potentially warned about — even though they compile to static methods on different IL types and there's no naming conflict in the output:

` sharp
module A =
type Builder with
[<CompiledName "UseDb">]
member this.UseDb(x: int) = ()

module B =
type Builder with
member this.UseDb(x: string) = () // ← false positive warning
`

Consider adding the containing module stamp/path to the grouping key, or scoping the check per-module similar to how CheckForDuplicateExtensionMemberNames works (called in CheckDefnInModule with allTopLevelValsOfModDef).

3. Minor: test regex in "different values" case

sharp |> withDiagnosticMessageMatches "Some, but not all, overloads of the extension member 'UseDb' have a \[<CompiledName>\] attribute|differ"

The |differ at the end creates a regex alternation that matches the full first clause or the literal string differ — which isn't a useful second alternative (it would match any message containing "differ"). This seems unintentional. Either remove |differ or group it properly: (...attribute.*differ|Some, but not all...).


Summary

Good work addressing a real usability gap. The core logic is correct and well-tested. The issues above are all "nice to have" improvements — #2 (false positives) being the most impactful, though likely rare in practice.

@T-Gro T-Gro added the AI-reviewed PR reviewed by AI review council label May 18, 2026
@auduchinok
Copy link
Copy Markdown
Member

auduchinok commented May 18, 2026

Overall this is a solid, well-scoped fix for #19604. The implementation is clean, the comment is helpful, the test coverage is good. A few observations:

I respectfully disagree 🙂

If we are to make something implicit like this, it has to be designed properly:

  • it should not be limited to extension members
  • it should also cover members in object and functional types
  • how would it handle static vs non-static members
  • how would it handle signatures vs implementations
  • how would it handle type augmentations vs extensions
  • how will it work when CompiledName is placed on a member hidden by a signature (i.e. absent in the fsi file)
  • how will it work for virtual members (i.e. abstract/default pairs)
  • how will it work when different compiled names are specified for different overloads

I propose we do not merge it like this and either accept it's by design or make a complete language feature guarded by a language version.

…sion overloads (#19604)

EmittedIL baselines demonstrate that [<CompiledName>] IS applied to the
annotated overload. The unannotated overload keeps the default
'Builder.MethodName' IL name, while the annotated one gets the exact
compiled name requested. This is correct by-design behavior - only the
single overload carrying the attribute should be renamed.

- CompiledNameAttribute06.fs: C#-style extension with one overload annotated
  [<CompiledName "UseCosmosDb">] - proves annotated overload becomes
  'UseCosmosDb' and the unannotated one stays 'Builder.UseCosmosDb'.
- CompiledNameAttribute07.fs: F#-style extension with [<CompiledName "Renamed">]
  on one overload - proves annotated overload becomes 'Renamed' while the
  other stays 'Builder.UseDb'.

Closes #19604

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@T-Gro T-Gro force-pushed the fix/19604-compiledname branch from 1b48b54 to 9d39162 Compare May 19, 2026 10:37
@T-Gro T-Gro changed the title Warn on inconsistent [<CompiledName>] across extension overloads Add EmittedIL tests proving [<CompiledName>] works by design on extension overloads (#19604) May 19, 2026
@T-Gro
Copy link
Copy Markdown
Member Author

T-Gro commented May 19, 2026

@auduchinok :

This is my bad, I initially trusted the behavior is indeed not working and not applying the changed name even to the first overload at all - what the issue indicated. But that is not the case, the tests now demonstrate the first overload is getting renamed correctly.

AI-workflow just maximized damage from that wrong initial assumption of mine (where that start should have been validation/better understanding).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AI-Auto-Resolve-CI Opt-in: LabelOps triages CI failures on this PR every 3h AI-Auto-Resolve-Conflicts Opt-in: LabelOps merges main into this PR and resolves conflicts every 3h AI-reviewed PR reviewed by AI review council AI-Tooling-Check-Bypassed Tooling check: non-fork PR, not diff-analyzed

Projects

Status: New

Development

Successfully merging this pull request may close these issues.

[<CompiledName>] attribute applied to only one overload of 2 is not applied at all

2 participants