Offer every open that resolves the name, and open type where a plain open cannot reach

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

@@ -1,6 +1,8 @@
 ### Fixed
 
 * Fix internal error FS0192 "Iterate2D" when a `[<ReflectedDefinition(true)>]` parameter auto-quotes an argument that captures a not-yet-generalized use of an inferred generically-recursive function. The auto-quoted (`Expr.WithValue`) copy now keeps a fresh link to the recursive-value use so it receives the same inferred type arguments as the executable expression at the letrec point. ([Issue #20379](https://github.com/dotnet/fsharp/issues/20379))
+* `ParsedInput.FindNearestPointToInsertOpenDeclaration` now reports where an `open` belongs from the syntax tree instead of leaving consumers to recognize a declaration header by its text. `InsertionContext.Pos` is the first line inside the scope — below the module or namespace header, or below the opens and directives above it — for every `ScopeKind`, so an `open` is no longer placed above a header the text sniffing failed to recognize (`[<AutoOpen>] module Ns.Name` on one line) or below the first declaration when no blank line separates it from the header. `ParsedInput.AdjustInsertionPoint` keeps only the cosmetic step of skipping the blank line under a header. ([PR #20500](https://github.com/dotnet/fsharp/pull/20500))
+* `AssemblySymbol` and `InsertionContextEntity` now carry how far a plain `open` reaches (`OpenableIdentCount`) and how much of the entity's name the suggested one covers (`NamespaceIdentCount`), so a consumer can tell the namespaces and F# modules a plain `open` takes from the types only `open type` opens. Until now nothing distinguished them, and tooling suggested opening a type as if it were a namespace. ([PR #20501](https://github.com/dotnet/fsharp/pull/20501))
 * Fix `NativePtr.stackalloc` nested in a larger expression (e.g. a call argument or the right of an assignment) producing an assembly that throws `InvalidProgramException` at load. ([Issue #8083](https://github.com/dotnet/fsharp/issues/8083), [PR #20302](https://github.com/dotnet/fsharp/pull/20302))
 * Fix internal error "Unexpected generalized type variables when compiling an active pattern" when an active pattern is used in a `let` binding whose right-hand side is a generic value, e.g. `let (T) = id`. Such a binding is now checked like the equivalent `match` and is not generalized. ([Issue #16856](https://github.com/dotnet/fsharp/issues/16856), [PR #20383](https://github.com/dotnet/fsharp/pull/20383))
 * Fix Release-only (`--optimize+`) `System.InvalidProgramException` from `Seq.collect` / `yield!` over a value-type (struct) collection implementing `seq<'T>` (e.g. `ImmutableArray<_>`) when materialised with `List.ofSeq` / `Seq.toList` / `Seq.toArray` or a list/array comprehension. The collector lowering now boxes a struct sub-collection to `seq<'T>` before calling `AddMany`/`AddManyAndClose` (matching the coercion the type checker already inserts for `yield!`), and uses `unit` as the try/finally result type instead of the body type (removing a spurious `ldnull` store). ([Issue #20203](https://github.com/dotnet/fsharp/issues/20203))

docs/release-notes/.VisualStudio/18.vNext.md

@@ -5,6 +5,9 @@
 
 ### Fixed
 
+* The "open namespace" code fix and the completion that adds an `open` now place it from the syntax tree rather than by recognizing a `module` line by its text, so it no longer lands above the file's own module declaration (`[<AutoOpen>] module Ns.Name` written on one line) or below the first declaration when no blank line follows the header. Both paths now share one implementation and follow the file's existing line endings. ([PR #20500](https://github.com/dotnet/fsharp/pull/20500))
+* The "open namespace" code fix offers `open type System.Console` where it used to offer `open System.Console`. A plain `open` only reaches namespaces and F# modules, so for a type nested in a type (`System.Net.WebRequestMethods.File`) or a static member of one (`System.Console.WriteLine`) every suggestion it made was code that does not compile — `FS0039: The namespace 'Console' is not defined`. ([PR #20501](https://github.com/dotnet/fsharp/pull/20501))
+* The "open namespace" code fix lists every namespace and type a name can be resolved from instead of one arbitrary suggestion. It computed them all along and then kept whichever one the assembly crawler happened to reach first, so an unresolved `File` was answered with `System.Net.WebRequestMethods` and `System.IO` was never offered at all. As in Roslyn's add-import fix, the list is capped — five opens, three qualifications — and ordered with `System` first. ([PR #20501](https://github.com/dotnet/fsharp/pull/20501))
 * Improve Find All References performance by throttling parallel typechecks. ([PR #20128](https://github.com/dotnet/fsharp/pull/20128))
 * 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))

src/Compiler/Service/ServiceAssemblyContent.fs

@@ -39,6 +39,7 @@ type AssemblySymbol =
     { FullName: string
       CleanedIdents: ShortIdents
       Namespace: ShortIdents option
+      OpenableIdentCount: int
       NearestRequireQualifiedAccessParent: ShortIdents option
       TopRequireQualifiedAccessParent: ShortIdents option
       AutoOpenParent: ShortIdents option
@@ -57,15 +58,24 @@ type Parent =
       TopRequiresQualifiedAccess: (* isForMemberOrValue *) bool -> ShortIdents option
       AutoOpen: ShortIdents option
       WithModuleSuffix: ShortIdents option
-      IsModule: bool }
+      IsModule: bool
+      /// How many leading idents a plain `open` reaches. A type never extends it: everything it
+      /// contains is reached by naming the type, or by opening the type itself.
+      OpenableIdentCount: int }
 
     static member Empty =
         { Namespace = None
           ThisRequiresQualifiedAccess = fun _ -> None
           TopRequiresQualifiedAccess = fun _ -> None
           AutoOpen = None
           WithModuleSuffix = None
-          IsModule = true }
+          IsModule = true
+          OpenableIdentCount = 0 }
+
+    /// The reach of a plain `open` over an entity declared in this parent: the enclosing namespace
+    /// at the very least, and whatever modules the parent chain has added to it.
+    member x.OpenableIdentCountFor (ns: ShortIdents option) =
+        max x.OpenableIdentCount (ns |> Option.map Array.length |> Option.defaultValue 0)
 
     static member RewriteParentIdents (parentIdents: ShortIdents option) (idents: ShortIdents) =
         match parentIdents with
@@ -149,6 +159,7 @@ module AssemblyContent =
             { FullName = fullName
               CleanedIdents = cleanIdents
               Namespace = ns
+              OpenableIdentCount = parent.OpenableIdentCountFor ns
               NearestRequireQualifiedAccessParent = parent.ThisRequiresQualifiedAccess false |> Option.map parent.FixParentModuleSuffix
               TopRequireQualifiedAccessParent = topRequireQualifiedAccessParent
               AutoOpenParent = parent.AutoOpen |> Option.map parent.FixParentModuleSuffix
@@ -179,6 +190,7 @@ module AssemblyContent =
                 { FullName = fullName
                   CleanedIdents = cleanedIdents
                   Namespace = ns
+                  OpenableIdentCount = parent.OpenableIdentCountFor ns
                   NearestRequireQualifiedAccessParent = parent.ThisRequiresQualifiedAccess true |> Option.map parent.FixParentModuleSuffix
                   TopRequireQualifiedAccessParent = topRequireQualifiedAccessParent
                   AutoOpenParent = autoOpenParent
@@ -250,7 +262,12 @@ module AssemblyContent =
                             else parent.WithModuleSuffix
 
                           Namespace = ns
-                          IsModule = entity.IsFSharpModule }
+                          IsModule = entity.IsFSharpModule
+
+                          OpenableIdentCount =
+                            match entity.IsNamespace || entity.IsFSharpModule, currentEntity with
+                            | true, Some e -> e.CleanedIdents.Length
+                            | _ -> parent.OpenableIdentCountFor ns }
 
                     match entity.TryGetMembersFunctionsAndValues() with
                     | xs when xs.Count > 0 ->

src/Compiler/Service/ServiceAssemblyContent.fsi

@@ -37,6 +37,10 @@ type public AssemblySymbol =
         /// `FSharpEntity.Namespace`.
         Namespace: ShortIdents option
 
+        /// How many leading `CleanedIdents` a plain `open` reaches: the namespace plus the enclosing
+        /// F# modules. Idents past it name types, whose contents `open type` brings into scope instead.
+        OpenableIdentCount: int
+
         /// The most narrative parent module that has `RequireQualifiedAccess` attribute.
         NearestRequireQualifiedAccessParent: ShortIdents option
 

src/Compiler/Service/ServiceParsedInputOps.fs

@@ -158,6 +158,7 @@ type InsertionContextEntity =
         FullRelativeName: string
         Qualifier: string
         Namespace: string option
+        NamespaceIdentCount: int
         FullDisplayName: string
         LastIdent: ShortIdent
     }
@@ -254,12 +255,18 @@ module Entity =
                     | _ ->
                         let fullRelativeName = Array.append (getRelativeNs fullOpenableNs) restIdents
 
+                        // What the suggested `open` covers, named relatively to the current scope and,
+                        // for the count, absolutely: the two differ by the prefix already in scope.
+                        let shortenByQualifiedIdents (idents: ShortIdents) =
+                            if identCount > 1 && relativeNs.Length >= identCount then
+                                idents[0 .. idents.Length - identCount]
+                            else
+                                idents
+
                         let ns =
                             match relativeNs with
                             | [||] -> None
-                            | _ when identCount > 1 && relativeNs.Length >= identCount ->
-                                Some(relativeNs[0 .. relativeNs.Length - identCount] |> String.concat ".")
-                            | _ -> Some(relativeNs |> String.concat ".")
+                            | _ -> Some(shortenByQualifiedIdents relativeNs |> String.concat ".")
 
                         let qualifier =
                             if fullRelativeName.Length > 1 && fullRelativeName.Length >= identCount then
@@ -272,6 +279,7 @@ module Entity =
                                 FullRelativeName = String.concat "." fullRelativeName //.[0..fullRelativeName.Length - identCount - 1]
                                 Qualifier = String.concat "." qualifier
                                 Namespace = ns
+                                NamespaceIdentCount = (shortenByQualifiedIdents openableNs).Length
                                 FullDisplayName =
                                     match restIdents with
                                     | [| _ |] -> ""
@@ -2476,6 +2484,18 @@ module ParsedInput =
                 | _ -> None
                 |> Option.map (fun r -> r.StartColumn)
 
+        // The line a declaration's header ends on: its leading keyword, the name it introduces and,
+        // for a nested module, the `=`. Attributes and doc comments sit above it, the body below.
+        let headerEndLine (keyword: range) (ident: LongIdent) (equals: range option) =
+            let nameEnd =
+                match List.tryLast ident with
+                | Some lastIdent -> max keyword.EndLine lastIdent.idRange.EndLine
+                | None -> keyword.EndLine
+
+            match equals with
+            | Some equalsRange -> max nameEnd equalsRange.EndLine
+            | None -> nameEnd
+
         let rec walkImplFileInput (file: ParsedImplFileInput) =
             List.iter (walkSynModuleOrNamespace []) file.Contents
 
@@ -2496,22 +2516,20 @@ module ParsedInput =
 
                 let fullIdent = parent @ ident
 
-                // Use trivia to get the actual module/namespace keyword line, which excludes attributes
-                let startLine =
+                let headerLine =
                     match trivia.LeadingKeyword with
-                    | SynModuleOrNamespaceLeadingKeyword.Module moduleRange -> moduleRange.StartLine
-                    | SynModuleOrNamespaceLeadingKeyword.Namespace namespaceRange -> namespaceRange.StartLine - 1
-                    | SynModuleOrNamespaceLeadingKeyword.None ->
-                        // No keyword (implicit module), use range.StartLine
-                        if isModule then range.StartLine else range.StartLine - 1
+                    | SynModuleOrNamespaceLeadingKeyword.Module keyword
+                    | SynModuleOrNamespaceLeadingKeyword.Namespace keyword -> headerEndLine keyword ident None
+                    // An implicit module has no header, so its first declaration opens the scope.
+                    | SynModuleOrNamespaceLeadingKeyword.None -> range.StartLine - 1
 
                 let scopeKind =
                     match isModule, parent with
                     | true, [] -> TopModule
                     | true, _ -> NestedModule
                     | _ -> Namespace
 
-                doRange scopeKind fullIdent startLine range.StartColumn
+                doRange scopeKind fullIdent headerLine range.StartColumn
                 addModule (fullIdent, range)
                 List.iter (walkSynModuleDecl fullIdent) decls
 
@@ -2524,16 +2542,15 @@ module ParsedInput =
                 addModule (fullIdent, range)
 
                 if range.EndLine >= currentLine then
-                    // Use trivia to get the actual module keyword line, which excludes attributes
-                    let moduleKeywordLine =
+                    let headerLine =
                         match trivia.ModuleKeyword with
-                        | Some moduleKeywordRange -> moduleKeywordRange.StartLine
+                        | Some moduleKeyword -> headerEndLine moduleKeyword ident trivia.EqualsRange
                         | None -> range.StartLine // Fallback if trivia unavailable
 
                     let moduleBodyIndentation =
                         getMinColumn decls |> Option.defaultValue (range.StartColumn + 4)
 
-                    doRange NestedModule fullIdent moduleKeywordLine moduleBodyIndentation
+                    doRange NestedModule fullIdent headerLine moduleBodyIndentation
                     List.iter (walkSynModuleDecl fullIdent) decls
             | SynModuleDecl.Open(_, range) -> doRange OpenDeclaration [] range.EndLine (range.StartColumn - 5)
             | SynModuleDecl.HashDirective(_, range) -> doRange HashDirective [] range.EndLine range.StartColumn
@@ -2609,50 +2626,14 @@ module ParsedInput =
                 entities
                 |> Array.map (fun e -> e, findBestPositionToInsertOpenDeclaration modules scope pos entity)
 
-    /// Corrects insertion line number based on kind of scope and text surrounding the insertion point.
+    /// Nudges the insertion point past the blank line that conventionally follows a declaration
+    /// header, so that the `open` joins the code below it instead of the gap above it.
     let AdjustInsertionPoint (getLineStr: int -> string) ctx =
-        let line =
-            match ctx.ScopeKind with
-            | ScopeKind.TopModule ->
-                if ctx.Pos.Line > 1 then
-                    // it's an implicit module without any open declarations
-                    let line = getLineStr (ctx.Pos.Line - 2)
-
-                    let isImplicitTopLevelModule =
-                        not (line.StartsWithOrdinal("module") && not (line.EndsWithOrdinal("=")))
-
-                    if isImplicitTopLevelModule then 1 else ctx.Pos.Line
-                else
-                    1
-
-            | ScopeKind.Namespace ->
-                // For namespaces the start line is start line of the first nested entity
-                // If we are not on the first line, try to find opening namespace, and return line after it (in F# format)
-                if ctx.Pos.Line > 1 then
-                    [ 0 .. ctx.Pos.Line - 1 ]
-                    |> List.mapi (fun i line -> i, getLineStr line)
-                    |> List.tryPick (fun (i, lineStr) ->
-                        if lineStr.StartsWithOrdinal("namespace") then
-                            Some i
-                        else
-                            None)
-                    |> function
-                        // move to the next line below "namespace" and convert it to F# 1-based line number
-                        | Some line -> line + 2
-                        | None -> ctx.Pos.Line
-                // If we are on 1st line in the namespace ctx, this line _should_ be the namespace declaration, check it and return next line.
-                // Otherwise, return first line (which theoretically should not happen).
-                else
-                    let lineStr = getLineStr (ctx.Pos.Line - 1)
-
-                    if lineStr.StartsWithOrdinal("namespace") then
-                        ctx.Pos.Line + 1
-                    else
-                        ctx.Pos.Line
-
-            | _ -> ctx.Pos.Line
-
-        mkPos line ctx.Pos.Column
+        match ctx.ScopeKind with
+        | ScopeKind.TopModule
+        | ScopeKind.Namespace
+        | ScopeKind.NestedModule when getLineStr (Line.toZ ctx.Pos.Line) = "" -> mkPos (ctx.Pos.Line + 1) ctx.Pos.Column
+        | _ -> ctx.Pos
 
     let FindNearestPointToInsertOpenDeclaration
         (currentLine: int)
@@ -2695,8 +2676,8 @@ module ParsedInput =
                 | _ -> 0
 
             if lastReferenceLine > 0 then
-                // `AdjustInsertionPoint` snaps a `TopModule` position up to line 1, above the directives;
-                // remap it to `HashDirective`, which (like the other scopes) it passes through unchanged.
+                // The open belongs directly under the directives, so report the scope that keeps it
+                // there rather than the module scope, which would push it past the blank line below.
                 let scopeKind =
                     if ctx.ScopeKind = ScopeKind.TopModule then
                         ScopeKind.HashDirective

src/Compiler/Service/ServiceParsedInputOps.fsi

@@ -131,7 +131,8 @@ type public InsertionContext =
         /// Current scope kind.
         ScopeKind: ScopeKind
 
-        /// Current position (F# compiler line number).
+        /// Where the `open` belongs (F# compiler line number): the first line inside the scope, that
+        /// is, below the declaration header or below the open declarations and directives above it.
         Pos: pos
     }
 
@@ -162,6 +163,11 @@ type public InsertionContextEntity =
         /// Namespace that is needed to open to make the entity resolvable in the current scope.
         Namespace: string option
 
+        /// How many leading idents of the entity's full name that namespace covers. Compare it with
+        /// `AssemblySymbol.OpenableIdentCount` to tell a namespace a plain `open` reaches from a type,
+        /// which only `open type` opens.
+        NamespaceIdentCount: int
+
         /// Full display name (i.e. last ident plus modules with `RequireQualifiedAccess` attribute prefixed).
         FullDisplayName: string
 
@@ -207,7 +213,9 @@ module public ParsedInput =
     /// Returns long identifier at position.
     val GetLongIdentAt: parsedInput: ParsedInput -> pos: pos -> LongIdent option
 
-    /// Corrects insertion line number based on kind of scope and text surrounding the insertion point.
+    /// Nudges the insertion point past the blank line that conventionally follows a declaration header,
+    /// so that the `open` joins the code below it instead of the gap above it. `getLineStr` returns the
+    /// trimmed text of a zero-based line, or an empty string past the end of the file.
     val AdjustInsertionPoint: getLineStr: (int -> string) -> ctx: InsertionContext -> pos
 
 // implementation details used by other code in the compiler