Linter: depend_on_referenced_packages requires analysis server restart to update

[jakemac53]

When using the depend_on_referenced_packages linte, you have to restart the analysis server after editing your pubspec in order to get the lint to go away. This only appears to happen after the first update to the pubspec though (the very first modification is respected, usually).

[jakemac53]

Actually this looks like it might happen in normal workspaces as well, at least sometimes.

[scheglov]

The way it supposed to work is that when a file that affects analysis environment (URI resolution, packages, etc - anything except just dart files) changes, we nuke the analysis context collection. This includes pubspec.yaml. In the process of rebuilding analysis contexts we read pubspec.yaml and rediscover workspaces and packages.

So, I would expect that this already works.

Might be related to https://github.com/dart-lang/sdk/issues/45996

[bwilkerson]

In an offline conversation we think that it might be related to some cached values in the Workspace / WorkspacePackage classes that isn't getting flushed when the pubspec.yaml file has changed. Jake is investigating.

[jakemac53]

Yes, after doing some digging I am very confused as to what is going on. As far as I can tell we should be nuking the entire analysis context collection. And that does from what I can tell include the workspace objects. Possibly it is not recognizing the pubspec.yaml file update properly?

[scheglov]

Well, it should be easy to check - log package:watcher notifications, and log when we see a pubspec.yaml file change, and when we nuke the analysis context collection.

As far as I can see, we don't have a static cache in PackageBuildWorkspace, so no data should survive recreating the workspace.

[jakemac53]

Note that I can also consistently reproduce this now in any package by doing the following:

• Add an import to a package you don't import
• Add the new dependency to your pubspec (this does remove the lint as expected)
• Remove the dependency from the pubspec

The lint will not trigger once you do this, when it should.

I think that possibly we stop watching the pubspec after the first update?

You can similarly do the following:

• Open a normal package with no existing lints
• Remove a dependency (should trigger lints)
• Add the dependency back

In this case the lint won't go away like it should

[scheglov]

OK, I think we do have a caching issue. With this lint the set of errors depends on the content of the pubspec.yaml file. But its content is not a part of the result key in the cache. For example we include the set of lints (from analysis_options.yaml) into the key. Similarly, we should include either the set of dependencies, or just the whole pubspec.yaml file content.

[scheglov]

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

[pq]

With @scheglov's fix, I think we can close this?

[scheglov]

Yes, I think so.
Worth testing with the latest analyzer, but as far as I know, this is fixed.

[pq]

/fyi @jakemac53

[jakemac53]

