Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Analyzer plugin] Only the latest version of the plugin is installed, despite the version in pubspec #50209

Closed
incendial opened this issue Oct 14, 2022 · 11 comments
Labels
area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. P1 A high priority bug; for example, a single project is unusable or has many test failures

Comments

@incendial
Copy link
Contributor

incendial commented Oct 14, 2022

So, long story short: somehow always the last plugin version from pub is installed no matter what version is listed in the pubspec and it installs even if dependency constraints block the update to the latest version.

Initially, I reported this issue here flutter/flutter#95092 (comment)

Another problem that people started to report the issues from the newer package version when they haven't yet updated. Like they have DCM 4.17.0, but IDE somehow reports an issue from 4.18.0. I'm not sure, if it's even possible, but it's already two reports.

but now we were able to reproduce it after I've added the version to the plugin name.

Note: the latest version in pub is exactly 4.21.2

Screenshot 1:
telegram-cloud-photo-size-2-5354945094597328432-x

Screenshot 2:
telegram-cloud-photo-size-2-5354945094597328434-y

Screenshot 3:
telegram-cloud-photo-size-2-5354945094597328428-y

Could you please take a look? Looks like it's the last broken part from the new plugins api. Also, all the reports we received were from VS Code, so I'm not sure, that it affects IDEA.

@Hideart
Copy link

Hideart commented Oct 14, 2022

Same here. It generates very non-obvious bugs 🥲

@lrhn lrhn added the area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. label Oct 15, 2022
@incendial incendial changed the title [Analyzer plugin] Only the latest version of the plugin is installed, despite the version in pub [Analyzer plugin] Only the latest version of the plugin is installed, despite the version in pubspec Oct 15, 2022
@incendial
Copy link
Contributor Author

@bwilkerson @scheglov could you please take a look?

@keertip
Copy link
Contributor

keertip commented Oct 17, 2022

@DanTup

@keertip keertip added the P1 A high priority bug; for example, a single project is unusable or has many test failures label Oct 17, 2022
@DanTup
Copy link
Collaborator

DanTup commented Oct 17, 2022

I'm not completely familiar with the plugin support but I had a quick look at this and was able to reproduce the issue above where a different version of the plugin compared to the pubspec was being used:

Screenshot 2022-10-17 at 18 57 09

I had a look through the code for that version and found that in tools/analyzer_plugin/pubspec.yaml, v4.18.2 of the dart_code_metrics package contains this:

dependencies:
  dart_code_metrics: ^4.18.2

I found some docs about this package here, which say:

it must have a file named tools/analyzer_plugin/pubspec.yaml that can be used by the pub command to produce a .packages file describing how to resolve the package: URIs found in it. Typically, the only dependency that needs to be included is a dependency on the plugin package.

So it seems like the analyzer uses this file to set the plugin up, and that file allows for newer versions to be fetched.

What I'm not sure about is whether:

  • the plugin (dart_code_metrics) should not include ^ in this file and instead have an exact version number if that's the behaviour that's expected
  • the analyzer/analyzer_plugin should overwrite the version constraint with one that matches what was in the users pubspec so these things are always in-sync

This is probably a question for @bwilkerson / @scheglov (and whatever the answer is, it probably makes sense to capture in those docs).

@incendial
Copy link
Contributor Author

incendial commented Oct 17, 2022

@DanTup thanks for looking into that!

Yes, the version is set with ^, but it resolves to the latest version despite the dependency constrains. For example, if the package can't be updated to the analyzer 5.0.0., it still has the latest version of DCM plugin running. That's what has changed and looks like a bug (and I'm pretty sure that we were releasing new versions before and our users had never reported such issues, before the new API).

But I agree that probably Brian or Konstantin could add more about this behaviour being expected or not.

@DanTup
Copy link
Collaborator

DanTup commented Oct 18, 2022

but it resolves to the latest version despite the dependency constrains. For example, if the package can't be updated to the analyzer 5.0.0., it still has the latest version of DCM plugin running.

Which dependency constraints do you mean? Do you mean the analyzer/analyzer_plugin constraints in the main pubspec.yaml for dart_code_metrics are not being honoured? Or perhaps that the version of the analysis server that's running is not constraining the packages to those that use versions of analyzer/analyer_plugin that match its own? (sorry if this is a silly question, I'm not too familiar with plugins but curious to better understand these things 🙂).

