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

"dart language-server" prints text that interferes with LSP if --enable-experiment includes enabled-by-default experiments #51820

Closed
DanTup opened this issue Mar 22, 2023 · 4 comments
Assignees
Labels
area-dart-cli Use area-dart-cli for issues related to the 'dart' command like tool.

Comments

@DanTup
Copy link
Collaborator

DanTup commented Mar 22, 2023

@CoderDake hit this earlier. In VS Code he had configured dart.analyzerAdditionalArgs to include --enable-experiment=records. After upgrading to a new SDK, all language functionality stopped working but there was no obvious reason why (and no visible error message).

In the VS Code developer tools console, this error was printed:

workbench.desktop.main.js:1979 Error: Header must provide a Content-Length property.
	at StreamMessageReader.onData (vscode-file://vscode-app/Users/danchevalier/.vscode/extensions/dart-code.dart-code-3.60.1/out/dist/extension.js:31505:27)
	at LoggingTransform.<anonymous> (vscode-file://vscode-app/Users/danchevalier/.vscode/extensions/dart-code.dart-code-3.60.1/out/dist/extension.js:31489:18)
	at LoggingTransform.emit (node:events:526:28)

This is coming from VS Code's LSP client. It's detecting invalid protocol traffic, but unfortunately the error is not visible without knowing to go looking for it. Enabling logging showed that this output had been sent:

'records' is now enabled by default; this flag is no longer required.

This comes from here:

print("'${feature.enableString}' is now enabled by default; this "
'flag is no longer required.');

This is a helpful message if the command is being run by a user, but it would be better suppressed when running something like LSP where the output to stdout needs to confirm to a protocol, because it's not certain that the tool consuming this (in this case VS Code, but with LSP it could be one of many LSP clients for different editors) will expose the message to the user.

The same probably applies to dart debug-adapter.

Some thoughts:

  • can we detect if we're running with a terminal attached, and only show in that case?
  • can we have a block/allow list for commands where this output should not appear?

@bkonyi @bwilkerson any thoughts? This printing isn't new/recent and I suspect not may people will hit it - but without knowing what to look for it could be frustrating to try to track down.

@bkonyi
Copy link
Contributor

bkonyi commented Mar 22, 2023

Would printing to stderr cause any issues for the LSP? We can always update that print to output there instead.

@DanTup
Copy link
Collaborator Author

DanTup commented Mar 22, 2023

I believe that should be fine. It's slightly LSP-client-specific (since the LSP protocol doesn't dictate how processes are started or which streams are used), but since LSP is only using one stream in each direction I wouldn't expect any LSP client was trying to parse stderr for anything.

For VS Code, we only pass the stdin/stdout streams to the LSP client so it wouldn't even see stderr.

It is possible some LSP clients interpret stderr as errors, but unless the server fails to initialize, I don't think they should do anything more than possibly show it to the user (which I think would be fine, since it doesn't break any functionality, and the user gets to see the message).

@bkonyi
Copy link
Contributor

bkonyi commented Mar 22, 2023

Sounds good. I've got a fix up for review here.

@DanTup
Copy link
Collaborator Author

DanTup commented Mar 22, 2023

Great, thanks!

@mit-mit mit-mit added the area-dart-cli Use area-dart-cli for issues related to the 'dart' command like tool. label Mar 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-dart-cli Use area-dart-cli for issues related to the 'dart' command like tool.
Projects
None yet
Development

No branches or pull requests

3 participants