Repository navigation
Add the F# Interactive window on the JSON-RPC protocol - #20565
xperiandri wants to merge 40 commits into
Conversation
✅ No release notes required |
|
|
||
| <ItemGroup> | ||
| <ProjectReference Include="$(FSharpSourcesRoot)\FSharp.Core\FSharp.Core.fsproj" /> | ||
| <ProjectReference Include="$(FSharpSourcesRoot)\Compiler\FSharp.Compiler.Service.fsproj" /> |
There was a problem hiding this comment.
How would the architecture need to change for the window to locate the SDK, and start the JSON-RPC server from the SDK? i.e. the Window NOT having a reference to FSharp.Compiler.Service.fsproj
That way, we decouple VS versions and SDK versions (steered via global.json).
We would also gain parity of CLI fsi, CLI build VS FSI - guaranteed to use same compiler bits.
And more importantly, the VS FSI could then be powered by net11 F# compiler, benefiting from any perf improvements on the .NET side.
There was a problem hiding this comment.
Done in the last commit: the session starts in the solution folder with the dotnet a shell there would run, so the host resolves the SDK from global.json exactly as dotnet fsi does — nothing in the window re-implements that, and the fsi it evaluates on is the SDK's. The FCS reference that remains serves the tokenizer only (Enter-vs-newline in SubmissionAnalysis, lexical colouring) — the same FCS FSharp.Editor already loads; it plays no part in evaluation.
There was a problem hiding this comment.
🤖🕵️ @xperiandri This is still occurring.
<ProjectReference Include="$(FSharpSourcesRoot)\Compiler\FSharp.Compiler.Service.fsproj" />SDK selection is fixed; the requested no-FCS window boundary is not.
There was a problem hiding this comment.
You are right, and my earlier answer only covered evaluation. The window no longer references FSharp.Compiler.Service.fsproj, and cannot compile against it: DisableTransitiveProjectReferences stops FSharp.VS.FSI (which keeps the compiler for the old window) from handing it on. What the window used the compiler for was its lexer, to colour input and output and to decide when Enter submits. That is now ILexicalScannerFactory in LexicalScanning.fs, and FSharp.Editor, which already runs on the compiler Visual Studio ships, exports the implementation. The session's own compiler stays the SDK's, in the dotnet fsi process. The submission rule is still tested, with the editor's scanner, in SubmissionAnalysisTests.
| UICultureLcid: int | ||
| } | ||
|
|
||
| module internal FsiLocator = |
There was a problem hiding this comment.
This should use the SDK locator as if you launched dotnet fsi from the solution folder (i.e. following global.json).
Also as long as we keep the legacy FSI in current shape, the new window via JSON-RPC could be netcoreapp only and not offer any of the .NET Framework options.
There was a problem hiding this comment.
Both done: the platform choice, the desktop executables and the shadow-copy switch are gone from this window (they stay with the old one), and the session starts in the solution folder so global.json picks the SDK. When a session comes up the window prints which fsi answered, on what runtime and where.
0bdeb79 to
1990f9e
Compare
|
Re-stacked on xperiandri/fsharp@c5501123 (#20396 after its second pass: the server is .NET-only, the pipe is Two things from the review are still open and are the next steps for this PR rather than for #20396:
One limit worth knowing: the client side cannot ask for |
1db5c63 to
0e32fd1
Compare
|
@xperiandri Current head |
| addSwitch $"{CommandLine.ServerOption}{pipeName}" | ||
| // How the host names itself: a session whose host dies, even before the handshake, exits instead of | ||
| // waiting on the pipe forever. | ||
| addSwitch $"--fsi-server-client-pid:{clientProcessId}" |
There was a problem hiding this comment.
🤖🕵️ @xperiandri user arguments can override the trusted owner PID; closing VS leaves FSI alive.
trusted: --fsi-server-client-pid:<VS pid>
UserArguments: --fsi-server-client-pid:0
owner exits -> server_alive=true
// Reject duplicate ownership switches, or append the trusted PID after UserArguments.There was a problem hiding this comment.
Fixed. The window's own switches now come last, after the user's arguments, and for --fsi-server-jsonrpc and --fsi-server-client-pid the last occurrence wins, so --fsi-server-client-pid:0 in the arguments no longer names the owner. the last owner named on the command line is the one that counts starts a session with two owners and closes the first; the session keeps serving.
7609924 to
5aeed62
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
The filter sits on every F# editor view, so answering S_OK for the whole VsStd11 group hid the status of every other command in it from the targets behind. Commands it does not handle now go to the next target. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The exit handler reports an exit only once the session is current, so one that exited between the handshake and the install was dropped and the dead session installed. The install now checks the session under the same lock and reports the exit as a failed start. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The origin was cleared only after the session started, so a send while fsi could not start left it for the next text typed into the window. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Closing disposed the evaluator but kept the window and evaluator fields, so the next open submitted through the disposed host. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Registration, the VSIX entry and the editor commands have landed; only the debugger commands are left. Also drop a stray prefix from the title. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
09338b1 to
d74dfcc
Compare
…fewer comments Round one of the NoBloat and code-compaction pass asked for in review. Removed from the pull request: - docs/ide/FSI-Modern-Interactive-Window-Plan.md, the plan and status of the work; - .claude/rules/CcrOptout.md, an agent rule that is no part of this feature; - members nothing calls: the host client's IsRunning, EvaluatingProcessId, Initialization, ExecuteFileAsync and SetPathsAsync, the session's EvaluatingProcessId and Process, the evaluator's EvaluatingProcessId and Host, the provider's Window and Evaluator, the package's Provider and its InitializeAsync override that only called the base; - TolerateUnusedBindings: the project compiles with unused bindings as errors; - comments that restated the code, told its history or defended the design. Reused instead of repeated: - PredefinedInteractiveContentTypes and PredefinedClassificationTypeNames for the names the classifier spelled as literals; - TextWriter.Null for a window that is not there, in place of four null-checking writers; - Result.bind, Result.map and List.sortBy where the host matched and filtered by hand. Extracted: - HeldOutput, the buffer that keeps a starting session's output back until its handshake; - SessionProcess, for the command line, the output pumps and the quiet teardown; - one way to build a candidate that runs on the dotnet host, one to describe an early exit, and one to install a started session; - the evaluator's subscriptions as one list, and one function that reports a failed start; - LexicalToken.IsWithin, the bounds check the classifier and the submission rule both made. The send-to-interactive active pattern yields the kind, so Exec has one arm for both commands, and the continuation tokens are three lines of text instead of fifty-five of list. FsiCommandService.fs keeps the byte-order mark it has on main. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…onstructor Round two of the NoBloat and code-compaction pass asked for in review. - SubmissionAnalysisTests: eleven facts that each asserted a list of inputs become one theory over named scenarios, and the terminator test a second one. The 32 pairs of input and expectation are the same. - FsiLocator builds its three candidates through one function, and the dotnet host is dotnet.exe without asking the platform: the assembly only loads in Visual Studio. - The output pump is a loop under the quiet wrapper the teardown uses, not a recursive function in a try inside a thread's lambda. - The execute request takes its two optional fields through ValueOption.toObj and toNullable. - The exit code is read in one place, the session's own member. - The project no longer references PresentationCore, PresentationFramework and WindowsBase, which nothing in it uses. - What an SDK too old for the server does is said once, where the candidates are ordered. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@T-Gro Two rounds are pushed, 9b95f7a and 512effa. The diff against |
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.
- This diff adds 88 new code comment lines and 7116 new code 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.
T-Gro
left a comment
There was a problem hiding this comment.
🤖🕵️ AI review — verify independently.
…fewer comments Round one of the NoBloat and code-compaction pass asked for in review. Removed from the pull request: - docs/ide/FSI-Modern-Interactive-Window-Plan.md, the plan and status of the work; - .claude/rules/CcrOptout.md, an agent rule that is no part of this feature; - members nothing calls: the host client's IsRunning, EvaluatingProcessId, Initialization, ExecuteFileAsync and SetPathsAsync, the session's EvaluatingProcessId and Process, the evaluator's EvaluatingProcessId and Host, the provider's Window and Evaluator, the package's Provider and its InitializeAsync override that only called the base; - TolerateUnusedBindings: the project compiles with unused bindings as errors; - comments that restated the code, told its history or defended the design. Reused instead of repeated: - PredefinedInteractiveContentTypes and PredefinedClassificationTypeNames for the names the classifier spelled as literals; - TextWriter.Null for a window that is not there, in place of four null-checking writers; - Result.bind, Result.map and List.sortBy where the host matched and filtered by hand. Extracted: - HeldOutput, the buffer that keeps a starting session's output back until its handshake; - SessionProcess, for the command line, the output pumps and the quiet teardown; - one way to build a candidate that runs on the dotnet host, one to describe an early exit, and one to install a started session; - the evaluator's subscriptions as one list, and one function that reports a failed start; - LexicalToken.IsWithin, the bounds check the classifier and the submission rule both made. The send-to-interactive active pattern yields the kind, so Exec has one arm for both commands, and the continuation tokens are three lines of text instead of fifty-five of list. FsiCommandService.fs keeps the byte-order mark it has on main. This is 9b95f7a of dotnet#20565 without its two deletions: docs/ide/FSI-Modern-Interactive-Window-Plan.md and .claude/rules/CcrOptout.md leave the pull request only and stay on this branch. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…fewer comments Round one of the NoBloat and code-compaction pass asked for in review. Removed from the pull request: - docs/ide/FSI-Modern-Interactive-Window-Plan.md, the plan and status of the work; - .claude/rules/CcrOptout.md, an agent rule that is no part of this feature; - members nothing calls: the host client's IsRunning, EvaluatingProcessId, Initialization, ExecuteFileAsync and SetPathsAsync, the session's EvaluatingProcessId and Process, the evaluator's EvaluatingProcessId and Host, the provider's Window and Evaluator, the package's Provider and its InitializeAsync override that only called the base; - TolerateUnusedBindings: the project compiles with unused bindings as errors; - comments that restated the code, told its history or defended the design. Reused instead of repeated: - PredefinedInteractiveContentTypes and PredefinedClassificationTypeNames for the names the classifier spelled as literals; - TextWriter.Null for a window that is not there, in place of four null-checking writers; - Result.bind, Result.map and List.sortBy where the host matched and filtered by hand. Extracted: - HeldOutput, the buffer that keeps a starting session's output back until its handshake; - SessionProcess, for the command line, the output pumps and the quiet teardown; - one way to build a candidate that runs on the dotnet host, one to describe an early exit, and one to install a started session; - the evaluator's subscriptions as one list, and one function that reports a failed start; - LexicalToken.IsWithin, the bounds check the classifier and the submission rule both made. The send-to-interactive active pattern yields the kind, so Exec has one arm for both commands, and the continuation tokens are three lines of text instead of fifty-five of list. FsiCommandService.fs keeps the byte-order mark it has on main. This is 9b95f7a of dotnet#20565 without its two deletions: docs/ide/FSI-Modern-Interactive-Window-Plan.md and .claude/rules/CcrOptout.md leave the pull request only and stay on this branch. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
T-Gro
left a comment
There was a problem hiding this comment.
🤖🕵️ AI review — verify independently.
Keep the existing 18.12.772 pin and remove the older duplicate that causes NU1506 during restore. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1750b0a0-85af-4cd4-b6a1-c4b5cc8240fb
Align Shell.15.0 with the newer VS dependencies, handle nullable editor API results, and update the integration-test generator and UI-thread checks. Replace the build-completion semaphore with a cancellable asynchronous signal. Verified clean bootstrap and full Debug VisualFSharp solution build; 53 focused editor classification tests passed. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1750b0a0-85af-4cd4-b6a1-c4b5cc8240fb
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
…fewer comments Round one of the NoBloat and code-compaction pass asked for in review. Removed from the pull request: - docs/ide/FSI-Modern-Interactive-Window-Plan.md, the plan and status of the work; - .claude/rules/CcrOptout.md, an agent rule that is no part of this feature; - members nothing calls: the host client's IsRunning, EvaluatingProcessId, Initialization, ExecuteFileAsync and SetPathsAsync, the session's EvaluatingProcessId and Process, the evaluator's EvaluatingProcessId and Host, the provider's Window and Evaluator, the package's Provider and its InitializeAsync override that only called the base; - TolerateUnusedBindings: the project compiles with unused bindings as errors; - comments that restated the code, told its history or defended the design. Reused instead of repeated: - PredefinedInteractiveContentTypes and PredefinedClassificationTypeNames for the names the classifier spelled as literals; - TextWriter.Null for a window that is not there, in place of four null-checking writers; - Result.bind, Result.map and List.sortBy where the host matched and filtered by hand. Extracted: - HeldOutput, the buffer that keeps a starting session's output back until its handshake; - SessionProcess, for the command line, the output pumps and the quiet teardown; - one way to build a candidate that runs on the dotnet host, one to describe an early exit, and one to install a started session; - the evaluator's subscriptions as one list, and one function that reports a failed start; - LexicalToken.IsWithin, the bounds check the classifier and the submission rule both made. The send-to-interactive active pattern yields the kind, so Exec has one arm for both commands, and the continuation tokens are three lines of text instead of fifty-five of list. FsiCommandService.fs keeps the byte-order mark it has on main. This is 9b95f7a of dotnet#20565 without its two deletions: docs/ide/FSI-Modern-Interactive-Window-Plan.md and .claude/rules/CcrOptout.md leave the pull request only and stay on this branch. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…fewer comments Round one of the NoBloat and code-compaction pass asked for in review. Removed from the pull request: - docs/ide/FSI-Modern-Interactive-Window-Plan.md, the plan and status of the work; - .claude/rules/CcrOptout.md, an agent rule that is no part of this feature; - members nothing calls: the host client's IsRunning, EvaluatingProcessId, Initialization, ExecuteFileAsync and SetPathsAsync, the session's EvaluatingProcessId and Process, the evaluator's EvaluatingProcessId and Host, the provider's Window and Evaluator, the package's Provider and its InitializeAsync override that only called the base; - TolerateUnusedBindings: the project compiles with unused bindings as errors; - comments that restated the code, told its history or defended the design. Reused instead of repeated: - PredefinedInteractiveContentTypes and PredefinedClassificationTypeNames for the names the classifier spelled as literals; - TextWriter.Null for a window that is not there, in place of four null-checking writers; - Result.bind, Result.map and List.sortBy where the host matched and filtered by hand. Extracted: - HeldOutput, the buffer that keeps a starting session's output back until its handshake; - SessionProcess, for the command line, the output pumps and the quiet teardown; - one way to build a candidate that runs on the dotnet host, one to describe an early exit, and one to install a started session; - the evaluator's subscriptions as one list, and one function that reports a failed start; - LexicalToken.IsWithin, the bounds check the classifier and the submission rule both made. The send-to-interactive active pattern yields the kind, so Exec has one arm for both commands, and the continuation tokens are three lines of text instead of fifty-five of list. FsiCommandService.fs keeps the byte-order mark it has on main. This is 9b95f7a of dotnet#20565 without its two deletions: docs/ide/FSI-Modern-Interactive-Window-Plan.md and .claude/rules/CcrOptout.md leave the pull request only and stay on this branch. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
An editor send left its source file and line in a single slot that the next send overwrote, so with two sends in a row the first ran with the second's origin and the second with none. The evaluator now keeps the origins in the order they were sent, and a submission takes the one that belongs to its own text. Text typed into the window takes none, and an origin whose submission never ran is dropped once a later one runs. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
The review found the added comments too many: 88 lines, 7116 characters. 23 lines, 2409 characters remain, 13 of them licence headers. What went restated a name or a test, defended a design the PR body already covers, or was wrong: the window package never replaces its output buffer, which one comment claimed. Where a comment made up for a name, the name took over (readAndAdvanceCaret, OneBasedStartLine, pumpWithoutWaitingForNewline, OutputHeldUntilHandshake, SessionExitedUnexpectedly, installUnlessExited, unguessablePipeName, carriesStateAcrossLines = true at the call sites). The solution directory lookup asserts the UI thread instead of saying so: its try would have hidden a call from another thread. Kept: the persisted tool-window id, the F# content type name, the Tools\Interactive layout, the rule that the last --fsi-server-* switch wins, Process.Exited being raised once, no CurrentUserOnly on net472, FS3511 and the global.json note on the working directory. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
|
Went through the diff with the code-compaction skill (three reviewers from different model families for the comments, one for reuse). Comments added by the PR, counted the same way as in the review (added Also fixed in this push: queued editor sends no longer overwrite each other's source origin (the open AI-review thread), with tests. |
…fewer comments Round one of the NoBloat and code-compaction pass asked for in review. Removed from the pull request: - docs/ide/FSI-Modern-Interactive-Window-Plan.md, the plan and status of the work; - .claude/rules/CcrOptout.md, an agent rule that is no part of this feature; - members nothing calls: the host client's IsRunning, EvaluatingProcessId, Initialization, ExecuteFileAsync and SetPathsAsync, the session's EvaluatingProcessId and Process, the evaluator's EvaluatingProcessId and Host, the provider's Window and Evaluator, the package's Provider and its InitializeAsync override that only called the base; - TolerateUnusedBindings: the project compiles with unused bindings as errors; - comments that restated the code, told its history or defended the design. Reused instead of repeated: - PredefinedInteractiveContentTypes and PredefinedClassificationTypeNames for the names the classifier spelled as literals; - TextWriter.Null for a window that is not there, in place of four null-checking writers; - Result.bind, Result.map and List.sortBy where the host matched and filtered by hand. Extracted: - HeldOutput, the buffer that keeps a starting session's output back until its handshake; - SessionProcess, for the command line, the output pumps and the quiet teardown; - one way to build a candidate that runs on the dotnet host, one to describe an early exit, and one to install a started session; - the evaluator's subscriptions as one list, and one function that reports a failed start; - LexicalToken.IsWithin, the bounds check the classifier and the submission rule both made. The send-to-interactive active pattern yields the kind, so Exec has one arm for both commands, and the continuation tokens are three lines of text instead of fifty-five of list. FsiCommandService.fs keeps the byte-order mark it has on main. This is 9b95f7a of dotnet#20565 without its two deletions: docs/ide/FSI-Modern-Interactive-Window-Plan.md and .claude/rules/CcrOptout.md leave the pull request only and stay on this branch. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Replaces the Visual Studio F# Interactive window with one hosted on
Microsoft.VisualStudio.InteractiveWindow— the same REPL engine C# Interactive and Python Interactive use — driving fsi over the JSON-RPC protocol fsi gained in #20396 instead of parsingSERVER-PROMPT>text on stdin.