dart-lang / sdk

The Dart SDK, including the VM, JS and Wasm compilers, analysis, core libraries, and more.
https://dart.dev
BSD 3-Clause "New" or "Revised" License
10.06k stars 1.56k forks source link

[CP] [beta] Fix analysis server exceptions communicating with analyzer_plugin when server has macro support enabled #56126

Closed DanTup closed 1 month ago

DanTup commented 2 months ago

Commit(s) to merge

916272331b64c5a104ddc8afdafae4be1231b8c4

Target

beta

Prepared changelist for beta/stable

https://dart-review.googlesource.com/c/sdk/+/374361

Issue Description

As part of changes to support macros, the protocol classes used for communication between the client IDE and server were updated to use URIs in place of file paths (so that we can represent resources that are macro-generated, such as dart-macro-file:///x.dart that is macro-generated content for file:///x.dart).

Some of the protocol classes are also used to communicate with analyzer plugins, which at this point are not handling macro sources and still expect to use file paths. This results in exceptions when using plugins because the server is trying to send (and expecting to receive) URIs instead of paths.

What is the fix

The fix is to stop using a global encoder for the path/URI translation from all protocol classes and instead pass the encoder to all toJson/fromJson methods (where it can be done based on whether the communication is with the client or a plugin).

Why cherry-pick

Analyzer plugins will stop working when the server is in macro (URI) mode without this fix. To simplify trying out the JSON macro, the VS Code extension has always enabled macro (URI) mode for Dart 3.5 pre-releases.

Risk

I'd say it's probably low-medium risk. The change does unfortunately touch a lot of code (because encoders have to be passed everywhere), however most of the change was code-generated, or updated by generating code and then fixing compile errors to ensure the encoder is passes everywhere it's necessary.

Issue link(s)

https://github.com/Dart-Code/Dart-Code/issues/5156

Extra Info

fyi @bwilkerson @jwren

dart-github-bot commented 2 months ago

Summary: The analysis server throws exceptions when communicating with analyzer plugins because it uses URIs for macro-generated files, while plugins expect file paths. The fix involves passing an encoder to toJson/fromJson methods to handle path/URI translation based on the communication target.

DanTup commented 2 months ago

A possible alternative to cherry-picking this would be to ship a new VS Code extension that puts enabling macro support back behind a flag for =3.5 and possibly automatically enabling it for >=3.6 (undoing this change).

However, that would require enabling this flag for anyone testing out macros (including JSON) again (cc @mit-mit).

vsmenon commented 2 months ago

Thanks, @DanTup ! @srawlins @bwilkerson @jwren - do agree on low risk / cherry picking?

srawlins commented 2 months ago

+1 from me. It's a big change but it's important, so the earlier the cherry-pick the better.

DanTup commented 2 months ago

The change was merged into beta in https://github.com/dart-lang/sdk/commit/0662a87c772dfbb95e51a6b7ba2f1c4b17d8d7ea

srawlins commented 1 month ago

I'm not 100% sure of the process here, but should this be closed, @itsjustkevin ?