-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Limit the number of analyzer plugins per context to one #50981
Comments
I would have guessed that there was only one format (presumably the map) after the included options file and the current options file had been merged. |
No, any of the three can be found. In particular, if there is no included options file, then the format parsed is the format given in the options file. |
This accounts for included options files, and the three forms of 'plugins' values that are supported: scalar, list, and map. This includes an analysis options warning which reports each plugin specified after the first. Fixes #50981 Cherry-pick: https://dart-review.googlesource.com/c/sdk/+/278805 Change-Id: Ib6fe3ddd459854cf02aba3687fdb82b62db18b64 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/281300 Reviewed-by: Brian Wilkerson <brianwilkerson@google.com> Commit-Queue: Samuel Rawlins <srawlins@google.com>
I hope this is a temporary workaround until the underlying issues (it would have been nice if you had linked them) are addressed, and then this can be reverted? |
As a follow-up to this, probably to @srawlins , has this changeset achieved what it meant to do? Is there ongoing work to allow us to use more than 1 plugin in the future again? |
As a plugin maintainer who has spent the last couple months planning a business case for package:sidecar, such a huge restriction decision feels like a blow to my investment into proving use cases for a dart plugin ecosystem, and I would have expected at least some transparency on a timeline or how we can work together to address a workaround. I fully understand that the current memory consumption of each plugin makes the experience less than ideal, and that plugin support has been experimental for a reason, but restricting the amount of plugins that can be used on a project to one will surely kill the recent investment into plugins made by sidecar, DCM, and custom_lint, to name a few. Users will stick with whichever currently adds the most value to their project. The consequence is that we will stifle the innovation of these tools before we fully understand the use cases of plugins in general. Surely there is another way to warn users of the consequences of plugins while allowing them to make the decision for themselves. @srawlins @bwilkerson |
Yes that is the hope. :) Not a simple, direct revert, but a new design for plugins.
We're not sure, only time will tell. CC @devoncarew @jacob314 @mraleph in case they have seen any changes.
Apologies. We don't have a timeline for implementing a new plugins architecture.
I hope there are. We're open to ideas. |
It would be awesome if this could be configured on user-side... It can be enabled to a single plugin by default, I don't mind. |
We have a use case where we want to use a static code analysis plugin that encourages best practices for dart code implementation, while we also use custom linting using custom_lints to encourage the use of custom classes we created within the app (I.E., a custom Widget state class). Would love to know that support for a use case like this would be reintroduced, because as is we would only be able to support either the custom linting or the general code quality linting. |
We are definitely working to see whether we can design and implement plugin support that would allow for the use of multiple plugins. I don't have any timeframe that I can give for when we'll have a decision about whether such support will be added, nor, of course, any guess as to how long it will take to implement. |
@bwilkerson would you consider, in the meantime, creating a flag to allow consumers to decide on using multiple plugins or not? It can have warnings and what not, to tell the users about enabling it... kind of "do it at your own risk". |
Our concern with doing so, and the reason we decided not to provide a flag to allow multiple plugins when we added the one-plugin limitation, is the performance problem I mentioned. We believe that the problem is severe enough that allowing multiple plugins (under the current implementation) makes the tooling unusable. That said, we value user feedback and are always willing to reconsider our decisions. That doesn't mean that the decision will be changed, just that we'll think about it again. |
Yes, I can understand and definitely respect your decision! The only thing I am not fully in agreement is this quote: 'the problem is severe enough that allowing multiple plugins makes the tooling unusable'. My reasoning is that, since the plugins are really configurable and can be even created from userland, there's no way that is 100% guaranteed for you to know if my specific use case is suffering from this problem. So, for my specific project, I suffered the opposite case, the tool itself became 'unusable'. Again, just to emphasise that I do respect your reasoning and whatever is decided, well, I will adapt to it (as I try to do with so many other stuff on a daily basis... everyone does that I guess... hehe 😅 ). |
Same here - I've personally never experienced problems regarding too much RAM consumption on my particular projects. Still unsure what causes some users to experience the "problem" mentioned (many barrel files, nested workspace packages, etc.). @incendial - I know you were actively monitoring high RAM usage issues for a few DCM plugin users, can you speak to how severe the problem is from your perspective? (total users experiencing a problem vs. population of DCM users) |
Most of the users that have a large project ended up removing DCM because of the analyzer + plugin performance issues. But the overall number of these users compared to all users is relatively small. I believe the number of packages within the workspaces contributes the most since the plugin is created for each context. |
RAM usage has never been a problem for our team as we have been using the DCM plugin for our project for a good while. Though even if RAM usage is a concern, why couldn't we inflict a bit more ram usage on ourselves if we really want to implement additional plugins to make the development process easier? Is it because of some diagnostic analytics for usage data, and removing the ability to add in more than one plugin makes diagnostics data more consistent? |
where is the issue tracking allowing multiple plugins? |
To the best of my knowledge there isn't any issue tracking this work. The Dart team tends to create issues to track the implementation of a language feature (but some smaller features don't have any tracking issues), but other implementation work typically doesn't have a tracking issue associated with it (again, there are exceptions). |
In order to prevent users from suffering the various woes of running too many analyzer plugins, we will limit the number of plugins-per-context to one.
In this design we limit the number of plugins per analysis context (as specified by the presence of analysis_options.yaml files) to one. We do this at the level of parsing the analysis options file.
Before an analysis_options.yaml file is processed for plugins and other options, the included options files are merged with it (recursively) (
AnalysisOptionsProvider.getOptionsFromSource
). Because of this, we can limit the number of enabled plugins directly in the code that parses the plugins YAML section (_OptionsProcessor.applyToAnalysisOptions
), at which point plugins from included analysis options files have already been merged in.The simplest change to make would be to continue to accept the singular format, the list format, and the map format, and add code to the list- and map-parsing code that limits the number of plugins to one.
The text was updated successfully, but these errors were encountered: