Repository navigation
Add the F# Interactive window on the JSON-RPC protocol - #20565
xperiandri wants to merge 37 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 window package records itself in the properties of the buffers it creates for input but not for output, so asking a buffer which window it belongs to could never answer for output. It also replaces the output buffer on reset, which rules out stamping one. The windows evaluating F# now keep a register, and an output buffer is ours when one of them currently owns it. Ownership is also settled later than before. The buffer reaches the provider before the window has finished claiming it, so deciding at that moment decided once and for all against us; it is now asked on each request until known. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The session used to learn its host from the handshake alone, so a Visual Studio that died between launching fsi and connecting left it waiting on the pipe for good. `--fsi-server-client-pid` names the process before the pipe opens; the handshake still carries it for a session started some other way. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The window picked its own fsi: `dotnet.exe` from Program Files, or one of the desktop executables the extension ships, chosen through the options page the old window reads. The desktop ones cannot serve the protocol at all — the server exists in the .NET fsi alone — and the machine-wide `dotnet` ignored the SDK a solution pins. The session now starts in the open solution's folder with the `dotnet` a shell there would run, so the host resolves the SDK from that folder's `global.json` exactly as `dotnet fsi` typed at a prompt does, and the window runs the same compiler bits as `dotnet build`. The platform choice, the desktop executables and the shadow-copy switch stay with the old window. When a session comes up the window prints which fsi answered, on what runtime and where, so a pinned SDK is seen rather than guessed at. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
For --fsi-server-jsonrpc and --fsi-server-client-pid the last occurrence wins, so the user's own arguments could name another pipe or another process as the session's owner, and the session would outlive the window. The window's switches now come last; a test pins the last-wins rule they rely on. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…the F# compiler The window used the compiler for one thing, its lexer, to colour input and output and to decide when Enter submits. It now asks an ILexicalScannerFactory, which FSharp.Editor, already running on the compiler Visual Studio ships, exports. The window drops its reference to FSharp.Compiler.Service, and DisableTransitiveProjectReferences keeps FSharp.VS.FSI from handing it back; the session's own compiler remains the SDK's, in the dotnet fsi process. The submission rule is tested with the editor's scanner. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The JSON-RPC server the interactive window drives first ships in SDK 11.0.200, and only the .NET build of fsi has it. Until that SDK is the floor, the Microsoft.FSharp.Compiler setup package also installs that build, in FSharp\Tools\Interactive: the assemblies of fsi's output folder and its runtime configuration, run with `dotnet exec`. The debug VSIX lays it out the same way, and the F5 profile points FSHARP_INTERACTIVE_PATH at the .NET fsi.dll instead of the desktop fsi.exe, which has no server. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
The window tries `dotnet fsi` first, so the SDK that global.json names still supplies the compiler. An SDK that predates the server rejects the option and exits before the handshake; the window then starts the copy installed with Visual Studio. What the rejected attempt printed is held back and shown only if nothing else starts. The fsi that worked is remembered per folder, so a reset does not try the old SDK again, and the session banner says where the fsi came from. The exit handler is now attached before the process starts, so an early exit cannot leave the connect waiting. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
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.
| let toolWindow = this.Open(0, focus = false) | ||
|
|
||
| evaluator |> ValueOption.iter _.SetNextSubmissionOrigin(sourcePath, startLine) |
There was a problem hiding this comment.
🤖🕵️ Queued submissions overwrite the single source-origin slot.
A from A.fsx:10
B from B.fsx:20
A runs with B's origin; B runs without one
…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>
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.