Warn on inconsistent [<CompiledName>] across extension overloads#19737
Warn on inconsistent [<CompiledName>] across extension overloads#19737T-Gro wants to merge 3 commits into
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
❗ Release notes required
Warning No PR link found in some release notes, please consider adding it.
|
|
🔍 Tooling Safety Check — Bypassed (non-fork) (scanned-sha)abddf12aab2a6aab67b72f91fbc74da76228262f(/scanned-sha)
|
|
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. |
T-Gro
left a comment
There was a problem hiding this comment.
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.
I respectfully disagree 🙂 If we are to make something implicit like this, it has to be designed properly:
I propose we do not merge like this and either accept it's by design or make a complete language feature guarded by a language version. |
Fixes #19604
When
[<CompiledName>]is applied to only some overloads of an extension method, the attribute is silently ignored. This PR adds a warning for that case.