Place the added open from the syntax tree, not from a line of text

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

@@ -8,6 +8,7 @@
 * Capture diagnostics emitted when transparent project checking finishes a project, including FS0247 for namespace-module name collisions. ([Issue #6036](https://github.com/dotnet/fsharp/issues/6036), [PR #20706](https://github.com/dotnet/fsharp/pull/20706))
 * A project reusing cached framework imports is checked with its own `--pathmap` instead of the map of the project that filled the cache, so the ranges of its in-memory reference data name its real files. ([Issue #20474](https://github.com/dotnet/fsharp/issues/20474), [PR #20476](https://github.com/dotnet/fsharp/pull/20476))
 * Split Linux CoreCLR CI into two parallel test batches to reduce test turnaround time. ([PR #20587](https://github.com/dotnet/fsharp/pull/20587))
+* `ParsedInput.FindNearestPointToInsertOpenDeclaration` reports where an `open` belongs from the syntax tree: `InsertionContext.Pos` is the first line inside the scope for every `ScopeKind`, so an `open` no longer lands above a header written as `[<AutoOpen>] module Ns.Name` or below the first declaration when no blank line follows the header. `ParsedInput.AdjustInsertionPoint` only skips the blank line under a header. ([PR #20500](https://github.com/dotnet/fsharp/pull/20500))
 * Fix `NativePtr.stackalloc` used inside the `with` handler, filter or `finally` block of a `try` expression producing an assembly that throws `InvalidProgramException` at method load. Direct or mandatory-inline allocations are now reported at compile time as error FS3924; calls to non-inline allocation helpers, including handler-local helpers, remain legal and retain their method boundary during inlining and let-binding elimination. ([Issue #20295](https://github.com/dotnet/fsharp/issues/20295), [PR #20491](https://github.com/dotnet/fsharp/pull/20491))
 * Preserve SRTP code generation when extension constraint solutions are disabled. ([PR #20773](https://github.com/dotnet/fsharp/pull/20773))
 * `FsiEvaluationSession.EvalInteraction` and `EvalInteractionNonThrowing` evaluate every `;;`-separated interaction in the text they are given, as their documentation has always said, instead of silently discarding everything after the first. ([PR #20396](https://github.com/dotnet/fsharp/pull/20396))

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

@@ -10,6 +10,7 @@
 * Go To Definition no longer blocks the UI thread with a bare `Task.Wait`: the synchronous `IFSharpGoToDefinitionService` call now waits through the cancellable threaded-wait dialog, and the editor's `TaskCompletionSource` bridges run their continuations on the thread pool instead of inline on whichever thread finished the check, so repeated F12 on a large solution no longer starves semantic classification and other main-thread work. ([PR #20482](https://github.com/dotnet/fsharp/pull/20482))
 * Peek Definition on an F# symbol whose definition lives in metadata no longer deadlocks Visual Studio. Peek holds the main thread in `JoinableTaskFactory.Run` without pumping messages while it asks the language service for the definition, and generating the metadata document needs that same thread; Peek now stops at definitions that already have a document, and Go To Definition, which owns the wait it makes, still opens the generated one. ([PR #20503](https://github.com/dotnet/fsharp/pull/20503))
 * Improve Find All References performance by throttling parallel typechecks. ([PR #20128](https://github.com/dotnet/fsharp/pull/20128), [PR #20695](https://github.com/dotnet/fsharp/pull/20695))
+* The "open namespace" code fix and the completion that adds an `open` place it from the syntax tree, so it no longer lands above the file's own module declaration (`[<AutoOpen>] module Ns.Name` on one line) or below the first declaration when no blank line follows the header, and both follow the file's line endings. ([PR #20500](https://github.com/dotnet/fsharp/pull/20500))
 * 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))

src/Compiler/Service/ServiceParsedInputOps.fs

@@ -2461,9 +2461,9 @@ module ParsedInput =
                             )
                     | _ -> ()
 
-        let getMinColumn decls =
+        let tryGetFirstDeclStart decls =
             match decls with
-            | [] -> None
+            | [] -> ValueNone
             | firstDecl :: _ ->
                 match firstDecl with
                 | SynModuleDecl.NestedModule(range = r)
@@ -2472,9 +2472,16 @@ module ParsedInput =
                 | SynModuleDecl.Types(range = r)
                 | SynModuleDecl.Exception(range = r)
                 | SynModuleDecl.Open(range = r)
-                | SynModuleDecl.HashDirective(range = r) -> Some r
-                | _ -> None
-                |> Option.map (fun r -> r.StartColumn)
+                | SynModuleDecl.HashDirective(range = r) -> ValueSome r.Start
+                | _ -> ValueNone
+
+        let headerEndLine (keyword: range) (ident: LongIdent) (equals: range option) =
+            let nameEndLine =
+                ident
+                |> List.fold (fun line part -> max line part.idRange.EndLine) keyword.EndLine
+
+            equals
+            |> Option.fold (fun line equalsSign -> max line equalsSign.EndLine) nameEndLine
 
         let rec walkImplFileInput (file: ParsedImplFileInput) =
             List.iter (walkSynModuleOrNamespace []) file.Contents
@@ -2496,45 +2503,62 @@ module ParsedInput =
 
                 let fullIdent = parent @ ident
 
-                // Use trivia to get the actual module/namespace keyword line, which excludes attributes
-                let startLine =
+                let lineAboveBody =
                     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
+                    | 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 lineAboveBody range.StartColumn
                 addModule (fullIdent, range)
                 List.iter (walkSynModuleDecl fullIdent) decls
 
         and walkSynModuleDecl (parent: LongIdent) (decl: SynModuleDecl) =
             match decl with
             | SynModuleDecl.NamespaceFragment fragment -> walkSynModuleOrNamespace parent fragment
             | SynModuleDecl.NestedModule(moduleInfo = compInfo; decls = decls; range = range; trivia = trivia) ->
+                let (SynComponentInfo(attributes = attributes)) = compInfo
                 let ident = compInfo.LongIdent
                 let fullIdent = parent @ ident
                 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 firstDeclStart = tryGetFirstDeclStart decls
+
                     let moduleBodyIndentation =
-                        getMinColumn decls |> Option.defaultValue (range.StartColumn + 4)
+                        firstDeclStart
+                        |> ValueOption.map _.Column
+                        |> ValueOption.defaultValue (range.StartColumn + 4)
+
+                    let bodyStartsOnHeaderLine =
+                        firstDeclStart |> ValueOption.exists (fun start -> start.Line = headerLine)
 
-                    doRange NestedModule fullIdent moduleKeywordLine moduleBodyIndentation
-                    List.iter (walkSynModuleDecl fullIdent) decls
+                    let lineAboveBody =
+                        if bodyStartsOnHeaderLine then
+                            headerLine - 1
+                        else
+                            headerLine
+
+                    let useMayBeInModuleAttribute =
+                        bodyStartsOnHeaderLine
+                        && currentLine = headerLine
+                        && attributes
+                           |> List.exists (fun attributeList -> attributeList.Range.EndLine = headerLine)
+
+                    if headerLine <= currentLine && not useMayBeInModuleAttribute then
+                        doRange NestedModule fullIdent lineAboveBody 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 +2633,12 @@ 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.
     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 +2681,7 @@ 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.
+                // AdjustInsertionPoint would move a TopModule position below the blank line under the directives.
                 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, below its header,
+        /// opens and directives. For a nested module whose body starts on the header's line, that line.
         Pos: pos
     }
 
@@ -207,7 +208,8 @@ 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.
+    /// Moves the insertion point below the blank line that follows a declaration header. `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

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

@@ -7,17 +7,11 @@ open FSharp.Compiler.Service.Tests.Common
 /// Computes the position an `open` would be inserted at, mirroring the real editor consumer
 /// (RoslynHelpers.insertContext): `FindNearestPointToInsertOpenDeclaration` followed by
 /// `AdjustInsertionPoint`. Asserting on the adjusted position is what the user actually sees.
-let private findOpenInsertion (insertionPoint: OpenStatementInsertionPoint) (fileName: string) (source: string) (entity: string) =
+let private findOpenInsertionAt currentLine insertionPoint fileName (source: string) (entity: string) =
     let parseTree = parseSourceCode (fileName, source)
 
     let lines = source.Replace("\r\n", "\n").Split('\n')
 
-    let currentLine =
-        let mutable i = lines.Length - 1
-        while i > 0 && System.String.IsNullOrWhiteSpace(lines.[i]) do
-            i <- i - 1
-        i + 1
-
     let idents = entity.Split('.')
 
     let ctx =
@@ -29,6 +23,9 @@ let private findOpenInsertion (insertionPoint: OpenStatementInsertionPoint) (fil
     let pos = ParsedInput.AdjustInsertionPoint getLineStr ctx
     pos.Line, pos.Column
 
+let private findOpenInsertion insertionPoint fileName (source: string) entity =
+    findOpenInsertionAt (source.TrimEnd().Split('\n').Length) insertionPoint fileName source entity
+
 let private findOpenInsertionLine fileName source entity =
     findOpenInsertion OpenStatementInsertionPoint.TopLevel fileName source entity |> fst
 
@@ -116,8 +113,40 @@ let ``Open not forced above named module in fsx`` () =
 let x = System.IO.File.ReadAllText "a"
 """
     let line = findOpenInsertionLine "test.fsx" source "System.IO"
-    // Below the `module Foo` header (line 1), inside the module.
-    Assert.Equal(2, line)
+    Assert.Equal(3, line)
+
+[<Theory>]
+[<InlineData("""[<AutoOpen>] module Foo
+
+let x = System.IO.File.ReadAllText "a"
+""", 3)>]
+[<InlineData("""module Foo
+let x = System.IO.File.ReadAllText "a"
+""", 2)>]
+[<InlineData("""namespace Ns
+type T() =
+    member _.M() = System.IO.File.ReadAllText "a"
+""", 2)>]
+let ``Open goes below the header of a module or namespace, with or without a blank line`` (source: string, expectedLine: int) =
+    Assert.Equal(expectedLine, findOpenInsertionLine "test.fs" source "System.IO")
+
+[<Fact>]
+let ``Open placed at the first declaration of a nested module declared on one line`` () =
+    let source = """module Top
+
+module M = let x = System.IO.File.ReadAllText "a"
+"""
+    let pos = findOpenInsertion OpenStatementInsertionPoint.Nearest "test.fs" source "System.IO"
+    Assert.Equal((3, 11), pos)
+
+[<Theory>]
+[<InlineData("module M = let y = 1", 2)>]
+[<InlineData("[<Obsolete>] module M = let y = 1", 2)>]
+[<InlineData("[<Obsolete>] module M = let y = 1", 3)>]
+let ``Open stays out of a nested module declared on one line that the use is not in`` (nestedModule: string, useLine: int) =
+    let source = $"module Top\nlet x = Console.WriteLine 42\n{nestedModule}\n"
+    let pos = findOpenInsertionAt useLine OpenStatementInsertionPoint.Nearest "test.fs" source "System"
+    Assert.Equal((2, 0), pos)
 
 [<Fact>]  // Only #r/#load drive placement; other directives (#time/#help/#I/...) must not
 let ``Non reference directive does not drive placement in fsx`` () =