[analyzer_plugin]: setContextRoots event never received

[rrousselGit]

Hello!

TL;DR, in certain situation, the setContextRoots event is never received by plugins.
I'm not sure what exactly triggers it, so I made a reproduction repository. It is composed of:

• the analyzer_plugin.
  It is the bare minimum for a plugin, with no logic in it besides extending ServerPlugin.
• many packages (some using analyzer_plugin, some not).
  They are composed if nothing but pubspec.yamls and maybe a pubspec_overrides.yaml
• The packages that uses the analyzer_plugin do so through a path dependency

The reproduction repository is here: https://github.com/rrousselGit/analyzer_plugin_bug_repro

Steps to reproduce:

• Clone this repository
• Do a global search of path: /Users/remirousselet/dev/rrousselGit/fake_riverpod/packages/custom_lint and replace it with an absolute path that points to packages/custom_lint
• Run dart pub get everywhere (the IDE should suggest it)
• Enable analyzer logs
• Restart the analyzer server

In the logs, you will see some requests made to the analyzer plugin. Such as:

1723714208683:PluginReq:{"id"::"14","method"::"plugin.versionCheck","params"::{"byteStorePath"::"/Users/remirousselet/.dartServer/.analysis-driver","sdkPath"::"/Users/remirousselet/dev/flutter/bin/cache/dart-sdk","version"::"1.0.0-alpha.0"}}:file::///Users/remirousselet/.dartServer/.plugin_manager/12c93ced09bb72bc715613b8b9aeae4c/analyzer_plugin/bin/plugin.dart::
1723714208690:PluginRes:{"id"::"14","requestTime"::1723714208686,"result"::{"isCompatible"::true,"name"::"Custom Lint","version"::"1.0.0-alpha.0","interestingFiles"::["*.dart"]}}:file::///Users/remirousselet/.dartServer/.plugin_manager/12c93ced09bb72bc715613b8b9aeae4c/analyzer_plugin/bin/plugin.dart::

But there's never a analysis.setContextRoots request sent by default.

Note:
If you restart the analyzer server while the plugin is running, and override
ServerPlugin.handleAnalysisSetContextRoots to log somewhere in a while,
you will see that the method is invoked right before shutdown is invoked.

It is hard to see in the analyzer logs, because the request appears in the log right
before the log file is cleared.

---

Dart SDK version: 3.5.0 (stable) (Tue Jul 30 02:17:59 2024 -0700) on "macos_arm64"

[dart-github-bot]

Summary: The setContextRoots event is not being received by analyzer plugins when using path dependencies in a specific project setup. This results in the plugin not being notified about changes to the project's context, potentially leading to incorrect analysis results.

[rrousselGit]

Note that this is a regression, and it completely breaks analyzer_plugins in this scenario.
No setContextRoots = no analysis at all.

So this is fairly important IMO

[DanTup]

