From 00c36e2421c18ce0fa68002a8071fbc61b180d58 Mon Sep 17 00:00:00 2001 From: Andrii Chebukin Date: Wed, 9 Sep 2026 19:09:33 +0200 Subject: [PATCH 1/6] 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.100.md | 1 + docs/release-notes/.VisualStudio/18.vNext.md | 1 + 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, 211 insertions(+), 141 deletions(-) diff --git a/docs/release-notes/.FSharp.Compiler.Service/11.0.100.md b/docs/release-notes/.FSharp.Compiler.Service/11.0.100.md index 27f2c0070d7..c3f1caca057 100644 --- a/docs/release-notes/.FSharp.Compiler.Service/11.0.100.md +++ b/docs/release-notes/.FSharp.Compiler.Service/11.0.100.md @@ -1,6 +1,7 @@ ### Fixed * Fix internal error FS0192 "Iterate2D" when a `[]` parameter auto-quotes an argument that captures a not-yet-generalized use of an inferred generically-recursive function. The auto-quoted (`Expr.WithValue`) copy now keeps a fresh link to the recursive-value use so it receives the same inferred type arguments as the executable expression at the letrec point. ([Issue #20379](https://github.com/dotnet/fsharp/issues/20379)) +* `ParsedInput.FindNearestPointToInsertOpenDeclaration` now reports where an `open` belongs from the syntax tree instead of leaving consumers to recognize a declaration header by its text. `InsertionContext.Pos` is the first line inside the scope — below the module or namespace header, or below the opens and directives above it — for every `ScopeKind`, so an `open` is no longer placed above a header the text sniffing failed to recognize (`[] module Ns.Name` on one line) or below the first declaration when no blank line separates it from the header. `ParsedInput.AdjustInsertionPoint` keeps only the cosmetic step of skipping the blank line under a header. ([PR #20500](https://github.com/dotnet/fsharp/pull/20500)) * Fix `NativePtr.stackalloc` nested in a larger expression (e.g. a call argument or the right of an assignment) producing an assembly that throws `InvalidProgramException` at load. ([Issue #8083](https://github.com/dotnet/fsharp/issues/8083), [PR #20302](https://github.com/dotnet/fsharp/pull/20302)) * Fix internal error "Unexpected generalized type variables when compiling an active pattern" when an active pattern is used in a `let` binding whose right-hand side is a generic value, e.g. `let (T) = id`. Such a binding is now checked like the equivalent `match` and is not generalized. ([Issue #16856](https://github.com/dotnet/fsharp/issues/16856), [PR #20383](https://github.com/dotnet/fsharp/pull/20383)) * Fix Release-only (`--optimize+`) `System.InvalidProgramException` from `Seq.collect` / `yield!` over a value-type (struct) collection implementing `seq<'T>` (e.g. `ImmutableArray<_>`) when materialised with `List.ofSeq` / `Seq.toList` / `Seq.toArray` or a list/array comprehension. The collector lowering now boxes a struct sub-collection to `seq<'T>` before calling `AddMany`/`AddManyAndClose` (matching the coercion the type checker already inserts for `yield!`), and uses `unit` as the try/finally result type instead of the body type (removing a spurious `ldnull` store). ([Issue #20203](https://github.com/dotnet/fsharp/issues/20203)) diff --git a/docs/release-notes/.VisualStudio/18.vNext.md b/docs/release-notes/.VisualStudio/18.vNext.md index ba03f663967..adf65dab12b 100644 --- a/docs/release-notes/.VisualStudio/18.vNext.md +++ b/docs/release-notes/.VisualStudio/18.vNext.md @@ -5,6 +5,7 @@ ### Fixed +* The "open namespace" code fix and the completion that adds an `open` now place it from the syntax tree rather than by recognizing a `module` line by its text, so it no longer lands above the file's own module declaration (`[] 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)) 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 2679740fbb3..d157875ab10 100644 --- a/vsintegration/src/FSharp.Editor/Common/RoslynHelpers.fs +++ b/vsintegration/src/FSharp.Editor/Common/RoslynHelpers.fs @@ -230,63 +230,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 f8bd9728b4b994011a1ceecc8730492c92f2feec Mon Sep 17 00:00:00 2001 From: Andrii Chebukin Date: Thu, 10 Sep 2026 01:33:30 +0200 Subject: [PATCH 2/6] 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 f5dd4ff5ad263d69bccbb1e807a0c81a4c6fcec8 Mon Sep 17 00:00:00 2001 From: Andrii Chebukin Date: Thu, 10 Sep 2026 15:02:44 +0200 Subject: [PATCH 3/6] 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 d157875ab10..8f47a6545f8 100644 --- a/vsintegration/src/FSharp.Editor/Common/RoslynHelpers.fs +++ b/vsintegration/src/FSharp.Editor/Common/RoslynHelpers.fs @@ -273,7 +273,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 59166e4d796f92c3b5e5e7fd74703f7df8a11221 Mon Sep 17 00:00:00 2001 From: Andrii Chebukin Date: Wed, 9 Sep 2026 19:11:56 +0200 Subject: [PATCH 4/6] Suggest open type where a plain open cannot reach MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A plain `open` takes namespaces and F# modules. The code fix offered one for every candidate it found, so an unresolved `File` was answered with `open System.Net.WebRequestMethods` and an unresolved `WriteLine` with `open System.Console`, `open System.Diagnostics.Debug` and `open System.Diagnostics.Trace` — all of them code that does not compile, because those name types, and a type's nested types and static members are only brought into scope by `open type`. Nothing in the entity model told the two apart: the namespace to open was the dotted name of the candidate's container, whatever kind of thing that container was. `AssemblySymbol` now records how far a plain open reaches — the namespace, extended by each enclosing F# module — and `InsertionContextEntity` how much of the candidate's name the suggested open covers, which the qualification already written at the use site and auto-open modules can both shorten. The code fix compares the two and writes `open type` when the suggestion reaches past what a plain open can. Co-Authored-By: Claude Opus 5 (1M context) --- .../.FSharp.Compiler.Service/11.0.100.md | 1 + docs/release-notes/.VisualStudio/18.vNext.md | 1 + .../Service/ServiceAssemblyContent.fs | 23 ++++++- .../Service/ServiceAssemblyContent.fsi | 4 ++ src/Compiler/Service/ServiceParsedInputOps.fs | 14 ++++- .../Service/ServiceParsedInputOps.fsi | 5 ++ ...iler.Service.SurfaceArea.netstandard20.bsl | 8 ++- .../CodeFixes/AddOpenCodeFixProvider.fs | 61 ++++++++++++------- .../src/FSharp.Editor/Common/RoslynHelpers.fs | 10 +-- .../CodeFixes/AddOpenOnTopOnTests.fs | 50 +++++++++++++++ 10 files changed, 141 insertions(+), 36 deletions(-) diff --git a/docs/release-notes/.FSharp.Compiler.Service/11.0.100.md b/docs/release-notes/.FSharp.Compiler.Service/11.0.100.md index c3f1caca057..cfed5a8a782 100644 --- a/docs/release-notes/.FSharp.Compiler.Service/11.0.100.md +++ b/docs/release-notes/.FSharp.Compiler.Service/11.0.100.md @@ -2,6 +2,7 @@ * Fix internal error FS0192 "Iterate2D" when a `[]` parameter auto-quotes an argument that captures a not-yet-generalized use of an inferred generically-recursive function. The auto-quoted (`Expr.WithValue`) copy now keeps a fresh link to the recursive-value use so it receives the same inferred type arguments as the executable expression at the letrec point. ([Issue #20379](https://github.com/dotnet/fsharp/issues/20379)) * `ParsedInput.FindNearestPointToInsertOpenDeclaration` now reports where an `open` belongs from the syntax tree instead of leaving consumers to recognize a declaration header by its text. `InsertionContext.Pos` is the first line inside the scope — below the module or namespace header, or below the opens and directives above it — for every `ScopeKind`, so an `open` is no longer placed above a header the text sniffing failed to recognize (`[] 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)) +* `AssemblySymbol` and `InsertionContextEntity` now carry how far a plain `open` reaches (`OpenableIdentCount`) and how much of the entity's name the suggested one covers (`NamespaceIdentCount`), so a consumer can tell the namespaces and F# modules a plain `open` takes from the types only `open type` opens. Until now nothing distinguished them, and tooling suggested opening a type as if it were a namespace. ([PR #20501](https://github.com/dotnet/fsharp/pull/20501)) * Fix `NativePtr.stackalloc` nested in a larger expression (e.g. a call argument or the right of an assignment) producing an assembly that throws `InvalidProgramException` at load. ([Issue #8083](https://github.com/dotnet/fsharp/issues/8083), [PR #20302](https://github.com/dotnet/fsharp/pull/20302)) * Fix internal error "Unexpected generalized type variables when compiling an active pattern" when an active pattern is used in a `let` binding whose right-hand side is a generic value, e.g. `let (T) = id`. Such a binding is now checked like the equivalent `match` and is not generalized. ([Issue #16856](https://github.com/dotnet/fsharp/issues/16856), [PR #20383](https://github.com/dotnet/fsharp/pull/20383)) * Fix Release-only (`--optimize+`) `System.InvalidProgramException` from `Seq.collect` / `yield!` over a value-type (struct) collection implementing `seq<'T>` (e.g. `ImmutableArray<_>`) when materialised with `List.ofSeq` / `Seq.toList` / `Seq.toArray` or a list/array comprehension. The collector lowering now boxes a struct sub-collection to `seq<'T>` before calling `AddMany`/`AddManyAndClose` (matching the coercion the type checker already inserts for `yield!`), and uses `unit` as the try/finally result type instead of the body type (removing a spurious `ldnull` store). ([Issue #20203](https://github.com/dotnet/fsharp/issues/20203)) diff --git a/docs/release-notes/.VisualStudio/18.vNext.md b/docs/release-notes/.VisualStudio/18.vNext.md index adf65dab12b..23f1b78c373 100644 --- a/docs/release-notes/.VisualStudio/18.vNext.md +++ b/docs/release-notes/.VisualStudio/18.vNext.md @@ -6,6 +6,7 @@ ### Fixed * The "open namespace" code fix and the completion that adds an `open` now place it from the syntax tree rather than by recognizing a `module` line by its text, so it no longer lands above the file's own module declaration (`[] 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)) +* The "open namespace" code fix offers `open type System.Console` where it used to offer `open System.Console`. A plain `open` only reaches namespaces and F# modules, so for a type nested in a type (`System.Net.WebRequestMethods.File`) or a static member of one (`System.Console.WriteLine`) every suggestion it made was code that does not compile — `FS0039: The namespace 'Console' is not defined`. ([PR #20501](https://github.com/dotnet/fsharp/pull/20501)) * 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)) diff --git a/src/Compiler/Service/ServiceAssemblyContent.fs b/src/Compiler/Service/ServiceAssemblyContent.fs index f08faa996a5..9fa739b2b5a 100644 --- a/src/Compiler/Service/ServiceAssemblyContent.fs +++ b/src/Compiler/Service/ServiceAssemblyContent.fs @@ -39,6 +39,7 @@ type AssemblySymbol = { FullName: string CleanedIdents: ShortIdents Namespace: ShortIdents option + OpenableIdentCount: int NearestRequireQualifiedAccessParent: ShortIdents option TopRequireQualifiedAccessParent: ShortIdents option AutoOpenParent: ShortIdents option @@ -57,7 +58,10 @@ type Parent = TopRequiresQualifiedAccess: (* isForMemberOrValue *) bool -> ShortIdents option AutoOpen: ShortIdents option WithModuleSuffix: ShortIdents option - IsModule: bool } + IsModule: bool + /// How many leading idents a plain `open` reaches. A type never extends it: everything it + /// contains is reached by naming the type, or by opening the type itself. + OpenableIdentCount: int } static member Empty = { Namespace = None @@ -65,7 +69,13 @@ type Parent = TopRequiresQualifiedAccess = fun _ -> None AutoOpen = None WithModuleSuffix = None - IsModule = true } + IsModule = true + OpenableIdentCount = 0 } + + /// The reach of a plain `open` over an entity declared in this parent: the enclosing namespace + /// at the very least, and whatever modules the parent chain has added to it. + member x.OpenableIdentCountFor (ns: ShortIdents option) = + max x.OpenableIdentCount (ns |> Option.map Array.length |> Option.defaultValue 0) static member RewriteParentIdents (parentIdents: ShortIdents option) (idents: ShortIdents) = match parentIdents with @@ -149,6 +159,7 @@ module AssemblyContent = { FullName = fullName CleanedIdents = cleanIdents Namespace = ns + OpenableIdentCount = parent.OpenableIdentCountFor ns NearestRequireQualifiedAccessParent = parent.ThisRequiresQualifiedAccess false |> Option.map parent.FixParentModuleSuffix TopRequireQualifiedAccessParent = topRequireQualifiedAccessParent AutoOpenParent = parent.AutoOpen |> Option.map parent.FixParentModuleSuffix @@ -179,6 +190,7 @@ module AssemblyContent = { FullName = fullName CleanedIdents = cleanedIdents Namespace = ns + OpenableIdentCount = parent.OpenableIdentCountFor ns NearestRequireQualifiedAccessParent = parent.ThisRequiresQualifiedAccess true |> Option.map parent.FixParentModuleSuffix TopRequireQualifiedAccessParent = topRequireQualifiedAccessParent AutoOpenParent = autoOpenParent @@ -250,7 +262,12 @@ module AssemblyContent = else parent.WithModuleSuffix Namespace = ns - IsModule = entity.IsFSharpModule } + IsModule = entity.IsFSharpModule + + OpenableIdentCount = + match entity.IsNamespace || entity.IsFSharpModule, currentEntity with + | true, Some e -> e.CleanedIdents.Length + | _ -> parent.OpenableIdentCountFor ns } match entity.TryGetMembersFunctionsAndValues() with | xs when xs.Count > 0 -> diff --git a/src/Compiler/Service/ServiceAssemblyContent.fsi b/src/Compiler/Service/ServiceAssemblyContent.fsi index 09756eee2e5..7fc789aa85e 100644 --- a/src/Compiler/Service/ServiceAssemblyContent.fsi +++ b/src/Compiler/Service/ServiceAssemblyContent.fsi @@ -37,6 +37,10 @@ type public AssemblySymbol = /// `FSharpEntity.Namespace`. Namespace: ShortIdents option + /// How many leading `CleanedIdents` a plain `open` reaches: the namespace plus the enclosing + /// F# modules. Idents past it name types, whose contents `open type` brings into scope instead. + OpenableIdentCount: int + /// The most narrative parent module that has `RequireQualifiedAccess` attribute. NearestRequireQualifiedAccessParent: ShortIdents option diff --git a/src/Compiler/Service/ServiceParsedInputOps.fs b/src/Compiler/Service/ServiceParsedInputOps.fs index aeea6420af6..096c698b730 100644 --- a/src/Compiler/Service/ServiceParsedInputOps.fs +++ b/src/Compiler/Service/ServiceParsedInputOps.fs @@ -158,6 +158,7 @@ type InsertionContextEntity = FullRelativeName: string Qualifier: string Namespace: string option + NamespaceIdentCount: int FullDisplayName: string LastIdent: ShortIdent } @@ -254,12 +255,18 @@ module Entity = | _ -> let fullRelativeName = Array.append (getRelativeNs fullOpenableNs) restIdents + // What the suggested `open` covers, named relatively to the current scope and, + // for the count, absolutely: the two differ by the prefix already in scope. + let shortenByQualifiedIdents (idents: ShortIdents) = + if identCount > 1 && relativeNs.Length >= identCount then + idents[0 .. idents.Length - identCount] + else + idents + let ns = match relativeNs with | [||] -> None - | _ when identCount > 1 && relativeNs.Length >= identCount -> - Some(relativeNs[0 .. relativeNs.Length - identCount] |> String.concat ".") - | _ -> Some(relativeNs |> String.concat ".") + | _ -> Some(shortenByQualifiedIdents relativeNs |> String.concat ".") let qualifier = if fullRelativeName.Length > 1 && fullRelativeName.Length >= identCount then @@ -272,6 +279,7 @@ module Entity = FullRelativeName = String.concat "." fullRelativeName //.[0..fullRelativeName.Length - identCount - 1] Qualifier = String.concat "." qualifier Namespace = ns + NamespaceIdentCount = (shortenByQualifiedIdents openableNs).Length FullDisplayName = match restIdents with | [| _ |] -> "" diff --git a/src/Compiler/Service/ServiceParsedInputOps.fsi b/src/Compiler/Service/ServiceParsedInputOps.fsi index 71aecf25a28..87fc37d575d 100644 --- a/src/Compiler/Service/ServiceParsedInputOps.fsi +++ b/src/Compiler/Service/ServiceParsedInputOps.fsi @@ -163,6 +163,11 @@ type public InsertionContextEntity = /// Namespace that is needed to open to make the entity resolvable in the current scope. Namespace: string option + /// How many leading idents of the entity's full name that namespace covers. Compare it with + /// `AssemblySymbol.OpenableIdentCount` to tell a namespace a plain `open` reaches from a type, + /// which only `open type` opens. + NamespaceIdentCount: int + /// Full display name (i.e. last ident plus modules with `RequireQualifiedAccess` attribute prefixed). FullDisplayName: string diff --git a/tests/FSharp.Compiler.Service.Tests/FSharp.Compiler.Service.SurfaceArea.netstandard20.bsl b/tests/FSharp.Compiler.Service.Tests/FSharp.Compiler.Service.SurfaceArea.netstandard20.bsl index 5c9c346b613..73749e8ce39 100644 --- a/tests/FSharp.Compiler.Service.Tests/FSharp.Compiler.Service.SurfaceArea.netstandard20.bsl +++ b/tests/FSharp.Compiler.Service.Tests/FSharp.Compiler.Service.SurfaceArea.netstandard20.bsl @@ -3108,6 +3108,8 @@ FSharp.Compiler.EditorServices.AssemblySymbol: FSharp.Compiler.EditorServices.Un FSharp.Compiler.EditorServices.AssemblySymbol: FSharp.Compiler.EditorServices.UnresolvedSymbol get_UnresolvedSymbol() FSharp.Compiler.EditorServices.AssemblySymbol: FSharp.Compiler.Symbols.FSharpSymbol Symbol FSharp.Compiler.EditorServices.AssemblySymbol: FSharp.Compiler.Symbols.FSharpSymbol get_Symbol() +FSharp.Compiler.EditorServices.AssemblySymbol: Int32 OpenableIdentCount +FSharp.Compiler.EditorServices.AssemblySymbol: Int32 get_OpenableIdentCount() FSharp.Compiler.EditorServices.AssemblySymbol: Microsoft.FSharp.Core.FSharpFunc`2[FSharp.Compiler.EditorServices.LookupType,FSharp.Compiler.EditorServices.EntityKind] Kind FSharp.Compiler.EditorServices.AssemblySymbol: Microsoft.FSharp.Core.FSharpFunc`2[FSharp.Compiler.EditorServices.LookupType,FSharp.Compiler.EditorServices.EntityKind] get_Kind() FSharp.Compiler.EditorServices.AssemblySymbol: Microsoft.FSharp.Core.FSharpOption`1[System.String[]] AutoOpenParent @@ -3123,7 +3125,7 @@ FSharp.Compiler.EditorServices.AssemblySymbol: System.String ToString() FSharp.Compiler.EditorServices.AssemblySymbol: System.String get_FullName() FSharp.Compiler.EditorServices.AssemblySymbol: System.String[] CleanedIdents FSharp.Compiler.EditorServices.AssemblySymbol: System.String[] get_CleanedIdents() -FSharp.Compiler.EditorServices.AssemblySymbol: Void .ctor(System.String, System.String[], Microsoft.FSharp.Core.FSharpOption`1[System.String[]], Microsoft.FSharp.Core.FSharpOption`1[System.String[]], Microsoft.FSharp.Core.FSharpOption`1[System.String[]], Microsoft.FSharp.Core.FSharpOption`1[System.String[]], FSharp.Compiler.Symbols.FSharpSymbol, Microsoft.FSharp.Core.FSharpFunc`2[FSharp.Compiler.EditorServices.LookupType,FSharp.Compiler.EditorServices.EntityKind], FSharp.Compiler.EditorServices.UnresolvedSymbol) +FSharp.Compiler.EditorServices.AssemblySymbol: Void .ctor(System.String, System.String[], Microsoft.FSharp.Core.FSharpOption`1[System.String[]], Int32, Microsoft.FSharp.Core.FSharpOption`1[System.String[]], Microsoft.FSharp.Core.FSharpOption`1[System.String[]], Microsoft.FSharp.Core.FSharpOption`1[System.String[]], FSharp.Compiler.Symbols.FSharpSymbol, Microsoft.FSharp.Core.FSharpFunc`2[FSharp.Compiler.EditorServices.LookupType,FSharp.Compiler.EditorServices.EntityKind], FSharp.Compiler.EditorServices.UnresolvedSymbol) FSharp.Compiler.EditorServices.CompletionContext+Inherit: FSharp.Compiler.EditorServices.InheritanceContext context FSharp.Compiler.EditorServices.CompletionContext+Inherit: FSharp.Compiler.EditorServices.InheritanceContext get_context() FSharp.Compiler.EditorServices.CompletionContext+Inherit: System.Tuple`2[Microsoft.FSharp.Collections.FSharpList`1[System.String],Microsoft.FSharp.Core.FSharpOption`1[System.String]] get_path() @@ -3726,6 +3728,8 @@ FSharp.Compiler.EditorServices.InsertionContextEntity: Int32 CompareTo(System.Ob FSharp.Compiler.EditorServices.InsertionContextEntity: Int32 CompareTo(System.Object, System.Collections.IComparer) FSharp.Compiler.EditorServices.InsertionContextEntity: Int32 GetHashCode() FSharp.Compiler.EditorServices.InsertionContextEntity: Int32 GetHashCode(System.Collections.IEqualityComparer) +FSharp.Compiler.EditorServices.InsertionContextEntity: Int32 NamespaceIdentCount +FSharp.Compiler.EditorServices.InsertionContextEntity: Int32 get_NamespaceIdentCount() FSharp.Compiler.EditorServices.InsertionContextEntity: Microsoft.FSharp.Core.FSharpOption`1[System.String] Namespace FSharp.Compiler.EditorServices.InsertionContextEntity: Microsoft.FSharp.Core.FSharpOption`1[System.String] get_Namespace() FSharp.Compiler.EditorServices.InsertionContextEntity: System.String FullDisplayName @@ -3737,7 +3741,7 @@ FSharp.Compiler.EditorServices.InsertionContextEntity: System.String get_FullDis FSharp.Compiler.EditorServices.InsertionContextEntity: System.String get_FullRelativeName() FSharp.Compiler.EditorServices.InsertionContextEntity: System.String get_LastIdent() FSharp.Compiler.EditorServices.InsertionContextEntity: System.String get_Qualifier() -FSharp.Compiler.EditorServices.InsertionContextEntity: Void .ctor(System.String, System.String, Microsoft.FSharp.Core.FSharpOption`1[System.String], System.String, System.String) +FSharp.Compiler.EditorServices.InsertionContextEntity: Void .ctor(System.String, System.String, Microsoft.FSharp.Core.FSharpOption`1[System.String], Int32, System.String, System.String) FSharp.Compiler.EditorServices.InterfaceData+Interface: FSharp.Compiler.Syntax.SynType get_interfaceType() FSharp.Compiler.EditorServices.InterfaceData+Interface: FSharp.Compiler.Syntax.SynType interfaceType FSharp.Compiler.EditorServices.InterfaceData+Interface: Microsoft.FSharp.Core.FSharpOption`1[Microsoft.FSharp.Collections.FSharpList`1[FSharp.Compiler.Syntax.SynMemberDefn]] get_memberDefns() diff --git a/vsintegration/src/FSharp.Editor/CodeFixes/AddOpenCodeFixProvider.fs b/vsintegration/src/FSharp.Editor/CodeFixes/AddOpenCodeFixProvider.fs index 9de03c976f1..42b9c207b78 100644 --- a/vsintegration/src/FSharp.Editor/CodeFixes/AddOpenCodeFixProvider.fs +++ b/vsintegration/src/FSharp.Editor/CodeFixes/AddOpenCodeFixProvider.fs @@ -27,10 +27,11 @@ type internal AddOpenCodeFixProvider [] (assemblyContentPr Changes = [ TextChange(context.Span, qualifier) ] } - let openNamespaceFix ctx name ns multipleNames sourceText = - let displayText = $"open {ns}" + (if multipleNames then " (" + name + ")" else "") + let openNamespaceFix ctx name declaration multipleNames sourceText = + let displayText = declaration + (if multipleNames then " (" + name + ")" else "") - let change = OpenDeclarationHelper.getOpenDeclarationChange sourceText ctx ns + let change = + OpenDeclarationHelper.getOpenDeclarationChange sourceText ctx declaration { Name = CodeFix.AddOpen @@ -38,31 +39,41 @@ type internal AddOpenCodeFixProvider [] (assemblyContentPr Changes = [ change ] } + // A plain `open` reaches namespaces and F# modules. When the entity sits deeper than that - a type + // nested in a type, or a static member of one - the type itself has to be opened. + let openDeclaration (entity: InsertionContextEntity) openableIdentCount ns = + if entity.NamespaceIdentCount > openableIdentCount then + $"open type {ns}" + else + $"open {ns}" + let getSuggestionsAsCodeFixes (context: CodeFixContext) (sourceText: SourceText) - (candidates: (InsertionContextEntity * InsertionContext) list) + (candidates: (InsertionContextEntity * InsertionContext * int) list) = seq { candidates - |> Seq.choose (fun (entity, ctx) -> entity.Namespace |> Option.map (fun ns -> ns, entity.FullDisplayName, ctx)) - |> Seq.groupBy (fun (ns, _, _) -> ns) - |> Seq.map (fun (ns, xs) -> - ns, + |> Seq.choose (fun (entity, ctx, openableIdentCount) -> + entity.Namespace + |> Option.map (fun ns -> openDeclaration entity openableIdentCount ns, entity.FullDisplayName, ctx)) + |> Seq.groupBy (fun (declaration, _, _) -> declaration) + |> Seq.map (fun (declaration, xs) -> + declaration, xs |> Seq.map (fun (_, name, ctx) -> name, ctx) |> Seq.distinctBy (fun (name, _) -> name) |> Seq.sortBy fst |> Seq.toArray) - |> Seq.map (fun (ns, names) -> + |> Seq.map (fun (declaration, names) -> let multipleNames = names |> Array.length > 1 - names |> Seq.map (fun (name, ctx) -> ns, name, ctx, multipleNames)) + names |> Seq.map (fun (name, ctx) -> declaration, name, ctx, multipleNames)) |> Seq.concat - |> Seq.map (fun (ns, name, ctx, multipleNames) -> openNamespaceFix ctx name ns multipleNames sourceText) + |> Seq.map (fun (declaration, name, ctx, multipleNames) -> openNamespaceFix ctx name declaration multipleNames sourceText) candidates - |> Seq.filter (fun (entity, _) -> not (entity.LastIdent.StartsWith "op_")) // Don't include qualified operator names. The resultant codefix won't compile because it won't be an infix operator anymore. - |> Seq.map (fun (entity, _) -> entity.FullRelativeName, entity.Qualifier) + |> Seq.filter (fun (entity, _, _) -> not (entity.LastIdent.StartsWith "op_")) // Don't include qualified operator names. The resultant codefix won't compile because it won't be an infix operator anymore. + |> Seq.map (fun (entity, _, _) -> entity.FullRelativeName, entity.Qualifier) |> Seq.distinct |> Seq.sort |> Seq.map (qualifySymbolFix context) @@ -130,7 +141,9 @@ type internal AddOpenCodeFixProvider [] (assemblyContentPr assemblyContentProvider.GetAllEntitiesInProjectAndReferencedAssemblies checkResults |> Array.collect (fun s -> [| - yield s.TopRequireQualifiedAccessParent, s.AutoOpenParent, s.Namespace, s.CleanedIdents + yield + s.OpenableIdentCount, + (s.TopRequireQualifiedAccessParent, s.AutoOpenParent, s.Namespace, s.CleanedIdents) if isAttribute then let lastIdent = s.CleanedIdents.[s.CleanedIdents.Length - 1] @@ -139,13 +152,14 @@ type internal AddOpenCodeFixProvider [] (assemblyContentPr && s.Kind LookupType.Precise = EntityKind.Attribute then yield - s.TopRequireQualifiedAccessParent, - s.AutoOpenParent, - s.Namespace, - s.CleanedIdents - |> Array.replace - (s.CleanedIdents.Length - 1) - (lastIdent.Substring(0, lastIdent.Length - 9)) + s.OpenableIdentCount, + (s.TopRequireQualifiedAccessParent, + s.AutoOpenParent, + s.Namespace, + s.CleanedIdents + |> Array.replace + (s.CleanedIdents.Length - 1) + (lastIdent.Substring(0, lastIdent.Length - 9))) |]) ParsedInput.GetLongIdentAt parseResults.ParseTree unresolvedIdentRange.End @@ -173,8 +187,9 @@ type internal AddOpenCodeFixProvider [] (assemblyContentPr insertionPoint entities - |> Seq.map createEntity - |> Seq.concat + |> Seq.collect (fun (openableIdentCount, symbol) -> + createEntity symbol + |> Seq.map (fun (entity, ctx) -> entity, ctx, openableIdentCount)) |> Seq.toList |> getSuggestionsAsCodeFixes context sourceText |> Seq.tryHead)) diff --git a/vsintegration/src/FSharp.Editor/Common/RoslynHelpers.fs b/vsintegration/src/FSharp.Editor/Common/RoslynHelpers.fs index 8f47a6545f8..26206509bd4 100644 --- a/vsintegration/src/FSharp.Editor/Common/RoslynHelpers.fs +++ b/vsintegration/src/FSharp.Editor/Common/RoslynHelpers.fs @@ -234,8 +234,8 @@ module internal OpenDeclarationHelper = /// /// SourceText. /// Insertion context. Typically returned from tryGetInsertionContext - /// Namespace to open. - let getOpenDeclarationChange (sourceText: SourceText) (ctx: InsertionContext) (ns: string) : TextChange = + /// The declaration to add, `open Foo` or `open type Foo`. + let getOpenDeclarationChange (sourceText: SourceText) (ctx: InsertionContext) (declaration: string) : TextChange = let getLineStr line = if line >= 0 && line < sourceText.Lines.Count then sourceText.Lines[line].ToString().Trim() @@ -283,10 +283,10 @@ module internal OpenDeclarationHelper = if trivia.Length > 0 then TextChange( TextSpan(line.Start + trivia.Length, column - trivia.Length), - lineBreak + margin + "open " + ns + lineBreak + lineBreak + margin + lineBreak + margin + declaration + lineBreak + lineBreak + margin ) else - TextChange(TextSpan(line.Start, 0), separatorAbove + margin + "open " + ns + separatorBelow) + TextChange(TextSpan(line.Start, 0), separatorAbove + margin + declaration + separatorBelow) /// /// Inserts open declaration into `SourceText`. @@ -295,7 +295,7 @@ module internal OpenDeclarationHelper = /// 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 + let change = getOpenDeclarationChange sourceText ctx ("open " + ns) sourceText.WithChanges change, change.Span.Start // http://www.fssnip.net/7S3/title/Intersperse-a-list diff --git a/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/AddOpenOnTopOnTests.fs b/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/AddOpenOnTopOnTests.fs index 23e1ae1de06..80d171aa536 100644 --- a/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/AddOpenOnTopOnTests.fs +++ b/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/AddOpenOnTopOnTests.fs @@ -466,6 +466,56 @@ module M1 = Assert.Equal(expected, actual) +[] // A plain `open` only reaches namespaces and modules; a type nested in a type needs `open type` +let ``Fixes FS0039 with open type for a type nested in a type`` () = + let code = + """module Module1 + +let folder () = SpecialFolder.Desktop +""" + + let expected = + Some + { + Message = "open type System.Environment" + FixedCode = + """module Module1 + +open type System.Environment + +let folder () = SpecialFolder.Desktop +""" + } + + let actual = codeFix |> tryFix code Auto + + Assert.Equal(expected, actual) + +[] // NEGATIVE: a type sitting directly in a namespace is reached by a plain open +let ``Fixes FS0039 with a plain open for a type in a namespace`` () = + let code = + """module Module1 + +let write () = Console.WriteLine "hi" +""" + + let expected = + Some + { + Message = "open System" + FixedCode = + """module Module1 + +open System + +let write () = Console.WriteLine "hi" +""" + } + + let actual = codeFix |> tryFix code Auto + + Assert.Equal(expected, actual) + [] let ``Doesn't fix FS0039 for random undefined symbols`` () = let code = From 7a4da37aada59399bc792c737df20a17777f741e Mon Sep 17 00:00:00 2001 From: Andrii Chebukin Date: Thu, 10 Sep 2026 00:22:35 +0200 Subject: [PATCH 5/6] Offer every open that resolves the name, not the first one found MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The fix computed a suggestion for every namespace and type a name could be resolved from, and then `Seq.tryHead` threw all but one away — whichever one the assembly crawler happened to reach first. An unresolved `File` was answered with `System.Net.WebRequestMethods` and never with `System.IO`; an unresolved `WriteLine` with `System.Diagnostics.Trace` and never with `System.Console`. `IFSharpMultiCodeFixProvider` already exists for providers with more than one suggestion, so the provider implements that instead and registers them all. Following Roslyn's add-import fix, which faces the same problem: the list is capped, at five opens and three qualifications, because a lightbulb is a menu somebody reads, and it is ordered with what `System` holds first rather than by whatever order the crawler walked the assemblies in -- without that, `Microsoft.VisualBasic.FileSystem` sorts above `System.Console` for `WriteLine`. Co-Authored-By: Claude Opus 5 (1M context) --- docs/release-notes/.VisualStudio/18.vNext.md | 1 + .../CodeFixes/AddOpenCodeFixProvider.fs | 33 ++- .../CodeFixes/AddOpenOnTopOffTests.fs | 133 +++++---- .../CodeFixes/AddOpenOnTopOnTests.fs | 260 +++++++++++++----- 4 files changed, 297 insertions(+), 130 deletions(-) diff --git a/docs/release-notes/.VisualStudio/18.vNext.md b/docs/release-notes/.VisualStudio/18.vNext.md index 23f1b78c373..84eaeec0e1a 100644 --- a/docs/release-notes/.VisualStudio/18.vNext.md +++ b/docs/release-notes/.VisualStudio/18.vNext.md @@ -7,6 +7,7 @@ * 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)) * The "open namespace" code fix offers `open type System.Console` where it used to offer `open System.Console`. A plain `open` only reaches namespaces and F# modules, so for a type nested in a type (`System.Net.WebRequestMethods.File`) or a static member of one (`System.Console.WriteLine`) every suggestion it made was code that does not compile — `FS0039: The namespace 'Console' is not defined`. ([PR #20501](https://github.com/dotnet/fsharp/pull/20501)) +* The "open namespace" code fix lists every namespace and type a name can be resolved from instead of one arbitrary suggestion. It computed them all along and then kept whichever one the assembly crawler happened to reach first, so an unresolved `File` was answered with `System.Net.WebRequestMethods` and `System.IO` was never offered at all. As in Roslyn's add-import fix, the list is capped — five opens, three qualifications — and ordered with `System` first. ([PR #20501](https://github.com/dotnet/fsharp/pull/20501)) * 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)) diff --git a/vsintegration/src/FSharp.Editor/CodeFixes/AddOpenCodeFixProvider.fs b/vsintegration/src/FSharp.Editor/CodeFixes/AddOpenCodeFixProvider.fs index 42b9c207b78..5bbda140350 100644 --- a/vsintegration/src/FSharp.Editor/CodeFixes/AddOpenCodeFixProvider.fs +++ b/vsintegration/src/FSharp.Editor/CodeFixes/AddOpenCodeFixProvider.fs @@ -2,6 +2,7 @@ namespace Microsoft.VisualStudio.FSharp.Editor +open System open System.Composition open System.Collections.Immutable @@ -18,6 +19,22 @@ open CancellableTasks type internal AddOpenCodeFixProvider [] (assemblyContentProvider: AssemblyContentProvider) = inherit CodeFixProvider() + // A name can be reachable from a great many places, and the lightbulb is a menu a person reads. + // The same reason Roslyn's add-import fix stops at five suggestions and its fully-qualify at three. + let maxOpenSuggestions = 5 + let maxQualifySuggestions = 3 + + // Which assembly the entity crawler reached first is no order to offer suggestions in. Sort them + // the way Roslyn's add-import fix does: what `System` holds first, the rest alphabetically after. + let suggestionOrder (declaration: string) = + let opened = declaration.Substring(declaration.LastIndexOf ' ' + 1) + + let isSystem = + opened.Equals("System", StringComparison.Ordinal) + || opened.StartsWith("System.", StringComparison.Ordinal) + + (if isSystem then 0 else 1), declaration + let fixUnderscoresInMenuText (text: string) = text.Replace("_", "__") let qualifySymbolFix (context: CodeFixContext) (fullName, qualifier) = @@ -65,10 +82,12 @@ type internal AddOpenCodeFixProvider [] (assemblyContentPr |> Seq.distinctBy (fun (name, _) -> name) |> Seq.sortBy fst |> Seq.toArray) + |> Seq.sortBy (fst >> suggestionOrder) |> Seq.map (fun (declaration, names) -> let multipleNames = names |> Array.length > 1 names |> Seq.map (fun (name, ctx) -> declaration, name, ctx, multipleNames)) |> Seq.concat + |> Seq.truncate maxOpenSuggestions |> Seq.map (fun (declaration, name, ctx, multipleNames) -> openNamespaceFix ctx name declaration multipleNames sourceText) candidates @@ -76,6 +95,7 @@ type internal AddOpenCodeFixProvider [] (assemblyContentPr |> Seq.map (fun (entity, _, _) -> entity.FullRelativeName, entity.Qualifier) |> Seq.distinct |> Seq.sort + |> Seq.truncate maxQualifySuggestions |> Seq.map (qualifySymbolFix context) } @@ -83,10 +103,10 @@ type internal AddOpenCodeFixProvider [] (assemblyContentPr override _.FixableDiagnosticIds = ImmutableArray.Create("FS0039", "FS0043") - override this.RegisterCodeFixesAsync context = context.RegisterFsharpFix this + override this.RegisterCodeFixesAsync context = context.RegisterFsharpFixes this - interface IFSharpCodeFixProvider with - member _.GetCodeFixIfAppliesAsync context = + interface IFSharpMultiCodeFixProvider with + member _.GetCodeFixesAsync context = cancellableTask { let document = context.Document @@ -163,7 +183,7 @@ type internal AddOpenCodeFixProvider [] (assemblyContentPr |]) ParsedInput.GetLongIdentAt parseResults.ParseTree unresolvedIdentRange.End - |> Option.bind (fun longIdent -> + |> Option.map (fun longIdent -> let maybeUnresolvedIdents = longIdent |> List.map (fun ident -> @@ -191,8 +211,7 @@ type internal AddOpenCodeFixProvider [] (assemblyContentPr createEntity symbol |> Seq.map (fun (entity, ctx) -> entity, ctx, openableIdentCount)) |> Seq.toList - |> getSuggestionsAsCodeFixes context sourceText - |> Seq.tryHead)) + |> getSuggestionsAsCodeFixes context sourceText)) - |> ValueOption.ofOption + |> Option.defaultValue Seq.empty } diff --git a/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/AddOpenOnTopOffTests.fs b/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/AddOpenOnTopOffTests.fs index 4518c5c4dd1..c6c116ea4e0 100644 --- a/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/AddOpenOnTopOffTests.fs +++ b/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/AddOpenOnTopOffTests.fs @@ -2,6 +2,8 @@ module FSharp.Editor.Tests.CodeFixes.AddOpenOnTopOffTests +open System + open Microsoft.VisualStudio.FSharp.Editor open Xunit @@ -9,6 +11,13 @@ open CodeFixTestFramework let private codeFix = AddOpenCodeFixProvider(AssemblyContentProvider()) +/// Every `open` suggestion the fix offers, in the order the lightbulb lists them. +let private openFixes code mode = + codeFix + |> multiFix code mode + |> Seq.filter (fun fix -> fix.Message.StartsWith("open ", StringComparison.Ordinal)) + |> Seq.toList + let mode = WithSettings { CodeFixesOptions.Default with @@ -22,7 +31,7 @@ let ``Fixes FS0039 for missing opens - basic`` () = """ let expected = - Some + [ { Message = "open System" FixedCode = @@ -31,10 +40,11 @@ let ``Fixes FS0039 for missing opens - basic`` () = Console.WriteLine 42 """ } + ] - let actual = codeFix |> tryFix code mode + let actual = openFixes code mode - Assert.Equal(expected, actual) + Assert.Equal(expected, actual) [] let ``Fixes FS0039 for missing opens - first line is empty`` () = @@ -44,7 +54,7 @@ Console.WriteLine 42 """ let expected = - Some + [ { Message = "open System" FixedCode = @@ -54,10 +64,11 @@ open System Console.WriteLine 42 """ } + ] - let actual = codeFix |> tryFix code mode + let actual = openFixes code mode - Assert.Equal(expected, actual) + Assert.Equal(expected, actual) [] let ``Fixes FS0039 for missing opens - multiple first lines are empty`` () = @@ -68,7 +79,7 @@ Console.WriteLine 42 """ let expected = - Some + [ { Message = "open System" FixedCode = @@ -79,10 +90,11 @@ open System Console.WriteLine 42 """ } + ] - let actual = codeFix |> tryFix code mode + let actual = openFixes code mode - Assert.Equal(expected, actual) + Assert.Equal(expected, actual) [] let ``Fixes FS0039 for missing opens - there is already an open directive`` () = @@ -93,7 +105,7 @@ Console.WriteLine 42 """ let expected = - Some + [ { Message = "open System" FixedCode = @@ -103,10 +115,11 @@ open System Console.WriteLine 42 """ } + ] - let actual = codeFix |> tryFix code mode + let actual = openFixes code mode - Assert.Equal(expected, actual) + Assert.Equal(expected, actual) [] let ``Fixes FS0039 for missing opens - top level module is explicit`` () = @@ -117,7 +130,7 @@ Console.WriteLine 42 """ let expected = - Some + [ { Message = "open System" FixedCode = @@ -128,10 +141,11 @@ open System Console.WriteLine 42 """ } + ] - let actual = codeFix |> tryFix code mode + let actual = openFixes code mode - Assert.Equal(expected, actual) + Assert.Equal(expected, actual) [] let ``Fixes FS0039 for missing opens - module has an attribute on the same line`` () = @@ -142,7 +156,7 @@ Console.WriteLine 42 """ let expected = - Some + [ { Message = "open System" FixedCode = @@ -153,10 +167,11 @@ open System Console.WriteLine 42 """ } + ] - let actual = codeFix |> tryFix code mode + let actual = openFixes code mode - Assert.Equal(expected, actual) + Assert.Equal(expected, actual) [] let ``Fixes FS0039 for missing opens - nested module`` () = @@ -167,7 +182,7 @@ let ``Fixes FS0039 for missing opens - nested module`` () = """ let expected = - Some + [ { Message = "open System" FixedCode = @@ -178,10 +193,11 @@ let ``Fixes FS0039 for missing opens - nested module`` () = Console.WriteLine 42 """ } + ] - let actual = codeFix |> tryFix code mode + let actual = openFixes code mode - Assert.Equal(expected, actual) + Assert.Equal(expected, actual) [] let ``Fixes FS0039 for missing opens - explicit module has attributes`` () = @@ -194,7 +210,7 @@ Console.WriteLine 42 """ let expected = - Some + [ { Message = "open System" FixedCode = @@ -207,10 +223,11 @@ open System Console.WriteLine 42 """ } + ] - let actual = codeFix |> tryFix code mode + let actual = openFixes code mode - Assert.Equal(expected, actual) + Assert.Equal(expected, actual) [] let ``Fixes FS0039 for missing opens - implicit module has attributes`` () = @@ -222,7 +239,7 @@ type MyType() = """ let expected = - Some + [ { Message = "open System" FixedCode = @@ -234,10 +251,11 @@ type MyType() = let now = DateTime.Now """ } + ] - let actual = codeFix |> tryFix code Auto + let actual = openFixes code Auto - Assert.Equal(expected, actual) + Assert.Equal(expected, actual) // TODO: the open statement should actually be within the module [] @@ -251,7 +269,7 @@ module Module1 = """ let expected = - Some + [ { Message = "open System" FixedCode = @@ -264,10 +282,11 @@ module Module1 = Console.WriteLine 42 """ } + ] - let actual = codeFix |> tryFix code Auto + let actual = openFixes code Auto - Assert.Equal(expected, actual) + Assert.Equal(expected, actual) [] let ``Fixes FS0039 for missing opens - module has multiple attributes`` () = @@ -281,7 +300,7 @@ Console.WriteLine 42 """ let expected = - Some + [ { Message = "open System" FixedCode = @@ -295,10 +314,11 @@ open System Console.WriteLine 42 """ } + ] - let actual = codeFix |> tryFix code mode + let actual = openFixes code mode - Assert.Equal(expected, actual) + Assert.Equal(expected, actual) [] let ``Fixes FS0039 for missing opens - attributes are mixed with empty lines`` () = @@ -313,7 +333,7 @@ Console.WriteLine 42 """ let expected = - Some + [ { Message = "open System" FixedCode = @@ -328,10 +348,11 @@ open System Console.WriteLine 42 """ } + ] - let actual = codeFix |> tryFix code mode + let actual = openFixes code mode - Assert.Equal(expected, actual) + Assert.Equal(expected, actual) [] let ``Fixes FS0039 for missing opens - multiple modules in one file`` () = @@ -347,7 +368,7 @@ module Module2 = """ let expected = - Some + [ { Message = "open System" FixedCode = @@ -363,10 +384,11 @@ module Module2 = Console.WriteLine(42) """ } + ] - let actual = codeFix |> tryFix code mode + let actual = openFixes code mode - Assert.Equal(expected, actual) + Assert.Equal(expected, actual) [] let ``Fixes FS0039 for missing opens - explicit namespace`` () = @@ -380,7 +402,7 @@ module M1 = """ let expected = - Some + [ { Message = "open System" FixedCode = @@ -394,10 +416,11 @@ module M1 = Console.WriteLine 42 """ } + ] - let actual = codeFix |> tryFix code mode + let actual = openFixes code mode - Assert.Equal(expected, actual) + Assert.Equal(expected, actual) [] let ``Doesn't fix FS0039 for random undefined symbols`` () = @@ -406,11 +429,11 @@ let ``Doesn't fix FS0039 for random undefined symbols`` () = let f = g """ - let expected = None + let expected = [] - let actual = codeFix |> tryFix code mode + let actual = openFixes code mode - Assert.Equal(expected, actual) + Assert.Equal(expected, actual) [] let ``Fixes FS0043 for missing opens`` () = @@ -424,7 +447,7 @@ module N = """ let expected = - Some + [ { Message = "open M" FixedCode = @@ -438,10 +461,11 @@ module N = let theAnswer = 4 ++ 2 """ } + ] - let actual = codeFix |> tryFix code mode + let actual = openFixes code mode - Assert.Equal(expected, actual) + Assert.Equal(expected, actual) [] let ``Doesn't fix FS0043 for random unsupported values`` () = @@ -452,11 +476,11 @@ type RecordType = { X : int } let x : RecordType = null """ - let expected = None + let expected = [] - let actual = codeFix |> tryFix code mode + let actual = openFixes code mode - Assert.Equal(expected, actual) + Assert.Equal(expected, actual) [] let ``Fixes FS0039 for missing opens - module has multiline attributes`` () = @@ -475,7 +499,7 @@ module FlatList = """ let expected = - Some + [ { Message = "open System.Collections.Generic" FixedCode = @@ -494,7 +518,8 @@ module FlatList = let a : KeyValuePair = KeyValuePair("key", 1) """ } + ] - let actual = codeFix |> tryFix code mode + let actual = openFixes code mode - Assert.Equal(expected, actual) + Assert.Equal(expected, actual) diff --git a/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/AddOpenOnTopOnTests.fs b/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/AddOpenOnTopOnTests.fs index 80d171aa536..d4c173db37a 100644 --- a/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/AddOpenOnTopOnTests.fs +++ b/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/AddOpenOnTopOnTests.fs @@ -2,6 +2,8 @@ module FSharp.Editor.Tests.CodeFixes.AddOpenOnTopOnTests +open System + open Microsoft.VisualStudio.FSharp.Editor open Xunit @@ -9,6 +11,13 @@ open CodeFixTestFramework let private codeFix = AddOpenCodeFixProvider(AssemblyContentProvider()) +/// Every `open` suggestion the fix offers, in the order the lightbulb lists them. +let private openFixes code mode = + codeFix + |> multiFix code mode + |> Seq.filter (fun fix -> fix.Message.StartsWith("open ", StringComparison.Ordinal)) + |> Seq.toList + [] let ``Fixes FS0039 for missing opens - basic`` () = let code = @@ -16,7 +25,7 @@ let ``Fixes FS0039 for missing opens - basic`` () = """ let expected = - Some + [ { Message = "open System" FixedCode = @@ -25,10 +34,11 @@ let ``Fixes FS0039 for missing opens - basic`` () = Console.WriteLine 42 """ } + ] - let actual = codeFix |> tryFix code Auto + let actual = openFixes code Auto - Assert.Equal(expected, actual) + Assert.Equal(expected, actual) [] let ``Fixes FS0039 for missing opens - first line is empty`` () = @@ -38,7 +48,7 @@ Console.WriteLine 42 """ let expected = - Some + [ { Message = "open System" FixedCode = @@ -48,10 +58,11 @@ open System Console.WriteLine 42 """ } + ] - let actual = codeFix |> tryFix code Auto + let actual = openFixes code Auto - Assert.Equal(expected, actual) + Assert.Equal(expected, actual) [] let ``Fixes FS0039 for missing opens - multiple first lines are empty`` () = @@ -62,7 +73,7 @@ Console.WriteLine 42 """ let expected = - Some + [ { Message = "open System" FixedCode = @@ -73,10 +84,11 @@ open System Console.WriteLine 42 """ } + ] - let actual = codeFix |> tryFix code Auto + let actual = openFixes code Auto - Assert.Equal(expected, actual) + 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`` () = @@ -86,7 +98,7 @@ let ``Fixes FS0039 for missing opens - declaration shares its line with the end """ let expected = - Some + [ { Message = "open System" FixedCode = @@ -97,10 +109,11 @@ let ``Fixes FS0039 for missing opens - declaration shares its line with the end Console.WriteLine 42 """ } + ] - let actual = codeFix |> tryFix code Auto + let actual = openFixes code Auto - Assert.Equal(expected, actual) + Assert.Equal(expected, actual) [] let ``Fixes FS0039 for missing opens - there is already an open directive`` () = @@ -111,7 +124,7 @@ Console.WriteLine 42 """ let expected = - Some + [ { Message = "open System" FixedCode = @@ -121,10 +134,11 @@ open System Console.WriteLine 42 """ } + ] - let actual = codeFix |> tryFix code Auto + let actual = openFixes code Auto - Assert.Equal(expected, actual) + Assert.Equal(expected, actual) [] let ``Fixes FS0039 for missing opens - top level module is explicit`` () = @@ -135,7 +149,7 @@ Console.WriteLine 42 """ let expected = - Some + [ { Message = "open System" FixedCode = @@ -146,10 +160,11 @@ open System Console.WriteLine 42 """ } + ] - let actual = codeFix |> tryFix code Auto + let actual = openFixes code Auto - Assert.Equal(expected, actual) + Assert.Equal(expected, actual) [] let ``Fixes FS0039 for missing opens - module has an attribute on the same line`` () = @@ -160,7 +175,7 @@ Console.WriteLine 42 """ let expected = - Some + [ { Message = "open System" FixedCode = @@ -171,10 +186,11 @@ open System Console.WriteLine 42 """ } + ] - let actual = codeFix |> tryFix code Auto + let actual = openFixes code Auto - Assert.Equal(expected, actual) + Assert.Equal(expected, actual) [] let ``Fixes FS0039 for missing opens - explicit top level module without a blank line`` () = @@ -184,7 +200,7 @@ Console.WriteLine 42 """ let expected = - Some + [ { Message = "open System" FixedCode = @@ -195,10 +211,11 @@ open System Console.WriteLine 42 """ } + ] - let actual = codeFix |> tryFix code Auto + let actual = openFixes code Auto - Assert.Equal(expected, actual) + Assert.Equal(expected, actual) [] let ``Fixes FS0039 for missing opens - namespace without a blank line`` () = @@ -209,7 +226,7 @@ module M1 = """ let expected = - Some + [ { Message = "open System" FixedCode = @@ -221,10 +238,11 @@ module M1 = Console.WriteLine 42 """ } + ] - let actual = codeFix |> tryFix code Auto + let actual = openFixes code Auto - Assert.Equal(expected, actual) + Assert.Equal(expected, actual) [] let ``Fixes FS0039 for missing opens - nested module`` () = @@ -235,7 +253,7 @@ let ``Fixes FS0039 for missing opens - nested module`` () = """ let expected = - Some + [ { Message = "open System" FixedCode = @@ -246,10 +264,11 @@ module Module1 = Console.WriteLine 42 """ } + ] - let actual = codeFix |> tryFix code Auto + let actual = openFixes code Auto - Assert.Equal(expected, actual) + Assert.Equal(expected, actual) [] let ``Fixes FS0039 for missing opens - explicit module has attributes`` () = @@ -262,7 +281,7 @@ Console.WriteLine 42 """ let expected = - Some + [ { Message = "open System" FixedCode = @@ -275,10 +294,11 @@ open System Console.WriteLine 42 """ } + ] - let actual = codeFix |> tryFix code Auto + let actual = openFixes code Auto - Assert.Equal(expected, actual) + Assert.Equal(expected, actual) [] let ``Fixes FS0039 for missing opens - implicit module has attributes`` () = @@ -290,7 +310,7 @@ type MyType() = """ let expected = - Some + [ { Message = "open System" FixedCode = @@ -302,10 +322,11 @@ type MyType() = let now = DateTime.Now """ } + ] - let actual = codeFix |> tryFix code Auto + let actual = openFixes code Auto - Assert.Equal(expected, actual) + Assert.Equal(expected, actual) [] let ``Fixes FS0039 for missing opens - nested module has attributes`` () = @@ -318,7 +339,7 @@ module Module1 = """ let expected = - Some + [ { Message = "open System" FixedCode = @@ -331,10 +352,11 @@ module Module1 = Console.WriteLine 42 """ } + ] - let actual = codeFix |> tryFix code Auto + let actual = openFixes code Auto - Assert.Equal(expected, actual) + Assert.Equal(expected, actual) [] let ``Fixes FS0039 for missing opens - module has multiple attributes`` () = @@ -348,7 +370,7 @@ Console.WriteLine 42 """ let expected = - Some + [ { Message = "open System" FixedCode = @@ -362,10 +384,11 @@ open System Console.WriteLine 42 """ } + ] - let actual = codeFix |> tryFix code Auto + let actual = openFixes code Auto - Assert.Equal(expected, actual) + Assert.Equal(expected, actual) [] let ``Fixes FS0039 for missing opens - attributes are mixed with empty lines`` () = @@ -380,7 +403,7 @@ Console.WriteLine 42 """ let expected = - Some + [ { Message = "open System" FixedCode = @@ -395,10 +418,11 @@ open System Console.WriteLine 42 """ } + ] - let actual = codeFix |> tryFix code Auto + let actual = openFixes code Auto - Assert.Equal(expected, actual) + Assert.Equal(expected, actual) [] let ``Fixes FS0039 for missing opens - multiple modules in one file`` () = @@ -414,7 +438,7 @@ module Module2 = """ let expected = - Some + [ { Message = "open System" FixedCode = @@ -430,10 +454,11 @@ module Module2 = Console.WriteLine(42) """ } + ] - let actual = codeFix |> tryFix code Auto + let actual = openFixes code Auto - Assert.Equal(expected, actual) + Assert.Equal(expected, actual) [] let ``Fixes FS0039 for missing opens - explicit namespace`` () = @@ -447,7 +472,7 @@ module M1 = """ let expected = - Some + [ { Message = "open System" FixedCode = @@ -461,10 +486,11 @@ module M1 = Console.WriteLine 42 """ } + ] - let actual = codeFix |> tryFix code Auto + let actual = openFixes code Auto - Assert.Equal(expected, actual) + Assert.Equal(expected, actual) [] // A plain `open` only reaches namespaces and modules; a type nested in a type needs `open type` let ``Fixes FS0039 with open type for a type nested in a type`` () = @@ -475,7 +501,7 @@ let folder () = SpecialFolder.Desktop """ let expected = - Some + [ { Message = "open type System.Environment" FixedCode = @@ -486,10 +512,103 @@ open type System.Environment let folder () = SpecialFolder.Desktop """ } + ] + + let actual = openFixes code Auto + + Assert.Equal(expected, actual) + +[] // `File` is both `System.IO.File` and the nested `System.Net.WebRequestMethods.File` +let ``Offers every namespace a name can be resolved from`` () = + let code = + """module Module1 + +let readFile () = File.ReadAllText "example.txt" +""" + + let expected = + [ + { + Message = "open System.IO" + FixedCode = + """module Module1 + +open System.IO + +let readFile () = File.ReadAllText "example.txt" +""" + } + { + Message = "open type System.Net.WebRequestMethods" + FixedCode = + """module Module1 + +open type System.Net.WebRequestMethods + +let readFile () = File.ReadAllText "example.txt" +""" + } + ] + + let actual = openFixes code Auto + + Assert.Equal(expected, actual) + +[] // `WriteLine` is a static member of four different types, `System` ones offered first +let ``Offers every type a static member can be resolved from`` () = + let code = + """module Module1 + +let write () = WriteLine "hi" +""" + + let expected = + [ + { + Message = "open type System.Console" + FixedCode = + """module Module1 + +open type System.Console + +let write () = WriteLine "hi" +""" + } + { + Message = "open type System.Diagnostics.Debug" + FixedCode = + """module Module1 + +open type System.Diagnostics.Debug + +let write () = WriteLine "hi" +""" + } + { + Message = "open type System.Diagnostics.Trace" + FixedCode = + """module Module1 + +open type System.Diagnostics.Trace + +let write () = WriteLine "hi" +""" + } + { + Message = "open type Microsoft.VisualBasic.FileSystem" + FixedCode = + """module Module1 + +open type Microsoft.VisualBasic.FileSystem + +let write () = WriteLine "hi" +""" + } + ] - let actual = codeFix |> tryFix code Auto + let actual = openFixes code Auto - Assert.Equal(expected, actual) + Assert.Equal(expected, actual) [] // NEGATIVE: a type sitting directly in a namespace is reached by a plain open let ``Fixes FS0039 with a plain open for a type in a namespace`` () = @@ -500,7 +619,7 @@ let write () = Console.WriteLine "hi" """ let expected = - Some + [ { Message = "open System" FixedCode = @@ -511,10 +630,11 @@ open System let write () = Console.WriteLine "hi" """ } + ] - let actual = codeFix |> tryFix code Auto + let actual = openFixes code Auto - Assert.Equal(expected, actual) + Assert.Equal(expected, actual) [] let ``Doesn't fix FS0039 for random undefined symbols`` () = @@ -523,11 +643,11 @@ let ``Doesn't fix FS0039 for random undefined symbols`` () = let f = g """ - let expected = None + let expected = [] - let actual = codeFix |> tryFix code Auto + let actual = openFixes code Auto - Assert.Equal(expected, actual) + Assert.Equal(expected, actual) [] let ``Fixes FS0043 for missing opens`` () = @@ -541,7 +661,7 @@ module N = """ let expected = - Some + [ { Message = "open M" FixedCode = @@ -555,10 +675,11 @@ module N = let theAnswer = 4 ++ 2 """ } + ] - let actual = codeFix |> tryFix code Auto + let actual = openFixes code Auto - Assert.Equal(expected, actual) + Assert.Equal(expected, actual) [] let ``Doesn't fix FS0043 for random unsupported values`` () = @@ -569,11 +690,11 @@ type RecordType = { X : int } let x : RecordType = null """ - let expected = None + let expected = [] - let actual = codeFix |> tryFix code Auto + let actual = openFixes code Auto - Assert.Equal(expected, actual) + Assert.Equal(expected, actual) [] let ``Fixes FS0039 for missing opens - module has multiline attributes`` () = @@ -592,7 +713,7 @@ module FlatList = """ let expected = - Some + [ { Message = "open System.Collections.Generic" FixedCode = @@ -610,7 +731,8 @@ module FlatList = let a : KeyValuePair = KeyValuePair("key", 1) """ } + ] - let actual = codeFix |> tryFix code Auto + let actual = openFixes code Auto - Assert.Equal(expected, actual) + Assert.Equal(expected, actual) From f0225963aac5ca1d42fb17cea51b29b6a007b4f8 Mon Sep 17 00:00:00 2001 From: Andrii Chebukin Date: Thu, 10 Sep 2026 00:53:21 +0200 Subject: [PATCH 6/6] Cover the qualification suggestions the fix offers alongside the opens MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The tests filtered the suggestions down to the opens, so what the fix offers for qualifying the name in place went unasserted — including that three of them is where the list stops. Co-Authored-By: Claude Opus 5 (1M context) --- .../CodeFixes/AddOpenOnTopOnTests.fs | 35 ++++++++++++++++--- 1 file changed, 30 insertions(+), 5 deletions(-) diff --git a/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/AddOpenOnTopOnTests.fs b/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/AddOpenOnTopOnTests.fs index d4c173db37a..34ff3748f99 100644 --- a/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/AddOpenOnTopOnTests.fs +++ b/vsintegration/tests/FSharp.Editor.Tests/CodeFixes/AddOpenOnTopOnTests.fs @@ -11,12 +11,14 @@ open CodeFixTestFramework let private codeFix = AddOpenCodeFixProvider(AssemblyContentProvider()) -/// Every `open` suggestion the fix offers, in the order the lightbulb lists them. +/// Everything the fix offers, in the order the lightbulb lists it: the opens, then the qualifications. +let private allFixes code mode = + codeFix |> multiFix code mode |> Seq.toList + +/// Just the `open` suggestions, for the tests that are about where the declaration lands. let private openFixes code mode = - codeFix - |> multiFix code mode - |> Seq.filter (fun fix -> fix.Message.StartsWith("open ", StringComparison.Ordinal)) - |> Seq.toList + allFixes code mode + |> List.filter (fun fix -> fix.Message.StartsWith("open ", StringComparison.Ordinal)) [] let ``Fixes FS0039 for missing opens - basic`` () = @@ -554,6 +556,29 @@ let readFile () = File.ReadAllText "example.txt" Assert.Equal(expected, actual) +[] // Qualifying the name in place is offered alongside opening what holds it +let ``Offers qualifying the name after the opens`` () = + let code = + """module Module1 + +let readFile () = File.ReadAllText "example.txt" +""" + + let expected = + [ + "open System.IO" + "open type System.Net.WebRequestMethods" + // Qualifications, three of them at most, `System.IO.File` twice over because the type and + // the member being reached through it are both candidates. + "System.IO.File" + "System.IO.File.ReadAllText" + "System.Net.WebRequestMethods.File" + ] + + let actual = allFixes code Auto |> List.map _.Message + + Assert.Equal(expected, actual) + [] // `WriteLine` is a static member of four different types, `System` ones offered first let ``Offers every type a static member can be resolved from`` () = let code =