Reject _ as a rename target for a binding that is used

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))

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))

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 }

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

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
+
+    [<Theory>]
+    [<InlineData "this">]
+    [<InlineData "self">]
+    [<InlineData "__">]
+    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}"
+
+    [<Fact>]
+    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}"
+
     [<Fact>]
     let ``Property symbol is resolved for property`` () =
         let symbols = Checker.getSymbolUses """

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 [<ImportingConstructor>] () =
 
     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<FSharpInlineRenameInfo> =
         cancellableTask {
             let! ct = CancellableTask.getCancellationToken ()
@@ -211,6 +225,7 @@ type internal InlineRenameService [<ImportingConstructor>] () =
 
                 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<_>

vsintegration/tests/FSharp.Editor.Tests/FSharp.Editor.Tests.fsproj

@@ -85,6 +85,7 @@
     <Compile Include="SemanticClassificationServiceTests.fs" />
     <Compile Include="SyntacticColorizationServiceTests.fs" />
     <Compile Include="DocumentHighlightsServiceTests.fs" />
+    <Compile Include="InlineRenameServiceTests.fs" />
   </ItemGroup>
 
   <ItemGroup>
@@ -94,6 +95,7 @@
   <ItemGroup>
 	<PackageReference Include="Microsoft.CodeAnalysis.Workspaces.Common" />
 	<PackageReference Include="Microsoft.CodeAnalysis.CSharp" />
+	<PackageReference Include="Microsoft.CodeAnalysis.EditorFeatures" />
 
 	<PackageReference Include="Microsoft.VisualStudio.LanguageServices.ExternalAccess" />
     <PackageReference Include="Microsoft.VisualStudio.Platform.VSEditor" />

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!"
+"""
+
+    [<Theory>]
+    [<InlineData("_", false)>]
+    [<InlineData("``_``", true)>]
+    [<InlineData("self", true)>]
+    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)
+
+    [<Fact>]
+    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
+
+    [<Theory>]
+    [<InlineData "Even|Odd">]
+    [<InlineData "Even ->">]
+    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
+
+    [<Fact>]
+    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)