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..9de03c976f1 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,10 @@ 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..8f47a6545f8 100644 --- a/vsintegration/src/FSharp.Editor/Common/RoslynHelpers.fs +++ b/vsintegration/src/FSharp.Editor/Common/RoslynHelpers.fs @@ -230,63 +230,73 @@ 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() - - 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 + if line >= 0 && line < sourceText.Lines.Count then + sourceText.Lines[line].ToString().Trim() 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 "" + 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 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 + "" + + 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 + + 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) - 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..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 = @@ -127,6 +151,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 =