Repository navigation
Offer every open that resolves the name, and open type where a plain open cannot reach - #20501
xperiandri wants to merge 36 commits into
Conversation
✅ Release notes checked
Warning No PR link found in some release notes, please consider adding it.
|
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.
There was a problem hiding this comment.
Done in 80e5c8b: InsertionContextEntity and AssemblySymbol are exactly as on main, so the surface-area baseline has no diff and the release note about the new field is gone. The count of idents that Namespace covers, which the editor needs to tell open from open type, now comes with the context from an internal function, ParsedInput.TryFindInsertionContextAt, visible to FSharp.Editor and the FCS tests only. TryFindInsertionContext keeps its public signature and behaviour.
| 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>
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()There was a problem hiding this comment.
Fixed in #20500 (6a7a4f7), which this PR is stacked on. The code fix and the completion now give the position of the use rather than its line, and a use after the start of the body of a nested module declared on one line is inside that module even when an attribute shares the line. For this example open type System.Console goes into M, so ReadLine() below keeps resolving to Original; a use in the attribute still stays outside. Covered here by Offers open type inside a nested module declared on one line with an attribute for a use in its body, with open System and completion counterparts in #20500.
…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>
The conflicts were in the add-open code fix and OpenDeclarationHelper, which main had also changed for active-pattern cases. The code fix keeps main's catalogue handling and calls the shared change builder; main's getInsertionPosition is dropped, since the insertion context now already names the first line inside the scope. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…ion of the use
A block comment that runs on from a module or namespace header made the line
below the header start inside the comment, so the open was written into it
and the name stayed unresolved:
module Top (* header
*)
let x = Console.WriteLine 42
The header now ends where such a comment does, and a declaration that begins
on that line is entered at its own position, as one declared on the header's
line already was.
A nested module declared on one line with an attribute was kept out for any
use on its line, because the code fix and the completion gave a line only.
The attribute can hold the use, and then the open must stay outside, but so
can the body, and then it must not: the open escaped the module and
changed what a name outside resolved to. The code fix and the completion
now give the position of the use, and only a use before the body counts as
in the attribute. FindNearestPointToInsertOpenDeclaration and
TryFindInsertionContext keep taking a line and behave as before.
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
The conflicts were in the add-open code fix and the entity construction, where main had added the active-pattern handling and identifier formatting. The code fix keeps both: main's catalogue and active-pattern entities, with the symbol each candidate came from carried along for the open type decision. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…open Main's active-pattern tests encoded two spacings that the code fix and the completion each had: the code fix left no blank line between an adjacent header and the open, the completion left none between a header and an open placed under a blank line. Both now go through one change builder, so the open always gets a blank line above it and below it, and InsertionContext.Pos names the first line inside a namespace instead of its header line. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…nt internally InsertionContextEntity gained a field, which changes the constructor of a public record for binary and source clients alike. The count says how many leading idents of the entity's full name its Namespace covers, and the editor needs it to tell a namespace or module, which a plain open reaches, from a type, which only open type does. It now comes with the internal TryFindInsertionContextAt, next to the context, and the public record and TryFindInsertionContext are exactly what they were. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…e body is unresolved Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
The provider offers several opens, System first, so the one a test is about need not be the 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>
The conflicts were in the add-open code fix, where this branch had split the work into small helpers and dotnet#20501 had moved the entity construction and the active-pattern handling of main. The helpers now build the same name candidates dotnet#20501 does, each with the symbol it came from and the count of idents its namespace covers, and a member access still looks for extension members instead. 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>
Merging dotnet#20500 into this branch kept both the old and the reworded note, because the release notes merge with the union driver. 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