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

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

@@ -14,6 +14,7 @@
 * Checker: report Bind resolution failures at the bound expression ([PR #20747](https://github.com/dotnet/fsharp/pull/20747))
 * 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, and it stays out of a block comment that runs on from the header. `ParsedInput.AdjustInsertionPoint` only skips the blank line under a header. ([PR #20500](https://github.com/dotnet/fsharp/pull/20500))
 * Fix wrong results or non-terminating loops in optimized code when an integer `for` loop's start or end expression binds a local. ([Issue #20732](https://github.com/dotnet/fsharp/issues/20732), [PR #20733](https://github.com/dotnet/fsharp/pull/20733))
 * Checker: resolve named arguments of failed and unfinished method calls ([PR #20749](https://github.com/dotnet/fsharp/pull/20749))
 * 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))

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

@@ -12,6 +12,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, or inside a block comment that runs on from the header, and both follow the file's line endings. ([PR #20500](https://github.com/dotnet/fsharp/pull/20500))
 * Fix record-type Rename corrupting copy-and-update expressions, including from abstract signatures, while retaining Find All References results. ([Issue #20791](https://github.com/dotnet/fsharp/issues/20791), [PR #20797](https://github.com/dotnet/fsharp/pull/20797))
 * 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/ServiceParsedInputOps.fs

@@ -2434,12 +2434,23 @@ module ParsedInput =
             Kind: ScopeKind
         }
 
-    let tryFindNearestPointAndModules currentLine (ast: ParsedInput) (insertionPoint: OpenStatementInsertionPoint) =
+    let tryFindNearestPointAndModules (currentPos: pos) (ast: ParsedInput) (insertionPoint: OpenStatementInsertionPoint) =
         // We ignore all diagnostics during this operation
         //
         // Based on an initial review, no diagnostics should be generated.  However the code should be checked more closely.
         use _ignoreAllDiagnostics = new DiagnosticsScope(false)
 
+        let currentLine = currentPos.Line
+
+        let blockComments =
+            match ast with
+            | ParsedInput.ImplFile input ->
+                input.Trivia.CodeComments
+                |> List.choose (function
+                    | CommentTrivia.BlockComment range -> Some range
+                    | CommentTrivia.LineComment _ -> None)
+            | ParsedInput.SigFile _ -> []
+
         let mutable result = None
         let mutable ns = None
         let modules = ResizeArray<FSharpModule>()
@@ -2487,9 +2498,9 @@ module ParsedInput =
                             )
                     | _ -> ()
 
-        let getMinColumn decls =
+        let tryGetFirstDeclStart decls =
             match decls with
-            | [] -> None
+            | [] -> ValueNone
             | firstDecl :: _ ->
                 match firstDecl with
                 | SynModuleDecl.NestedModule(range = r)
@@ -2498,9 +2509,33 @@ 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 rec skipTrailingBlockComments (firstDeclStart: pos voption) line =
+            match
+                blockComments
+                |> List.tryFind (fun comment ->
+                    comment.StartLine <= line
+                    && comment.EndLine > line
+                    && firstDeclStart |> ValueOption.forall (posLt comment.Start))
+            with
+            | Some comment -> skipTrailingBlockComments firstDeclStart comment.EndLine
+            | None -> line
+
+        let headerEndLine (keyword: range) (ident: LongIdent) (equals: range option) firstDeclStart =
+            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
+            |> skipTrailingBlockComments firstDeclStart
+
+        let bodyAnchor headerLine (firstDeclStart: pos voption) defaultColumn =
+            match firstDeclStart with
+            | ValueSome start when start.Line = headerLine -> struct (headerLine - 1, start.Column)
+            | _ -> struct (headerLine, defaultColumn)
 
         let rec walkImplFileInput (file: ParsedImplFileInput) =
             List.iter (walkSynModuleOrNamespace []) file.Contents
@@ -2522,45 +2557,61 @@ module ParsedInput =
 
                 let fullIdent = parent @ ident
 
-                // Use trivia to get the actual module/namespace keyword line, which excludes attributes
-                let startLine =
+                let firstDeclStart = tryGetFirstDeclStart decls
+
+                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 firstDeclStart
+                    | SynModuleOrNamespaceLeadingKeyword.None -> range.StartLine - 1
+
+                let struct (lineAboveBody, column) =
+                    bodyAnchor headerLine firstDeclStart range.StartColumn
 
                 let scopeKind =
                     match isModule, parent with
                     | true, [] -> TopModule
                     | true, _ -> NestedModule
                     | _ -> Namespace
 
-                doRange scopeKind fullIdent startLine range.StartColumn
+                doRange scopeKind fullIdent lineAboveBody column
                 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 firstDeclStart = tryGetFirstDeclStart decls
+
+                    let headerLine =
                         match trivia.ModuleKeyword with
-                        | Some moduleKeywordRange -> moduleKeywordRange.StartLine
+                        | Some moduleKeyword -> headerEndLine moduleKeyword ident trivia.EqualsRange firstDeclStart
                         | None -> range.StartLine // Fallback if trivia unavailable
 
                     let moduleBodyIndentation =
-                        getMinColumn decls |> Option.defaultValue (range.StartColumn + 4)
-
-                    doRange NestedModule fullIdent moduleKeywordLine moduleBodyIndentation
-                    List.iter (walkSynModuleDecl fullIdent) decls
+                        firstDeclStart
+                        |> ValueOption.map _.Column
+                        |> ValueOption.defaultValue (range.StartColumn + 4)
+
+                    let struct (lineAboveBody, bodyColumn) =
+                        bodyAnchor headerLine firstDeclStart moduleBodyIndentation
+
+                    let useMayBeInModuleAttribute =
+                        firstDeclStart
+                        |> ValueOption.exists (fun start -> start.Line = headerLine && posLt currentPos start)
+                        && attributes
+                           |> List.exists (fun attributeList -> attributeList.Range.EndLine = headerLine)
+
+                    if headerLine <= currentLine && not useMayBeInModuleAttribute then
+                        doRange NestedModule fullIdent lineAboveBody bodyColumn
+                        List.iter (walkSynModuleDecl fullIdent) decls
             | SynModuleDecl.Open(_, range) -> doRange OpenDeclaration [] range.EndLine (range.StartColumn - 5)
             | SynModuleDecl.HashDirective(_, range) -> doRange HashDirective [] range.EndLine range.StartColumn
             | _ -> ()
@@ -2604,14 +2655,14 @@ module ParsedInput =
                         Pos = mkPos (Line.fromZ m.Range.EndLine) m.Range.StartColumn
                     })
 
-    let TryFindInsertionContext
-        (currentLine: int)
+    let TryFindInsertionContextAt
+        (currentPos: pos)
         (parsedInput: ParsedInput)
         (partiallyQualifiedName: MaybeUnresolvedIdent[])
         (insertionPoint: OpenStatementInsertionPoint)
         =
         let res, modules =
-            tryFindNearestPointAndModules currentLine parsedInput insertionPoint
+            tryFindNearestPointAndModules currentPos parsedInput insertionPoint
 
         let res =
             res
@@ -2658,59 +2709,29 @@ module ParsedInput =
                     match find () with
                     | struct (_, context) -> e, context)
 
-    /// 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
-
-    let FindNearestPointToInsertOpenDeclaration
+    let TryFindInsertionContext
         (currentLine: int)
         (parsedInput: ParsedInput)
+        (partiallyQualifiedName: MaybeUnresolvedIdent[])
+        (insertionPoint: OpenStatementInsertionPoint)
+        =
+        TryFindInsertionContextAt (mkPos currentLine 0) parsedInput partiallyQualifiedName insertionPoint
+
+    let AdjustInsertionPoint (getLineStr: int -> string) ctx =
+        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 FindNearestPointToInsertOpenDeclarationAt
+        (currentPos: pos)
+        (parsedInput: ParsedInput)
         (entity: ShortIdents)
         (insertionPoint: OpenStatementInsertionPoint)
         =
         let ctx =
-            match tryFindNearestPointAndModules currentLine parsedInput insertionPoint with
+            match tryFindNearestPointAndModules currentPos parsedInput insertionPoint with
             | Some(scope, _, point), modules ->
                 let struct (_, context) =
                     findBestPositionToInsertOpenDeclaration modules scope point entity
@@ -2748,8 +2769,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
@@ -2763,3 +2783,11 @@ module ParsedInput =
             else
                 ctx
         | _ -> ctx
+
+    let FindNearestPointToInsertOpenDeclaration
+        (currentLine: int)
+        (parsedInput: ParsedInput)
+        (entity: ShortIdents)
+        (insertionPoint: OpenStatementInsertionPoint)
+        =
+        FindNearestPointToInsertOpenDeclarationAt (mkPos currentLine 0) parsedInput entity insertionPoint

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
     }
 
