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

Limit the number of analyzer plugins per context to one #50981

Closed
srawlins opened this issue Jan 10, 2023 · 18 comments
Closed

Limit the number of analyzer plugins per context to one #50981

srawlins opened this issue Jan 10, 2023 · 18 comments
Labels
analyzer-plugin 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 type-enhancement A request for a change that isn't a bug

Comments

@srawlins
Copy link
Member

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.

@srawlins srawlins added P1 A high priority bug; for example, a single project is unusable or has many test failures area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. type-enhancement A request for a change that isn't a bug analyzer-plugin labels Jan 10, 2023
@bwilkerson
Copy link
Member

... add code to the list- and map-parsing code that limits the number of plugins to one.

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.

@srawlins
Copy link
Member Author

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.

copybara-service bot pushed a commit that referenced this issue Feb 21, 2023
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>
@PGScully
Copy link

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?

@daniel-v
Copy link

daniel-v commented Mar 31, 2023

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?

@pattobrien
Copy link

pattobrien commented Apr 4, 2023

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.

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

@bwilkerson
Copy link
Member

@jacob314

@srawlins
Copy link
Member Author

srawlins commented Apr 5, 2023

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?

Yes that is the hope. :) Not a simple, direct revert, but a new design for plugins.

has this changeset achieved what it meant to do?

We're not sure, only time will tell. CC @devoncarew @jacob314 @mraleph in case they have seen any changes.

As a plugin maintainer [...] I would have expected at least some transparency on a timeline or how we can work together to address a workaround.

Apologies. We don't have a timeline for implementing a new plugins architecture.

Surely there is another way to warn users of the consequences of plugins while allowing them to make the decision for themselves.

I hope there are. We're open to ideas.

@marceloadsj
Copy link

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.
But, leaving this decision to the user would be great, because sometimes, the tradeoff of performance/memory is worth it (my case).

@ElDuderini
Copy link

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.

@bwilkerson
Copy link
Member

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.

@marceloadsj
Copy link

@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".

@bwilkerson
Copy link
Member

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.

@jacob314 @srawlins For wider visibility.

@marceloadsj
Copy link

marceloadsj commented Aug 17, 2023

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 😅 ).

@pattobrien
Copy link

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.

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)

@incendial
Copy link
Contributor

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.

@ElDuderini
Copy link

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?

@amrgetment
Copy link

where is the issue tracking allowing multiple plugins?

@bwilkerson
Copy link
Member

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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
analyzer-plugin 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 type-enhancement A request for a change that isn't a bug
Projects
None yet
Development

No branches or pull requests

9 participants