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 d8c0a819a27..2d82952dcfb 100644 --- a/docs/release-notes/.FSharp.Compiler.Service/11.0.100.md +++ b/docs/release-notes/.FSharp.Compiler.Service/11.0.100.md @@ -95,6 +95,7 @@ * Fix nativeptr in interfaces leads to TypeLoadException. (Issue [#14508](https://github.com/dotnet/fsharp/issues/14508), [PR #19338](https://github.com/dotnet/fsharp/pull/19338)) * Fix box instruction for literal upcasts. (Issue [#18319](https://github.com/dotnet/fsharp/issues/18319), [PR #19338](https://github.com/dotnet/fsharp/pull/19338)) * Fix Decimal Literal causes InvalidProgramException in Debug builds. (Issue [#18956](https://github.com/dotnet/fsharp/issues/18956), [PR #19338](https://github.com/dotnet/fsharp/pull/19338)) +* Report a self identifier named `__` (as in `member __.M()`) to editor tooling. Only its uses were reported, not its declaration, so Rename rewrote the body of such a member and left `member __.` behind. The self identifier synthesized by the auto-property desugaring, which the name check was really aimed at, is now suppressed by its synthetic range instead. ([Issue #20566](https://github.com/dotnet/fsharp/issues/20566), [PR #20598](https://github.com/dotnet/fsharp/pull/20598)) * Fix `AttributeUsage.AllowMultiple` not being inherited for attributes subclassed in C#. ([Issue #17107](https://github.com/dotnet/fsharp/issues/17107), [PR #19315](https://github.com/dotnet/fsharp/pull/19315)) * Fix signature generation: recursive module `do` binding leaking compiler-generated val. ([Issue #13832](https://github.com/dotnet/fsharp/issues/13832), [PR #19586](https://github.com/dotnet/fsharp/pull/19586)) * Fix signature generation: literal values in attribute arguments now preserve literal identifier name. ([Issue #13810](https://github.com/dotnet/fsharp/issues/13810), [PR #19586](https://github.com/dotnet/fsharp/pull/19586)) diff --git a/docs/release-notes/.VisualStudio/18.vNext.md b/docs/release-notes/.VisualStudio/18.vNext.md index daf844623b1..e73cfd0464a 100644 --- a/docs/release-notes/.VisualStudio/18.vNext.md +++ b/docs/release-notes/.VisualStudio/18.vNext.md @@ -11,6 +11,7 @@ * Fixed Rename incorrectly renaming `get` and `set` keywords for properties with explicit accessors. ([Issue #18270](https://github.com/dotnet/fsharp/issues/18270), [PR #19252](https://github.com/dotnet/fsharp/pull/19252)) * Fixed Find All References crash when F# project contains non-F# files like `.cshtml`. ([Issue #16394](https://github.com/dotnet/fsharp/issues/16394), [PR #19252](https://github.com/dotnet/fsharp/pull/19252)) * Avoid using `cancellableTask` in `DocumentCache`; the editor cache now uses direct `CancellationToken`-aware `task` wrappers, avoiding the background `Task.Run` offload and a larger wrapper closure from the `cancellableTask` builder. ([Issue #20268](https://github.com/dotnet/fsharp/issues/20268)) +* Rename no longer accepts `_` as a new name for a binding that is used, which produced a file that no longer compiles because `_.M()` in expression position is the shorthand lambda rather than a reference. Rename also refuses to run at all when the declaration it would have to rewrite is missing from the locations it found. ([Issue #20597](https://github.com/dotnet/fsharp/issues/20597), [PR #20599](https://github.com/dotnet/fsharp/pull/20599)) * Cache document diagnostics by version stamp, so an unchanged document is not reanalyzed on every crawler pass. ([Issue #20120](https://github.com/dotnet/fsharp/issues/20120), [PR #20121](https://github.com/dotnet/fsharp/pull/20121)) * Find All References for external DLL symbols now only searches projects that reference the specific assembly. ([Issue #10227](https://github.com/dotnet/fsharp/issues/10227), [PR #19252](https://github.com/dotnet/fsharp/pull/19252)) * Improve static compilation of state machines. ([PR #19297](https://github.com/dotnet/fsharp/pull/19297)) diff --git a/src/Compiler/Checking/CheckDeclarations.fs b/src/Compiler/Checking/CheckDeclarations.fs index 76906d027b9..1fe016c8190 100644 --- a/src/Compiler/Checking/CheckDeclarations.fs +++ b/src/Compiler/Checking/CheckDeclarations.fs @@ -4841,7 +4841,7 @@ module TcDeclarations = // Only the keep the non-field-targeted attributes let attribs = attribs |> List.filter (fun a -> match a.Target with Some t when t.idText = "field" -> false | _ -> true) let fldId = ident (CompilerGeneratedName id.idText, mMemberPortion.MakeSynthetic()) - let headPatIds = if isStatic then [id] else [ident ("__", mMemberPortion);id] + let headPatIds = if isStatic then [id] else [ident ("__", mMemberPortion.MakeSynthetic());id] let headPat = SynPat.LongIdent (SynLongIdent(headPatIds, [], List.replicate headPatIds.Length None), None, Some noInferredTypars, SynArgPats.Pats [], None, mMemberPortion) let memberFlags = { memberFlags with GetterOrSetterIsCompilerGenerated = true } let memberFlagsForSet = { memberFlagsForSet with GetterOrSetterIsCompilerGenerated = true } diff --git a/src/Compiler/Checking/Expressions/CheckExpressions.fs b/src/Compiler/Checking/Expressions/CheckExpressions.fs index 3fd7e014332..f90bb60207b 100644 --- a/src/Compiler/Checking/Expressions/CheckExpressions.fs +++ b/src/Compiler/Checking/Expressions/CheckExpressions.fs @@ -1451,13 +1451,9 @@ let MakeAndPublishVal (cenv: cenv) env (altActualParent, inSig, declKind, valRec let shouldNotifySink (vspec: Val) = match vspec.MemberInfo with - // `this` reference named `__`. It's either: - // * generated by compiler for auto properties or - // * provided by source code (i.e. `member _.Method = ...`) - // We don't notify sink about it to prevent generating `FSharpSymbol` for it and appearing in completion list. - | None when - vspec.IsBaseVal || - vspec.IsMemberThisVal && vspec.LogicalName = "__" -> false + // visualfsharp#3699: the range of `base` spans the whole `inherit` expression, so reporting it + // makes its environment the most deeply nested one at the constructor arguments, shadowing theirs. + | None when vspec.IsBaseVal -> false | _ -> true match cenv.tcSink.CurrentSink with diff --git a/tests/FSharp.Compiler.Service.Tests/Symbols.fs b/tests/FSharp.Compiler.Service.Tests/Symbols.fs index 11c7a091226..5d416891f0a 100644 --- a/tests/FSharp.Compiler.Service.Tests/Symbols.fs +++ b/tests/FSharp.Compiler.Service.Tests/Symbols.fs @@ -689,6 +689,50 @@ type Foo = let backingFieldSymbols = allMfvs |> List.filter (fun mfv -> mfv.DisplayName.Contains("@")) Assert.True(backingFieldSymbols.IsEmpty, $"Compiler-generated backing field should not appear, but found: {backingFieldSymbols |> List.map (fun m -> m.DisplayName)}") + let private selfIdentifierRanges (checkResults: FSharpCheckFileResults) = + checkResults.GetAllUsesOfAllSymbolsInFile() + |> Seq.filter (fun su -> + match su.Symbol with + | :? FSharpMemberOrFunctionOrValue as mfv -> mfv.IsMemberThisValue + | _ -> false) + |> Seq.map _.Range + |> Seq.sortBy (fun m -> m.StartLine, m.StartColumn) + |> Seq.toList + + [] + [] + [] + [] + let ``Self identifier is reported at its declaration and at its uses`` (selfId: string) = + let _, checkResults = getParseAndCheckResults $""" +namespace Foo + +type Foo() = + member {selfId}.M() = {selfId}.N() + member {selfId}.N() = 1 +""" + let n = selfId.Length + + match selfIdentifierRanges checkResults with + | [ mDeclInM; mUseInM; mDeclInN ] -> + assertRange (5, 11) (5, 11 + n) mDeclInM + assertRange (5, 18 + n) (5, 18 + 2 * n) mUseInM + assertRange (6, 11) (6, 11 + n) mDeclInN + | ranges -> failwith $"Expected three self identifier symbol uses, got %A{ranges}" + + [] + let ``AutoProperty does not expose its compiler-generated self identifier`` () = + let _, checkResults = getParseAndCheckResults """ +namespace Foo + +type Foo = + member val AutoPropGetSet = 0 with get, set +""" + // The synthesized `__` sits on the property name's range, so reporting it would shadow the property + match selfIdentifierRanges checkResults with + | [] -> () + | ranges -> failwith $"Expected no self identifier symbol uses, got %A{ranges}" + [] let ``Property symbol is resolved for property`` () = let symbols = Checker.getSymbolUses """ diff --git a/vsintegration/src/FSharp.Editor/InlineRename/InlineRenameService.fs b/vsintegration/src/FSharp.Editor/InlineRename/InlineRenameService.fs index 8a5debfdc84..a7fa06f0a1d 100644 --- a/vsintegration/src/FSharp.Editor/InlineRename/InlineRenameService.fs +++ b/vsintegration/src/FSharp.Editor/InlineRename/InlineRenameService.fs @@ -60,6 +60,10 @@ type internal InlineRenameLocationSet let! newSolution = applyChanges replacementText originalSolution (locations |> Array.toList |> List.groupBy (fun x -> x.Document)) + // Bare `_` names a binding nothing refers to: in expression position `_.M()` is the shorthand lambda. + // Read before normalization strips the backticks of ``_``, which is a referenceable name. + let isBareUnderscore = replacementText = "_" + let replacementText = match symbolKind with | LexerSymbolKind.GenericTypeParameter @@ -69,6 +73,7 @@ type internal InlineRenameLocationSet let replacementTextValid = Tokenizer.isValidNameForSymbol (symbolKind, symbol, replacementText) + && not (isBareUnderscore && locations.Length > 1) let documentIds = locations |> Seq.map (fun doc -> doc.Document.Id) |> Seq.distinct return new InlineRenameReplacementInfo(newSolution, replacementTextValid, documentIds) :> FSharpInlineRenameReplacementInfo @@ -184,6 +189,15 @@ type internal InlineRenameService [] () = inherit FSharpInlineRenameServiceImplementation() + // Rewriting the uses without the declaration leaves a file that no longer compiles. + // The declaration of an active pattern case seen from a use is the whole `(|A|B|)`, so it must contain the use, not equal it. + static let declarationWouldBeRenamed (checkFileResults: FSharpCheckFileResults) (symbolUse: FSharpSymbolUse) ct = + match symbolUse.Symbol.DeclarationLocation with + | Some declRange when String.Equals(declRange.FileName, symbolUse.Range.FileName, StringComparison.Ordinal) -> + checkFileResults.GetUsesOfSymbolInFile(symbolUse.Symbol, cancellationToken = ct) + |> Array.exists (fun su -> Range.rangeContainsRange declRange su.Range) + | _ -> true + override _.GetRenameInfoAsync(document: Document, position: int, cancellationToken: CancellationToken) : Task = cancellableTask { let! ct = CancellableTask.getCancellationToken () @@ -211,6 +225,7 @@ type internal InlineRenameService [] () = match symbolUse with | None -> return Unchecked.defaultof<_> + | Some symbolUse when not (declarationWouldBeRenamed checkFileResults symbolUse ct) -> return Unchecked.defaultof<_> | Some symbolUse -> match RoslynHelpers.TryFSharpRangeToTextSpan(sourceText, symbolUse.Range) with | ValueNone -> return Unchecked.defaultof<_> diff --git a/vsintegration/tests/FSharp.Editor.Tests/FSharp.Editor.Tests.fsproj b/vsintegration/tests/FSharp.Editor.Tests/FSharp.Editor.Tests.fsproj index ecce1205b8c..b546ce52881 100644 --- a/vsintegration/tests/FSharp.Editor.Tests/FSharp.Editor.Tests.fsproj +++ b/vsintegration/tests/FSharp.Editor.Tests/FSharp.Editor.Tests.fsproj @@ -85,6 +85,7 @@ + @@ -94,6 +95,7 @@ + diff --git a/vsintegration/tests/FSharp.Editor.Tests/InlineRenameServiceTests.fs b/vsintegration/tests/FSharp.Editor.Tests/InlineRenameServiceTests.fs new file mode 100644 index 00000000000..be84a262c54 --- /dev/null +++ b/vsintegration/tests/FSharp.Editor.Tests/InlineRenameServiceTests.fs @@ -0,0 +1,97 @@ +// Copyright (c) Microsoft Corporation. All Rights Reserved. See License.txt in the project root for license information. + +namespace FSharp.Editor.Tests + +module InlineRenameServiceTests = + + open System + open System.Threading + open Xunit + open Microsoft.VisualStudio.FSharp.Editor + open FSharp.Editor.Tests.Helpers + + /// Drives the rename the way Visual Studio does: resolve the symbol under the caret, + /// collect what would be rewritten, then ask whether the new name is accepted. + let private rename (fileContents: string) (caretAt: string) (newName: string) = + let document = + RoslynTestHelpers.CreateSolution(fileContents) + |> RoslynTestHelpers.GetSingleDocument + + let position = fileContents.IndexOf(caretAt, StringComparison.Ordinal) + Assert.True(position >= 0, $"'{caretAt}' is not in the test source") + + let info = + InlineRenameService().GetRenameInfoAsync(document, position, CancellationToken.None).Result + + Assert.NotNull info + + let locationSet = + info.FindRenameLocationsAsync(Unchecked.defaultof<_>, Unchecked.defaultof<_>, CancellationToken.None).Result + + let replacements = + locationSet.GetReplacementsAsync(newName, CancellationToken.None).Result + + locationSet.Locations.Count, replacements.ReplacementTextValid + + let private selfIdentifierWithUse = + """ +type RenameTest() = + member this.TestMethod() = "Hello, World!" + member this.TestMethodThis() = this.TestMethod() +""" + + let private selfIdentifierWithoutUse = + """ +type RenameTest() = + member this.TestMethod() = "Hello, World!" +""" + + [] + [] + [] + [] + let ``A self identifier that is used is renamed only to a referenceable name`` (newName: string, isAccepted: bool) = + let locations, isValid = rename selfIdentifierWithUse "this.TestMethodThis" newName + Assert.Equal(2, locations) + Assert.Equal(isAccepted, isValid) + + [] + let ``Renaming a self identifier that is not used to _ is accepted`` () = + let locations, isValid = rename selfIdentifierWithoutUse "this.TestMethod" "_" + Assert.Equal(1, locations) + Assert.True isValid + + [] + [] + [">] + let ``An active pattern case is renamed from its declaration and from its uses`` (caretAt: string) = + let source = + """ +module M + +let (|Even|Odd|) n = if n % 2 = 0 then Even else Odd + +let f n = + match n with + | Even -> true + | Odd -> false +""" + + let locations, isValid = rename source caretAt "DivisibleByTwo" + Assert.True(locations >= 3, $"Expected the declaration and both uses, got {locations} locations") + Assert.True isValid + + [] + let ``A self identifier named __ renames its declaration together with its uses`` () = + let source = + """ +type RenameTest() = + member this.TestMethod() = "Hello, World!" + member __.TestMethodDoubleUnderscore() = __.TestMethod() +""" + + let fromDeclaration, _ = rename source "__.TestMethodDoubleUnderscore" "this" + Assert.Equal(2, fromDeclaration) + + let fromUse, _ = rename source "__.TestMethod()" "this" + Assert.Equal(2, fromUse)