search

dart-lang / sdk

The Dart SDK, including the VM, dart2js, core libraries, and more.
https://dart.dev
BSD 3-Clause "New" or "Revised" License
10k stars 1.54k forks source link

analyzer plugins do not reanalyze non-Dart files when they are modified #55944

Open ishchhabra opened 1 month ago

ishchhabra commented 1 month ago

This tracker is for issues related to:

Description

When creating an analyzer plugin, the analysis server does not seem to respect the value specified in additionalAnalyzerFileExtensions in settings.json (in VSCode). It looks like the language server has a hardcodeed list of types that it listens changes for (https://github.com/dart-lang/sdk/blob/main/pkg/analysis_server/lib/src/lsp/handlers/handler_text_document_changes.dart#L172-L178), which does not include the additional file extensions specified through the configuration.

The error was identified while debugging https://github.com/invertase/dart_custom_lint/issues/252#issue-2332007043, in case the additional context (or the POC at https://github.com/invertase/dart_custom_lint/issues/252#issuecomment-2148748259) might be helpful.

bwilkerson commented 1 month ago

@srawlins @DanTup

DanTup commented 1 month ago

The dart.additionalAnalyzerFileExtensions setting in the VS Code extension was for the legacy analysis server integration because we needed to know which extensions to interact with the server for.

In LSP, the server can declare these file types itself dynamically, so the setting is unnecessary.

In the code linked above, it uses the getter fullySupportedTypes, which types that plugins declare they are interested in:

https://github.com/dart-lang/sdk/blob/adabc902cadedfd4cf2067cac704d2747ce476a9/pkg/analysis_server/lib/src/lsp/registration/feature_registration.dart#L72-L76

An example of how a plugin declares this interest is here:

https://github.com/dart-lang/sdk/blob/main/pkg/analyzer_plugin/doc/tutorial/getting_started.md#:~:text=String%3E%20get-,fileGlobsToAnalyze,-%3D%3E%20%3CString

It's not clear to me if the plugin here knows what types the lint is trying to use, but I do think it would be better to handle this somehow via the plugin rather than use this setting because that setting requires specific client configuration and this seems like something the server/plugin should be able to just make work automatically.

ishchhabra commented 1 month ago

I wasn't aware that dart.additionalAnalyzerFileExtensions setting is only for legacy analysis server integration. Perhaps a documentation update here and in VSCode at some point would make sense?

For the bug, I looked into it more. It looks like there was an update needed in custom_lint and it wasn't a bug in the sdk. However, even after the feature registration is done with the extension included, I still did not see the files getting reanalyzed. I'm not sure if this is correct, but I believe at https://github.com/dart-lang/sdk/blob/d91f05c1c7d2d5f8ffe388a8a087a06e009977a0/pkg/analyzer/lib/src/dart/analysis/driver.dart#L611-L618 since the file is not a dart file, it does not get added to the list of _pendingFileChanges, resulting in affected at https://github.com/dart-lang/sdk/blob/d91f05c1c7d2d5f8ffe388a8a087a06e009977a0/pkg/analyzer_plugin/lib/plugin/plugin.dart#L140-L145 to not include the file without the dart extension.

DanTup commented 1 month ago

Perhaps a documentation update here and in VSCode at some point would make sense?

Yes, this should be made clearer - I've opened https://github.com/Dart-Code/Dart-Code/issues/5133 to update the description against the setting (which shows both in VS Code and on the site).

However, even after the feature registration is done with the extension included, I still did not see the files getting reanalyzed. I'm not sure if this is correct, but

I'm not too familiar with this code - @scheglov or @srawlins may be best placed to comment. I think the question is whether ServerPlugin.handleAffectedFiles should be called with/passed non-Dart files that were passed to contentChanged. Currently it gets just the result from analysisContext.applyPendingFileChanges() which seems to only include Dart files.

(I've updated the title to something I think is more accurate for the current issue)

bwilkerson commented 1 month ago

IIRC, this support was added to support the Angular Dart plugin. Those products were discontinued a long time ago and a lot of code has changed since then. I suspect that support for this feature has bit-rotted in the interval.

We'd probably need a fairly compelling case to justify the effort required to make it work again. Can you tell us why you want/need support for this?

ishchhabra commented 1 month ago

IIRC, this support was added to support the Angular Dart plugin. Those products were discontinued a long time ago and a lot of code has changed since then. I suspect that support for this feature has bit-rotted in the interval.

We'd probably need a fairly compelling case to justify the effort required to make it work again. Can you tell us why you want/need support for this?

I was trying to build a linter that analyzes non dart files. I didn't want to create that linter using a non-dart tool since ARB files are used for localization by flutter apps, and so it made sense to use a tool already available there - dart analyzer. At the time of creating that, it was mostly just a prototype that I built for learning purposes. While doing that, I encountered an issue, so I thought to create an issue and debug.

bwilkerson commented 1 month ago

While doing that, I encountered an issue, so I thought to create an issue and debug.

And we appreciate that, thanks!

I didn't want to create that linter using a non-dart tool since ARB files are used for localization by flutter apps ...

Linting ARB files might very well be a compelling enough case. Thanks!

@mossmana @anderdobo for visibility