Add file-based program API for use by IDE #48749
Conversation
There was a problem hiding this comment.
Pull Request Overview
This pull request adds a new file-based program API intended for IDE integration by introducing a new run-api command, updating parser and project conversion logic, and expanding test coverage.
- Introduces new tests for API behavior and diagnostic responses in RunFileTests.
- Refactors VirtualProjectBuildingCommand to use static helpers and updated error reporting.
- Updates TestCommand and SdkCommandSpec to support standard input redirection and adds the run-api command to the parser.
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| test/dotnet.Tests/CommandTests/Run/RunFileTests.cs | Added tests for API functionality and diagnostics for file-based programs. |
| test/dotnet.Tests/CommandTests/Project/Convert/DotnetProjectConvertTests.cs | Updated project conversion calls to use revised directive methods. |
| test/Microsoft.NET.TestFramework/Commands/TestCommand.cs | Added support for redirecting standard input in tests. |
| test/Microsoft.NET.TestFramework/Commands/SdkCommandSpec.cs | Updated ProcessStartInfo configuration for standard input redirection. |
| src/Cli/dotnet/Parser.cs | Integrated the new run-api command into the parser. |
| src/Cli/dotnet/Commands/Run/VirtualProjectBuildingCommand.cs | Refactored methods to use static helpers and improved error reporting with a diagnostics builder. |
| src/Cli/dotnet/Commands/Run/Api/RunApiCommandParser.cs and RunApiCommand.cs | Added a new API command for handling file-based program inputs and outputs. |
| src/Cli/dotnet/Commands/Project/Convert/ProjectConvertCommand.cs | Updated project conversion to call the revised directive methods correctly. |
There was a problem hiding this comment.
LGTM. It would be good to provide a self contained sample usage of the CLI--just a simple case which shows, say, running the command from CLI, piping in some data, and getting a result out on the other end, which would indicate the expected schema of the command.
| { | ||
| public override int Execute() | ||
| { | ||
| for (string line; (line = Console.ReadLine()) != null;) |
There was a problem hiding this comment.
I haven't seen this kind of pattern before, is this a norm for apps which communicate using json over stdio? To essentially put each message on its own line?
There was a problem hiding this comment.
JSON separated by newlines seemed simple and straightforward, but feel free to suggest alternatives (especially what IDE would like to communicate with). I guess JSON-RPC is more standard, but likely needs more code / libraries. Instead of stdio we could also use named pipes like compiler server does.
There was a problem hiding this comment.
I don't have particular concerns about the current solution, just trying to make sure I understand the design. Thanks!
There was a problem hiding this comment.
I'd definitely consider JSON-RPC as it is a standard for this kind of stuff.
There was a problem hiding this comment.
When I did the out-of-proc build host stuff, I implemented my own RPC via single-line as well. That said, it's a bit unclear to me why we're needing multiple lines like this.
| public sealed class GetProject : RunApiInput | ||
| { | ||
| public string? ArtifactsPath { get; init; } | ||
| public required string EntryPointFileFullPath { get; init; } |
There was a problem hiding this comment.
This represents the primary bit of information that IDE must provide, right? This means that IDE must come up with a solution for searching, from a loose file that was opened, to find a "nearest relevant entry point", cracking the files as we go, and pass that in to dotnet cli. I think that's something we can do, just making sure of the expectation.
There was a problem hiding this comment.
This API is for single-file currently, so it should be simple - just pass the current file as the EntryPointFileFullPath.
For multi-file scenarios, I agree you would need to do the search you describe. The API will then also take the list of files to exclude (i.e., the other entry points). The implementation can automatically discover the other .cs files to include (which it needs to read for directives) based on the entry point or we can pass them to the API if that would be better for perf (since the IDE might have already discovered them via its more efficient file watchers).
There was a problem hiding this comment.
Btw, it might be useful if the IDE checks SDK's version to determine whether run-api is present and whether it supports single-file or multi-file scenarios (and perhaps more in the future) - so the IDE can give better error messages when the SDK being used is not in sync with the features implemented in the IDE.
We could also introduce some run-api call like "GetCapabilities" that the IDE could use (and for SDKs that don't contain run-api command at all, presumably the IDE could detect that from the error message SDK produces when trying to run dotnet run-api).
There was a problem hiding this comment.
Maybe a version stamp in requests/responses would give us enough expressiveness here? That would be a bit like SymbolKey.FormatVersion.
| """)}},"Diagnostics": | ||
| [{"Location":{ | ||
| "Path":{{ToJson(programPath)}}, | ||
| "Span":{"Start":{"Line":0,"Character":0},"End":{"Line":1,"Character":0}{{nop}}}{{nop}}}, |
There was a problem hiding this comment.
Oh, is this nop breaking-up the }s in order to keep distinct which ones are ending an interpolation and which ones are part of the content?
There was a problem hiding this comment.
The nop is to avoid compiler errors due to the closing brackets }} inside an interpolated string $$""". (the errors are redundant IMO, the interpolated string could be compiled just fine even though it contains unmatched }}.) See also the doc comment at the nop constant.
Example:
var s1 = $$"""{"x":{{1}},"y":{"z":2}}"""; // error on the last }}
var s2 = $$$"""{"x":{{{1}}},"y":{"z":2}}"""; // ok but interpolation holes need soo many brackets (even more when the JSON is more nested)
const string nop = "";
var s3 = $$"""{"x":{{1}},"y":{"z":2}{{nop}}}"""; // workaround (we can keep the number of `$$` constant wrt nesting of the JSON)
| /// (e.g., show an error message when an incompatible SDK version is being used). | ||
| /// </summary> | ||
| [JsonPropertyOrder(-1)] | ||
| public int Version { get; } = 1; |
There was a problem hiding this comment.
It might also make sense for caller to pass in the "version" of the API they are using, implying the set of behaviors they expect, and the run-api command can reject the request in a well-defined way if it's not "compatible" with that version.
There was a problem hiding this comment.
Actually, it looks like this property is on the base type? So the caller can always just read the version that comes out. It would be good to verify that this property is present in an error case. No need for caller to pass in the version.
There was a problem hiding this comment.
I just recreated this issue as it relates to output from the CLI. JSON will be one of the bespoke formats.
| { | ||
| public override int Execute() | ||
| { | ||
| for (string line; (line = Console.ReadLine()) != null;) |
There was a problem hiding this comment.
I'd definitely consider JSON-RPC as it is a standard for this kind of stuff.
No description provided.