@incendial
Copy link
Contributor Author

incendial commented Oct 18, 2022

Or perhaps that the version of the analysis server that's running is not constraining the packages to those that use versions of analyzer/analyer_plugin that match its own? (sorry if this is a silly question, I'm not too familiar with plugins but curious to better understand these things 🙂).

Yeah, that's how it has been working before. If a package lists dart_code_metrics with a ^ and a version and upgrading to an upper version is blocked (dart pub get fails), then only the plugin version from the installed dart_code_metrics is used.

Edit: actually, the plugin was just always used from the installed package.

@bwilkerson
Copy link
Member

Yes, the version is set with ^, but it resolves to the latest version despite the dependency constrains.

My understanding of version constraints is that ^4.18.2 means that any version between 4.18.2 (inclusive) and 5.0.0 (exclusive) is allowed to match. If pub is run on the tools/analyzer_plugin/pubspec.yaml file given above, then it's valid for it to choose version 4.21.2.

For example, if the package can't be updated to the analyzer 5.0.0., it still has the latest version of DCM plugin running.

I'm a bit confused. Which package are you referring to here? (See https://github.com/dart-lang/sdk/blob/main/pkg/analyzer_plugin/doc/tutorial/package_structure.md for a description of the four possible meanings of "package" in this context.) If the below doesn't clear things up for you, you might need to restate the problem in more precise terms.

Or perhaps that the version of the analysis server that's running is not constraining the packages to those that use versions of analyzer/analyer_plugin that match its own?

The analysis server never constrains the versions of the packages used by plugins, and that's intentional.

If we shipped a version of the analysis server that was built on analyzer version 6.0.0, then any plugins that had not yet been upgraded to run on version 6.0.0 would not be able to be loaded and run. In other words, every new version of the analyzer would potentially disable some or all of the plugins being used by a given user. I think that would significantly hurt the user's experience.

Even if we did something to coordinate with plugin authors so that their plugins would all be updated before we shipped, it would mean that plugin authors couldn't provide version-specific support for their packages. (Remember that the original use case was not for a stand-alone plugin like DCM, but was for a very package-specific plugin like the one for Angular Dart, where a different version of the plugin might be needed for each new version of the host package.) If a user is using an older version of the host package, then a newer version of the plugin package might not be able to correctly support the user's code.

We therefore deemed it essential that the versions on packages used by a given plugin had to be independent of either the versions of those packages used by the analysis server or the versions used by other plugins. We achieved that goal by running each plugin in a separate isolate, making the running code completely independent of what code was being run by any other isolate.

