Conversation
❗ Release notes required
|
…d extension lookups (CE, Dispose, etc.)
3f7dd89 to
170baf1
Compare
|
It seems that my changes broke FSharp.Core. I suppose that we have to adjust type equality check to work with the core code. |
… the new test case
| intrinsic @ ExtensionMethInfosOfTypeInScope collectionSettings infoReader nenv optFilter LookupIsInstance.Ambivalent m ty | ||
| intrinsic @ ExtensionMethInfosOfTypeInScope collectionSettings infoReader nenv ad optFilter LookupIsInstance.Ambivalent m ty | ||
|
|
||
| let IsExtensionMethCompatibleWithTy g amap m (ty: TType) (minfo: MethInfo) = |
There was a problem hiding this comment.
I'm wondering how is it already done in other cases? Is there a place that this function could be unified with?
| | TyparConstraint.CoercesTo(targetCTy, _) -> | ||
| let cTy = targetCTy |> stripTyEqns g | ||
| TypeRelations.TypeFeasiblySubsumesType 0 g amap m cTy TypeRelations.CanCoerce ty2 | ||
| | _ -> false) |
There was a problem hiding this comment.
[Breaking Changes] When ty1 is a TType_var with no CoercesTo constraint, List.exists returns false and the extension method is silently excluded. This incorrectly filters out:
- C# extensions like
public static T Run<T>(this T x)(unconstrained genericthis) - F# extensions like
static member Run(this: 'T)with non-CoercesTo constraints (e.g.'T : struct) - Any extension where the
thisparameter is a type variable constrained only by member/struct/equality constraints
An unconstrained (or non-CoercesTo-constrained) type variable should match any target type. Consider:
| TType_var (tp1, _), _ ->
let coercesToConstraints =
tp1.Constraints |> List.choose (function
| TyparConstraint.CoercesTo(targetCTy, _) -> Some targetCTy
| _ -> None)
match coercesToConstraints with
| [] -> true // No CoercesTo constraint means it could match anything
| constraints ->
constraints |> List.exists (fun targetCTy ->
let cTy = targetCTy |> stripTyEqns g
TypeRelations.TypeFeasiblySubsumesType 0 g amap m cTy TypeRelations.CanCoerce ty2)| """ | ||
| |> asExe | ||
| |> compile | ||
| |> shouldFail |
There was a problem hiding this comment.
[Test Coverage] The existing tests all use #SomeType (which produces a CoercesTo constraint). Please add a test for a C# or F# extension with a truly unconstrained generic this parameter, e.g.:
[<Extension>]
type GenericExtensions =
[<Extension>]
static member inline Run(this: 'T) = thisThis would currently fail due to the TType_var / no-CoercesTo bug noted above.
T-Gro
left a comment
There was a problem hiding this comment.
⚠️ Automated Copilot Review — AI-assisted initial review. Feedback welcome.
[Breaking Changes] IsExtensionMethCompatibleWithTy has a logic gap in the TType_var branch: when the extension method's this parameter is a type variable with no CoercesTo constraint (unconstrained generic, or constrained only by struct/member/equality), the current code returns false and the method is silently excluded. This affects C# extensions like public static T Run<T>(this T x) and F# extensions with (this: 'T) or (this: 'T when 'T : struct). A type variable with no CoercesTo bounds should match any target type — the [] -> true fallback is missing.
[Test Coverage] All CE extension method tests use #SomeType (which always has a CoercesTo constraint). A test with a genuinely unconstrained 'T on the this parameter would catch the above bug.
Fixes #19349
-> Fix accessibility and type-matching for pattern-based extension lookups (CE, Dispose, etc.)
Summary
This PR fixes an issue where pattern-based extension method lookups (specifically for Computation Expressions and use bindings) would incorrectly resolve methods that are either inaccessible (private/internal) or belong to incompatible types.
Problem
Previously, the compiler's name resolution for CE-specific methods (like Bind, Delay, Run, etc.) and the Dispose pattern for resource cleanup was overly optimistic. It often bypassed accessibility checks, leading to incorrect method resolution or even compilation of calls to private extension methods from outside their scope.
Checklist