depend_on_referenced_packages requires analysis server restart to update #58425

jakemac53 · 2021-06-03T14:50:41Z

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 · 2021-06-03T15:59:51Z

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

scheglov · 2021-06-03T16:31:54Z

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 #45996

bwilkerson · 2021-06-03T16:42:40Z

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 · 2021-06-03T16:46:21Z

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 · 2021-06-03T16:50:03Z

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 · 2021-06-03T17:14:32Z

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 · 2021-06-03T17:22:01Z

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 · 2021-06-03T21:07:31Z

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

Bug: https://github.com/dart-lang/linter/issues/2694 Change-Id: I239863703c70c6bd810be6847606e5fc6695073a Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/202265 Reviewed-by: Samuel Rawlins <[email protected]> Commit-Queue: Konstantin Shcheglov <[email protected]>

pq · 2021-06-04T17:12:05Z

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

scheglov · 2021-06-04T17:17:10Z

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

pq · 2021-06-04T17:18:00Z

/fyi @jakemac53

jakemac53 · 2021-06-04T18:09:41Z

Unfortunately this still appears very flaky to me :(

scheglov · 2021-06-04T20:54:41Z

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:

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

jakemac53 · 2021-06-07T14:38:41Z

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 · 2021-06-07T14:52:18Z

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 · 2021-06-07T14:57:53Z

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 · 2021-06-07T15:00:35Z

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 · 2021-06-07T17:39:44Z

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 · 2021-06-07T18:23:40Z

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}

This makes it less probably that we will get inconsistent results, such as computing the salt with one file content, but then read a different content for parsing and reporting lints. But I think there there is still something not quite correct with deciding that there was a change to pubspec.yaml, we might miss some events. Bug: https://github.com/dart-lang/linter/issues/2694 Change-Id: Ied76dc2f6c1203d3eed68e5220196d91c6d8e717 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/202726 Reviewed-by: Brian Wilkerson <[email protected]> Commit-Queue: Konstantin Shcheglov <[email protected]>

jakemac53 · 2021-06-15T14:39:52Z

@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 · 2021-06-15T19:30:36Z

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 · 2021-06-15T19:54:47Z

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 · 2021-06-15T20:51:08Z

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.
dart-lang/tools#1730

scheglov · 2021-06-15T22:23:35Z

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

Bug: https://github.com/dart-lang/linter/issues/2694 Change-Id: I13cae115e48ae794e0751b6099f864ca9d202cdc Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/203821 Reviewed-by: Brian Wilkerson <[email protected]> Commit-Queue: Konstantin Shcheglov <[email protected]>

jakemac53 · 2021-06-16T15:33:50Z

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 · 2021-06-17T15:44:48Z

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 · 2021-06-17T15:50:48Z

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 · 2021-06-17T17:39:05Z

"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 · 2021-06-17T17:47:22Z

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

jakemac53 · 2021-06-17T17:54:28Z

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 · 2021-06-17T18:03:28Z

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 · 2021-06-17T18:18:54Z

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.

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

jakemac53 · 2021-06-18T15:56:45Z

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 · 2021-06-18T15:57:29Z

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

jakemac53 · 2021-06-18T16:13:01Z

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 · 2021-06-18T16:30:09Z

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 · 2021-06-19T23:20:20Z

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

jakemac53 · 2021-06-21T14:40:44Z

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 · 2021-06-21T16:28:19Z

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.

FMorschel · 2024-12-16T11:16:36Z

Is this still happening @jakemac53? Noticed this is three years old now.

jakemac53 · 2024-12-17T15:18:48Z

Is this still happening @jakemac53? Noticed this is three years old now.

I couldn't reproduce it just now so I think its resolved

jakemac53 mentioned this issue Jun 3, 2021

Consider depend_on_referenced_packages for core lints inclusion dart-lang/core#749

Closed

jakemac53 changed the title ~~depend_on_referenced_packages requires restart in package:build workspaces~~ depend_on_referenced_packages requires analysis server restart to update Jun 3, 2021

scheglov mentioned this issue Jun 15, 2021

Does it work synchronously? dart-lang/tools#1730

Closed

pq added the linter-set-core label Jul 18, 2022

pq added the P2 A bug or feature request we're likely to work on label Nov 14, 2022

srawlins added the type-bug Incorrect behavior (everything from a crash to more subtle misbehavior) label Mar 29, 2024

devoncarew added devexp-linter Issues with the analyzer's support for the linter package legacy-area-analyzer Use area-devexp instead. labels Nov 19, 2024

devoncarew transferred this issue from dart-archive/linter Nov 19, 2024

jakemac53 closed this as completed Dec 17, 2024

depend_on_referenced_packages requires analysis server restart to update #58425

depend_on_referenced_packages requires analysis server restart to update #58425

Comments

jakemac53 commented Jun 3, 2021 • edited Loading

jakemac53 commented Jun 3, 2021

scheglov commented Jun 3, 2021 • edited Loading

bwilkerson commented Jun 3, 2021

jakemac53 commented Jun 3, 2021 • edited Loading

scheglov commented Jun 3, 2021

jakemac53 commented Jun 3, 2021

scheglov commented Jun 3, 2021

scheglov commented Jun 3, 2021

pq commented Jun 4, 2021

scheglov commented Jun 4, 2021

pq commented Jun 4, 2021

jakemac53 commented Jun 4, 2021

scheglov commented Jun 4, 2021

jakemac53 commented Jun 7, 2021

jakemac53 commented Jun 7, 2021 • edited Loading

jakemac53 commented Jun 7, 2021 • edited Loading

jakemac53 commented Jun 7, 2021 • edited Loading

scheglov commented Jun 7, 2021

scheglov commented Jun 7, 2021

jakemac53 commented Jun 15, 2021

scheglov commented Jun 15, 2021

jakemac53 commented Jun 15, 2021

scheglov commented Jun 15, 2021

scheglov commented Jun 15, 2021

jakemac53 commented Jun 16, 2021

jakemac53 commented Jun 17, 2021

scheglov commented Jun 17, 2021

jakemac53 commented Jun 17, 2021

bwilkerson commented Jun 17, 2021

jakemac53 commented Jun 17, 2021

bwilkerson commented Jun 17, 2021

scheglov commented Jun 17, 2021

jakemac53 commented Jun 18, 2021

jakemac53 commented Jun 18, 2021

jakemac53 commented Jun 18, 2021

scheglov commented Jun 18, 2021

scheglov commented Jun 19, 2021

jakemac53 commented Jun 21, 2021

scheglov commented Jun 21, 2021

FMorschel commented Dec 16, 2024

jakemac53 commented Dec 17, 2024

jakemac53 commented Jun 3, 2021 •

edited

Loading

scheglov commented Jun 3, 2021 •

edited

Loading

jakemac53 commented Jun 3, 2021 •

edited

Loading

jakemac53 commented Jun 7, 2021 •

edited

Loading

jakemac53 commented Jun 7, 2021 •

edited

Loading

jakemac53 commented Jun 7, 2021 •

edited

Loading