"dart language-server" command no longer defaults to LSP

@bwilkerson

Using a bleeding edge build, dart language-server runs the original protocol (not LSP), which causes it to fail from VS Code.

@bwilkerson @jwren not certain yet, but I think this may be related to 1598182 which removed code that had defaultToLsp: true. Is it still intended for LSP to be the default?

There does seem to be a test specifically for this:

sdk/pkg/dartdev/test/commands/language_server_test.dart

Lines 72 to 74 in e51af7c

    
           test('protocol default', () async { 
        
             return runWithLsp(['language-server']); 
        
           });

I'll try to figure out why it's not failing.

Edit: That test does fail for me locally when running with a bleeding edge build:

00:00 +0 -1: language-server protocol default [E]

  Expected: a string starting with 'Content-Length: '
    Actual: '{"event":"server.connected","params":{"version":"1.33.1","pid":18179}}\n'

I can see some flaky reports here:

https://dart-ci.firebaseapp.com/current_results/#/filter=pkg/dartdev/test/commands/language_server_test&showAll
https://dart-ci.firebaseapp.com/#showLatestFailures=false&test=pkg/dartdev/test/commands/language_server_test

Although there don't seem to be any results on that latest link since the change in question above landed. They show as unittest-asserts-release-linux, although I can't see an obvious column on https://ci.chromium.org/p/dart/g/alt/console that looks like it's that bot.

Yeah, there's a good chance that we needed to have added an explicit argument to specify the protocol when running the snapshot.

I intentionally didn't touch the argument parsing logic so I'm a bit surprised this happened, thanks for bringing this to our attention

@jwren

@jwren @bwilkerson so it turns out that the server has supported both --lsp and --protocol=lsp itself (without dart language-server) since the dart language-server command was added:

sdk/pkg/analysis_server/lib/src/server/driver.dart

Lines 153 to 159 in 3e1bad6

    
           if (results.wasParsed(USE_LSP)) { 
        
             analysisServerOptions.useLanguageServerProtocol = 
        
                 results[USE_LSP] as bool; 
        
           } else { 
        
             analysisServerOptions.useLanguageServerProtocol = 
        
                 results[SERVER_PROTOCOL] == PROTOCOL_LSP; 
        
           }

sdk/pkg/analysis_server/lib/src/server/driver.dart

Lines 707 to 713 in 3e1bad6

    
           // This option is hidden but still accepted; it's effectively translated to 
        
           // the 'protocol' option above. 
        
           parser.addFlag(USE_LSP, 
        
               defaultsTo: false, 
        
               negatable: false, 
        
               help: 'Whether to use the Language Server Protocol (LSP).', 
        
               hide: true);

I believe this means it's definitely safe for VS Code to always pass --protocol=lsp any time it's using the dart language-server command (because both the dartdev command and the server handle it directly). For older SDKs that don't support dart language-server we'll continue to use --lsp (and call the snapshot).

I'll ship a fix for this shortly.

@jwren

@jwren I've shipped a fix for this in v3.42.1 of the (Dart) VS Code extension and confirmed it's working.

If we intend to leave it as-is, the output of dart language-server --help should be updated to not show LSP as the default:

dart language-server --help
// ...
    --protocol=<protocol>                 Specify the protocol to use to communicate with the analysis server.

          [analyzer]                      Dart's analysis server protocol (https://dart.dev/go/analysis-server-protocol)
          [lsp] (default)                 The Language Server Protocol (https://microsoft.github.io/language-server-protocol)
// ...

Something that does occur to me, is that anyone on an older version of the VS Code extension that's using Flutter master/bleeding-edge Dart will not have this fix. That's hopefully not a lot of people, but something to be aware of if issues are reported.

I think we do want to default to LSP - it was an explicit choice when authoring the dart language-server command.

If a fix here is not in progress, I suggest we just revert https://dart-review.googlesource.com/c/sdk/+/246059. That's inexpensive and means we can re-apply a fixed version w/o rushing.

@DanTup

Reverting that change would break internal users (unbreaking them why that change was made).

There might still be some small amount of work left to restore the default, but that won't impact most users of VS Code because of the change @DanTup made. Not getting that fix in place might break other LSP clients, however.

@DanTup

Reverting that change would break internal users (unbreaking them why that change was made).

It's not actually breaking - they just get less functionally than they'd had.

There might still be some small amount of work left

There definitely is - this switched the default.

to restore the default, but that won't impact most users of VS Code because of the change @DanTup made. Not getting that fix in place might break other LSP clients, however.

I don't think the answer here is to hope that all the VS Code clients upgrade quickly. If we don't expect to land a fix shortly, we should just revert the CL, and re-land w/ a fix applied.

@christopherfujino

FWIW, @christopherfujino found that neovim is affected by this - it was changed to use dart language-server fairly recently:

https://github.com/neovim/nvim-lspconfig/pull/1906/files

I also realised we'd noted the command here:

https://github.com/dart-lang/sdk/tree/master/pkg/analysis_server/tool/lsp_spec#running-the-server

If we don't plan to revert the default, we'll need to update that too.

Just to update people - the original CL was reverted, and we're working on an updated CL which will preserve the existing default for dart language-server.

Thanks Devon. https://dart-review.googlesource.com/c/sdk/+/246992

fix(dartls): explicitly pass the protocol to dartls (#1943)

@jwren

@jwren am I right in thinking that this change re-landed, while preserving the default now?

I believe this is done, but feel free to re-open if there's more to do.

Navigation Menu

Repository navigation

"dart language-server" command no longer defaults to LSP #49157

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

	test('protocol default', () async {
	return runWithLsp(['language-server']);
	});

	if (results.wasParsed(USE_LSP)) {
	analysisServerOptions.useLanguageServerProtocol =
	results[USE_LSP] as bool;
	} else {
	analysisServerOptions.useLanguageServerProtocol =
	results[SERVER_PROTOCOL] == PROTOCOL_LSP;
	}

	// This option is hidden but still accepted; it's effectively translated to
	// the 'protocol' option above.
	parser.addFlag(USE_LSP,
	defaultsTo: false,
	negatable: false,
	help: 'Whether to use the Language Server Protocol (LSP).',
	hide: true);

"dart language-server" command no longer defaults to LSP #49157

Description

Activity

DanTup commented on Jun 1, 2022

bwilkerson commented on Jun 1, 2022

jwren commented on Jun 1, 2022

DanTup commented on Jun 1, 2022

DanTup commented on Jun 1, 2022

devoncarew commented on Jun 1, 2022

bwilkerson commented on Jun 1, 2022

devoncarew commented on Jun 1, 2022

DanTup commented on Jun 2, 2022

DanTup commented on Jun 2, 2022

devoncarew commented on Jun 2, 2022

jwren commented on Jun 2, 2022

DanTup commented on Jun 8, 2022

DanTup commented on Jun 28, 2022

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions