search

dart-lang / linter

Linter for Dart.
https://dart.dev/tools/linter-rules
BSD 3-Clause "New" or "Revised" License
631 stars 170 forks source link

depend_on_referenced_packages requires analysis server restart to update #2694

Open jakemac53 opened 3 years ago

jakemac53 commented 3 years ago

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 commented 3 years ago

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

scheglov commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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

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:

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

scheglov commented 3 years ago

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 commented 3 years ago

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

pq commented 3 years ago

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

scheglov commented 3 years ago

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

pq commented 3 years ago

/fyi @jakemac53

jakemac53 commented 3 years ago

Unfortunately this still appears very flaky to me :(

scheglov commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

@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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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

jakemac53 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

"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 commented 3 years ago

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

jakemac53 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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

jakemac53 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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

jakemac53 commented 3 years ago

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 commented 3 years ago

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.