Fix #14375 by showing and inserting correct name of entities from unopened namespace/module

docs/release-notes/.FSharp.Compiler.Service/8.0.400.md

@@ -30,3 +30,4 @@
 * AsyncLocal diagnostics context. ([PR #16779](https://github.com/dotnet/fsharp/pull/16779))
 * Reduce allocations in compiler checking via `ValueOption` usage ([PR #16822](https://github.com/dotnet/fsharp/pull/16822))
 * Use AsyncLocal instead of ThreadStatic to hold Cancellable.Token ([PR #17156](https://github.com/dotnet/fsharp/pull/17156))
+* Showing and inserting correct name of entities from unopened namespace/module ([Issue #14375](https://github.com/dotnet/fsharp/issues/14375), [PR #17261](https://github.com/dotnet/fsharp/pull/17261))

docs/release-notes/.VisualStudio/17.11.md

@@ -5,4 +5,5 @@
 ### Changed
 
 * Use AsyncLocal diagnostics context. ([PR #16779](https://github.com/dotnet/fsharp/pull/16779))
-* Add Custom Visualizer support for F# in Visual Studio 2022 ([Issue #361](https://github.com/microsoft/VSExtensibility/issues/361), [PR #17239](https://github.com/dotnet/fsharp/pull/17239)).
+* Add Custom Visualizer support for F# in Visual Studio 2022 ([Issue #361](https://github.com/microsoft/VSExtensibility/issues/361), [PR #17239](https://github.com/dotnet/fsharp/pull/17239)).
+* Do not insert duplicated opens for completions from "Unopened namespaces". Can filter completions from "Unopened namespaces" by class/module name. ([PR #17261](https://github.com/dotnet/fsharp/pull/17261))

src/Compiler/Service/ServiceAssemblyContent.fs

@@ -13,6 +13,19 @@ open Internal.Utilities.Library
 open FSharp.Compiler.Diagnostics
 open FSharp.Compiler.IO
 open FSharp.Compiler.Symbols
+open FSharp.Compiler.Syntax
+
+module Utils =
+    let replaceLastIdentToDisplayName idents (displayName: string) =
+        match idents |> Array.tryFindIndexBack (fun i -> displayName.StartsWith(i, System.StringComparison.Ordinal)) with
+        | Some x when x = idents.Length - 1 -> idents |> Array.replace (idents.Length - 1) displayName
+        | Some x -> 
+            let newIdents = Array.zeroCreate (x + 1)
+            Array.Copy(idents, newIdents, x)
+            newIdents[x] <- displayName
+            newIdents
+        | _ -> idents
+
 
 type IsAutoOpen = bool
 
@@ -80,10 +93,9 @@ type Parent =
                 else ident)
 
         let removeModuleSuffix (idents: ShortIdents) =
-            if entity.IsFSharpModule && idents.Length > 0 then
+            if (entity.IsFSharpModule || PrettyNaming.DoesIdentifierNeedBackticks entity.DisplayName) && idents.Length > 0 then
                 let lastIdent = idents[idents.Length - 1]
-                if lastIdent <> entity.DisplayName then
-                    idents |> Array.replace (idents.Length - 1) entity.DisplayName
+                if lastIdent <> entity.DisplayName then Utils.replaceLastIdentToDisplayName idents entity.DisplayName
                 else idents
             else idents
 
@@ -108,19 +120,22 @@ type IAssemblyContentCache =
 
 module AssemblyContent =
 
-    let UnresolvedSymbol (topRequireQualifiedAccessParent: ShortIdents option) (cleanedIdents: ShortIdents) (fullName: string) =
+    let UnresolvedSymbol (topRequireQualifiedAccessParent: ShortIdents option) (cleanedIdents: ShortIdents) (fullName: string) ns =
         let getNamespace (idents: ShortIdents) = 
             if idents.Length > 1 then Some idents[..idents.Length - 2] else None
 
+        // 1. get namespace/module to open from topRequireQualifiedAccessParent 
+        // 2. if the topRequireQualifiedAccessParent is None, use the namespace, as we don't know whether an ident is namespace/module or not
         let ns = 
             topRequireQualifiedAccessParent 
-            |> Option.bind getNamespace 
-            |> Option.orElseWith (fun () -> getNamespace cleanedIdents)
-            |> Option.defaultValue [||]
-
+            |> Option.bind getNamespace
+            |> Option.orElse ns
+            |> Option.defaultWith (fun _ -> Array.empty)
+            |> Array.map PrettyNaming.NormalizeIdentifierBackticks
+
         let displayName = 
             let nameIdents = if cleanedIdents.Length > ns.Length then cleanedIdents |> Array.skip ns.Length else cleanedIdents
-            nameIdents |> String.concat "."
+            nameIdents |> Array.map PrettyNaming.NormalizeIdentifierBackticks |> String.concat "."
 
         { FullName = fullName
           DisplayName = displayName
@@ -130,6 +145,7 @@ module AssemblyContent =
         parent.FormatEntityFullName entity
         |> Option.map (fun (fullName, cleanIdents) ->
             let topRequireQualifiedAccessParent = parent.TopRequiresQualifiedAccess false |> Option.map parent.FixParentModuleSuffix
+
             { FullName = fullName
               CleanedIdents = cleanIdents
               Namespace = ns
@@ -149,7 +165,7 @@ module AssemblyContent =
                     match entity with
                     | FSharpSymbolPatterns.Attribute -> EntityKind.Attribute 
                     | _ -> EntityKind.Type
-              UnresolvedSymbol = UnresolvedSymbol topRequireQualifiedAccessParent cleanIdents fullName
+              UnresolvedSymbol = UnresolvedSymbol topRequireQualifiedAccessParent cleanIdents fullName ns
             })
 
     let traverseMemberFunctionAndValues ns (parent: Parent) (membersFunctionsAndValues: seq<FSharpMemberOrFunctionOrValue>) =
@@ -168,10 +184,18 @@ module AssemblyContent =
                   AutoOpenParent = autoOpenParent
                   Symbol = func
                   Kind = fun _ -> EntityKind.FunctionOrValue func.IsActivePattern
-                  UnresolvedSymbol = UnresolvedSymbol topRequireQualifiedAccessParent cleanedIdents fullName }
+                  UnresolvedSymbol = UnresolvedSymbol topRequireQualifiedAccessParent cleanedIdents fullName ns }
 
             [ yield! func.TryGetFullDisplayName() 
-                     |> Option.map (fun fullDisplayName -> processIdents func.FullName (fullDisplayName.Split '.'))
+                     |> Option.map (fun fullDisplayName -> 
+                        let idents = (fullDisplayName.Split '.')
+                        let lastIdent = idents[idents.Length - 1]
+                        let idents =
+                            match Option.attempt (fun _ -> func.DisplayName) with
+                            | Some shortDisplayName when PrettyNaming.DoesIdentifierNeedBackticks shortDisplayName && lastIdent <> shortDisplayName ->
+                                Utils.replaceLastIdentToDisplayName idents shortDisplayName
+                            | _ -> idents
+                        processIdents func.FullName idents)
                      |> Option.toList
               (* for 
                  [<CompilationRepresentation (CompilationRepresentationFlags.ModuleSuffix)>]

tests/FSharp.Compiler.Service.Tests/AssemblyContentProviderTests.fs

@@ -142,3 +142,88 @@ let ``TopRequireQualifiedAccessParent property should be valid``() =
     let actual = source |> getSymbolMap getTopRequireQualifiedAccessParentName
 
     assertAreEqual (expectedResult, actual)
+
+
+[<Fact>]
+let ``Check Unresolved Symbols``() =
+    let source = """
+namespace ``1 2 3``
+
+module Test =
+    module M1 = 
+        let v1 = 1
+
+        module M11 = 
+            let v11 = 1
+
+            module M111 = 
+                let v111 = 1
+
+        [<RequireQualifiedAccess>]
+        module M12 = 
+            let v12 = 1
+
+            module M121 = 
+                let v121 = 1
+
+                [<RequireQualifiedAccess>]
+                module M1211 = 
+                    let v1211 = 1
+
+        type A = 
+            static member val B = 0
+            static member C() = ()
+            static member (++) s s2 = s + "/" + s2
+
+        type B =
+            abstract D: int -> int
+
+        let ``a.b.c`` = "999"
+
+        type E = { x: int; y: int }
+        type F =
+            | A = 1
+            | B = 2
+        type G =
+            | A of int
+            | B of string
+
+        let (|Is1|_|) x = x = 1
+        let (++) s s2 = s + "/" + s2
+    """
+
+    let expectedResult = 
+        [ 
+            "1 2 3.Test", "open ``1 2 3`` - Test";
+            "1 2 3.Test.M1", "open ``1 2 3`` - Test.M1";
+            "1 2 3.Test.M1.(++)", "open ``1 2 3`` - Test.M1.``(++)``";
+            "1 2 3.Test.M1.A", "open ``1 2 3`` - Test.M1.A";
+            "1 2 3.Test.M1.A.(++)", "open ``1 2 3`` - Test.M1.A.``(++)``";
+            "1 2 3.Test.M1.A.B", "open ``1 2 3`` - Test.M1.A.B";
+            "1 2 3.Test.M1.A.C", "open ``1 2 3`` - Test.M1.A.C";
+            "1 2 3.Test.M1.A.op_PlusPlus", "open ``1 2 3`` - Test.M1.A.op_PlusPlus";
+            "1 2 3.Test.M1.B", "open ``1 2 3`` - Test.M1.B";
+            "1 2 3.Test.M1.E", "open ``1 2 3`` - Test.M1.E";
+            "1 2 3.Test.M1.F", "open ``1 2 3`` - Test.M1.F";
+            "1 2 3.Test.M1.G", "open ``1 2 3`` - Test.M1.G";
+            "1 2 3.Test.M1.M11", "open ``1 2 3`` - Test.M1.M11";
+            "1 2 3.Test.M1.M11.M111", "open ``1 2 3`` - Test.M1.M11.M111";
+            "1 2 3.Test.M1.M11.M111.v111", "open ``1 2 3`` - Test.M1.M11.M111.v111";
+            "1 2 3.Test.M1.M11.v11", "open ``1 2 3`` - Test.M1.M11.v11";
+            "1 2 3.Test.M1.M12", "open ``1 2 3`` - Test.M1.M12";
+            "1 2 3.Test.M1.M12.M121", "open ``1 2 3``.Test.M1 - M12.M121";
+            "1 2 3.Test.M1.M12.M121.M1211", "open ``1 2 3``.Test.M1 - M12.M121.M1211";
+            "1 2 3.Test.M1.M12.M121.M1211.v1211", "open ``1 2 3``.Test.M1 - M12.M121.M1211.v1211";
+            "1 2 3.Test.M1.M12.M121.v121", "open ``1 2 3``.Test.M1 - M12.M121.v121";
+            "1 2 3.Test.M1.M12.v12", "open ``1 2 3``.Test.M1 - M12.v12";
+            "1 2 3.Test.M1.``a.b.c``", "open ``1 2 3`` - Test.M1.``a.b.c``";
+            "1 2 3.Test.M1.op_PlusPlus", "open ``1 2 3`` - Test.M1.op_PlusPlus";
+            "1 2 3.Test.M1.v1", "open ``1 2 3`` - Test.M1.v1";
+        ]
+        |> Map.ofList
+
+    let actual = source |> getSymbolMap (fun i -> 
+        let ns = i.UnresolvedSymbol.Namespace |> String.concat "."
+        $"open {ns} - {i.UnresolvedSymbol.DisplayName}")
+
+    assertAreEqual (expectedResult, actual)

vsintegration/src/FSharp.Editor/Completion/CompletionProvider.fs

@@ -235,14 +235,10 @@ type internal FSharpCompletionProvider
                 let glyph =
                     Tokenizer.FSharpGlyphToRoslynGlyph(declarationItem.Glyph, declarationItem.Accessibility)
 
-                let namespaceName, filterText =
-                    match declarationItem.NamespaceToOpen, declarationItem.NameInList.Split '.' with
-                    // There is no namespace to open and the item name does not contain dots, so we don't need to pass special FilterText to Roslyn.
-                    | None, [| _ |] -> null, null
-                    | Some namespaceToOpen, idents -> namespaceToOpen, Array.last idents
-                    // Either we have a namespace to open ("DateTime (open System)") or item name contains dots ("Array.map"), or both.
-                    // We are passing last part of long ident as FilterText.
-                    | None, idents -> null, Array.last idents
+                let namespaceName =
+                    match declarationItem.NamespaceToOpen with
+                    | None -> null
+                    | Some namespaceToOpen -> namespaceToOpen
 
                 let completionItem =
                     FSharpCommonCompletionItem
@@ -251,7 +247,7 @@ type internal FSharpCompletionProvider
                             null,
                             rules = noCommitOnSpaceRules,
                             glyph = Nullable glyph,
-                            filterText = filterText,
+                            filterText = declarationItem.NameInList,
                             inlineDescription = namespaceName
                         )
                         .AddProperty(FullNamePropName, declarationItem.FullName)
@@ -434,40 +430,64 @@ type internal FSharpCompletionProvider
             | true, ns ->
                 let! sourceText = document.GetTextAsync(cancellationToken)
 
-                let textWithItemCommitted =
-                    sourceText.WithChanges(TextChange(item.Span, nameInCode))
+                let! _, checkFileResults = document.GetFSharpParseAndCheckResultsAsync("ProvideCompletionsAsyncAux")
+
+                let completionInsertRange =
+                    RoslynHelpers.TextSpanToFSharpRange(document.FilePath, item.Span, sourceText)
+
+                let isNamespaceOrModuleInserted =
+                    checkFileResults.OpenDeclarations
+                    |> Array.exists (fun i ->
+                        Range.rangeContainsPos i.AppliedScope completionInsertRange.Start
+                        && i.Modules
+                           |> List.distinct
+                           |> List.exists (fun i ->
+                               (i.IsNamespace || i.IsFSharpModule)
+                               && match i.Namespace with
+                                  | Some x -> $"{x}.{i.DisplayName}" = ns
+                                  | _ -> i.DisplayName = ns))
+
+                if isNamespaceOrModuleInserted then
+                    return CompletionChange.Create(TextChange(item.Span, nameInCode))
+                else
+                    let textWithItemCommitted =
+                        sourceText.WithChanges(TextChange(item.Span, nameInCode))
 
-                let line = sourceText.Lines.GetLineFromPosition(item.Span.Start)
+                    let line = sourceText.Lines.GetLineFromPosition(item.Span.Start)
 
-                let! parseResults = document.GetFSharpParseResultsAsync(nameof (FSharpCompletionProvider))
+                    let! parseResults = document.GetFSharpParseResultsAsync(nameof (FSharpCompletionProvider))
 
-                let fullNameIdents =
-                    fullName
-                    |> ValueOption.map (fun x -> x.Split '.')
-                    |> ValueOption.defaultValue [||]
+                    let fullNameIdents =
+                        fullName
+                        |> ValueOption.map (fun x -> x.Split '.')
+                        |> ValueOption.defaultValue [||]
 
-                let insertionPoint =
-                    if settings.CodeFixes.AlwaysPlaceOpensAtTopLevel then
-                        OpenStatementInsertionPoint.TopLevel
-                    else
-                        OpenStatementInsertionPoint.Nearest
+                    let insertionPoint =
+                        if settings.CodeFixes.AlwaysPlaceOpensAtTopLevel then
+                            OpenStatementInsertionPoint.TopLevel
+                        else
+                            OpenStatementInsertionPoint.Nearest
 
-                let ctx =
-                    ParsedInput.FindNearestPointToInsertOpenDeclaration line.LineNumber parseResults.ParseTree fullNameIdents insertionPoint
+                    let ctx =
+                        ParsedInput.FindNearestPointToInsertOpenDeclaration
+                            line.LineNumber
+                            parseResults.ParseTree
+                            fullNameIdents
+                            insertionPoint
 
-                let finalSourceText, changedSpanStartPos =
-                    OpenDeclarationHelper.insertOpenDeclaration textWithItemCommitted ctx ns
+                    let finalSourceText, changedSpanStartPos =
+                        OpenDeclarationHelper.insertOpenDeclaration textWithItemCommitted ctx ns
 
-                let fullChangingSpan = TextSpan.FromBounds(changedSpanStartPos, item.Span.End)
+                    let fullChangingSpan = TextSpan.FromBounds(changedSpanStartPos, item.Span.End)
 
-                let changedSpan =
-                    TextSpan.FromBounds(changedSpanStartPos, item.Span.End + (finalSourceText.Length - sourceText.Length))
+                    let changedSpan =
+                        TextSpan.FromBounds(changedSpanStartPos, item.Span.End + (finalSourceText.Length - sourceText.Length))
 
-                let changedText = finalSourceText.ToString(changedSpan)
+                    let changedText = finalSourceText.ToString(changedSpan)
 
-                return
-                    CompletionChange
-                        .Create(TextChange(fullChangingSpan, changedText))
-                        .WithNewPosition(Nullable(changedSpan.End))
+                    return
+                        CompletionChange
+                            .Create(TextChange(fullChangingSpan, changedText))
+                            .WithNewPosition(Nullable(changedSpan.End))
         }
         |> CancellableTask.start cancellationToken