From 2d3216748c00b783085b48b7deb663c487e104d1 Mon Sep 17 00:00:00 2001 From: Andrii Chebukin Date: Wed, 9 Sep 2026 19:09:33 +0200 Subject: [PATCH 1/8] Place the added open from the syntax tree, not from a line of text MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The "open namespace" code fix and the completion that adds an open both decided where the declaration goes by recognizing a module header in the text of a line — `line.StartsWith "module" && not (line.EndsWith "=")` — followed by arithmetic that assumed a blank line under that header. Every shape the spelling missed put the open where it does not compile: `[] module Ns.Name` written on one line read as an implicit module and took the open above the module declaration, and a header with no blank line under it took the open below the first declaration, leaving the identifier as unresolved as before. InsertionContext.Pos now means one thing for every ScopeKind — the first line inside the scope — taken from the leading keyword the trivia records, the name it introduces and, for a nested module, the `=`. Attributes and doc comments sit above that line by construction, so no spelling has to be recognized. AdjustInsertionPoint keeps only the cosmetic step of stepping over the blank line under a header, and the editor's two insertion paths now share one implementation, which also follows the file's own line endings instead of the host's. Co-Authored-By: Claude Opus 5 (1M context) --- .../.FSharp.Compiler.Service/11.0.200.md | 1 + docs/release-notes/.VisualStudio/18.vNext.md | 2 + src/Compiler/Service/ServiceParsedInputOps.fs | 87 ++++++------------ .../Service/ServiceParsedInputOps.fsi | 7 +- .../OpenDeclarationInsertionTests.fs | 28 +++++- .../CodeFixes/AddOpenCodeFixProvider.fs | 37 +------- .../src/FSharp.Editor/Common/RoslynHelpers.fs | 91 +++++++++---------- .../CodeFixes/AddOpenOnTopOffTests.fs | 25 +++++ .../CodeFixes/AddOpenOnTopOnTests.fs | 75 +++++++++++++++ 9 files changed, 212 insertions(+), 141 deletions(-) diff --git a/docs/release-notes/.FSharp.Compiler.Service/11.0.200.md b/docs/release-notes/.FSharp.Compiler.Service/11.0.200.md index 99464a277dd..ba740603b79 100644 --- a/docs/release-notes/.FSharp.Compiler.Service/11.0.200.md +++ b/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` 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 (`[] 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` 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)) diff --git a/docs/release-notes/.VisualStudio/18.vNext.md b/docs/release-notes/.VisualStudio/18.vNext.md index 8f47e2e0dbe..bf579df4973 100644 --- a/docs/release-notes/.VisualStudio/18.vNext.md +++ b/docs/release-notes/.VisualStudio/18.vNext.md @@ -10,6 +10,8 @@ * 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` 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 (`[] 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)) * 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)) diff --git a/src/Compiler/Service/ServiceParsedInputOps.fs b/src/Compiler/Service/ServiceParsedInputOps.fs index 7ee76f4652e..aeea6420af6 100644 --- a/src/Compiler/Service/ServiceParsedInputOps.fs +++ b/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,14 +2508,12 @@ 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 @@ -2511,7 +2521,7 @@ module ParsedInput = | 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 diff --git a/src/Compiler/Service/ServiceParsedInputOps.fsi b/src/Compiler/Service/ServiceParsedInputOps.fsi index 1b28bfb18d3..71aecf25a28 100644 --- a/src/Compiler/Service/ServiceParsedInputOps.fsi +++ b/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 diff --git a/tests/FSharp.Compiler.Service.Tests/OpenDeclarationInsertionTests.fs b/tests/FSharp.Compiler.Service.Tests/OpenDeclarationInsertionTests.fs index ef1f09f466b..21c4a98cec0 100644 --- a/tests/FSharp.Compiler.Service.Tests/OpenDeclarationInsertionTests.fs +++ b/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) + +[] // 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 = """[] module Foo + +let x = System.IO.File.ReadAllText "a" +""" + let line = findOpenInsertionLine "test.fs" source "System.IO" + Assert.Equal(3, line) + +[] // 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) + +[] +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) [] // Only #r/#load drive placement; other directives (#time/#help/#I/...) must not diff --git a/vsintegration/src/FSharp.Editor/CodeFixes/AddOpenCodeFixProvider.fs b/vsintegration/src/FSharp.Editor/CodeFixes/AddOpenCodeFixProvider.fs index 57bca908da6..c7299ae13b4 100644 --- a/vsintegration/src/FSharp.Editor/CodeFixes/AddOpenCodeFixProvider.fs +++ b/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 [] (assemblyContentProvider: AssemblyContentProvider) = inherit CodeFixProvider() - static let br = Environment.NewLine - let fixUnderscoresInMenuText (text: string) = text.Replace("_", "__") let qualifySymbolFix (context: CodeFixContext) (fullName, qualifier) = @@ -30,39 +27,11 @@ type internal AddOpenCodeFixProvider [] (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 diff --git a/vsintegration/src/FSharp.Editor/Common/RoslynHelpers.fs b/vsintegration/src/FSharp.Editor/Common/RoslynHelpers.fs index d7439b18b57..ebd6adb2789 100644 --- a/vsintegration/src/FSharp.Editor/Common/RoslynHelpers.fs +++ b/vsintegration/src/FSharp.Editor/Common/RoslynHelpers.fs @@ -232,63 +232,60 @@ module internal RoslynHelpers = module internal OpenDeclarationHelper = /// - /// Inserts open declaration into `SourceText`. + /// The change that adds an open declaration at the point the insertion context names. /// /// SourceText. /// Insertion context. Typically returned from tryGetInsertionContext /// Namespace to open. - let insertOpenDeclaration (sourceText: SourceText) (ctx: InsertionContext) (ns: string) : SourceText * int = - let mutable minPos = None - - let insert line lineStr (sourceText: SourceText) : SourceText = - let ln = sourceText.Lines.[line] - let pos = ln.Start - - minPos <- - match minPos with - | None -> Some pos - | Some oldPos -> Some(min oldPos pos) - - // find the line break characters on the previous line to use, Environment.NewLine should not be used - // as it makes assumptions on the line endings in the source. - let lineBreak = - ln.Text.ToString(TextSpan(ln.End, ln.EndIncludingLineBreak - ln.End)) - - sourceText.WithChanges(TextChange(TextSpan(pos, 0), lineStr + lineBreak)) - + let getOpenDeclarationChange (sourceText: SourceText) (ctx: InsertionContext) (ns: string) : TextChange = let getLineStr line = - sourceText.Lines.[line].ToString().Trim() + if line >= 0 && line < sourceText.Lines.Count then + sourceText.Lines[line].ToString().Trim() + else + "" let pos = ParsedInput.AdjustInsertionPoint getLineStr ctx - let docLine = Line.toZ pos.Line - let lineStr = (String.replicate pos.Column " ") + "open " + ns - - // If we're at the top of a file (e.g., F# script) then add a newline before adding the open declaration - let sourceText = - if docLine = 0 then - sourceText |> insert docLine Environment.NewLine |> insert docLine lineStr + let line = sourceText.Lines[min (Line.toZ pos.Line) (sourceText.Lines.Count - 1)] + + // Follow the line endings the file itself uses rather than assuming the host's. + let lineBreak = + match line.Text.ToString(TextSpan(line.End, line.EndIncludingLineBreak - line.End)) with + | "" -> Environment.NewLine + | breakChars -> breakChars + + let isHeaderScope = + match ctx.ScopeKind with + | ScopeKind.TopModule + | ScopeKind.Namespace + | ScopeKind.NestedModule -> true + | _ -> false + + // A declaration header sitting directly above becomes its own paragraph, so the open block + // below it reads as a block rather than as part of the header. + let separatorAbove = + if isHeaderScope && getLineStr (line.LineNumber - 1) <> "" then + lineBreak else - sourceText |> insert docLine lineStr + "" - // if there's no a blank line between open declaration block and the rest of the code, we add one - let sourceText = - if sourceText.Lines.[docLine + 1].ToString().Trim() <> "" then - sourceText |> insert (docLine + 1) "" - else - sourceText - - let sourceText = - // for top level module we add a blank line between the module declaration and first open statement - if - (pos.Column = 0 || ctx.ScopeKind = ScopeKind.Namespace) - && docLine > 0 - && not (sourceText.Lines.[docLine - 1].ToString().Trim().StartsWith "open") - then - sourceText |> insert docLine "" - else - sourceText + let separatorBelow = + match ctx.ScopeKind with + // The open joins the block of opens right above it. + | ScopeKind.OpenDeclaration -> lineBreak + | _ when getLineStr line.LineNumber = "" -> lineBreak + | _ -> lineBreak + lineBreak + + TextChange(TextSpan(line.Start, 0), separatorAbove + String(' ', pos.Column) + "open " + ns + separatorBelow) - sourceText, minPos |> Option.defaultValue 0 + /// + /// Inserts open declaration into `SourceText`. + /// + /// SourceText. + /// Insertion context. Typically returned from tryGetInsertionContext + /// Namespace to open. + let insertOpenDeclaration (sourceText: SourceText) (ctx: InsertionContext) (ns: string) : SourceText * int = + let change = getOpenDeclarationChange sourceText ctx ns + sourceText.WithChanges change, change.Span.Start // http://www.fssnip.net/7S3/title/Intersperse-a-list module List = diff --git a/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/AddOpenOnTopOffTests.fs b/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/AddOpenOnTopOffTests.fs index 765b1aa6210..4518c5c4dd1 100644 --- a/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/AddOpenOnTopOffTests.fs +++ b/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/AddOpenOnTopOffTests.fs @@ -125,6 +125,31 @@ Console.WriteLine 42 open System +Console.WriteLine 42 +""" + } + + let actual = codeFix |> tryFix code mode + + Assert.Equal(expected, actual) + +[] +let ``Fixes FS0039 for missing opens - module has an attribute on the same line`` () = + let code = + """[] module Module1 + +Console.WriteLine 42 +""" + + let expected = + Some + { + Message = "open System" + FixedCode = + """[] module Module1 + +open System + Console.WriteLine 42 """ } diff --git a/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/AddOpenOnTopOnTests.fs b/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/AddOpenOnTopOnTests.fs index 2614a54a493..2b10e82113d 100644 --- a/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/AddOpenOnTopOnTests.fs +++ b/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/AddOpenOnTopOnTests.fs @@ -127,6 +127,81 @@ Console.WriteLine 42 Assert.Equal(expected, actual) +[] +let ``Fixes FS0039 for missing opens - module has an attribute on the same line`` () = + let code = + """[] module Module1 + +Console.WriteLine 42 +""" + + let expected = + Some + { + Message = "open System" + FixedCode = + """[] module Module1 + +open System + +Console.WriteLine 42 +""" + } + + let actual = codeFix |> tryFix code Auto + + Assert.Equal(expected, actual) + +[] +let ``Fixes FS0039 for missing opens - explicit top level module without a blank line`` () = + let code = + """module Module1 +Console.WriteLine 42 +""" + + let expected = + Some + { + Message = "open System" + FixedCode = + """module Module1 + +open System + +Console.WriteLine 42 +""" + } + + let actual = codeFix |> tryFix code Auto + + Assert.Equal(expected, actual) + +[] +let ``Fixes FS0039 for missing opens - namespace without a blank line`` () = + let code = + """namespace N1 +module M1 = + Console.WriteLine 42 +""" + + let expected = + Some + { + Message = "open System" + FixedCode = + """namespace N1 + +open System + +module M1 = + Console.WriteLine 42 +""" + } + + let actual = codeFix |> tryFix code Auto + + Assert.Equal(expected, actual) + [] let ``Fixes FS0039 for missing opens - nested module`` () = let code = From 9d7c8dc33d8c2fbd562ae1d39ea149014892bc66 Mon Sep 17 00:00:00 2001 From: Andrii Chebukin Date: Thu, 10 Sep 2026 01:33:30 +0200 Subject: [PATCH 2/8] Run dotnet fantomas The formatting-check CI job flagged AddOpenCodeFixProvider.fs. Co-Authored-By: Claude Sonnet 5 --- .../src/FSharp.Editor/CodeFixes/AddOpenCodeFixProvider.fs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/vsintegration/src/FSharp.Editor/CodeFixes/AddOpenCodeFixProvider.fs b/vsintegration/src/FSharp.Editor/CodeFixes/AddOpenCodeFixProvider.fs index c7299ae13b4..9de03c976f1 100644 --- a/vsintegration/src/FSharp.Editor/CodeFixes/AddOpenCodeFixProvider.fs +++ b/vsintegration/src/FSharp.Editor/CodeFixes/AddOpenCodeFixProvider.fs @@ -30,8 +30,7 @@ type internal AddOpenCodeFixProvider [] (assemblyContentPr let openNamespaceFix ctx name ns multipleNames sourceText = let displayText = $"open {ns}" + (if multipleNames then " (" + name + ")" else "") - let change = - OpenDeclarationHelper.getOpenDeclarationChange sourceText ctx ns + let change = OpenDeclarationHelper.getOpenDeclarationChange sourceText ctx ns { Name = CodeFix.AddOpen From 6182b921752c9616793d546c1bc9e87d2cfeda96 Mon Sep 17 00:00:00 2001 From: Andrii Chebukin Date: Thu, 10 Sep 2026 15:02:44 +0200 Subject: [PATCH 3/8] Break the line when the declaration shares it with what precedes it The insertion point is a position, and the change was applied at the start of its line. That is the same thing whenever what precedes the declaration on that line is whitespace, and something else entirely when it is not: (* header *) let x = StringBuilder() Here the scope's first declaration is on the line that closes the comment, so the open went between `(* header` and `*)`, inside the comment, leaving the name as unresolved as it was. Whether the old code got this right was luck of the same kind: it snapped an implicit module to line 1 and so wrote above the comment here, but wrote inside it when the comment closed on a line of its own. What precedes the declaration is trivia that has to stay where it is, so the line is broken at the declaration instead, and the declaration keeps its column - moving it to the start of the line would put the open the implicit module now begins with offside of it. Co-Authored-By: Claude Opus 5 (1M context) --- .../src/FSharp.Editor/Common/RoslynHelpers.fs | 15 +++++++++++- .../CodeFixes/AddOpenOnTopOnTests.fs | 24 +++++++++++++++++++ 2 files changed, 38 insertions(+), 1 deletion(-) diff --git a/vsintegration/src/FSharp.Editor/Common/RoslynHelpers.fs b/vsintegration/src/FSharp.Editor/Common/RoslynHelpers.fs index ebd6adb2789..d815da18ecc 100644 --- a/vsintegration/src/FSharp.Editor/Common/RoslynHelpers.fs +++ b/vsintegration/src/FSharp.Editor/Common/RoslynHelpers.fs @@ -275,7 +275,20 @@ module internal OpenDeclarationHelper = | _ when getLineStr line.LineNumber = "" -> lineBreak | _ -> lineBreak + lineBreak - TextChange(TextSpan(line.Start, 0), separatorAbove + String(' ', pos.Column) + "open " + ns + separatorBelow) + let margin = String(' ', pos.Column) + let column = min pos.Column (line.End - line.Start) + let trivia = sourceText.ToString(TextSpan(line.Start, column)).TrimEnd() + + // Anything but whitespace before the insertion point is trivia the scope's first declaration + // follows on its line - a block comment closing there, say. Break the line at the declaration + // rather than write the open into the middle of what precedes it. + if trivia.Length > 0 then + TextChange( + TextSpan(line.Start + trivia.Length, column - trivia.Length), + lineBreak + margin + "open " + ns + lineBreak + lineBreak + margin + ) + else + TextChange(TextSpan(line.Start, 0), separatorAbove + margin + "open " + ns + separatorBelow) /// /// Inserts open declaration into `SourceText`. diff --git a/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/AddOpenOnTopOnTests.fs b/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/AddOpenOnTopOnTests.fs index 2b10e82113d..23e1ae1de06 100644 --- a/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/AddOpenOnTopOnTests.fs +++ b/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/AddOpenOnTopOnTests.fs @@ -78,6 +78,30 @@ Console.WriteLine 42 Assert.Equal(expected, actual) +[] // The first declaration follows a block comment closing on its line +let ``Fixes FS0039 for missing opens - declaration shares its line with the end of a comment`` () = + let code = + """(* header +*) Console.WriteLine 42 +""" + + let expected = + Some + { + Message = "open System" + FixedCode = + """(* header +*) + open System + + Console.WriteLine 42 +""" + } + + let actual = codeFix |> tryFix code Auto + + Assert.Equal(expected, actual) + [] let ``Fixes FS0039 for missing opens - there is already an open directive`` () = let code = From 9542a9b666419c50c4e746c400292ecd2198676a Mon Sep 17 00:00:00 2001 From: XperiAndri Date: Thu, 1 Oct 2026 01:21:57 +0200 Subject: [PATCH 4/8] Enter a nested module declared on one line at its first declaration In `module M = let x = Console.WriteLine 42` the line below the header is already past the use, so an open placed there left FS0039 in place. Such a module is now entered at its first declaration, and the code fix breaks the line there as it already did after a closing block comment. Co-Authored-By: Claude Opus 5.5 --- src/Compiler/Service/ServiceParsedInputOps.fs | 27 +++++++++++++------ .../Service/ServiceParsedInputOps.fsi | 2 ++ .../OpenDeclarationInsertionTests.fs | 9 +++++++ .../src/FSharp.Editor/Common/RoslynHelpers.fs | 6 ++--- .../CodeFixes/AddOpenOnTopOffTests.fs | 22 +++++++++++++++ 5 files changed, 55 insertions(+), 11 deletions(-) diff --git a/src/Compiler/Service/ServiceParsedInputOps.fs b/src/Compiler/Service/ServiceParsedInputOps.fs index aeea6420af6..3ad925f9728 100644 --- a/src/Compiler/Service/ServiceParsedInputOps.fs +++ b/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,8 @@ 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 // 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. @@ -2539,10 +2538,22 @@ module ParsedInput = | Some moduleKeyword -> headerEndLine moduleKeyword ident trivia.EqualsRange | None -> range.StartLine // Fallback if trivia unavailable - let moduleBodyIndentation = - getMinColumn decls |> Option.defaultValue (range.StartColumn + 4) + let firstDeclStart = tryGetFirstDeclStart decls - doRange NestedModule fullIdent headerLine moduleBodyIndentation + let moduleBodyIndentation = + firstDeclStart + |> ValueOption.map _.Column + |> ValueOption.defaultValue (range.StartColumn + 4) + + // 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 | SynModuleDecl.Open(_, range) -> doRange OpenDeclaration [] range.EndLine (range.StartColumn - 5) | SynModuleDecl.HashDirective(_, range) -> doRange HashDirective [] range.EndLine range.StartColumn diff --git a/src/Compiler/Service/ServiceParsedInputOps.fsi b/src/Compiler/Service/ServiceParsedInputOps.fsi index 71aecf25a28..ae868126275 100644 --- a/src/Compiler/Service/ServiceParsedInputOps.fsi +++ b/src/Compiler/Service/ServiceParsedInputOps.fsi @@ -133,6 +133,8 @@ type public InsertionContext = /// 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. + /// A nested module whose first declaration shares the header's line is entered at that + /// declaration, so there `Pos` is on the header's line, at the declaration's column. Pos: pos } diff --git a/tests/FSharp.Compiler.Service.Tests/OpenDeclarationInsertionTests.fs b/tests/FSharp.Compiler.Service.Tests/OpenDeclarationInsertionTests.fs index 21c4a98cec0..5ac6accc6ae 100644 --- a/tests/FSharp.Compiler.Service.Tests/OpenDeclarationInsertionTests.fs +++ b/tests/FSharp.Compiler.Service.Tests/OpenDeclarationInsertionTests.fs @@ -145,6 +145,15 @@ type T() = let line = findOpenInsertionLine "test.fs" source "System.IO" Assert.Equal(2, line) +[] // Regression: the line below a nested module declared on one line is already past the use +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) + [] // 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" diff --git a/vsintegration/src/FSharp.Editor/Common/RoslynHelpers.fs b/vsintegration/src/FSharp.Editor/Common/RoslynHelpers.fs index d815da18ecc..39367e54e77 100644 --- a/vsintegration/src/FSharp.Editor/Common/RoslynHelpers.fs +++ b/vsintegration/src/FSharp.Editor/Common/RoslynHelpers.fs @@ -279,9 +279,9 @@ module internal OpenDeclarationHelper = let column = min pos.Column (line.End - line.Start) let trivia = sourceText.ToString(TextSpan(line.Start, column)).TrimEnd() - // Anything but whitespace before the insertion point is trivia the scope's first declaration - // follows on its line - a block comment closing there, say. Break the line at the declaration - // rather than write the open into the middle of what precedes it. + // Anything but whitespace before the insertion point is what the scope's first declaration + // follows on its line - a block comment closing there, or the header of a module declared on + // one line. Break the line at the declaration rather than write the open into what precedes it. if trivia.Length > 0 then TextChange( TextSpan(line.Start + trivia.Length, column - trivia.Length), diff --git a/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/AddOpenOnTopOffTests.fs b/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/AddOpenOnTopOffTests.fs index 4518c5c4dd1..38c9cab6958 100644 --- a/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/AddOpenOnTopOffTests.fs +++ b/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/AddOpenOnTopOffTests.fs @@ -183,6 +183,28 @@ let ``Fixes FS0039 for missing opens - nested module`` () = Assert.Equal(expected, actual) +[] +let ``Fixes FS0039 for missing opens - nested module declared on one line`` () = + let code = + """module Module1 = let x = Console.WriteLine 42 +""" + + let expected = + Some + { + Message = "open System" + FixedCode = + """module Module1 = + open System + + let x = Console.WriteLine 42 +""" + } + + let actual = codeFix |> tryFix code mode + + Assert.Equal(expected, actual) + [] let ``Fixes FS0039 for missing opens - explicit module has attributes`` () = let code = From bced9912911a2b838f8e6fe6ca7b865e7887124e Mon Sep 17 00:00:00 2001 From: XperiAndri Date: Thu, 1 Oct 2026 01:21:59 +0200 Subject: [PATCH 5/8] Take the line break from the line above when the last line has none The open goes in front of a line, and the last line of a file often ends without a line break. The file's own line ending is then on the line above, so a file with LF endings no longer gets CRLF from the host. Co-Authored-By: Claude Opus 5.5 --- .../src/FSharp.Editor/Common/RoslynHelpers.fs | 9 +++++++-- .../CodeFixes/AddOpenOnTopOnTests.fs | 15 +++++++++++++++ 2 files changed, 22 insertions(+), 2 deletions(-) diff --git a/vsintegration/src/FSharp.Editor/Common/RoslynHelpers.fs b/vsintegration/src/FSharp.Editor/Common/RoslynHelpers.fs index 39367e54e77..9db142141d7 100644 --- a/vsintegration/src/FSharp.Editor/Common/RoslynHelpers.fs +++ b/vsintegration/src/FSharp.Editor/Common/RoslynHelpers.fs @@ -247,9 +247,14 @@ module internal OpenDeclarationHelper = let pos = ParsedInput.AdjustInsertionPoint getLineStr ctx let line = sourceText.Lines[min (Line.toZ pos.Line) (sourceText.Lines.Count - 1)] - // Follow the line endings the file itself uses rather than assuming the host's. + let lineBreakOf (line: TextLine) = + sourceText.ToString(TextSpan.FromBounds(line.End, line.EndIncludingLineBreak)) + + // Follow the line endings the file itself uses rather than assuming the host's. Only the last + // line can lack a line break, and every line above it has one. let lineBreak = - match line.Text.ToString(TextSpan(line.End, line.EndIncludingLineBreak - line.End)) with + match lineBreakOf line with + | "" when line.LineNumber > 0 -> lineBreakOf sourceText.Lines[line.LineNumber - 1] | "" -> Environment.NewLine | breakChars -> breakChars diff --git a/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/AddOpenOnTopOnTests.fs b/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/AddOpenOnTopOnTests.fs index 23e1ae1de06..3e75878f9f8 100644 --- a/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/AddOpenOnTopOnTests.fs +++ b/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/AddOpenOnTopOnTests.fs @@ -200,6 +200,21 @@ Console.WriteLine 42 Assert.Equal(expected, actual) +[] +let ``Fixes FS0039 for missing opens - the open goes before a last line with no line break`` () = + let code = "module Module1\nConsole.WriteLine 42" + + let expected = + Some + { + Message = "open System" + FixedCode = "module Module1\n\nopen System\n\nConsole.WriteLine 42" + } + + let actual = codeFix |> tryFix code Auto + + Assert.Equal(expected, actual) + [] let ``Fixes FS0039 for missing opens - namespace without a blank line`` () = let code = From 1eff62052ba565bcb1422fec0e659a22e1d04c68 Mon Sep 17 00:00:00 2001 From: XperiAndri Date: Sun, 11 Oct 2026 10:50:00 +0200 Subject: [PATCH 6/8] 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 --- src/Compiler/Service/ServiceParsedInputOps.fs | 29 +++++-- .../OpenDeclarationInsertionTests.fs | 39 ++++++--- .../Completion/CompletionProvider.fs | 19 +++-- .../CodeFixes/AddOpenOnTopOffTests.fs | 17 ++++ .../CompletionProviderTests.fs | 81 +++++++++++++++++++ 5 files changed, 163 insertions(+), 22 deletions(-) diff --git a/src/Compiler/Service/ServiceParsedInputOps.fs b/src/Compiler/Service/ServiceParsedInputOps.fs index 3ad925f9728..d1d01dcd978 100644 --- a/src/Compiler/Service/ServiceParsedInputOps.fs +++ b/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 | _ -> () diff --git a/tests/FSharp.Compiler.Service.Tests/OpenDeclarationInsertionTests.fs b/tests/FSharp.Compiler.Service.Tests/OpenDeclarationInsertionTests.fs index 5ac6accc6ae..2444993dc7c 100644 --- a/tests/FSharp.Compiler.Service.Tests/OpenDeclarationInsertionTests.fs +++ b/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) +[] // Regression: the open has to stay in `Top`, the scope of the use, and not enter `M` +[] +[] module M = let y = 1", 2)>] +[] 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) + [] // 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" diff --git a/vsintegration/src/FSharp.Editor/Completion/CompletionProvider.fs b/vsintegration/src/FSharp.Editor/Completion/CompletionProvider.fs index 45d13b0fef8..a1456218745 100644 --- a/vsintegration/src/FSharp.Editor/Completion/CompletionProvider.fs +++ b/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 = + 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 = + let insertionPoint = + if settings.CodeFixes.AlwaysPlaceOpensAtTopLevel then + OpenStatementInsertionPoint.TopLevel + else + OpenStatementInsertionPoint.Nearest + + FSharpCompletionProvider.GetChangeAsyncAux(document, item, insertionPoint) |> CancellableTask.start cancellationToken diff --git a/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/AddOpenOnTopOffTests.fs b/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/AddOpenOnTopOffTests.fs index 38c9cab6958..ffb4d0b70bb 100644 --- a/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/AddOpenOnTopOffTests.fs +++ b/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) +[] // The open has to stay in `Module1`, the scope of the use, and not enter `Module2` +[] module Module2 = let y = 1\n")>] +[] 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) + [] let ``Fixes FS0039 for missing opens - explicit module has attributes`` () = let code = diff --git a/vsintegration/tests/FSharp.Editor.Tests/CompletionProviderTests.fs b/vsintegration/tests/FSharp.Editor.Tests/CompletionProviderTests.fs index b1086037c00..9de9bf254a1 100644 --- a/vsintegration/tests/FSharp.Editor.Tests/CompletionProviderTests.fs +++ b/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()) + [] let ShouldTriggerCompletionAtCorrectMarkers () = let testCases = @@ -2167,3 +2229,22 @@ let _ = ] |> Map.ofList ) + + [] + 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 +[] module M = let y = 1 +""" + + let expected = + """module Top + +open System + +let x = Console.WriteLine 42 +[] module M = let y = 1 +""" + + VerifyCompletionWithOpen(fileContents, "[ Date: Thu, 15 Oct 2026 02:56:53 +0100 Subject: [PATCH 7/8] Name the open placement instead of commenting it The nested-module walk, the insertion helper and the public docs told in comments what their bindings can say: lineAboveBody, useMayBeInModuleAttribute, blankLineAboveOpen, blankLineBelowOpen and textBeforeDeclaration. The two public doc comments get shorter, and one comment is left, on the cross-function reason a TopModule position is reported as HashDirective. The release notes of this PR are shorter, and the "Improve Find All References" bullet that a union merge duplicated in 18.vNext.md is gone. Co-Authored-By: Claude Sonnet 5.5 --- .../.FSharp.Compiler.Service/11.0.200.md | 2 +- docs/release-notes/.VisualStudio/18.vNext.md | 3 +- src/Compiler/Service/ServiceParsedInputOps.fs | 38 +++++---------- .../Service/ServiceParsedInputOps.fsi | 11 ++--- .../src/FSharp.Editor/Common/RoslynHelpers.fs | 48 +++++++------------ 5 files changed, 33 insertions(+), 69 deletions(-) diff --git a/docs/release-notes/.FSharp.Compiler.Service/11.0.200.md b/docs/release-notes/.FSharp.Compiler.Service/11.0.200.md index ba740603b79..218f418aa3a 100644 --- a/docs/release-notes/.FSharp.Compiler.Service/11.0.200.md +++ b/docs/release-notes/.FSharp.Compiler.Service/11.0.200.md @@ -8,7 +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` 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 (`[] 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)) +* `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 `[] 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)) diff --git a/docs/release-notes/.VisualStudio/18.vNext.md b/docs/release-notes/.VisualStudio/18.vNext.md index bf579df4973..6a4d55ef8ef 100644 --- a/docs/release-notes/.VisualStudio/18.vNext.md +++ b/docs/release-notes/.VisualStudio/18.vNext.md @@ -10,8 +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` 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 (`[] 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)) +* 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 (`[] 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)) diff --git a/src/Compiler/Service/ServiceParsedInputOps.fs b/src/Compiler/Service/ServiceParsedInputOps.fs index d1d01dcd978..2c9e5d27b0c 100644 --- a/src/Compiler/Service/ServiceParsedInputOps.fs +++ b/src/Compiler/Service/ServiceParsedInputOps.fs @@ -2475,17 +2475,13 @@ module ParsedInput = | SynModuleDecl.HashDirective(range = r) -> ValueSome r.Start | _ -> ValueNone - // 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 + let nameEndLine = + ident + |> List.fold (fun line part -> max line part.idRange.EndLine) keyword.EndLine - match equals with - | Some equalsRange -> max nameEnd equalsRange.EndLine - | None -> nameEnd + equals + |> Option.fold (fun line equalsSign -> max line equalsSign.EndLine) nameEndLine let rec walkImplFileInput (file: ParsedImplFileInput) = List.iter (walkSynModuleOrNamespace []) file.Contents @@ -2507,11 +2503,10 @@ module ParsedInput = let fullIdent = parent @ ident - let headerLine = + let lineAboveBody = match trivia.LeadingKeyword with | 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 = @@ -2520,7 +2515,7 @@ module ParsedInput = | true, _ -> NestedModule | _ -> Namespace - doRange scopeKind fullIdent headerLine range.StartColumn + doRange scopeKind fullIdent lineAboveBody range.StartColumn addModule (fullIdent, range) List.iter (walkSynModuleDecl fullIdent) decls @@ -2547,29 +2542,21 @@ module ParsedInput = |> ValueOption.defaultValue (range.StartColumn + 4) let bodyStartsOnHeaderLine = - match firstDeclStart with - | ValueSome start -> start.Line = headerLine - | ValueNone -> false + firstDeclStart |> ValueOption.exists (fun start -> start.Line = headerLine) - // 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 = 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 = + let useMayBeInModuleAttribute = bodyStartsOnHeaderLine && currentLine = headerLine && attributes |> List.exists (fun attributeList -> attributeList.Range.EndLine = headerLine) - if headerLine <= currentLine && not mayBeInAttribute then + 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) @@ -2646,8 +2633,6 @@ module ParsedInput = entities |> Array.map (fun e -> e, findBestPositionToInsertOpenDeclaration modules scope pos entity) - /// 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 = match ctx.ScopeKind with | ScopeKind.TopModule @@ -2696,8 +2681,7 @@ module ParsedInput = | _ -> 0 if lastReferenceLine > 0 then - // 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. + // AdjustInsertionPoint would move a TopModule position below the blank line under the directives. let scopeKind = if ctx.ScopeKind = ScopeKind.TopModule then ScopeKind.HashDirective diff --git a/src/Compiler/Service/ServiceParsedInputOps.fsi b/src/Compiler/Service/ServiceParsedInputOps.fsi index ae868126275..82ed18424e9 100644 --- a/src/Compiler/Service/ServiceParsedInputOps.fsi +++ b/src/Compiler/Service/ServiceParsedInputOps.fsi @@ -131,10 +131,8 @@ type public InsertionContext = /// Current scope kind. ScopeKind: ScopeKind - /// 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. - /// A nested module whose first declaration shares the header's line is entered at that - /// declaration, so there `Pos` is on the header's line, at the declaration's column. + /// 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 } @@ -210,9 +208,8 @@ module public ParsedInput = /// Returns long identifier at position. val GetLongIdentAt: parsedInput: ParsedInput -> pos: pos -> LongIdent option - /// 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. + /// 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 diff --git a/vsintegration/src/FSharp.Editor/Common/RoslynHelpers.fs b/vsintegration/src/FSharp.Editor/Common/RoslynHelpers.fs index 9db142141d7..27449e189a3 100644 --- a/vsintegration/src/FSharp.Editor/Common/RoslynHelpers.fs +++ b/vsintegration/src/FSharp.Editor/Common/RoslynHelpers.fs @@ -231,12 +231,6 @@ module internal RoslynHelpers = Async.Start(computation, cancellationToken) module internal OpenDeclarationHelper = - /// - /// The change that adds an open declaration at the point the insertion context names. - /// - /// SourceText. - /// Insertion context. Typically returned from tryGetInsertionContext - /// Namespace to open. let getOpenDeclarationChange (sourceText: SourceText) (ctx: InsertionContext) (ns: string) : TextChange = let getLineStr line = if line >= 0 && line < sourceText.Lines.Count then @@ -250,50 +244,40 @@ module internal OpenDeclarationHelper = let lineBreakOf (line: TextLine) = sourceText.ToString(TextSpan.FromBounds(line.End, line.EndIncludingLineBreak)) - // Follow the line endings the file itself uses rather than assuming the host's. Only the last - // line can lack a line break, and every line above it has one. let lineBreak = match lineBreakOf line with | "" when line.LineNumber > 0 -> lineBreakOf sourceText.Lines[line.LineNumber - 1] | "" -> Environment.NewLine | breakChars -> breakChars - let isHeaderScope = + let blankLineAboveOpen = match ctx.ScopeKind with | ScopeKind.TopModule | ScopeKind.Namespace - | ScopeKind.NestedModule -> true - | _ -> false + | ScopeKind.NestedModule when getLineStr (line.LineNumber - 1) <> "" -> lineBreak + | _ -> "" - // A declaration header sitting directly above becomes its own paragraph, so the open block - // below it reads as a block rather than as part of the header. - let separatorAbove = - if isHeaderScope && getLineStr (line.LineNumber - 1) <> "" then - lineBreak - else + let blankLineBelowOpen = + if ctx.ScopeKind = ScopeKind.OpenDeclaration || getLineStr line.LineNumber = "" then "" - - let separatorBelow = - match ctx.ScopeKind with - // The open joins the block of opens right above it. - | ScopeKind.OpenDeclaration -> lineBreak - | _ when getLineStr line.LineNumber = "" -> lineBreak - | _ -> lineBreak + lineBreak + else + lineBreak let margin = String(' ', pos.Column) let column = min pos.Column (line.End - line.Start) - let trivia = sourceText.ToString(TextSpan(line.Start, column)).TrimEnd() - // Anything but whitespace before the insertion point is what the scope's first declaration - // follows on its line - a block comment closing there, or the header of a module declared on - // one line. Break the line at the declaration rather than write the open into what precedes it. - if trivia.Length > 0 then + let textBeforeDeclaration = + sourceText.ToString(TextSpan(line.Start, column)).TrimEnd() + + let openLine = margin + "open " + ns + lineBreak + + if textBeforeDeclaration.Length > 0 then TextChange( - TextSpan(line.Start + trivia.Length, column - trivia.Length), - lineBreak + margin + "open " + ns + lineBreak + lineBreak + margin + TextSpan(line.Start + textBeforeDeclaration.Length, column - textBeforeDeclaration.Length), + lineBreak + openLine + lineBreak + margin ) else - TextChange(TextSpan(line.Start, 0), separatorAbove + margin + "open " + ns + separatorBelow) + TextChange(TextSpan(line.Start, 0), blankLineAboveOpen + openLine + blankLineBelowOpen) /// /// Inserts open declaration into `SourceText`. From 9e488eaed14b979181e23b188be6f5ca70ef3473 Mon Sep 17 00:00:00 2001 From: XperiAndri Date: Thu, 15 Oct 2026 02:57:04 +0100 Subject: [PATCH 8/8] Run the open placement cases as theories The placement facts become theories over named scenarios, in the FCS tests and in the On and Off settings files of the editor tests. The case of a module with an attribute on its own line was copied into both settings files; it is one theory in AddOpenOnTopSharedTests now, run under both settings, and the helper that expects the open System fix lives there too. The completion test helper drops its doc comment and a binding. The cases and their expected results are the same as before. Co-Authored-By: Claude Sonnet 5.5 --- .../OpenDeclarationInsertionTests.fs | 61 +++------ .../CodeFixes/AddOpenOnTopOffTests.fs | 83 +++--------- .../CodeFixes/AddOpenOnTopOnTests.fs | 127 +++--------------- .../CodeFixes/AddOpenOnTopSharedTests.fs | 39 ++++++ .../CompletionProviderTests.fs | 61 ++++----- .../FSharp.Editor.Tests.fsproj | 1 + 6 files changed, 122 insertions(+), 250 deletions(-) create mode 100644 vsintegration/tests/FSharp.Editor.Tests/CodeFixes/AddOpenOnTopSharedTests.fs diff --git a/tests/FSharp.Compiler.Service.Tests/OpenDeclarationInsertionTests.fs b/tests/FSharp.Compiler.Service.Tests/OpenDeclarationInsertionTests.fs index 2444993dc7c..3c035504121 100644 --- a/tests/FSharp.Compiler.Service.Tests/OpenDeclarationInsertionTests.fs +++ b/tests/FSharp.Compiler.Service.Tests/OpenDeclarationInsertionTests.fs @@ -4,16 +4,10 @@ open Xunit open FSharp.Compiler.EditorServices open FSharp.Compiler.Service.Tests.Common -/// Computes the position an `open` would be inserted at for a use on `currentLine`, mirroring the real -/// editor consumer (RoslynHelpers.insertContext): `FindNearestPointToInsertOpenDeclaration` followed by +/// 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 findOpenInsertionAt - (currentLine: int) - (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') @@ -29,17 +23,8 @@ let private findOpenInsertionAt 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 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 @@ -128,36 +113,24 @@ let ``Open not forced above named module in fsx`` () = let x = System.IO.File.ReadAllText "a" """ let line = findOpenInsertionLine "test.fsx" source "System.IO" - // Inside the module, next to the code rather than in the gap under the header. Assert.Equal(3, line) -[] // 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 = """[] module Foo +[] +[] module Foo let x = System.IO.File.ReadAllText "a" -""" - let line = findOpenInsertionLine "test.fs" source "System.IO" - Assert.Equal(3, line) - -[] // 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 +""", 3)>] +[] -let ``Open placed under namespace without a blank line under the header`` () = - let source = """namespace Ns +""", 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") -[] // Regression: the line below a nested module declared on one line is already past the use +[] let ``Open placed at the first declaration of a nested module declared on one line`` () = let source = """module Top @@ -166,10 +139,10 @@ module M = let x = System.IO.File.ReadAllText "a" let pos = findOpenInsertion OpenStatementInsertionPoint.Nearest "test.fs" source "System.IO" Assert.Equal((3, 11), pos) -[] // Regression: the open has to stay in `Top`, the scope of the use, and not enter `M` +[] [] [] module M = let y = 1", 2)>] -[] module M = let y = 1", 3)>] // the use is the attribute on the header's line +[] 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" diff --git a/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/AddOpenOnTopOffTests.fs b/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/AddOpenOnTopOffTests.fs index ffb4d0b70bb..05be83a2cc2 100644 --- a/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/AddOpenOnTopOffTests.fs +++ b/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/AddOpenOnTopOffTests.fs @@ -6,6 +6,7 @@ open Microsoft.VisualStudio.FSharp.Editor open Xunit open CodeFixTestFramework +open AddOpenOnTopSharedTests let private codeFix = AddOpenCodeFixProvider(AssemblyContentProvider()) @@ -125,31 +126,6 @@ Console.WriteLine 42 open System -Console.WriteLine 42 -""" - } - - let actual = codeFix |> tryFix code mode - - Assert.Equal(expected, actual) - -[] -let ``Fixes FS0039 for missing opens - module has an attribute on the same line`` () = - let code = - """[] module Module1 - -Console.WriteLine 42 -""" - - let expected = - Some - { - Message = "open System" - FixedCode = - """[] module Module1 - -open System - Console.WriteLine 42 """ } @@ -183,44 +159,25 @@ let ``Fixes FS0039 for missing opens - nested module`` () = Assert.Equal(expected, actual) -[] -let ``Fixes FS0039 for missing opens - nested module declared on one line`` () = - let code = - """module Module1 = let x = Console.WriteLine 42 -""" - - let expected = - Some - { - Message = "open System" - FixedCode = - """module Module1 = - open System - - let x = Console.WriteLine 42 -""" - } - - let actual = codeFix |> tryFix code mode - - Assert.Equal(expected, actual) - -[] // The open has to stay in `Module1`, the scope of the use, and not enter `Module2` -[] module Module2 = let y = 1\n")>] -[] 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) +let placements = + scenarios + [ + ("nested module declared on one line", + "module Module1 = let x = Console.WriteLine 42\n", + "module Module1 =\n open System\n\n let x = Console.WriteLine 42\n") + + ("use before a nested module declared on one line", + "module Module1\nlet x = Console.WriteLine 42\n[] module Module2 = let y = 1\n", + "module Module1\n\nopen System\n\nlet x = Console.WriteLine 42\n[] module Module2 = let y = 1\n") + + ("use in the attribute of a nested module declared on one line", + "module Module1\n[] module Module2 = let y = 1\n", + "module Module1\n\nopen System\n\n[] module Module2 = let y = 1\n") + ] + +[] +let ``Fixes FS0039 for missing opens - placement`` (_scenario: string, code: string, fixedCode: string) = + expectOpenSystem mode code fixedCode [] let ``Fixes FS0039 for missing opens - explicit module has attributes`` () = diff --git a/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/AddOpenOnTopOnTests.fs b/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/AddOpenOnTopOnTests.fs index 3e75878f9f8..bc12d8b54f3 100644 --- a/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/AddOpenOnTopOnTests.fs +++ b/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/AddOpenOnTopOnTests.fs @@ -6,6 +6,7 @@ open Microsoft.VisualStudio.FSharp.Editor open Xunit open CodeFixTestFramework +open AddOpenOnTopSharedTests let private codeFix = AddOpenCodeFixProvider(AssemblyContentProvider()) @@ -78,29 +79,27 @@ Console.WriteLine 42 Assert.Equal(expected, actual) -[] // The first declaration follows a block comment closing on its line -let ``Fixes FS0039 for missing opens - declaration shares its line with the end of a comment`` () = - let code = - """(* header -*) Console.WriteLine 42 -""" +let placements = + scenarios + [ + ("declaration shares its line with the end of a comment", + "(* header\n*) Console.WriteLine 42\n", + "(* header\n*)\n open System\n\n Console.WriteLine 42\n") - let expected = - Some - { - Message = "open System" - FixedCode = - """(* header -*) - open System + ("explicit top level module without a blank line", + "module Module1\nConsole.WriteLine 42\n", + "module Module1\n\nopen System\n\nConsole.WriteLine 42\n") - Console.WriteLine 42 -""" - } + ("last line with no line break", "module Module1\nConsole.WriteLine 42", "module Module1\n\nopen System\n\nConsole.WriteLine 42") - let actual = codeFix |> tryFix code Auto + ("namespace without a blank line", + "namespace N1\nmodule M1 =\n Console.WriteLine 42\n", + "namespace N1\n\nopen System\n\nmodule M1 =\n Console.WriteLine 42\n") + ] - Assert.Equal(expected, actual) +[] +let ``Fixes FS0039 for missing opens - placement`` (_scenario: string, code: string, fixedCode: string) = + expectOpenSystem Auto code fixedCode [] let ``Fixes FS0039 for missing opens - there is already an open directive`` () = @@ -151,96 +150,6 @@ Console.WriteLine 42 Assert.Equal(expected, actual) -[] -let ``Fixes FS0039 for missing opens - module has an attribute on the same line`` () = - let code = - """[] module Module1 - -Console.WriteLine 42 -""" - - let expected = - Some - { - Message = "open System" - FixedCode = - """[] module Module1 - -open System - -Console.WriteLine 42 -""" - } - - let actual = codeFix |> tryFix code Auto - - Assert.Equal(expected, actual) - -[] -let ``Fixes FS0039 for missing opens - explicit top level module without a blank line`` () = - let code = - """module Module1 -Console.WriteLine 42 -""" - - let expected = - Some - { - Message = "open System" - FixedCode = - """module Module1 - -open System - -Console.WriteLine 42 -""" - } - - let actual = codeFix |> tryFix code Auto - - Assert.Equal(expected, actual) - -[] -let ``Fixes FS0039 for missing opens - the open goes before a last line with no line break`` () = - let code = "module Module1\nConsole.WriteLine 42" - - let expected = - Some - { - Message = "open System" - FixedCode = "module Module1\n\nopen System\n\nConsole.WriteLine 42" - } - - let actual = codeFix |> tryFix code Auto - - Assert.Equal(expected, actual) - -[] -let ``Fixes FS0039 for missing opens - namespace without a blank line`` () = - let code = - """namespace N1 -module M1 = - Console.WriteLine 42 -""" - - let expected = - Some - { - Message = "open System" - FixedCode = - """namespace N1 - -open System - -module M1 = - Console.WriteLine 42 -""" - } - - let actual = codeFix |> tryFix code Auto - - Assert.Equal(expected, actual) - [] let ``Fixes FS0039 for missing opens - nested module`` () = let code = diff --git a/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/AddOpenOnTopSharedTests.fs b/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/AddOpenOnTopSharedTests.fs new file mode 100644 index 00000000000..b52f6a0d087 --- /dev/null +++ b/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/AddOpenOnTopSharedTests.fs @@ -0,0 +1,39 @@ +// Copyright (c) Microsoft Corporation. All Rights Reserved. See License.txt in the project root for license information. + +module FSharp.Editor.Tests.CodeFixes.AddOpenOnTopSharedTests + +open Microsoft.VisualStudio.FSharp.Editor +open Xunit + +open CodeFixTestFramework + +let private codeFix = AddOpenCodeFixProvider(AssemblyContentProvider()) + +let scenarios (cases: (string * string * string) list) : obj[] seq = + cases + |> Seq.map (fun (name, code, fixedCode) -> [| box name; box code; box fixedCode |]) + +let expectOpenSystem mode code fixedCode = + let expected = + Some + { + Message = "open System" + FixedCode = fixedCode + } + + Assert.Equal(expected, codeFix |> tryFix code mode) + +[] +[] +[] +let ``Fixes FS0039 for missing opens - module has an attribute on the same line`` (alwaysPlaceOpensAtTopLevel: bool) = + let mode = + WithSettings + { CodeFixesOptions.Default with + AlwaysPlaceOpensAtTopLevel = alwaysPlaceOpensAtTopLevel + } + + expectOpenSystem + mode + "[] module Module1\n\nConsole.WriteLine 42\n" + "[] module Module1\n\nopen System\n\nConsole.WriteLine 42\n" diff --git a/vsintegration/tests/FSharp.Editor.Tests/CompletionProviderTests.fs b/vsintegration/tests/FSharp.Editor.Tests/CompletionProviderTests.fs index 9de9bf254a1..1f288031a63 100644 --- a/vsintegration/tests/FSharp.Editor.Tests/CompletionProviderTests.fs +++ b/vsintegration/tests/FSharp.Editor.Tests/CompletionProviderTests.fs @@ -149,16 +149,14 @@ 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 sourceText = SourceText.From(fileContents) let document = RoslynTestHelpers.CreateSolution(fileContents) |> RoslynTestHelpers.GetSingleDocument - let sourceText = SourceText.From(fileContents) - let span = CompletionUtils.getDefaultCompletionListSpan ( sourceText, @@ -170,9 +168,6 @@ module CompletionProviderTests = 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 @@ -182,34 +177,32 @@ module CompletionProviderTests = 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()) + cancellableTask { + let! items = + FSharpCompletionProvider.ProvideCompletionsAsyncAux( + document, + caretPosition, + AssemblyContentProvider().GetAllEntitiesInProjectAndReferencedAssemblies, + false + ) + + let item = + items + |> Seq.find (fun item -> item.DisplayText = itemName && item.Properties.ContainsKey "NamespaceToOpen") + + // CompletionItem.WithSpan is obsolete: the context hands the item its span. + context.AddItem item + + return! + FSharpCompletionProvider.GetChangeAsyncAux( + document, + item, + FSharp.Compiler.EditorServices.OpenStatementInsertionPoint.Nearest + ) + } + |> CancellableTask.start CancellationToken.None + + Assert.Equal(expected, sourceText.WithChanges(change.Result.TextChange).ToString()) [] let ShouldTriggerCompletionAtCorrectMarkers () = diff --git a/vsintegration/tests/FSharp.Editor.Tests/FSharp.Editor.Tests.fsproj b/vsintegration/tests/FSharp.Editor.Tests/FSharp.Editor.Tests.fsproj index a62b25af1cf..0fa3b751ed7 100644 --- a/vsintegration/tests/FSharp.Editor.Tests/FSharp.Editor.Tests.fsproj +++ b/vsintegration/tests/FSharp.Editor.Tests/FSharp.Editor.Tests.fsproj @@ -62,6 +62,7 @@ +