Plugins do need to update to newer versions of the analyzer package anytime the newer version supports new language features (or the plugin is likely to break when run on a code base that's using those features), but there are no constraints for other version updates.

@incendial
Copy link
Contributor Author

incendial commented Oct 19, 2022

My understanding of version constraints is that ^4.18.2 means that any version between 4.18.2 (inclusive) and 5.0.0 (exclusive) is allowed to match. If pub is run on the tools/analyzer_plugin/pubspec.yaml file given above, then it's valid for it to choose version 4.21.2.

I always treated plugin and the host package as one and from this perspective update to the 4.21.2 wouldn't be possible in the target package that has DCM listed in pubspec, since the closest resolvable version is 4.18.2. Otherwise it creates a problem with the CLI and the plugin running different code.

I'm a bit confused. Which package are you referring to here? (See https://github.com/dart-lang/sdk/blob/main/pkg/analyzer_plugin/doc/tutorial/package_structure.md for a description of the four possible meanings of "package" in this context.) If the below doesn't clear things up for you, you might need to restate the problem in more precise terms.

Sorry for that, by the package I meant a target package (like a flutter app), that has DCM listed in its pubspec. If the package is unable to have 4.21.2 as a resolved dependency, but somehow has a plugin from 4.21.2 - that was not expected.

If we shipped a version of the analysis server that was built on analyzer version 6.0.0, then any plugins that had not yet been upgraded to run on version 6.0.0 would not be able to be loaded and run. In other words, every new version of the analyzer would potentially disable some or all of the plugins being used by a given user. I think that would significantly hurt the user's experience.

Even if we did something to coordinate with plugin authors so that their plugins would all be updated before we shipped, it would mean that plugin authors couldn't provide version-specific support for their packages. (Remember that the original use case was not for a stand-alone plugin like DCM, but was for a very package-specific plugin like the one for Angular Dart, where a different version of the plugin might be needed for each new version of the host package.) If a user is using an older version of the host package, then a newer version of the plugin package might not be able to correctly support the user's code.

We therefore deemed it essential that the versions on packages used by a given plugin had to be independent of either the versions of those packages used by the analysis server or the versions used by other plugins. We achieved that goal by running each plugin in a separate isolate, making the running code completely independent of what code was being run by any other isolate.

Thanks for the explanation! Then I'll release the fix removing the ^ and this should be it.

Plugins do need to update to newer versions of the analyzer package anytime the newer version supports new language features (or the plugin is likely to break when run on a code base that's using those features), but there are no constraints for other version updates.

Thats a very good point, any chance I can find a release date for new features, like patterns, records, etc. anywhere, in order to support them before they are live? Or at least an approximate date.

@bwilkerson
Copy link
Member

... any chance I can find a release date for new features, like patterns, records, etc. ...

I don't believe that we have a firm release date. It will depend on a lot of factors including how long it takes the sub-teams (analyzer, dart2js, vm, etc.) to implement the feature. We won't ship the feature until it's solid, so we don't set a date.

I can tell you that the analyzer team has started working on records and patterns in the analyzer package. The support for records is mostly complete at this point, though the feature's spec might still change, so "complete" is a fairly vague concept at this point. :-) The work for patterns has just started, and probably isn't far enough along to be helpful.

I can also tell you that the server portion of the records work has started. We have basic support mostly finished, but are still evaluating whether there are any new fixes, assists, lints, etc. that we think should be part of the initial launch. (We often wait to see how the community will use a feature before adding stylistic lints.) The server work for patterns hasn't really started yet because the analyzer package isn't quite far enough along for that.

@incendial
Copy link
Contributor Author

... any chance I can find a release date for new features, like patterns, records, etc. ...

I don't believe that we have a firm release date. It will depend on a lot of factors including how long it takes the sub-teams (analyzer, dart2js, vm, etc.) to implement the feature. We won't ship the feature until it's solid, so we don't set a date.

I can tell you that the analyzer team has started working on records and patterns in the analyzer package. The support for records is mostly complete at this point, though the feature's spec might still change, so "complete" is a fairly vague concept at this point. :-) The work for patterns has just started, and probably isn't far enough along to be helpful.

I can also tell you that the server portion of the records work has started. We have basic support mostly finished, but are still evaluating whether there are any new fixes, assists, lints, etc. that we think should be part of the initial launch. (We often wait to see how the community will use a feature before adding stylistic lints.) The server work for patterns hasn't really started yet because the analyzer package isn't quite far enough along for that.

Thank you so much, sounds amazing, actually. I think I've seen some references to the records, but that they're very close to be competed is great.

We often wait to see how the community will use a feature before adding stylistic lints.

Fair point, especially taking the amount of work for other features 🙂

If you have any kind of notification for the external (or internal) package maintainers, like "you can now prepare your code for the feature X", I'd be happy to receive it too 🙂

I'm closing this, thanks again for the detailed explanation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. P1 A high priority bug; for example, a single project is unusable or has many test failures
Projects
None yet
Development

No branches or pull requests

6 participants