Enter a one-line nested module only from its own line

A nested module whose body starts on the header's line is entered at its
first declaration, and the walk gated that by the line above the header.
A use on the line above the module then got its open inside the module,
where it does not reach the use. The completion that adds an open asked
for that same line and composed its change from a position past the item,
which threw ArgumentOutOfRangeException.

The module is now entered from the header's line on. When an attribute
shares that line with the body, the use may be the attribute, which the
enclosing scope resolves, so the open stays there.

The body of GetChangeAsync moves into a static GetChangeAsyncAux, as
ProvideCompletionsAsyncAux already is, so that a test can commit a
completion item without a Visual Studio service provider.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Author: xperiandri

src/Compiler/Service/ServiceParsedInputOps.fs

@@ -2528,6 +2528,7 @@ module ParsedInput =
             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)
@@ -2545,16 +2546,32 @@ module ParsedInput =
                         |> ValueOption.map _.Column
                         |> ValueOption.defaultValue (range.StartColumn + 4)
 
+                    let bodyStartsOnHeaderLine =
+                        match firstDeclStart with
+                        | ValueSome start -> start.Line = headerLine
+                        | ValueNone -> false
+
                     // The open goes on the line below this one. A body that starts on the header's own
                     // line (`module M = let x = 1`) is entered at its first declaration instead, since
                     // the line below the header may already be past the use the open has to precede.
                     let lineAboveBody =
-                        match firstDeclStart with
-                        | ValueSome start when start.Line = headerLine -> headerLine - 1
-                        | _ -> headerLine
-
-                    doRange NestedModule fullIdent lineAboveBody moduleBodyIndentation
-                    List.iter (walkSynModuleDecl fullIdent) decls
+                        if bodyStartsOnHeaderLine then
+                            headerLine - 1
+                        else
+                            headerLine
+
+                    // A use on the line the header shares with the body is in that body or in an attribute
+                    // of the module, which the enclosing scope resolves. The line alone cannot tell the two
+                    // apart, and an open in the enclosing scope serves both.
+                    let mayBeInAttribute =
+                        bodyStartsOnHeaderLine
+                        && currentLine = headerLine
+                        && attributes
+                           |> List.exists (fun attributeList -> attributeList.Range.EndLine = headerLine)
+
+                    if headerLine <= currentLine && not mayBeInAttribute 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
             | _ -> ()

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

@@ -4,20 +4,20 @@ open Xunit
 open FSharp.Compiler.EditorServices
 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