I can repro - the plugin does get requests when I open Dart files, but it never seems to get context roots (I don't even see this during shutdown).

Will debug and post back.

[DanTup]

Doesn't seem to happen when I use latest Flutter master:

But does happen on 3.5. Not sure if it's a race (and coincidence), or if the issue is fixed since the release. I'll try to narrow it down to confirm.

[DanTup]

Debugging this in 3.5, I think there is a race condition. Haven't compared to master yet, but documenting it here while I have it in my head.

There's code here for addPluginToContextRoot which fires for each context root for the plugin:

https://github.com/dart-lang/sdk/blob/3.5.0/pkg/analysis_server/lib/src/plugin/plugin_manager.dart#L320

It starts the plugin if it's not already started, and then it calls plugin.addContextRoot(contextRoot). For a single plugin and single context root, this code works fine - the plugin is started, the SendPort is received (during the await for plugin.start) and then the root is added.

However, in the bad case, it appears we run this code twice for the same plugin/root at approximately the same time:

In this case, the first call starts setting up the plugin and waits for it (so it can then add the root), but the second request comes in and skips most of the method (because plugin != null at this point) and then adds the root itself. Because the plugin isn't initialized yet, it triggers setContextRoots which gets dropped because there is no SendPort:

https://github.com/dart-lang/sdk/blob/3.5.0/pkg/analyzer_plugin/lib/src/channel/isolate_channel.dart#L246

Then when the first call finishes starting the plugin and tries to add the root, it finds the root has already been added (even though the plugin wasn't notified because it wasn't fully started at the time) so it skips adding the root (and therefore notifying the plugin).

The reason we're calling this multiple times is because driver.enabledPluginNames has dupes in this loop:

https://github.com/dart-lang/sdk/blob/3.5.0/pkg/analysis_server/lib/src/plugin/plugin_watcher.dart#L44-L45

1724078222205:Info:adding plugins custom_lint, custom_lint, custom_lint, custom_lint, custom_lint, custom_lint to C::\Dev\Temp Projects\analyzer_plugin_bug_repro

(Edit: The below was incorrect, applying that cherry-pick did not fix the issue)

I haven't verified yet, but I'm wondering if this could be related to #56047 because the child analysis options were being recording against the root context instead of their own. If so, then the cherry-pick for that fix should resolve this in the next 3.5 hotfix. I'll patch this in and verify shortly.

But, I also wonder whether this code here should use a Set because I feel like you could still trigger this by having the plugin names in multiple analysis_options files within a single context even with the fix? @srawlins @bwilkerson thoughts?

https://github.com/dart-lang/sdk/blob/3.5.0/pkg/analyzer/lib/src/dart/analysis/driver.dart#L349-L353

[bwilkerson]

I suspect that nobody considered the impact of the multiple-analysis-options-per-context work on plugins. I know that I didn't.

I suspect that even if setContextRoots were being sent like it should be that we'd be passing the root of the analysis context(s). What we really need to be passing in are the directories containing the analysis options file in which the plugin is enabled, otherwise the plugin might start reporting against code for which it wasn't enabled.

[DanTup]

The cherry-pick above didn't solve the issue - I think it was coincidence I initially didn't see it on master, it certainly reproduces when I'm running from source with my additional debug logging.

Changing the List to a set reduces the issue, but does not solve it - because the same race still appears to occur for a single plugin and multiple context roots, because PluginWatcher.addedDriver does not await manager.addPluginToContextRoot, and so when multiple drivers are created at the same time, they can all still enter plugin.addContextRoot(contextRoot) while the plugin is still starting.

We can perhaps fix this by having plugin.start() wait on the plugin to be initialized if it's already in progress (instead of just skipping to adding the root), however I can't help but think something else isn't quite right here. For example in the log I see:

1724079379402:Info:adding plugins custom_lint to C::\Dev\Temp Projects\analyzer_plugin_bug_repro

However as far as I can tell, custom_lint is not enabled in the root of this repository, so I wonder if this is a difference that's caused this to show up. Specifically, I'm wondering whether looking at the whole of analysisOptionsMap here is valid (or if it's the entire workspace). I need to better understand the changes made relating to combining the contexts to properly understand what this should do.

https://github.com/dart-lang/sdk/blob/3.5.0/pkg/analyzer/lib/src/dart/analysis/driver.dart#L349-L353

[DanTup]

@bwilkerson

I suspect that nobody considered the impact of the multiple-analysis-options-per-context work on plugins. I know that I didn't.

I suspect that even if setContextRoots were being sent like it should be that we'd be passing the root of the analysis context(s).

Yeah, I think you're right that this wouldn't work and probably we should only look at the analysis_options at the root of the package instead.

But that aside, there's still something wrong here based on above. This also looks suspicious to me - the context for a nested project is shown here as using the analysis options for the root, so if I fixed it to only look at the root, I feel like it'd still get the wrong results.

Edit: nm that, this project is one that doesn't have an analysis_options and doesn't have the plugin. So I think this issue is explained by looking at the entire tree of analysis_options instead of only the one for the context (eg. this project shouldn't have the plugin at all, but it's getting it from the flattened analysis options of the whole project).

[bwilkerson]

Yeah, I think you're right that this wouldn't work and probably we should only look at the analysis_options at the root of the package instead.

I don't think "package"s have anything to do with it because the algorithm that determines where the analysis contexts are doesn't take package boundaries into consideration.

So I think this issue is explained by looking at the entire tree of analysis_options instead of only the one for the context (eg. this project shouldn't have the plugin at all, but it's getting it from the flattened analysis options of the whole project).

That doesn't match my understanding of how things are suppose to work now, so if that's how they're actually working that does sound like a bug.

For one thing, there isn't suppose to be any concept of "the flattened analysis options of the whole project". Every library (technically file, but we never access the options for a part) knows the options that should be used when analyzing it.

The most consistent semantics would be for any library for which a plugin P is enabled it would be analyzed by P, and any library that hasn't enabled P wouldn't be analyzed by P. That means that we do need to look in every analysis options file to see whether P is enabled, and build the included and excluded lists appropriately.

[DanTup]

@bwilkerson

For one thing, there isn't suppose to be any concept of "the flattened analysis options of the whole project".

Right, so I think this code here is wrong?

https://github.com/dart-lang/sdk/blob/3.5.0/pkg/analyzer/lib/src/dart/analysis/driver.dart#L349-L353

It seems to read all enable plugins from every project, so for the root folder in your workspace, it tries to enable every plugin from every nested project (which is both incorrect, and includes dupes).

If I change that code like this:

Then it appears to solve the problem - we only look at the analysis options for the root of that context (although it seems odd to use the analysis options file path to get the appropriate file path). I don't know if this is a good fix or whether it has other mismatches though. The additional race I noted above when a plugin exists for multiple drivers still occurs, but turns out to not actually be a problem because when the first context finishes initializing the plugin, it re-sends all roots (not only the one it was adding), so this "race" has probably existed forever and isn't actually impacting anything.

However, I'm less sure that this "fix" above works for Pub workspaces, because if I understand correctly, that's when we may have a single context/driver that spans multiple projects and/or analysis_options, in which case I think the nested projects would not be able to have their own plugins enabled - although I'm also unsure if that is a problem or an expectation (I think it's not clear to me what the definition of "context root" in the plugin API really means in a Pub workspace).

[DanTup]

Also:

I don't think "package"s have anything to do with it because the algorithm that determines where the analysis contexts are doesn't take package boundaries into consideration.

I'm a little confused by this. I thought contexts were where pubspec.yaml or analysis_options.yaml are (so not a perfect match, but close) - however looking at the code it seems like _createContextRoots looks for package_config.jsons (or BUILD.gn). However, if I open the SDK pkg folder, I do see individual contexts for each package like analyzer even though it doesn't have a package_config.json.

I think I know less about this than I assumed. In particular it's not clear to me:

• what are context roots in the server (if they're not package boundaries and they're based on package_config, why do I get them for each pkg/* in the SDK?)
• does the definition of context roots sent to plugins match the servers definition
• where is it valid to enable a server plugin (only at a context root, or at any that may be inside one but is not a context root)?

(if the answers to these already exist somewhere I should have read, I'm happy to be pointed there instead of just given the answers!)

[rrousselGit]

^ These are questions I've asked myself too.

I think ContextRoot would benefit from dartdoc that explains what those are. The current doc is fairly unhelpful about it:

/// Information about the root directory associated with an analysis context.
///
/// Clients may not extend, implement or mix-in this class.

[bwilkerson]

It seems to read all enable plugins from every project, so for the root folder in your workspace, it tries to enable every plugin from every nested project (which is both incorrect, and includes dupes).

(I'm not sure what you mean by "project", but I'm assuming you mean "the directory that was opened in the IDE".)

Yes, that's wrong.

analysisContext?.contextRoot.optionsFile

As far as I'm aware, ContextRoot.optionsFile shouldn't exist; it's meaningless. @pq In case I'm missing something.

I don't know if this is a good fix or whether it has other mismatches though.

It doesn't match the semantics I think we want. 🙂

However, I'm less sure that this "fix" above works for Pub workspaces ...

Right. The semantics I described earlier we intended for that situation (though I think they also nicely handle the non-workspace case as well).

Although, in all fairness, the semantics I gave were ignoring the fact that we will only load a single plugin.

Given that restriction, I think the right algorithm is to iterate over all of the analysis options files (as it's doing now), and look for enabled plugins. When it finds the first enabled plugin it needs to remember the path of the plugin and the set of files in the directory containing the analysis options file that are to be analyzed. After that it can ignore analysis options files that don't enable the one chosen plugin, and add the included and excluded files if the options file does enable the plugin.

I'm a little confused by this. I thought contexts were where pubspec.yaml or analysis_options.yaml are (so not a perfect match, but close) - however looking at the code it seems like _createContextRoots looks for package_config.jsons (or BUILD.gn).

The first part used to be true, but we should now only be creating a context when we find a package_config.json file.

However, if I open the SDK pkg folder, I do see individual contexts for each package like analyzer even though it doesn't have a package_config.json.

That isn't what I would have expected. @keertip

what are context roots in the server

They are the information about where to create an analysis context that is gathered before we create any actual contexts. There's a 1-to-1 correspondence between a context root and an analysis context.

does the definition of context roots sent to plugins match the servers definition

Good question. It used to, but I don't know whether that was preserved in the workspace support because I don't think anyone considered the impact on plugins. It probably shouldn't match anymore because of the way I'd like to see plugin enablement work (see the next paragraph), but I'm open to other designs than the one I proposed.

where is it valid to enable a server plugin (only at a context root, or at any that may be inside one but is not a context root)?

I think that the UX would be best if users could enable plugins for any subset of an analysis context. I'd do that by allowing them to enable plugins in any analysis options file and use the location of the options file to build the included and excluded lists sent to the plugin. That seems consistent with the way the rest of the analysis options work.

That definition would also minimize the number of changes that users have to make to their options files in order to use a workspace.

[DanTup]

(I'm not sure what you mean by "project", but I'm assuming you mean "the directory that was opened in the IDE".)

Sorry, I meant "package" (eg. a think with a pubspec that has some name). In C#, these were called "Projects" (and a workspace of multiple projects was a "Solution") and I often use package and project interchangeably (but try to use package!).

However, if I open the SDK pkg folder, I do see individual contexts for each package like analyzer even though it doesn't have a package_config.json.

That isn't what I would have expected. @keertip

Ok, let me debug why that's happening and I can open a separate issue if it seems like something is wrong there.

It probably shouldn't match anymore because of the way I'd like to see plugin enablement work (see the next paragraph), but I'm open to other designs than the one I proposed.
...
I think that the UX would be best if users could enable plugins for any subset of an analysis context. I'd do that by allowing them to enable plugins in any analysis options file and use the location of the options file to build the included and excluded lists sent to the plugin.

Currently it appears that we can't pass an include list to a plugin. When we call setContextRoots, we pass a single root folder and a set of exclude, but no include. So is your suggestion that we pass the folder that contained the analysis_options that enabled the plugin as the "context root" folder (so that context roots for plugins are actually "the roots for where the plugin is enabled" rather than what the server now considers roots, and the package_config.jsons could be further up the tree)?

I think this means we'll need to track "plugin roots" specifically so that if a plugin is added to an analysis_options file, the plugin is notified of that new root even though it didn't change any server context roots?

I'm not up-to-speed with the plugin changes that were in progress - is this something that makes sense to try and handle now, or when that work is further along (@srawlins?).

And since this is a much larger change than could be cherry-picked, do we need to do anything about this issue for 3.5? (it's not currently clear to me if there is an easy fix that restores the previous behaviour and I'm less sure now that the change I made above matches the previous behaviour (or whether doing something like that is sound anyway).

[DanTup]

However, if I open the SDK pkg folder, I do see individual contexts for each package like analyzer even though it doesn't have a package_config.json.

That isn't what I would have expected. @keertip

Sorry, this was user error. I see many contexts when I open my "analysis server workspace" because it's a multi-root workspace. So when I tested, I closed that and opened the pkg folder directly, but since that doesn't have my VS Code setting to point at a bleeding-edge SDK, it was loading with a stable version 3.4 (because I hadn't updated my stable version since the release).

I just verified with bleeding-edge opening the pkg folder and I only had 4 contexts (one for pkg, and others because of nested projects I'd triggered pub get in).

[bwilkerson]

So is your suggestion that we pass ...

No, my suggestion was based on a faulty memory of what information we could pass to the plugin.

I think this means we'll need to track "plugin roots" specifically so that if a plugin is added to an analysis_options file, the plugin is notified of that new root even though it didn't change any server context roots?

That is probably the right long term solution. I just don't know whether it makes sense to try to do that in the short term.

And since this is a much larger change than could be cherry-picked, do we need to do anything about this issue for 3.5?

I'm not ready to answer that question because I don't understand either the problem or the solution well enough at this point. We're also taking a look at it.

[DanTup]

The fix for this issue was included in Dart 3.5.2 (released last week) and Flutter 3.24.2 (release today).