Repository navigation
Offer every open that resolves the name, and open type where a plain open cannot reach - #20501
xperiandri wants to merge 26 commits into
Conversation
✅ Release notes checked
|
This comment has been minimized.
This comment has been minimized.
52437eb to
93b84ca
Compare
T-Gro
left a comment
There was a problem hiding this comment.
🤖 🕵️ AI review — verify independently.
| |> Seq.toList | ||
| |> getSuggestionsAsCodeFixes context sourceText | ||
| |> Seq.tryHead)) | ||
| |> getSuggestionsAsCodeFixes context sourceText)) |
There was a problem hiding this comment.
🤖 🕵️ [P2] Choosing N.M.FixTarget817 rewrites M.FixTarget817() to M.N.M() (FS0039). Replace the matching long-identifier prefix, not just the diagnostic span.
module Review
module M = let marker = ()
module N =
module M =
type FixTarget817() = class end
let x = M.FixTarget817()There was a problem hiding this comment.
@xperiandri TODO - I think this hasnt been addressed
There was a problem hiding this comment.
Fixed in d6de3fc: the qualification now replaces the first identifier written (M) with the qualifier, not the diagnostic span, so M.FixTarget817() becomes N.M.FixTarget817(). Test: Qualifying a partly qualified name replaces what is written before the unresolved part.
f7e3ac6 to
f022596
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
Generic-type fixes can remain uncompilable, ordering and newline handling are inconsistent, and two public record constructors are broken.
Review effort: Balanced
Findings: 2
Open (5)
Adding a field breaks the public AssemblySymbol record constructor · New Adding metadata breaks the public InsertionContextEntity constructor · New Generic declaring types produce invalid open type suggestions · New Alphabetical sorting can truncate required System qualifications · New Fallback incorrectly changes line endings on unterminated final lines · New
What changed in this PR
Improves unresolved-name fixes by using syntax-aware insertion, offering multiple candidates, and choosing open type when necessary.
Changes:
- Shares syntax-tree-based
openinsertion logic. - Adds ranked, capped open and qualification suggestions.
- Tracks namespace/module reach and expands regression coverage.
| File | Description |
|---|---|
AddOpenOnTopOnTests.fs |
Expands top-level placement and suggestion tests. |
AddOpenOnTopOffTests.fs |
Updates nearest-scope placement tests. |
RoslynHelpers.fs |
Centralizes open-declaration text changes. |
AddOpenCodeFixProvider.fs |
Adds multiple, ranked open type and qualification fixes. |
OpenDeclarationInsertionTests.fs |
Covers syntax-tree insertion positions. |
FSharp.Compiler.Service.SurfaceArea.netstandard20.bsl |
Updates the FCS API baseline. |
ServiceParsedInputOps.fsi |
Exposes insertion and namespace-count metadata. |
ServiceParsedInputOps.fs |
Computes syntax-aware insertion positions and namespace counts. |
ServiceAssemblyContent.fsi |
Exposes openable identifier counts. |
ServiceAssemblyContent.fs |
Tracks namespace/module reach while traversing symbols. |
.VisualStudio/18.vNext.md |
Documents editor fixes. |
.FSharp.Compiler.Service/11.0.200.md |
Documents FCS behavior and API changes. |
| /// 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 |
There was a problem hiding this comment.
Right, and it did not need to change: 7fdc34a drops OpenableIdentCount and reads what the suggested open names from AssemblySymbol.Symbol, walking its declaring entities. AssemblySymbol is as on main.
| /// 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 |
There was a problem hiding this comment.
This field stays: Namespace is relative to the current scope, so which namespace, module or type it names cannot be recovered from it. 7fdc34a moves the release note to ### Changed and says that code constructing the record has to pass it.
There was a problem hiding this comment.
Please preserve source and binary compatibility here. Do not merge this with a public record shape break. An intentional break would need separate explicit release-owner approval and documented handling.
Auto-resolved by the GitHub Copilot app.
| let openDeclaration (entity: InsertionContextEntity) openableIdentCount ns = | ||
| if entity.NamespaceIdentCount > openableIdentCount then | ||
| $"open type {ns}" | ||
| else | ||
| $"open {ns}" |
There was a problem hiding this comment.
Fixed in ecbc332: when the type to open is generic, no open type is offered; the qualifications still are. Covered by Doesn't offer to open a generic type.
| |> 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 |
There was a problem hiding this comment.
Fixed in 6aaa651: the qualifications are ordered like the opens before the list is cut, so for WriteLine all three are System ones. Covered by Offers the System qualifications first.
| let lineBreak = | ||
| match line.Text.ToString(TextSpan(line.End, line.EndIncludingLineBreak - line.End)) with | ||
| | "" -> Environment.NewLine | ||
| | breakChars -> breakChars |
d6de3fc to
6aaa651
Compare
6aaa651 to
b33f201
Compare
b33f201 to
853849e
Compare
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>
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>
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
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>
Brings in the compaction of the placement fix: names in place of comments, theories in place of copied facts, and AddOpenOnTopSharedTests with the case both settings files had a copy of. The editor tests of this branch use the fix through multiFix, so the shared file offers allFixes, openFixes and a tryFix that returns the one open the fix offers, and the facts of the base branch keep reading codeFix |> tryFix code mode. The release notes list each change once, the comment above the helper that shortens a namespace is gone (it is withoutQualifierWritten now) and the doc of NamespaceIdentCount is two lines. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
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 lookup is tryFindModuleOrTypeNamedByLeading with named counts, the decision is one flat match with a case for each outcome, and the operator filter is isQualifiable. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
The eight facts that spelled out the same module, the same source and the same fix text become two theories and one fact. One table has the source line and the opens the fix offers, and the expected fix texts are built from the declaration and the source line. A second table has the source line and the messages in lightbulb order, which now also lists the opens in front of the System qualifications. The qualified-name fact derives its expected text from its source. The helpers allFixes and openFixes come from the shared file. The cases and what they expect are the same as before. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
|
Compacted in 529c3f7 (the merge of the compacted #20500, which brings the attribute-header case once, in In the editor tests the existing facts keep their bodies, because the shared file offers Left as is: the row for the |
…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>
Port of the dotnet#20670 compaction (fix/add-open-extension-members 642c00c..32a3ec1) to all-fixes. The part of that range which merges the compacted dotnet#20501 head came with the previous two commits; this one carries what is specific to the extension member code fix: - Find the extension opens in flat helpers named for what they find - Run the extension member cases as theories The release note bullet of dotnet#20670 is the shortened one. 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>
Port of the dotnet#20670 compaction (fix/add-open-extension-members 642c00c..32a3ec1) to all-fixes. The part of that range which merges the compacted dotnet#20501 head came with the previous two commits; this one carries what is specific to the extension member code fix: - Find the extension opens in flat helpers named for what they find - Run the extension member cases as theories The release note bullet of dotnet#20670 is the shortened one. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
|
|
||
| doRange NestedModule fullIdent moduleKeywordLine moduleBodyIndentation | ||
| List.iter (walkSynModuleDecl fullIdent) decls | ||
| if headerLine <= currentLine && not useMayBeInModuleAttribute then |
There was a problem hiding this comment.
🤖 🕵️ [P2] Nearest-scope open type System.Console escapes the attributed module and breaks sibling ReadLine() (FS0193)
// AlwaysPlaceOpensAtTopLevel = false; apply open type System.Console to WriteLine
module Top
module Original =
let ReadLine () = 123
open Original
[<System.Obsolete>] module M = let x = WriteLine "hi"
let number: int = ReadLine()…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>
Port of the dotnet#20670 compaction (fix/add-open-extension-members 642c00c..32a3ec1) to all-fixes. The part of that range which merges the compacted dotnet#20501 head came with the previous two commits; this one carries what is specific to the extension member code fix: - Find the extension opens in flat helpers named for what they find - Run the extension member cases as theories The release note bullet of dotnet#20670 is the shortened one. 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>
…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>
Port of the dotnet#20670 compaction (fix/add-open-extension-members 642c00c..32a3ec1) to all-fixes. The part of that range which merges the compacted dotnet#20501 head came with the previous two commits; this one carries what is specific to the extension member code fix: - Find the extension opens in flat helpers named for what they find - Run the extension member cases as theories The release note bullet of dotnet#20670 is the shortened one. 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>




Stacked on #20500.
For an unresolved
FileinFile.ReadAllText "x", the "open namespace" fix offeredopen System.Net.WebRequestMethods, which does not compile (FS0039), and neverSystem.IO.open typeinstead of a plainopen; a type inside a generic type is not offered, sinceopen typewould need type arguments.Systemfirst, as in Roslyn) instead of just the first one found.🤖 Generated with Claude Code