Unfortunately this still appears very flaky to me :(

[scheglov]

Works for me, I tested it in Dart SDK repo, by commenting out and restoring path and pub_semver dependencies in analyzer_cli package.
Make sure that:

1. You use the build of SDK that has the fix.
2. You save pubspec.yaml file in your IDE.

[jakemac53]

I have tried again today, and rebuilt the sdk as of this morning (including a gclient sync). But I still don't see it updating. I tried doing a similar flow (commenting out a dependency and then uncommenting it).

I am using VsCode and not intellij, not sure if that makes a difference?

[jakemac53]

Ok I have a bit more info - this appears to happen when I have multiple workspaces open and not just a single package. I am not sure how that might affect things...

Or more specifically it might only be happening when I also have a package:build workspace open?

[jakemac53]

Further clarification, this appears to be happening only when opening the root of a mono_repo - in other words a directory containing multiple packages in subdirectories.

The example I am testing with is the build repo (http://github.com/dart-lang/build). If I open the root of that repo then pubspec edits are not respected without a restart. If I open a subdirectory then things work as expected.

Note that in general pubspecs may be recursively nested as well (this is common in flutter example dirs for instance).

[jakemac53]

Another note here - as soon as you add a mono repo to your workspace, updates to the pubspecs in other, non-monorepo dirs also stop working.

[scheglov]

Hm... I can reproduce it, sometimes. My best theory for now is that it is a race condition between reading the file content following changes. The fact that we may read the file more than once means that we might compute the salt with one content, and parse it with another. I will try to fix this issue.

[scheglov]

https://dart-review.googlesource.com/c/sdk/+/202726 should make results consistent.

But I think that we still have an issue here - I still can see that we don't react to some changes to pubspec.yaml files, note that there are no Watch event in the log at the end (and I did save). We might ignore analysis.updateContent events for pubspec.yaml, and/or might not start watching soon enough so that miss events that happen while we scanning file system for analysis contexts (which we kicked off because of some previous pubspec.yaml change).

1623088794851:Noti:{"event"::"analysis.errors","params"::{"file"::"/Users/scheglov/dart/build/build/pubspec.yaml","errors"::[]}}
1623088795066:Req:{"id"::"6","method"::"analysis.setPriorityFiles","params"::{"files"::["/Users/scheglov/dart/build/build/pubspec.yaml"]},"clientRequestTime"::1623088782490}
1623088795097:Req:{"id"::"7","method"::"analysis.setSubscriptions","params"::{"subscriptions"::{"OVERRIDES"::["/Users/scheglov/dart/build/build/pubspec.yaml"],"OUTLINE"::["/Users/scheglov/dart/build/build/pubspec.yaml"],"HIGHLIGHTS"::["/Users/scheglov/dart/build/build/pubspec.yaml"],"NAVIGATION"::["/Users/scheglov/dart/build/build/pubspec.yaml"],"IMPLEMENTED"::["/Users/scheglov/dart/build/build/pubspec.yaml"]}},"clientRequestTime"::1623088782490}
1623088809112:Req:{"id"::"8","method"::"analysis.updateContent","params"::{"files"::{"/Users/scheglov/dart/build/build/pubspec.yaml"::{"type"::"add","content"::"name:: build\nversion:: 2.0.2\ndescription:: A build system for Dart.\nrepository:: https:://github.com/dart-lang/build/tree/master/build\n\nenvironment::\n  sdk:: \">=2.12.0 <3.0.0\"\n\ndependencies::\n  analyzer:: ^1.0.0\n  async:: ^2.5.0\n#  convert:: ^3.0.0\n  crypto:: ^3.0.0\n  glob:: ^2.0.0\n  logging:: ^1.0.0\n  meta:: ^1.3.0\n  path:: ^1.8.0\n\ndev_dependencies::\n  build_resolvers:: ^2.0.0\n  build_test:: ^2.0.0\n  pedantic:: ^1.0.0\n  test:: ^1.16.0\n"}}},"clientRequestTime"::1623088809111}
1623088809428:Watch:<unknown>:/Users/scheglov/dart/build/build/pubspec.yaml:modify
1623088809444:Noti:{"event"::"analysis.flushResults","params"::{"files"::["/Users/scheglov/dart/build/analysis_options.yaml",
1623088810030:Noti:{"event"::"analysis.errors","params"::{"file"::"/Users/scheglov/dart/build/build/pubspec.yaml","errors"::[]}}
1623088810193:Req:{"id"::"9","method"::"analysis.updateContent","params"::{"files"::{"/Users/scheglov/dart/build/build/pubspec.yaml"::{"type"::"remove"}}},"clientRequestTime"::1623088809447}
1623088873603:Watch:<unknown>:/Users/scheglov/dart/build/build/pubspec.yaml:modify
1623088873612:Noti:{"event"::"analysis.flushResults","params"::{"files"::["/Users/scheglov/dart/build/analysis_options.yaml",
1623088874187:Noti:{"event"::"analysis.errors","params"::{"file"::"/Users/scheglov/dart/build/build/pubspec.yaml","errors"::[]}}
1623088874435:Req:{"id"::"10","method"::"analysis.updateContent","params"::{"files"::{"/Users/scheglov/dart/build/build/pubspec.yaml"::{"type"::"add","content"::"name:: build\nversion:: 2.0.2\ndescription:: A build system for Dart.\nrepository:: https:://github.com/dart-lang/build/tree/master/build\n\nenvironment::\n  sdk:: \">=2.12.0 <3.0.0\"\n\ndependencies::\n  analyzer:: ^1.0.0\n  async:: ^2.5.0\n  convert:: ^3.0.0\n#  crypto:: ^3.0.0\n  glob:: ^2.0.0\n  logging:: ^1.0.0\n  meta:: ^1.3.0\n  path:: ^1.8.0\n\ndev_dependencies::\n  build_resolvers:: ^2.0.0\n  build_test:: ^2.0.0\n  pedantic:: ^1.0.0\n  test:: ^1.16.0\n"}}},"clientRequestTime"::1623088873988}
1623088874437:Req:{"id"::"11","method"::"analysis.updateContent","params"::{"files"::{"/Users/scheglov/dart/build/build/pubspec.yaml"::{"type"::"remove"}}},"clientRequestTime"::1623088874228}
1623088876269:Req:{"id"::"12","method"::"analysis.updateContent","params"::{"files"::{"/Users/scheglov/dart/build/build/pubspec.yaml"::{"type"::"add","content"::"name:: build\nversion:: 2.0.2\ndescription:: A build system for Dart.\nrepository:: https:://github.com/dart-lang/build/tree/master/build\n\nenvironment::\n  sdk:: \">=2.12.0 <3.0.0\"\n\ndependencies::\n  analyzer:: ^1.0.0\n  async:: ^2.5.0\n#  convert:: ^3.0.0\n#  crypto:: ^3.0.0\n  glob:: ^2.0.0\n  logging:: ^1.0.0\n  meta:: ^1.3.0\n  path:: ^1.8.0\n\ndev_dependencies::\n  build_resolvers:: ^2.0.0\n  build_test:: ^2.0.0\n  pedantic:: ^1.0.0\n  test:: ^1.16.0\n"}}},"clientRequestTime"::1623088876269}
1623088876404:Watch:<unknown>:/Users/scheglov/dart/build/build/pubspec.yaml:modify
1623088876409:Noti:{"event"::"analysis.flushResults","params"::{"files"::["/Users/scheglov/dart/build/analysis_options.yaml",
1623088876775:Noti:{"event"::"analysis.errors","params"::{"file"::"/Users/scheglov/dart/build/build/pubspec.yaml","errors"::[]}}
1623088880258:Watch:<unknown>:/Users/scheglov/dart/build/build/pubspec.yaml:modify
1623088880266:Noti:{"event"::"analysis.flushResults","params"::{"files"::["/Users/scheglov/dart/build/analysis_options.yaml",
1623088880658:Noti:{"event"::"analysis.errors","params"::{"file"::"/Users/scheglov/dart/build/build/pubspec.yaml","errors"::[]}}
1623088880845:Req:{"id"::"13","method"::"analysis.updateContent","params"::{"files"::{"/Users/scheglov/dart/build/build/pubspec.yaml"::{"type"::"add","content"::"name:: build\nversion:: 2.0.2\ndescription:: A build system for Dart.\nrepository:: https:://github.com/dart-lang/build/tree/master/build\n\nenvironment::\n  sdk:: \">=2.12.0 <3.0.0\"\n\ndependencies::\n  analyzer:: ^1.0.0\n  async:: ^2.5.0\n  convert:: ^3.0.0\n  crypto:: ^3.0.0\n  glob:: ^2.0.0\n  logging:: ^1.0.0\n  meta:: ^1.3.0\n  path:: ^1.8.0\n\ndev_dependencies::\n  build_resolvers:: ^2.0.0\n  build_test:: ^2.0.0\n  pedantic:: ^1.0.0\n  test:: ^1.16.0\n"}}},"clientRequestTime"::1623088880542}
1623088880846:Req:{"id"::"14","method"::"analysis.updateContent","params"::{"files"::{"/Users/scheglov/dart/build/build/pubspec.yaml"::{"type"::"remove"}}},"clientRequestTime"::1623088880745}

[jakemac53]

@scheglov do you think this is something we can reasonably resolve?

I am still very regularly running into this issue so I think it would preclude any lint that requires reacting to pubspec.yaml files from being included in the core set as the usability is not up to the bar for something we enforce on pub. If we need to keep that in mind as a general rule that is fine, but I would like to reach a decision one way or the other :).

[scheglov]

I tried to start watching the included folders on the file system before starting to build analysis context, but I just don't get any events from package:watcher. My current suspicion is that because all the work of creating analysis context is synchronous, package:watcher, or dart:io don't get a chance to actually set up watching. And once we are done creating analysis context, I cancel the subscription. So, it never happens. If this is accurate, then it is sad - the API does not tell me that it will not work when synchronous execution.

My next idea is to check pertinent pubspec.yaml files after creating analysis contexts, maybe after some short timeout, to give package:watcher time to set up.

[jakemac53]

Ok, if you intend to still work on this a bit more I am happy to wait - the urgency isn't super high here. I just want to avoid a forever stale issue as this is a blocker for another decision (whether to include depend_on_referenced_packages in the core lint set).

So basically all I ask is that if you reach a point where you don't think there is anything else reasonable to do on the analyzer side of things, that we update this issue as such and close it :).

[scheglov]

I will also open a question issue for package:watcher, if it is not supposed to work, I'd like to know this for sure.
https://github.com/dart-lang/watcher/issues/114

[scheglov]

For me this seems to work reliably for fast pubspec.yaml changes.
https://dart-review.googlesource.com/c/sdk/+/203821

[jakemac53]

I just rebuilt as of this morning and at least for the build repo I am still consistently not seeing any updates after the first update to a pubspec.

A regular, non-mono repo seems to be working consistently though.

[jakemac53]

Fwiw I had the same thing happen with the analysis_options.yaml file for me today. First edit to it worked and then no subsequent edits did anything until I restarted the analysis server.

[scheglov]

We have now checks for changes that happen during analysis contents creation to pubspec.yaml files.
But only for these files - nothing for other files, such as analysis_options.yaml.

"No subsequent changes" sounds potentially as something different, I need to check.
I would expect that slow changes when package:watcher can keep up with it would work.

[jakemac53]

"No subsequent changes" sounds potentially as something different, I need to check.

This is generally the behavior I am seeing now for pubspec.yaml as well. The very first edit does work, but no future edits work.

[bwilkerson]

Are the future edits putting the file content back to the original state, by chance?

[jakemac53]

Are the future edits putting the file content back to the original state, by chance?

Yes that is generally what I am testing (comment/uncomment a line to add/remove a dependency)

[bwilkerson]

Does it catch the second change when it doesn't return it to its original state?

I could imagine the behavior you're seeing if we're not updating the content in the cache after the first change. That is, we notice the change by comparing the cached content with the actual content and re-analyze, but the next time we get new content we compare it against the unchanged cached version and think that there hasn't been a change.

[scheglov]

I cannot reproduce this.
I have https://github.com/dart-lang/build.git clone open (so with all its packages), and I edit build/build/pubspec.yaml.

1. Clean initially.
2. Comment out convert: ^3.0.0, save, wait, get one lint.
3. Comment out crypto: ^3.0.0, save, wait, get +3 lints, 4 total.
4. Un-comment convert: ^3.0.0, save, wait, -1 lint, 3 total.
5. Un-comment crypto: ^3.0.0, save, wait, no lints.

[jakemac53]

When I follow those same instructions only Step 1 gives the same behavior... 😕 . Nothing else works for me after that point. I just rebuilt my sdk as of this morning.

[jakemac53]

@scheglov can you try with vscode? I don't see why it would make a difference but maybe somehow it does?

[jakemac53]

I turned on some logging and I confirm that I do see update events for the pubspec in the logs, I sent Konstantin the full logs offline so hopefully that helps.

[scheglov]

I don't have VS Code installed, it will take some time to get there.
However I note that it uses different protocol, and slightly different implementation.

[scheglov]

Did you save pubspec.yaml file after changes?
Just analysis.updateContent to it will not cause re-analysis.

[jakemac53]

Yes, I did save. If that message is not what it prints for file events (I just assumed it was) then I was probably wrong and it is indeed still not seeing the file events?

[scheglov]

In order to see watch events you need to provide --protocol-traffic-log=/Users/scheglov/tmp/das_protocol.log as one of the arguments in DAS command line. In IntelliJ this is done using dart.server.additional.arguments in Registry....

Then you will see something like:

1624292464240:Req:{"id"::"8","method"::"analysis.updateContent","params"::{"files"::{"/Users/scheglov/dart/build/build/pubspec.yaml"<cut>
1624292464248:Res:{"id"::"8","result"::{}}
1624292464257:Noti:{"event"::"server.status","params"::{"analysis"::{"isAnalyzing"::true}}}
<cut>
1624292464372:Noti:{"event"::"server.status","params"::{"analysis"::{"isAnalyzing"::false}}}
1624292476042:Watch:<unknown>:/Users/scheglov/dart/build/build/pubspec.yaml:modify
1624292476052:Noti:{"event"::"analysis.flushResults","params"::{"files"<cut>
1624292476254:Noti:{"event"::"analysis.errors","params"::{"file"::"/Users/scheglov/dart/build/analysis_options.yaml","errors"::[]}}
<cut>

Note Watch line, only after it DAS started reanalyzing.