Repository navigation
Place the added open from the syntax tree, not from a line of text - #20500
xperiandri wants to merge 8 commits into
Conversation
✅ Release notes checked
|
What this looked like before, and what it does nowThree shapes, each with the 1. An attribute on the module's own line[<AutoOpen>] module Ns.Helpers
let write () = Console.WriteLine "hi"
open System // ← before: above the module declaration
[<AutoOpen>] module Ns.Helpers
let write () = Console.WriteLine "hi"[<AutoOpen>] module Ns.Helpers
open System // ← after: inside the module
let write () = Console.WriteLine "hi"2. No blank line under the headermodule Ns.Helpers
let write () = Console.WriteLine "hi"The arithmetic added two lines to reach "past the blank line under the header". With no blank line there, that landed past the first declaration, so the module Ns.Helpers
let write () = Console.WriteLine "hi" // ← before: still FS0039, the open is below it
open Systemmodule Ns.Helpers
open System // ← after
let write () = Console.WriteLine "hi"3.
|
This comment has been minimized.
This comment has been minimized.
T-Gro
left a comment
There was a problem hiding this comment.
🤖 🕵️ AI review — verify independently.
ca89ff8 to
f5dd4ff
Compare
T-Gro
left a comment
There was a problem hiding this comment.
🤖🕵️ Please shorten the description using this guidance. Focus on the problem and why the change is needed, in simplified technical English. Leave the implementation inventory to the Files tab and retain necessary caveats.
This comment has been minimized.
This comment has been minimized.
|
🔍 Tooling Safety Check — Affects-Design-Time
|
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Single-line nested modules remain incorrectly handled, and newline preservation fails on unterminated final lines.
Review effort: Balanced
Findings: 1
Open (2)
What changed in this PR
Moves open insertion placement from text matching to syntax-tree-derived positions shared by editor features.
Changes:
- Derives module and namespace insertion points from syntax ranges.
- Unifies code-fix and completion insertion logic.
- Adds regression tests and release notes.
| File | Description |
|---|---|
vsintegration/tests/FSharp.Editor.Tests/CodeFixes/AddOpenOnTopOnTests.fs |
Adds top-level insertion regressions. |
vsintegration/tests/FSharp.Editor.Tests/CodeFixes/AddOpenOnTopOffTests.fs |
Tests attributed module insertion. |
vsintegration/src/FSharp.Editor/Common/RoslynHelpers.fs |
Centralizes open text changes. |
vsintegration/src/FSharp.Editor/CodeFixes/AddOpenCodeFixProvider.fs |
Uses the shared insertion helper. |
tests/FSharp.Compiler.Service.Tests/OpenDeclarationInsertionTests.fs |
Tests syntax-derived positions. |
src/Compiler/Service/ServiceParsedInputOps.fsi |
Documents revised insertion semantics. |
src/Compiler/Service/ServiceParsedInputOps.fs |
Computes positions from syntax ranges. |
docs/release-notes/.VisualStudio/18.vNext.md |
Documents the editor fix. |
docs/release-notes/.FSharp.Compiler.Service/11.0.200.md |
Documents FCS behavior changes. |
| let headerLine = | ||
| match trivia.ModuleKeyword with | ||
| | Some moduleKeywordRange -> moduleKeywordRange.StartLine | ||
| | Some moduleKeyword -> headerEndLine moduleKeyword ident trivia.EqualsRange | ||
| | None -> range.StartLine // Fallback if trivia unavailable |
There was a problem hiding this comment.
Fixed in 777c4bd: a nested module whose first declaration shares the header's line is now entered at that declaration, so the fix breaks the line there and the open lands before the use. Covered by Fixes FS0039 for missing opens - nested module declared on one line and Open placed at the first declaration of a nested module declared on one line.
| // 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 |
There was a problem hiding this comment.
Fixed in 34c331e: only the last line can lack a line break, so the line above it supplies the file's; Environment.NewLine is left for a file of one line. Covered by Fixes FS0039 for missing opens - the open goes before a last line with no line break.
34c331e to
dfe04ec
Compare
| // the line below the header may already be past the use the open has to precede. | ||
| let lineAboveBody = | ||
| match firstDeclStart with | ||
| | ValueSome start when start.Line = headerLine -> headerLine - 1 |
There was a problem hiding this comment.
🤖🕵️ Nearest-scope opens enter the following module: open System leaves FS0039 on Console; accepting Obsolete completion throws ArgumentOutOfRangeException.
module Top
let x = Console.WriteLine 42
[<Obsolete>] module M = let y = 1There was a problem hiding this comment.
Fixed in dc46686: a nested module declared on one line is entered only by a use on its own line or below, and not when an attribute shares that line, so the open stays in the enclosing scope and the completion no longer composes its change from a position past the item. Covered by Open stays out of a nested module declared on one line that the use is not in, Fixes FS0039 for missing opens - the use is not in the nested module declared on one line and Completion that adds an open keeps it out of a nested module declared on one line.
dfe04ec to
b42a8c7
Compare
b42a8c7 to
d34a393
Compare
T-Gro
left a comment
There was a problem hiding this comment.
The diff is still way too big - apply the NoBloat instructions and CodeCompaction skill.
Especially the massive code comments would benefit not just with drastic reduction, but ideally with well named functions/arguments/bindings/types making the intention clear.
If a well named piece of code just tells what the comment says - delete the comment.
- Excluding 7 public API documentation lines, this diff still adds 34 implementation and test comment lines and 2609 comment chars.
- Comments bypass the type system and become stale maintenance state. Keep only durable public contracts; express implementation intent through names, functions, active patterns, discriminated unions, and types.
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: `[<AutoOpen>] 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) <noreply@anthropic.com>
The formatting-check CI job flagged AddOpenCodeFixProvider.fs. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
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) <noreply@anthropic.com>
In `module M = let x = Console.WriteLine 42` the line below the header is already past the use, so an open placed there left FS0039 in place. Such a module is now entered at its first declaration, and the code fix breaks the line there as it already did after a closing block comment. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The open goes in front of a line, and the last line of a file often ends without a line break. The file's own line ending is then on the line above, so a file with LF endings no longer gets CRLF from the host. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A nested module whose body starts on the header's line is entered at its first declaration, and the walk gated that by the line above the header. A use on the line above the module then got its open inside the module, where it does not reach the use. The completion that adds an open asked for that same line and composed its change from a position past the item, which threw ArgumentOutOfRangeException. The module is now entered from the header's line on. When an attribute shares that line with the body, the use may be the attribute, which the enclosing scope resolves, so the open stays there. The body of GetChangeAsync moves into a static GetChangeAsyncAux, as ProvideCompletionsAsyncAux already is, so that a test can commit a completion item without a Visual Studio service provider. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
dc46686 to
1eff620
Compare
The nested-module walk, the insertion helper and the public docs told in comments what their bindings can say: lineAboveBody, useMayBeInModuleAttribute, blankLineAboveOpen, blankLineBelowOpen and textBeforeDeclaration. The two public doc comments get shorter, and one comment is left, on the cross-function reason a TopModule position is reported as HashDirective. The release notes of this PR are shorter, and the "Improve Find All References" bullet that a union merge duplicated in 18.vNext.md is gone. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
The placement facts become theories over named scenarios, in the FCS tests and in the On and Off settings files of the editor tests. The case of a module with an attribute on its own line was copied into both settings files; it is one theory in AddOpenOnTopSharedTests now, run under both settings, and the helper that expects the open System fix lives there too. The completion test helper drops its doc comment and a binding. The cases and their expected results are the same as before. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
|
Compacted in 64ee01b (source) and 9e488ea (tests), behaviour unchanged. Added comment lines went from 41 to 7 (34 to 3 without the 4 public API doc lines, one of the 3 being the license header of the new test file; 2985 to 335 characters) and added lines from 478 to 315; what the comments said is in names now ( The new tests are 7 methods instead of 14 over the same 17 cases, and the attribute-on-the-module-line case that was copied into both settings files is one theory run under both settings in Test counts and results are the same as before the change: FCS OpenDeclarationInsertion 23 and AssemblyContent 5 (net472) pass, as do SurfaceArea and editor AddOpen 43/43, and the completion tests pass except the known |
T-Gro
left a comment
There was a problem hiding this comment.
🤖🕵️ AI review — verify independently.
| else | ||
| "" | ||
|
|
||
| let pos = ParsedInput.AdjustInsertionPoint getLineStr ctx |
There was a problem hiding this comment.
🤖🕵️ open System is inserted inside the header comment; FS0039 remains.
module Top (* header
*)
let x = Console.WriteLine 42…he integration branch The add-open branches link their release notes to PRs dotnet#20500 and dotnet#20501 and check the shared-line case through the multi-suggestion fixture dotnet#20501 moved the tests to. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Port of the dotnet#20500 compaction (fix/add-open-placement 1eff620..9e488ea) to all-fixes, in the form the stack's merge into dotnet#20501 gave it (6682313..529c3f7): - Name the open placement instead of commenting it - Run the open placement cases as theories The attribute-on-header cases live once in the new AddOpenOnTopSharedTests.fs, which gets its Compile line in FSharp.Editor.Tests.fsproj. The release note bullets of dotnet#20500 and dotnet#20501 are the shortened ones; all-fixes has no compiler-service bullet for dotnet#20500, so none is added, and it has no duplicate of the "Improve Find All References" bullet. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Port of the dotnet#20501 compaction (fix/add-open-type 6682313..db6709e) to all-fixes, without the merge of dotnet#20500's head (the previous commit): - Say in names what the open suggestions comment on - Run the open type and every-candidate cases as theories The comments on the suggestion caps, the System-first order, the qualifier span, the entity lookup, the open versus open type decision and the operator filter are gone. The provider keeps the extension member code that all-fixes has from dotnet#20670; it is the one dotnet#20670's merge of this head resolved to (642c00c..7f44dba). The release note bullets of dotnet#20501 were shortened with the previous commit, as the stack's merge of dotnet#20500 shortens them. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…he integration branch The add-open branches link their release notes to PRs dotnet#20500 and dotnet#20501 and check the shared-line case through the multi-suggestion fixture dotnet#20501 moved the tests to. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Port of the dotnet#20500 compaction (fix/add-open-placement 1eff620..9e488ea) to all-fixes, in the form the stack's merge into dotnet#20501 gave it (6682313..529c3f7): - Name the open placement instead of commenting it - Run the open placement cases as theories The attribute-on-header cases live once in the new AddOpenOnTopSharedTests.fs, which gets its Compile line in FSharp.Editor.Tests.fsproj. The release note bullets of dotnet#20500 and dotnet#20501 are the shortened ones; all-fixes has no compiler-service bullet for dotnet#20500, so none is added, and it has no duplicate of the "Improve Find All References" bullet. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Port of the dotnet#20501 compaction (fix/add-open-type 6682313..db6709e) to all-fixes, without the merge of dotnet#20500's head (the previous commit): - Say in names what the open suggestions comment on - Run the open type and every-candidate cases as theories The comments on the suggestion caps, the System-first order, the qualifier span, the entity lookup, the open versus open type decision and the operator filter are gone. The provider keeps the extension member code that all-fixes has from dotnet#20670; it is the one dotnet#20670's merge of this head resolved to (642c00c..7f44dba). The release note bullets of dotnet#20501 were shortened with the previous commit, as the stack's merge of dotnet#20500 shortens them. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…he integration branch The add-open branches link their release notes to PRs dotnet#20500 and dotnet#20501 and check the shared-line case through the multi-suggestion fixture dotnet#20501 moved the tests to. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Port of the dotnet#20500 compaction (fix/add-open-placement 1eff620..9e488ea) to all-fixes, in the form the stack's merge into dotnet#20501 gave it (6682313..529c3f7): - Name the open placement instead of commenting it - Run the open placement cases as theories The attribute-on-header cases live once in the new AddOpenOnTopSharedTests.fs, which gets its Compile line in FSharp.Editor.Tests.fsproj. The release note bullets of dotnet#20500 and dotnet#20501 are the shortened ones; all-fixes has no compiler-service bullet for dotnet#20500, so none is added, and it has no duplicate of the "Improve Find All References" bullet. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Port of the dotnet#20501 compaction (fix/add-open-type 6682313..db6709e) to all-fixes, without the merge of dotnet#20500's head (the previous commit): - Say in names what the open suggestions comment on - Run the open type and every-candidate cases as theories The comments on the suggestion caps, the System-first order, the qualifier span, the entity lookup, the open versus open type decision and the operator filter are gone. The provider keeps the extension member code that all-fixes has from dotnet#20670; it is the one dotnet#20670's merge of this head resolved to (642c00c..7f44dba). The release note bullets of dotnet#20501 were shortened with the previous commit, as the stack's merge of dotnet#20500 shortens them. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
dotnet#20719 pinned the spacing the Add Open fix and the completion commit produced when it merged: no blank line between a header and the open added under it, and the completion path and the code fix disagreeing about a nested module. dotnet#20500 and dotnet#20501 place the open from the syntax tree for both paths and keep one blank line between a non-blank line above and the open, so the expected texts gain that line. The open an applied-case test is about is looked up by its message, since the provider offers several and lists System first. One row stays skipped: the operator (=) matches static op_Equality of five System types, which fill the five offered opens ahead of open Candidates. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…ferenced-case test dotnet#20500 makes InsertionContext.Pos the first line inside the scope for every scope kind, so a top-level namespace answers (2,0) where dotnet#20719 pinned the namespace line (1,0). The edit the test builds from the entity is unchanged. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…he integration branch The add-open branches link their release notes to PRs dotnet#20500 and dotnet#20501 and check the shared-line case through the multi-suggestion fixture dotnet#20501 moved the tests to. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Port of the dotnet#20500 compaction (fix/add-open-placement 1eff620..9e488ea) to all-fixes, in the form the stack's merge into dotnet#20501 gave it (6682313..529c3f7): - Name the open placement instead of commenting it - Run the open placement cases as theories The attribute-on-header cases live once in the new AddOpenOnTopSharedTests.fs, which gets its Compile line in FSharp.Editor.Tests.fsproj. The release note bullets of dotnet#20500 and dotnet#20501 are the shortened ones; all-fixes has no compiler-service bullet for dotnet#20500, so none is added, and it has no duplicate of the "Improve Find All References" bullet. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Port of the dotnet#20501 compaction (fix/add-open-type 6682313..db6709e) to all-fixes, without the merge of dotnet#20500's head (the previous commit): - Say in names what the open suggestions comment on - Run the open type and every-candidate cases as theories The comments on the suggestion caps, the System-first order, the qualifier span, the entity lookup, the open versus open type decision and the operator filter are gone. The provider keeps the extension member code that all-fixes has from dotnet#20670; it is the one dotnet#20670's merge of this head resolved to (642c00c..7f44dba). The release note bullets of dotnet#20501 were shortened with the previous commit, as the stack's merge of dotnet#20500 shortens them. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
dotnet#20719 pinned the spacing the Add Open fix and the completion commit produced when it merged: no blank line between a header and the open added under it, and the completion path and the code fix disagreeing about a nested module. dotnet#20500 and dotnet#20501 place the open from the syntax tree for both paths and keep one blank line between a non-blank line above and the open, so the expected texts gain that line. The open an applied-case test is about is looked up by its message, since the provider offers several and lists System first. One row stays skipped: the operator (=) matches static op_Equality of five System types, which fill the five offered opens ahead of open Candidates. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…ferenced-case test dotnet#20500 makes InsertionContext.Pos the first line inside the scope for every scope kind, so a top-level namespace answers (2,0) where dotnet#20719 pinned the namespace line (1,0). The edit the test builds from the entity is unchanged. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>


The "open namespace" code fix and
open-adding completion found the insertion point by matching the text of a module header line, so several shapes produced code that does not compile:[<AutoOpen>] module Ns.Nameon one line put theopenabove the module.openbelow the first declaration.namespace Nswithout a blank line after it split atypefrom its first member.The insertion point now comes from the syntax tree. One existing test expectation moves by a line.
🤖 Generated with Claude Code