diff --git a/docs/release-notes/.FSharp.Compiler.Service/11.0.100.md b/docs/release-notes/.FSharp.Compiler.Service/11.0.100.md index 965957dbc0c..79288bc9f77 100644 --- a/docs/release-notes/.FSharp.Compiler.Service/11.0.100.md +++ b/docs/release-notes/.FSharp.Compiler.Service/11.0.100.md @@ -75,6 +75,7 @@ ([PR #19724](https://github.com/dotnet/fsharp/pull/19724)) * Emit debug points at a stack-empty position ([PR #19877](https://github.com/dotnet/fsharp/pull/19877)) * Fix spurious XmlDoc warnings (unknown parameter / no documentation for parameter) under `--warnon:3390` when a get/set property documents the full parameter set across both accessors. ([Issue #13684](https://github.com/dotnet/fsharp/issues/13684), [PR #19884](https://github.com/dotnet/fsharp/pull/19884)) +* Render the declaring type for C#-style extension methods in overload-resolution error messages so the suggestion no longer falsely attributes the extension to the receiver type. ([Issue #9838](https://github.com/dotnet/fsharp/issues/9838), [PR #19925](https://github.com/dotnet/fsharp/pull/19925)) ### Added diff --git a/src/Compiler/Checking/NicePrint.fs b/src/Compiler/Checking/NicePrint.fs index de8daffa7c5..da69f68e9e5 100644 --- a/src/Compiler/Checking/NicePrint.fs +++ b/src/Compiler/Checking/NicePrint.fs @@ -1698,7 +1698,7 @@ module InfoMemberPrinting = // That is, this style: // Container(argName1: argType1, ..., argNameN: argTypeN) : retType // Container.Method(argName1: argType1, ..., argNameN: argTypeN) : retType - let layoutMethInfoCSharpStyle amap m denv (minfo: MethInfo) minst = + let layoutMethInfoCSharpStyle useDeclaringTypeForCSharpExt amap m denv (minfo: MethInfo) minst = let retTy = if minfo.IsConstructor then minfo.ApparentEnclosingType else minfo.GetFSharpReturnType(amap, m, minst) let layout = if minfo.IsExtensionMember then @@ -1707,7 +1707,11 @@ module InfoMemberPrinting = let layout = layout ^^ if isAppTy minfo.TcGlobals minfo.ApparentEnclosingAppType then - let tcref = minfo.ApparentEnclosingTyconRef + let tcref = + if useDeclaringTypeForCSharpExt && minfo.IsCSharpStyleExtensionMember then + minfo.DeclaringTyconRef + else + minfo.ApparentEnclosingTyconRef PrintTypes.layoutTyconRef denv tcref else emptyL @@ -1781,7 +1785,7 @@ module InfoMemberPrinting = // // For C# extension members: // ApparentContainer.Method(argName1: argType1, ..., argNameN: argTypeN) : retType - let rec prettyLayoutOfMethInfoFreeStyle (infoReader: InfoReader) m denv typarInst methInfo = + let rec prettyLayoutOfMethInfoFreeStyle useDeclaringTypeForCSharpExt (infoReader: InfoReader) m denv typarInst methInfo = let amap = infoReader.amap match methInfo with @@ -1795,17 +1799,17 @@ module InfoMemberPrinting = | MethInfoWithModifiedReturnType(ILMeth(_, ilminfo, _) as wrappedInfo,retTy) -> let prettyTyparInst, prettyMethInfo, minst = prettifyILMethInfo amap m wrappedInfo typarInst ilminfo let prettyMethInfo = MethInfoWithModifiedReturnType(prettyMethInfo,retTy) - let resL = layoutMethInfoCSharpStyle amap m denv prettyMethInfo minst + let resL = layoutMethInfoCSharpStyle useDeclaringTypeForCSharpExt amap m denv prettyMethInfo minst prettyTyparInst, resL - | MethInfoWithModifiedReturnType(mi,_) -> prettyLayoutOfMethInfoFreeStyle infoReader m denv typarInst mi + | MethInfoWithModifiedReturnType(mi,_) -> prettyLayoutOfMethInfoFreeStyle useDeclaringTypeForCSharpExt infoReader m denv typarInst mi | ILMeth(_, ilminfo, _) -> let prettyTyparInst, prettyMethInfo, minst = prettifyILMethInfo amap m methInfo typarInst ilminfo - let resL = layoutMethInfoCSharpStyle amap m denv prettyMethInfo minst + let resL = layoutMethInfoCSharpStyle useDeclaringTypeForCSharpExt amap m denv prettyMethInfo minst prettyTyparInst, resL #if !NO_TYPEPROVIDERS | ProvidedMeth _ -> let prettyTyparInst, _ = PrettyTypes.PrettifyInst amap.g typarInst - prettyTyparInst, layoutMethInfoCSharpStyle amap m denv methInfo methInfo.FormalMethodInst + prettyTyparInst, layoutMethInfoCSharpStyle useDeclaringTypeForCSharpExt amap m denv methInfo methInfo.FormalMethodInst #endif let prettyLayoutOfPropInfoFreeStyle g amap m denv (pinfo: PropInfo) = @@ -1841,8 +1845,8 @@ module InfoMemberPrinting = let resL = prettyLayoutOfPropInfoFreeStyle g amap m denv pinfo resL |> bufferL os - let formatMethInfoToBufferFreeStyle amap m denv os (minfo: MethInfo) = - let _, resL = prettyLayoutOfMethInfoFreeStyle amap m denv emptyTyparInst minfo + let formatMethInfoToBufferFreeStyle useDeclaringTypeForCSharpExt amap m denv os (minfo: MethInfo) = + let _, resL = prettyLayoutOfMethInfoFreeStyle useDeclaringTypeForCSharpExt amap m denv emptyTyparInst minfo resL |> bufferL os /// Format a method to a layout (actually just containing a string) using "free style" (aka "standalone"). @@ -2866,10 +2870,10 @@ let stringOfQualifiedValOrMember denv infoReader vref = /// Convert a MethInfo to a string let formatMethInfoToBufferFreeStyle infoReader m denv buf d = - InfoMemberPrinting.formatMethInfoToBufferFreeStyle infoReader m denv buf d + InfoMemberPrinting.formatMethInfoToBufferFreeStyle false infoReader m denv buf d let prettyLayoutOfMethInfoFreeStyle infoReader m denv typarInst minfo = - InfoMemberPrinting.prettyLayoutOfMethInfoFreeStyle infoReader m denv typarInst minfo + InfoMemberPrinting.prettyLayoutOfMethInfoFreeStyle false infoReader m denv typarInst minfo /// Convert a PropInfo to a string let prettyLayoutOfPropInfoFreeStyle g amap m denv d = @@ -2877,7 +2881,14 @@ let prettyLayoutOfPropInfoFreeStyle g amap m denv d = /// Convert a MethInfo to a string let stringOfMethInfo infoReader m denv minfo = - buildString (fun buf -> InfoMemberPrinting.formatMethInfoToBufferFreeStyle infoReader m denv buf minfo) + buildString (fun buf -> InfoMemberPrinting.formatMethInfoToBufferFreeStyle false infoReader m denv buf minfo) + +/// Convert a MethInfo to a string, suitable for the "Available overloads" list +/// in overload-resolution error messages. For C#-style extension methods, the +/// rendering uses the extension's declaring type rather than the receiver type, +/// so the message is not misleading (issue dotnet/fsharp#9838). +let stringOfMethInfoForOverloadError infoReader m denv minfo = + buildString (fun buf -> InfoMemberPrinting.formatMethInfoToBufferFreeStyle true infoReader m denv buf minfo) let stringOfMethInfoFSharpStyle infoReader m denv minfo = InfoMemberPrinting.layoutMethInfoFSharpStyle infoReader m denv minfo diff --git a/src/Compiler/Checking/NicePrint.fsi b/src/Compiler/Checking/NicePrint.fsi index 8b90e66ce92..ff55f6cbb03 100644 --- a/src/Compiler/Checking/NicePrint.fsi +++ b/src/Compiler/Checking/NicePrint.fsi @@ -81,6 +81,13 @@ val prettyLayoutOfPropInfoFreeStyle: val stringOfMethInfo: infoReader: InfoReader -> m: range -> denv: DisplayEnv -> minfo: MethInfo -> string +/// Convert a MethInfo to a string, suitable for the "Available overloads" list +/// in overload-resolution error messages. For C#-style extension methods, the +/// rendering uses the extension's declaring type rather than the receiver type, +/// so the message is not misleading (issue dotnet/fsharp#9838). +val stringOfMethInfoForOverloadError: + infoReader: InfoReader -> m: range -> denv: DisplayEnv -> minfo: MethInfo -> string + /// Convert a MethInfo to a F# signature val stringOfMethInfoFSharpStyle: infoReader: InfoReader -> m: range -> denv: DisplayEnv -> minfo: MethInfo -> string diff --git a/src/Compiler/Driver/CompilerDiagnostics.fs b/src/Compiler/Driver/CompilerDiagnostics.fs index 4ecbfc081ef..86dafc5c4c0 100644 --- a/src/Compiler/Driver/CompilerDiagnostics.fs +++ b/src/Compiler/Driver/CompilerDiagnostics.fs @@ -960,7 +960,7 @@ type Exception with sprintf " // %s" nameOrOneBasedIndexMessage | _ -> "" - (NicePrint.stringOfMethInfo x.infoReader m displayEnv x.methodSlot.Method) + (NicePrint.stringOfMethInfoForOverloadError x.infoReader m displayEnv x.methodSlot.Method) + paramInfo let nl = Environment.NewLine diff --git a/tests/FSharp.Compiler.ComponentTests/ErrorMessages/Issue9838Tests.fs b/tests/FSharp.Compiler.ComponentTests/ErrorMessages/Issue9838Tests.fs new file mode 100644 index 00000000000..d6e83bd2ead --- /dev/null +++ b/tests/FSharp.Compiler.ComponentTests/ErrorMessages/Issue9838Tests.fs @@ -0,0 +1,167 @@ +module ErrorMessages.Issue9838Tests + +open Xunit +open FSharp.Test +open FSharp.Test.Compiler + +// https://github.com/dotnet/fsharp/issues/9838 +// When an F# program calls a C#-style extension method on the wrong receiver type, +// the overload-error message should render the extension's declaring type +// (e.g. `MemoryExtensions`), not the receiver type (e.g. `Foo`). +// +// The Span / MemoryExtensions APIs used by the RED tests below ship in the BCL on +// .NET (Core), but on .NET Framework (net48) they live in the optional System.Memory +// NuGet package which the test sandbox does not reference. We therefore gate those +// tests on NETCOREAPP so they exercise the actual overload-resolution diagnostic +// rather than failing with FS0039 "The value or constructor 'Span' is not defined". + +[] +let ``Issue 9838 - C#-style extension method overload error shows declaring type, not receiver`` () = + FSharp """ +open System +type Foo() = class end +let foo = Foo() +let span = Span.Empty +foo.CopyTo(span) +""" + |> typecheck + |> shouldFail + |> withDiagnostics + [ (Error 41, Line 6, Col 5, Line 6, Col 11, + "No overloads match for method 'CopyTo'. + +Known type of argument: Span + +Available overloads: + - (extension) MemoryExtensions.CopyTo<'T>(destination: Memory<'T>) : unit + - (extension) MemoryExtensions.CopyTo<'T>(destination: Span<'T>) : unit") ] + +[] +let ``Issue 9838 - error message mentions declaring type MemoryExtensions`` () = + FSharp """ +open System +type Foo() = class end +let foo = Foo() +let span = Span.Empty +foo.CopyTo(span) +""" + |> typecheck + |> shouldFail + |> withDiagnosticMessageMatches "\\(extension\\) MemoryExtensions\\.CopyTo" + +[] +let ``Issue 9838 - error message must not mislead by mentioning Foo.CopyTo`` () = + let result = + FSharp """ +open System +type Foo() = class end +let foo = Foo() +let span = Span.Empty +foo.CopyTo(span) +""" + |> typecheck + + result |> shouldFail |> ignore + + let allMessages = + result.Output.Diagnostics + |> List.map (fun d -> d.Message) + |> String.concat "\n" + + Assert.DoesNotContain("Foo.CopyTo", allMessages) + Assert.Contains("MemoryExtensions.CopyTo", allMessages) + +[] +let ``F#-style extension member overload error keeps current rendering`` () = + FSharp """ +type Foo() = class end + +module FooExtensions = + type Foo with + member _.Bar(_: int) = () + member _.Bar(_: double) = () + +open FooExtensions + +let foo = Foo() +foo.Bar("") +""" + |> typecheck + |> shouldFail + |> withDiagnosticMessageMatches "member Foo\\.Bar" + +[] +let ``Regular instance method overload error is unaffected by issue 9838 fix`` () = + FSharp """ +type T() = + member _.M(_: int) = () + member _.M(_: double) = () + +let t = T() +t.M("") +""" + |> typecheck + |> shouldFail + |> withDiagnostics + [ (Error 41, Line 7, Col 3, Line 7, Col 4, + "No overloads match for method 'M'. + +Known type of argument: string + +Available overloads: + - member T.M: double -> unit // Argument at index 1 doesn't match + - member T.M: int -> unit // Argument at index 1 doesn't match") ] + +[] +let ``Issue 9838 - C#-style extension on generic receiver mismatch shows declaring type`` () = + FSharp """ +open System +open System.Collections.Generic +let xs = List() +let span = Span.Empty +xs.CopyTo(span) +""" + |> typecheck + |> shouldFail + |> withDiagnosticMessageMatches "\\(extension\\) MemoryExtensions\\.CopyTo" + +(* + * Captured RED-phase failure output (net10.0) - kept as a reference for the + * implementation sprint. Each RED test asserts the desired post-fix wording + * (`(extension) MemoryExtensions.CopyTo<'T>(...)`) and currently FAILS because + * the compiler renders the *receiver* type instead of the extension's + * declaring type. Once the fix lands the actual text below should change to + * match the expected text and the RED tests will go green. + * + * Test: Issue 9838 - C#-style extension method overload error shows declaring type, not receiver + * Expected (post-fix): + * "No overloads match for method 'CopyTo'. + * Known type of argument: Span + * Available overloads: + * - (extension) MemoryExtensions.CopyTo<'T>(destination: Memory<'T>) : unit // Argument 'destination' doesn't match + * - (extension) MemoryExtensions.CopyTo<'T>(destination: Span<'T>) : unit // Argument 'destination' doesn't match" + * Actual (current bug): + * "No overloads match for method 'CopyTo'. + * Known type of argument: Span + * Available overloads: + * - (extension) Foo.CopyTo<'T>(destination: Memory<'T>) : unit + * - (extension) Foo.CopyTo<'T>(destination: Span<'T>) : unit" + * + * Test: Issue 9838 - error message mentions declaring type MemoryExtensions + * regex "\(extension\) MemoryExtensions\.CopyTo" does NOT match actual text + * "(extension) Foo.CopyTo<'T>(destination: ...)". + * + * Test: Issue 9838 - error message must not mislead by mentioning Foo.CopyTo + * Assert.DoesNotContain("Foo.CopyTo", ...) fails because the diagnostic + * contains "(extension) Foo.CopyTo<'T>(destination: Memory<'T>)". + * + * Test: Issue 9838 - C#-style extension on generic receiver mismatch shows declaring type + * Expected regex "\(extension\) MemoryExtensions\.CopyTo" against actual: + * "(extension) List.CopyTo<'T>(destination: Span<'T>) : unit ..." - the + * extensions are attributed to the receiver `List` rather than to + * `MemoryExtensions`. + * + * GATE tests (F#-style extension and regular instance method) PASS today and + * must continue to pass after the fix, guaranteeing the fix is scoped to + * C#-style (`[]`) extension overload rendering only. + *) diff --git a/tests/FSharp.Compiler.ComponentTests/FSharp.Compiler.ComponentTests.fsproj b/tests/FSharp.Compiler.ComponentTests/FSharp.Compiler.ComponentTests.fsproj index 954c02b8aca..ee8f2925a6e 100644 --- a/tests/FSharp.Compiler.ComponentTests/FSharp.Compiler.ComponentTests.fsproj +++ b/tests/FSharp.Compiler.ComponentTests/FSharp.Compiler.ComponentTests.fsproj @@ -314,6 +314,7 @@ +