[ Tool ] Migrate flutter analyze to use LSP#183785
Conversation
The `analyze` command was still launching the `AnalysisServer` using the "legacy" protocol instead of LSP.
There was a problem hiding this comment.
Code Review
This pull request migrates the flutter analyze command to use the Language Server Protocol (LSP) for communicating with the Dart analysis server, replacing the previous legacy protocol. The changes include updating the server startup process, implementing LSP message framing with Content-Length headers, and handling LSP notifications for analysis progress and diagnostics. The tests have also been updated to reflect this new communication protocol. While the overall migration appears to be well-implemented, I've identified a critical issue in the request ID generation that could prevent proper request-response matching.
bwilkerson
left a comment
There was a problem hiding this comment.
The AI review seems reasonable from what I can see, but I'm not sure how the tests would pass if it were a real issue.
DanTup
left a comment
There was a problem hiding this comment.
Added a bunch of comments, many are minor and some are just questions/thinking out loud, I don't think anything is pressing.
There was a problem hiding this comment.
nit: If this were final process = _process = await _processManager() you could avoid a bunch of !s further down (!s tend to make me nervous!).
| void connectToDtd({required Uri dtdUri}) { | ||
| _sendCommand('dart/connectToDtd', {'uri': dtdUri.toString()}); | ||
| } |
There was a problem hiding this comment.
Is this used? (If so, I'm curious what for, since I assume this process will be short-lived, it hardly seems worth connecting it up to DTD).
There was a problem hiding this comment.
Good catch, this isn't used in this PR. However, it will be once I migrate widget previews to use the DAS. I'll remove it from this PR.
| break; | ||
| } | ||
|
|
||
| final String headers = utf8.decode(_byteBuffer.sublist(0, byteHeaderEnd)); |
There was a problem hiding this comment.
It would be nice to not have to duplicate this, but looking at the analysis server implementation, it might be spread across two classes for the two different directions (LspPacketTransformer and LspByteStreamServerChannel implements LspServerCommunicationChannel) which might be all that reusable.
I wonder if we should tidy it up a bit in server, and then switch this code to it after?
There was a problem hiding this comment.
That would be ideal!
There was a problem hiding this comment.
It would also be nice to extract a lot of this code into a package that implements support for clients of the analyzer using LSP. There's no point in duplicating all of this support when we go to move the dart command-line tool to use LSP.
There was a problem hiding this comment.
Yeah, we should probably support this in a couple of forms:
- Just using the classes that handle the over-the-wire protocol (packet transformer etc.)
- A basic client that lets you call arbitrary requests/send notifications
- A strongly-typed class that provides wrappers for each of the LSP-spec'd features
We have a lot of this in the server already (the tests are affectively LSP clients), but mostly built for the convenience of tests rather than reuse. It would be good to make a new package (that doesn't depend on server) and extract it.
We also have some of this in https://github.com/dart-lang/sdk/tree/main/third_party/pkg/language_server_protocol/lib but I don't completely understand why the generated code had to go into third_party and what that would mean for trying to publish such a package?
There was a problem hiding this comment.
... why the generated code had to go into
third_party...
I'm happy to talk about this off-line.
| void _handleProgress(Map<String, Object?> params) { | ||
| // LSP progress for analysis is typically reported via tokens. | ||
| // The server sends begin/report/end for a token. | ||
| final Object? value = params['value']; | ||
| if (value is Map<String, Object?>) { | ||
| final kind = value['kind'] as String?; | ||
| if (kind == 'begin') { | ||
| _analyzingController.add(true); | ||
| } else if (kind == 'end') { | ||
| _analyzingController.add(false); | ||
| } |
There was a problem hiding this comment.
I feel slightly uneasy about using the progress notification for detection of analysis finished (because if we add debouncing or something, it might be fragile).
We shouldn't hold up this change for it, but I wonder if we should add a new explicit custom notification for "initial analysis complete" or something that is documented as being a reliable signal (and not something that is just to indicate work to the user), then move this over? (@bwilkerson thoughts? it'd only be a few lines in the LSP server).
There was a problem hiding this comment.
I'd love to, but I'm not sure we can. If we're using the server in a command-line environment, then we'll know when the analysis driver is idle and can use that as a signal. But in an interactive environment we might not transition to idle between finishing the initial analysis and doing user-initiated work.
Even from the command-line, I'm not sure how reliable that would be in the --watch case.
But if it's doable, I'm for it.
There was a problem hiding this comment.
Oh, I didn't realise we had a --watch, that does complicate things. Although I'm not sure that prevents improving this - the progress reporting signal here directly drives the UI in VS Code, so we might want to add things like debouncing. So even if we just split this into two notifications (the LSP standard one for UI, and a copy of it for actual analysis detection) it might avoid accidentally breaking something like this.
But in an interactive environment we might not transition to idle between finishing the initial analysis
What do you mean by "interactive environment" here? Because if you mean an IDE started the server, then this could be problematic, because we might not even send these notifications depending on the client capabilities.
There was a problem hiding this comment.
Sorry, I could have said that better. I mean any environment in which the server is incrementally responding to changes in the code being analyzed. Right now I think that means either the IDE or an invocation of flutter analyze --watch.
The issue is that the only signal we get is whether the analysis driver has work to do. On start-up we give the driver some set of directories and it figures out all of the files that need to be analyzed, adding them to the work queue. If a file on disk changes (whether by an agent or something like git pull), or if the client updates the content of a file because the user is typing, then that file gets added to the work queue. In that case, any work added before the initial analysis is complete will delay the notification that the queue is empty.
I'm just saying that there isn't a reliable way to know when the initial analysis is actually competed, only to know when the driver is idle. We're using the 'driver is idle' signal as a proxy. It isn't perfect, but I'm not sure we can do better.
That isn't a problem in the IDE because the results are being updated and displayed as analysis occurs, and the 'analysis is happening' notification is accurate.
I don't know whether that's a problem here. It's unlikely that a user would change code on disk while an analysis is being run, so it isn't a huge issue either way. Without the more accurate signal it means that flutter analyze will report the state at an unpredictable point in time, but maybe users that change the disk content during an analysis deserve whatever they get.
(I'm assuming that because of the --watch flag there's already code here that will replace an older report of diagnostics for a given file with a newer report for the same file. If that's not the case then there's an existing bug.)
| // LSP is 0-indexed, legacy is 1-indexed. | ||
| startLine: (start['line']! as int) + 1, | ||
| startColumn: (start['character']! as int) + 1, | ||
| offset: -1, // LSP doesn't provide offset |
There was a problem hiding this comment.
Is this class (WrittenError) only used for this purpose? If so, would it be better to delete things like offset and type so it better reflects the new data? I presume once this is switched over, we don't need to retain any kind of compatibility with the old way?
|
autosubmit label was removed for flutter/flutter/183785, because The base commit of the PR is older than 7 days and can not be merged. Please merge the latest changes from the main into this branch and resubmit the PR. |
Roll Flutter from fb03253e32ce to 3d69471c0bf9 (69 revisions) flutter/flutter@fb03253...3d69471 2026-04-01 engine-flutter-autoroll@skia.org Roll Dart SDK from 1308a3076402 to 043a2bfd56ff (1 revision) (flutter/flutter#184453) 2026-04-01 engine-flutter-autoroll@skia.org Roll Skia from a657b5446209 to c2363c39c283 (2 revisions) (flutter/flutter#184448) 2026-04-01 104349824+huycozy@users.noreply.github.com Fix layout overflowed in small screen in SensitiveContent's example (flutter/flutter#184179) 2026-04-01 engine-flutter-autoroll@skia.org Roll Skia from e0b25041a5d5 to a657b5446209 (1 revision) (flutter/flutter#184445) 2026-04-01 engine-flutter-autoroll@skia.org Roll Dart SDK from 75a089eb6bf9 to 1308a3076402 (3 revisions) (flutter/flutter#184444) 2026-04-01 jesswon@google.com [AGP 9] Bumping KGP error minimum to 2.0.0 (flutter/flutter#184385) 2026-04-01 bkonyi@google.com [ Tool ] Migrate `flutter analyze` to use LSP (flutter/flutter#183785) 2026-04-01 30870216+gaaclarke@users.noreply.github.com Adds uber sdf shader gradients with blend (flutter/flutter#184090) 2026-04-01 sys.int64@gmail.com Add bottom safe area padding to licenses package license page (flutter/flutter#182425) 2026-04-01 ahmedsameha1@gmail.com Handle#6537 third grouped tests (flutter/flutter#183059) 2026-04-01 engine-flutter-autoroll@skia.org Roll Skia from f37239a7a689 to e0b25041a5d5 (9 revisions) (flutter/flutter#184436) 2026-03-31 jason-simmons@users.noreply.github.com [Impeller] Do not log an error when wrapping an empty texture as a TextureGLES (flutter/flutter#184377) 2026-03-31 jason-simmons@users.noreply.github.com Remove the default_git_folder GN argument (flutter/flutter#184152) 2026-03-31 jason-simmons@users.noreply.github.com Remove the cupertino_icons dependency from the spell_check integration test (flutter/flutter#184398) 2026-03-31 engine-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from pM94cWC9cSgao0CG0... to fV-JIWUt4FQGeDtEe... (flutter/flutter#184383) 2026-03-31 engine-flutter-autoroll@skia.org Roll Dart SDK from eaeccf98848d to 75a089eb6bf9 (1 revision) (flutter/flutter#184379) 2026-03-31 mdebbar@google.com [web] Fix autofill in iOS 26 Safari (flutter/flutter#182024) 2026-03-31 engine-flutter-autoroll@skia.org Roll Fuchsia GN SDK from SEfYx3xgueX3aFAY3... to JLBh4Z9PKsjIJcqDU... (flutter/flutter#184368) 2026-03-31 loic.peron@inetum.com [Windows] Restore and enable IAccessibleEx implementation (flutter/flutter#175406) 2026-03-31 30870216+gaaclarke@users.noreply.github.com Revert "Even more awaits (#184042)" (flutter/flutter#184429) 2026-03-31 engine-flutter-autoroll@skia.org Roll Skia from dfd8f8002800 to f37239a7a689 (2 revisions) (flutter/flutter#184374) 2026-03-31 jacksongardner@google.com Remove workaround for fake impeller images in iOS simulator. (flutter/flutter#184264) 2026-03-31 victorsanniay@gmail.com Even more awaits (flutter/flutter#184042) 2026-03-31 engine-flutter-autoroll@skia.org Roll Packages from 582f0e7 to b04f3e5 (6 revisions) (flutter/flutter#184393) 2026-03-30 30870216+gaaclarke@users.noreply.github.com Fixes a flake in reload shaders tests (flutter/flutter#184268) 2026-03-30 jason-simmons@users.noreply.github.com Remove an obsolete script for setting up remote GDB sessions on Android devices (flutter/flutter#184357) 2026-03-30 engine-flutter-autoroll@skia.org Roll Skia from 8dcde79fef2a to dfd8f8002800 (10 revisions) (flutter/flutter#184363) 2026-03-30 engine-flutter-autoroll@skia.org Roll Dart SDK from 0aaccc3c8004 to eaeccf98848d (2 revisions) (flutter/flutter#184362) 2026-03-30 bkonyi@google.com [ Tool ] Remove `flutter running-apps` command (flutter/flutter#183742) 2026-03-30 137456488+flutter-pub-roller-bot@users.noreply.github.com Roll pub packages (flutter/flutter#184045) 2026-03-30 jmccandless@google.com Rick roll triagers on/near April 1st (flutter/flutter#184355) 2026-03-30 49699333+dependabot[bot]@users.noreply.github.com Bump codecov/codecov-action from 5.5.0 to 6.0.0 in the all-github-actions group (flutter/flutter#184364) 2026-03-30 1961493+harryterkelsen@users.noreply.github.com fix(web): call ui.Picture.onDispose for the original picture only (flutter/flutter#184348) 2026-03-30 engine-flutter-autoroll@skia.org Roll Skia from e001e6901e3b to 8dcde79fef2a (7 revisions) (flutter/flutter#184356) 2026-03-30 jason-simmons@users.noreply.github.com [web] Make it safe to call dispose multiple times on a CkSurface (flutter/flutter#184270) 2026-03-30 jason-simmons@users.noreply.github.com Roll HarfBuzz to 13.2.1 (flutter/flutter#184210) 2026-03-30 srawlins@google.com web_ui: Remove unused parameters in a few places (flutter/flutter#183156) 2026-03-30 saurabhmirajkar000@gmail.com Update TabBar documentation to clarify indicatorWeight behavior (flutter/flutter#184104) 2026-03-30 36861262+QuncCccccc@users.noreply.github.com Add title evaluation (flutter/flutter#184084) 2026-03-30 47866232+chunhtai@users.noreply.github.com fixes crash when invisible semantics nodes dropped from semantics tree (flutter/flutter#184226) 2026-03-30 engine-flutter-autoroll@skia.org Roll Skia from cdaae3e3fdef to e001e6901e3b (4 revisions) (flutter/flutter#184345) 2026-03-30 engine-flutter-autoroll@skia.org Roll Packages from 7ae082a to 582f0e7 (8 revisions) (flutter/flutter#184341) 2026-03-30 matej.knopp@gmail.com Add alwaysSizeToContent argument to Overlay. (flutter/flutter#182009) 2026-03-30 engine-flutter-autoroll@skia.org Roll Dart SDK from 598088a8a67f to 0aaccc3c8004 (1 revision) (flutter/flutter#184331) 2026-03-30 engine-flutter-autoroll@skia.org Roll Skia from 6d7ade938643 to cdaae3e3fdef (2 revisions) (flutter/flutter#184329) 2026-03-30 engine-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from EnoD6zNQebz4EYuLk... to pM94cWC9cSgao0CG0... (flutter/flutter#184323) ...
The `analyze` command was still launching the `AnalysisServer` using the "legacy" protocol instead of LSP.
The `analyze` command was still launching the `AnalysisServer` using the "legacy" protocol instead of LSP.
…r#11408) Roll Flutter from fb03253e32ce to 3d69471c0bf9 (69 revisions) flutter/flutter@fb03253...3d69471 2026-04-01 engine-flutter-autoroll@skia.org Roll Dart SDK from 1308a3076402 to 043a2bfd56ff (1 revision) (flutter/flutter#184453) 2026-04-01 engine-flutter-autoroll@skia.org Roll Skia from a657b5446209 to c2363c39c283 (2 revisions) (flutter/flutter#184448) 2026-04-01 104349824+huycozy@users.noreply.github.com Fix layout overflowed in small screen in SensitiveContent's example (flutter/flutter#184179) 2026-04-01 engine-flutter-autoroll@skia.org Roll Skia from e0b25041a5d5 to a657b5446209 (1 revision) (flutter/flutter#184445) 2026-04-01 engine-flutter-autoroll@skia.org Roll Dart SDK from 75a089eb6bf9 to 1308a3076402 (3 revisions) (flutter/flutter#184444) 2026-04-01 jesswon@google.com [AGP 9] Bumping KGP error minimum to 2.0.0 (flutter/flutter#184385) 2026-04-01 bkonyi@google.com [ Tool ] Migrate `flutter analyze` to use LSP (flutter/flutter#183785) 2026-04-01 30870216+gaaclarke@users.noreply.github.com Adds uber sdf shader gradients with blend (flutter/flutter#184090) 2026-04-01 sys.int64@gmail.com Add bottom safe area padding to licenses package license page (flutter/flutter#182425) 2026-04-01 ahmedsameha1@gmail.com Handle#6537 third grouped tests (flutter/flutter#183059) 2026-04-01 engine-flutter-autoroll@skia.org Roll Skia from f37239a7a689 to e0b25041a5d5 (9 revisions) (flutter/flutter#184436) 2026-03-31 jason-simmons@users.noreply.github.com [Impeller] Do not log an error when wrapping an empty texture as a TextureGLES (flutter/flutter#184377) 2026-03-31 jason-simmons@users.noreply.github.com Remove the default_git_folder GN argument (flutter/flutter#184152) 2026-03-31 jason-simmons@users.noreply.github.com Remove the cupertino_icons dependency from the spell_check integration test (flutter/flutter#184398) 2026-03-31 engine-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from pM94cWC9cSgao0CG0... to fV-JIWUt4FQGeDtEe... (flutter/flutter#184383) 2026-03-31 engine-flutter-autoroll@skia.org Roll Dart SDK from eaeccf98848d to 75a089eb6bf9 (1 revision) (flutter/flutter#184379) 2026-03-31 mdebbar@google.com [web] Fix autofill in iOS 26 Safari (flutter/flutter#182024) 2026-03-31 engine-flutter-autoroll@skia.org Roll Fuchsia GN SDK from SEfYx3xgueX3aFAY3... to JLBh4Z9PKsjIJcqDU... (flutter/flutter#184368) 2026-03-31 loic.peron@inetum.com [Windows] Restore and enable IAccessibleEx implementation (flutter/flutter#175406) 2026-03-31 30870216+gaaclarke@users.noreply.github.com Revert "Even more awaits (#184042)" (flutter/flutter#184429) 2026-03-31 engine-flutter-autoroll@skia.org Roll Skia from dfd8f8002800 to f37239a7a689 (2 revisions) (flutter/flutter#184374) 2026-03-31 jacksongardner@google.com Remove workaround for fake impeller images in iOS simulator. (flutter/flutter#184264) 2026-03-31 victorsanniay@gmail.com Even more awaits (flutter/flutter#184042) 2026-03-31 engine-flutter-autoroll@skia.org Roll Packages from 582f0e7 to b04f3e5 (6 revisions) (flutter/flutter#184393) 2026-03-30 30870216+gaaclarke@users.noreply.github.com Fixes a flake in reload shaders tests (flutter/flutter#184268) 2026-03-30 jason-simmons@users.noreply.github.com Remove an obsolete script for setting up remote GDB sessions on Android devices (flutter/flutter#184357) 2026-03-30 engine-flutter-autoroll@skia.org Roll Skia from 8dcde79fef2a to dfd8f8002800 (10 revisions) (flutter/flutter#184363) 2026-03-30 engine-flutter-autoroll@skia.org Roll Dart SDK from 0aaccc3c8004 to eaeccf98848d (2 revisions) (flutter/flutter#184362) 2026-03-30 bkonyi@google.com [ Tool ] Remove `flutter running-apps` command (flutter/flutter#183742) 2026-03-30 137456488+flutter-pub-roller-bot@users.noreply.github.com Roll pub packages (flutter/flutter#184045) 2026-03-30 jmccandless@google.com Rick roll triagers on/near April 1st (flutter/flutter#184355) 2026-03-30 49699333+dependabot[bot]@users.noreply.github.com Bump codecov/codecov-action from 5.5.0 to 6.0.0 in the all-github-actions group (flutter/flutter#184364) 2026-03-30 1961493+harryterkelsen@users.noreply.github.com fix(web): call ui.Picture.onDispose for the original picture only (flutter/flutter#184348) 2026-03-30 engine-flutter-autoroll@skia.org Roll Skia from e001e6901e3b to 8dcde79fef2a (7 revisions) (flutter/flutter#184356) 2026-03-30 jason-simmons@users.noreply.github.com [web] Make it safe to call dispose multiple times on a CkSurface (flutter/flutter#184270) 2026-03-30 jason-simmons@users.noreply.github.com Roll HarfBuzz to 13.2.1 (flutter/flutter#184210) 2026-03-30 srawlins@google.com web_ui: Remove unused parameters in a few places (flutter/flutter#183156) 2026-03-30 saurabhmirajkar000@gmail.com Update TabBar documentation to clarify indicatorWeight behavior (flutter/flutter#184104) 2026-03-30 36861262+QuncCccccc@users.noreply.github.com Add title evaluation (flutter/flutter#184084) 2026-03-30 47866232+chunhtai@users.noreply.github.com fixes crash when invisible semantics nodes dropped from semantics tree (flutter/flutter#184226) 2026-03-30 engine-flutter-autoroll@skia.org Roll Skia from cdaae3e3fdef to e001e6901e3b (4 revisions) (flutter/flutter#184345) 2026-03-30 engine-flutter-autoroll@skia.org Roll Packages from 7ae082a to 582f0e7 (8 revisions) (flutter/flutter#184341) 2026-03-30 matej.knopp@gmail.com Add alwaysSizeToContent argument to Overlay. (flutter/flutter#182009) 2026-03-30 engine-flutter-autoroll@skia.org Roll Dart SDK from 598088a8a67f to 0aaccc3c8004 (1 revision) (flutter/flutter#184331) 2026-03-30 engine-flutter-autoroll@skia.org Roll Skia from 6d7ade938643 to cdaae3e3fdef (2 revisions) (flutter/flutter#184329) 2026-03-30 engine-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from EnoD6zNQebz4EYuLk... to pM94cWC9cSgao0CG0... (flutter/flutter#184323) ...
The
analyzecommand was still launching theAnalysisServerusing the "legacy" protocol instead of LSP.