+/// Computes the position an `open` would be inserted at for a use on `currentLine`, 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: int)
+    (insertionPoint: OpenStatementInsertionPoint)
+    (fileName: string)
+    (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 +29,18 @@ let private findOpenInsertion (insertionPoint: OpenStatementInsertionPoint) (fil
     let pos = ParsedInput.AdjustInsertionPoint getLineStr ctx
     pos.Line, pos.Column
 
+/// The same for a use on the last line of the source that is not blank.
+let private findOpenInsertion (insertionPoint: OpenStatementInsertionPoint) (fileName: string) (source: string) (entity: string) =
+    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
+
+    findOpenInsertionAt currentLine insertionPoint fileName source entity
+
 let private findOpenInsertionLine fileName source entity =
     findOpenInsertion OpenStatementInsertionPoint.TopLevel fileName source entity |> fst
 
@@ -154,6 +166,15 @@ 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>]  // Regression: the open has to stay in `Top`, the scope of the use, and not enter `M`
+[<InlineData("module M = let y = 1", 2)>]
+[<InlineData("[<Obsolete>] module M = let y = 1", 2)>]
+[<InlineData("[<Obsolete>] module M = let y = 1", 3)>]  // the use is the attribute on the header's line
+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`` () =
     let source = """#time "on"

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

@@ -405,11 +405,13 @@ type internal FSharpCompletionProvider
             | true, keywordDescription -> Task.FromResult(CompletionDescription.FromText(keywordDescription))
             | false, _ -> Task.FromResult(CompletionDescription.Empty)
 
-    override _.GetChangeAsync(document, item, _, cancellationToken) : Task<CompletionChange> =
+    static member GetChangeAsyncAux(document: Document, item: Completion.CompletionItem, insertionPoint: OpenStatementInsertionPoint) =
         cancellableTask {
             use _logBlock =
                 Logger.LogBlockMessage document.Name LogEditorFunctionId.Completion_GetChangeAsync
 
+            let! cancellationToken = CancellableTask.getCancellationToken ()
+
             let fullName =
                 match item.Properties.TryGetValue FullNamePropName with
                 | true, x -> ValueSome x
@@ -469,12 +471,6 @@ type internal FSharpCompletionProvider
                         |> ValueOption.map (fun x -> x.Split '.')
                         |> ValueOption.defaultValue [||]
 
-                    let insertionPoint =
-                        if settings.CodeFixes.AlwaysPlaceOpensAtTopLevel then
-                            OpenStatementInsertionPoint.TopLevel
-                        else
-                            OpenStatementInsertionPoint.Nearest
-
                     let ctx =
                         ParsedInput.FindNearestPointToInsertOpenDeclaration
                             line.LineNumber
@@ -494,4 +490,13 @@ type internal FSharpCompletionProvider
 
                     return CompletionChange.Create(TextChange(fullChangingSpan, changedText)).WithNewPosition(Nullable(changedSpan.End))
         }
+
+    override _.GetChangeAsync(document, item, _, cancellationToken) : Task<CompletionChange> =
+        let insertionPoint =
+            if settings.CodeFixes.AlwaysPlaceOpensAtTopLevel then
+                OpenStatementInsertionPoint.TopLevel
+            else
+                OpenStatementInsertionPoint.Nearest
+
+        FSharpCompletionProvider.GetChangeAsyncAux(document, item, insertionPoint)
         |> CancellableTask.start cancellationToken

vsintegration/tests/FSharp.Editor.Tests/CodeFixes/AddOpenOnTopOffTests.fs

@@ -205,6 +205,23 @@ let ``Fixes FS0039 for missing opens - nested module declared on one line`` () =
 
     Assert.Equal(expected, actual)
 
+[<Theory>] // The open has to stay in `Module1`, the scope of the use, and not enter `Module2`
+[<InlineData("let x = Console.WriteLine 42\n[<Obsolete>] module Module2 = let y = 1\n")>]
+[<InlineData("[<Obsolete>] module Module2 = let y = 1\n")>]
+let ``Fixes FS0039 for missing opens - the use is not in the nested module declared on one line`` (declarations: string) =
+    let code = "module Module1\n" + declarations
+
+    let expected =
+        Some
+            {
+                Message = "open System"
+                FixedCode = "module Module1\n\nopen System\n\n" + declarations
+            }
+
+    let actual = codeFix |> tryFix code mode
+
+    Assert.Equal(expected, actual)
+
 [<Fact>]
 let ``Fixes FS0039 for missing opens - explicit module has attributes`` () =
     let code =

vsintegration/tests/FSharp.Editor.Tests/CompletionProviderTests.fs

@@ -149,6 +149,68 @@ module CompletionProviderTests =
 
         Assert.Equal(expected, sourceText.ToString(resultSpan))
 
+    /// Verify the text that committing an item from a namespace that is not opened leaves, the `open` included.
+    let VerifyCompletionWithOpen (fileContents: string, marker: string, itemName: string, expected: string) =
+        let caretPosition = fileContents.IndexOf(marker) + marker.Length
+
+        let document =
+            RoslynTestHelpers.CreateSolution(fileContents)
+            |> RoslynTestHelpers.GetSingleDocument
+
+        let sourceText = SourceText.From(fileContents)
+
+        let span =
+            CompletionUtils.getDefaultCompletionListSpan (
+                sourceText,
+                caretPosition,
+                document.Id,
+                filePath,
+                [],
+                None,
+                CancellationToken.None
+            )
+
+        let assemblyContentProvider = AssemblyContentProvider()
+
+        // A completion item gets its span from the context it is added to.
+        let context =
+            let provider =
+                { new CompletionProvider() with
+                    member _.ProvideCompletionsAsync _ = Tasks.Task.CompletedTask
+                }
+
+            CompletionContext(provider, document, caretPosition, span, CompletionTrigger.Invoke, null, CancellationToken.None)
+
+        let change =
+            let task =
+                cancellableTask {
+                    let! items =
+                        FSharpCompletionProvider.ProvideCompletionsAsyncAux(
+                            document,
+                            caretPosition,
+                            assemblyContentProvider.GetAllEntitiesInProjectAndReferencedAssemblies,
+                            false
+                        )
+
+                    let item =
+                        items
+                        |> Seq.find (fun item -> item.DisplayText = itemName && item.Properties.ContainsKey "NamespaceToOpen")
+
+                    context.AddItem item
+
+                    return!
+                        FSharpCompletionProvider.GetChangeAsyncAux(
+                            document,
+                            item,
+                            FSharp.Compiler.EditorServices.OpenStatementInsertionPoint.Nearest
+                        )
+                }
+                |> CancellableTask.start CancellationToken.None
+
+            task.Result
+
+        Assert.Equal(expected, sourceText.WithChanges(change.TextChange).ToString())
+
     [<Fact>]
     let ShouldTriggerCompletionAtCorrectMarkers () =
         let testCases =
@@ -2167,3 +2229,22 @@ let _ =
                 ]
             |> Map.ofList
         )
+
+    [<Fact>]
+    let ``Completion that adds an open keeps it out of a nested module declared on one line`` () =
+        let fileContents =
+            """module Top
+let x = Console.WriteLine 42
+[<Obsolete>] module M = let y = 1
+"""
+
+        let expected =
+            """module Top
+
+open System
+
+let x = Console.WriteLine 42
+[<Obsolete>] module M = let y = 1
+"""
+
+        VerifyCompletionWithOpen(fileContents, "[<Obsol", "Obsolete", expected)