@@ -198,6 +199,19 @@ module public ParsedInput =
             ShortIdents)
                 -> (InsertionContextEntity * InsertionContext)[])
 
+    /// Like `TryFindInsertionContext`, for a use at a position: on the line of a nested module declared in one line
+    /// that carries an attribute, only a use after the start of the module's body is inside the module.
+    val internal TryFindInsertionContextAt:
+        currentPos: pos ->
+        parsedInput: ParsedInput ->
+        partiallyQualifiedName: MaybeUnresolvedIdent[] ->
+        insertionPoint: OpenStatementInsertionPoint ->
+            (( (* requiresQualifiedAccessParent: *) ShortIdents option (* autoOpenParent: *) *
+            ShortIdents option (*  entityNamespace *) *
+            ShortIdents option (* entity: *) *
+            ShortIdents)
+                -> (InsertionContextEntity * InsertionContext)[])
+
     /// Returns `InsertContext` based on current position and symbol idents.
     val FindNearestPointToInsertOpenDeclaration:
         currentLine: int ->
@@ -206,10 +220,19 @@ module public ParsedInput =
         insertionPoint: OpenStatementInsertionPoint ->
             InsertionContext
 
+    /// Like `FindNearestPointToInsertOpenDeclaration`, for a use at a position.
+    val internal FindNearestPointToInsertOpenDeclarationAt:
+        currentPos: pos ->
+        parsedInput: ParsedInput ->
+        entity: ShortIdents ->
+        insertionPoint: OpenStatementInsertionPoint ->
+            InsertionContext
+
     /// 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
 
 [<CompilationRepresentation(CompilationRepresentationFlags.ModuleSuffix)>]