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

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

@@ -1,6 +1,7 @@
 ### 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))
 * 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,7 @@
 
 ### 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))
 * 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/ServiceParsedInputOps.fs

@@ -2476,6 +2476,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 +2508,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 +2534,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 +2618,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 +2668,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
     }
 
@@ -207,7 +208,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

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

@@ -116,7 +116,33 @@ 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.
+    // Inside the module, next to the code rather than in the gap under the header.
+    Assert.Equal(3, line)
+
+[<Fact>]  // Regression: an attribute sharing the header's line must not read as an implicit module
+let ``Open not forced above module with attribute on same line`` () =
+    let source = """[<AutoOpen>] module Foo
+
+let x = System.IO.File.ReadAllText "a"
+"""
+    let line = findOpenInsertionLine "test.fs" source "System.IO"
+    Assert.Equal(3, line)
+
+[<Fact>]  // Regression: the header line is known from the tree, not from a blank line below it
+let ``Open placed inside module without a blank line under the header`` () =
+    let source = """module Foo
+let x = System.IO.File.ReadAllText "a"
+"""
+    let line = findOpenInsertionLine "test.fs" source "System.IO"
+    Assert.Equal(2, line)
+
+[<Fact>]
+let ``Open placed under namespace without a blank line under the header`` () =
+    let source = """namespace Ns
+type T() =
+    member _.M() = System.IO.File.ReadAllText "a"
+"""
+    let line = findOpenInsertionLine "test.fs" source "System.IO"
     Assert.Equal(2, line)
 
 [<Fact>]  // Only #r/#load drive placement; other directives (#time/#help/#I/...) must not

vsintegration/src/FSharp.Editor/CodeFixes/AddOpenCodeFixProvider.fs

@@ -2,7 +2,6 @@
 
 namespace Microsoft.VisualStudio.FSharp.Editor
 
-open System
 open System.Composition
 open System.Collections.Immutable
 
@@ -19,8 +18,6 @@ open CancellableTasks
 type internal AddOpenCodeFixProvider [<ImportingConstructor>] (assemblyContentProvider: AssemblyContentProvider) =
     inherit CodeFixProvider()
 
-    static let br = Environment.NewLine
-
     let fixUnderscoresInMenuText (text: string) = text.Replace("_", "__")
 
     let qualifySymbolFix (context: CodeFixContext) (fullName, qualifier) =
@@ -30,39 +27,10 @@ type internal AddOpenCodeFixProvider [<ImportingConstructor>] (assemblyContentPr
             Changes = [ TextChange(context.Span, qualifier) ]
         }
 
-    // With the fix in ServiceParsedInputOps, the InsertionContext now correctly
-    // points to the line after the module/namespace keyword (excluding attributes).
-    // However, we still need to handle implicit top-level modules and nested modules.
-    let getOpenDeclaration (sourceText: SourceText) (ctx: InsertionContext) (ns: string) =
-        // insertion context counts from 2, make the world sane
-        let insertionLineNumber = ctx.Pos.Line - 2
-        let margin = String(' ', ctx.Pos.Column)
-
-        let startLineNumber, openDeclaration =
-            match ctx.ScopeKind with
-            | ScopeKind.TopModule ->
-                match sourceText.Lines[insertionLineNumber].ToString().Trim() with
-
-                // explicit top level module
-                | line when line.StartsWith "module" && not (line.EndsWith "=") -> insertionLineNumber + 2, $"{margin}open {ns}{br}{br}"
-
-                // nested module, shouldn't be here
-                | line when line.StartsWith "module" -> insertionLineNumber, $"{margin}open {ns}{br}{br}"
-
-                // implicit top level module
-                | _ -> insertionLineNumber, $"{margin}open {ns}{br}{br}"
-
-            | ScopeKind.Namespace -> insertionLineNumber + 3, $"{margin}open {ns}{br}{br}"
-            | ScopeKind.NestedModule -> insertionLineNumber + 2, $"{margin}open {ns}{br}{br}"
-            | ScopeKind.OpenDeclaration -> insertionLineNumber + 1, $"{margin}open {ns}{br}"
-            | ScopeKind.HashDirective -> insertionLineNumber + 1, $"open {ns}{br}{br}"
-
-        let start = sourceText.Lines[startLineNumber].Start
-        TextChange(TextSpan(start, 0), openDeclaration)
-
     let openNamespaceFix ctx name ns multipleNames sourceText =
         let displayText = $"open {ns}" + (if multipleNames then " (" + name + ")" else "")
-        let change = getOpenDeclaration sourceText ctx ns
+
+        let change = OpenDeclarationHelper.getOpenDeclarationChange sourceText ctx ns
 
         {
             Name = CodeFix.AddOpen