Resolve a script's #r lines from its current text - #20645
Open
xperiandri wants to merge 9 commits into
Open
xperiandri wants to merge 9 commits into
xperiandri wants to merge 9 commits into
Conversation
The project options reactor looked up the caret itself through ServiceProvider.GlobalProvider, the RDT, IVsTextView and an IVsTextViewEvents connection point, all of which need the UI thread. When the UI thread synchronously waited on project options (breakpoint validation when a document frame is shown), the reactor waited for the UI thread and the UI thread for the reactor. An IWpfTextViewCreationListener now publishes the caret of the focused editor into the text buffer's properties, and the reactor only reads it. Only scripts look for it: FCS uses the caret only to skip the `#r "nuget: ..."` line being typed. Fixes dotnet#20522 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Only the shared FocusedCaret.Position field needed to become `option` for the atomic reference write; the reactor's own local binding was converted along with it for no reason. Route it back through ValueOption and land on `option` once, at the two points that need it. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The caret belongs to the buffer, which outlives the view. A view closed while it held focus left its line published, and the reactor went on skipping the `#r "nuget: …"` on a line no editor was on. `FocusedCaretTests` covers what the reactor reads and when it is told to read again: a move to another line, a move along one, focus leaving, focus already gone, and a text no view is open on. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…aves a line The options recomputed when the caret moves to another line were built from the text captured when the cache entry was created. Edits made since then had not reached them, so leaving a `#r "nuget: ..."` line could resolve the half-typed name it held at that point - the extra package download the caret was introduced to avoid. Read the buffer's current text instead; it is a snapshot read and needs no UI thread. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Contributor
✅ Release notes checked
|
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Two moderate issues remain in caret initialization and project-options cache reconnection.
Review effort: Lite
Findings: 1
What changed in this PR
This pull request updates script #r resolution to use the editor’s current text and tracks focused caret changes.
Changes:
- Adds focused-caret tracking and tests.
- Refreshes project options from current buffer text.
- Removes legacy view-event helpers and adds release notes.
| File | Summary | Review notes |
|---|---|---|
vsintegration/tests/FSharp.Editor.Tests/FSharp.Editor.Tests.fsproj |
Includes new tests. | — |
vsintegration/tests/FSharp.Editor.Tests/FocusedCaretTests.fs |
Tests caret behavior. | Exercises the failing null-container path. |
vsintegration/src/FSharp.Editor/LanguageService/FSharpProjectOptionsManager.fs |
Uses current buffer text and caret tracking. | Moderate: cached entries may not reconnect when a view appears. |
vsintegration/src/FSharp.Editor/LanguageService/FocusedCaret.fs |
Publishes focused caret state. | Moderate: accessing Container can throw before the null case. |
vsintegration/src/FSharp.Editor/FSharp.Editor.fsproj |
Includes caret tracking implementation. | — |
vsintegration/src/FSharp.Editor/Common/Extensions.fs |
Removes legacy view-event helpers. | — |
docs/release-notes/.VisualStudio/18.vNext.md |
Documents the changes. | — |
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+276
to
+278
| focusedCaret | ||
| |> ValueOption.map _.LineChanged.Subscribe(updateProjectOptions) | ||
| |> ValueOption.toOption |
github-actions
Bot
added
⚠️ Affects-Design-Time
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Contributor
|
🔍 Tooling Safety Check — Affects-Design-Time, Affects-Restore
|
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on #20523
When the caret leaves a line of a script, its
#r "nuget: …"references are resolved again, but from the text the file had when its options were last computed rather than the text now in the editor. Edits made since then had not reached that copy, so leaving a#rline could resolve the half-typed package name it held at that point – the download the caret exists to prevent. They are now resolved from the buffer's current text.The options the file is checked with still keep that line excluded until the next edit, as before.
🤖 Generated with Claude Code