Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

support for executeCommand to produce additional output streams through the progress notification mechanism #2014

Open
Tracked by #69349
adonovan opened this issue Sep 8, 2024 · 4 comments
Labels
commands feature-request Request for new features or functionality
Milestone

Comments

@adonovan
Copy link

adonovan commented Sep 8, 2024

[Originally proposed here, but this is a more appropriate place.]

Background: Go's LSP server, gopls, replaces a number of analysis and testing tools that were command-line tools that reported textual output. An LSP server has many advantages: it has a hot in-memory cache, it can operate correctly on unsaved editor buffers, and it is of course tightly integrated into the editor. There remain a large variety of analysis tools we would like to build as features of gopls that need only the loosest of integration: at a minimum, the ability to stream text output to a terminal-like viewer, so that file:line:column line prefixes are marked up as source links that, when clicked, navigate the user's editor.

For example, Gopls today has a CodeLens command, go.test, that can be invoked via LSP executeCommand. It causes the server to execute the Go test identified by the selected source line and sends back the mixed stdout/stderr stream via the LSP progress notification mechanism. Unfortunately, the command is not very useful because of the variety of ways in which this output stream is handled by clients; many display only the most recent notification in some kind of current status bar.

It would enable new categories of tool integration with low effort if there were a way to indicate that the output of a command such as go.test should be handled differently: by streaming the output into a new, visible read-only textual buffer, much the same way VS Code's Output panel displays other streams such as the LSP server log or the LSP server's standard output.

Proposal: I propose that we add an option to executeCommandParams to indicate that the output of the progress stream for this command should be displayed in the client's canonical style of streaming output buffer. The option could be a string providing the name of the output window:

    interface executeCommandParams { 
          stream?: string; // if nonempty, the command's progress notifications should be directed to an output window of this name.
    }
@dbaeumer
Copy link
Member

dbaeumer commented Sep 9, 2024

From an implementation point of view this should IMO be implemented in the following ways:

  • the request defines a partial result
  • the client creates a progress token the server can stream against
  • the server reports progress against that token.

@dbaeumer dbaeumer added feature-request Request for new features or functionality commands labels Sep 9, 2024
@dbaeumer dbaeumer added this to the Backlog milestone Sep 9, 2024
@dbaeumer
Copy link
Member

dbaeumer commented Sep 9, 2024

The downside of this might be that it only allows one flat stream (e.g. no error, warning, info, debug)

@nthykier
Copy link
Contributor

nthykier commented Sep 9, 2024

The downside of this might be that it only allows one flat stream (e.g. no error, warning, info, debug)

Additionally:

  • The progress system I have seen in IntelliJ, then it is build around short one-line status updates with no accumulation of previous status updates.
  • The editor does not know it is a "build" and there is also no way of marking the process as "failed" vs. "successful". So the editor cannot do a popup saying "Build failed, click here for the log". Once the progress finishes in IntelliJ, it immediately disappears (into a notification pane where it is marked as done). In contrast, running a test or app keeps the run pane with associated log visible even when the application terminates. For tests, IntelliJ even does a little pop notification highlighting whether your test was successful or failed. It does not do that for all builds/runs nor for progresses as I recall.
  • Even if we could do log transfers this way, the provided progress messages is not considered a log in its own right nor have any rules for how the updates are merged (should the server accumulate or should the client do it?). Which means you cannot apply editor log support on it. As an example, IntelliJ has log-patterns that it applies to outputs from processes it runs. But these do not apply to progress updates (plain text vs. annotatable text)
  • For build/test/run app processes, it is common to have the "only one at the time rule". As an example, if I request a build, run an application (etc.) in IntelliJ, then IntelliJ will check if said "Run configuration" is already in progress and ask me if I want to restart it. We can ask all LSP servers to implement this logic themselves, but it smells like being the clients job to handle that singleton logic and the servers just providing the "This command is a singleton build/run command" (instead of a "This is an arbitrary formatting/code rewrite command").

There is also a question of how should this kind of command interact with https://microsoft.github.io/debug-adapter-protocol/specification. Because the moment we have "build", "test" and "run commands", we basically also want to provide debug support (if both parties support it). Related, we would want to have "singleton" support as in, "If the user asks to run this command again, it generally means restart the action rather than spawning a parallel instance". This is quite useful in all cases - but especially when the app being run is a webapp that binds on a specify port.

Then there is the question of run configuration. Sure, in the go.test case, the server might be able to figure that out with zero-configuration. But in the general case, you need per application stack configuration rules. Coming back to the Spring-boot app, which has at least ENV variables, java (CLI) parameters, application (CLI) parameters. But often, you want to provide specialized values for "port", "profiles", etc. because the user is looking for that high level feature and not the CLI options/ENV variable that happens to implement it. Here is starting to become awkward, because the server knows the application but the client provides the configuration view and I think we do not want the protocol to become a "write your own tab-pane GUI configuration dialogue over JSON-RPC"

So, I am starting to think it might take a bit more design work to consider what the strategy is here before we try to "retrofit" this into the existing "Command" interface. I think the current "Command" interface is sufficient for "edit-related commands", but I would be hard pressed to say whether it is the correct solution for a more generalized "Build", "Test", "Run app" workflow. I would say the current "Command" design fits a synchronous command better than an asynchronous command. Certainly, the server can spawn the process and do everything as notifications from there on, but it feels like the client should know about this as a first-class citizen feature rather than "Here is an opaque progress bar with an ever growing message field".

Note I want these features because I have users asking me for them in the other end. Like OP, I am also having a suboptimal experience with the clients with the current Command interface and the available options for providing messages/progress. There is also no doubt to me that the server should be involved, since it can identify where tests are defined and which framework is being used, it might know how to perform the build and it might know how to start the app (if there is an app to be run). But then the language server is not a debug adapter, so should it be involved in performing the task or is that the clients job...? I assume there is a point in not having all LSs implement the client side of the debug adapter spec.

@adonovan
Copy link
Author

adonovan commented Sep 9, 2024

@nthykier I fear you are over-anchoring on the domain of testing, which I gave only as an example. There are many other analyses we'd like to offer that take the form of "given a source selection, emit a stream of textual information about it".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
commands feature-request Request for new features or functionality
Projects
None yet
Development

No branches or pull requests

3 participants