dart-lang / sdk

The Dart SDK, including the VM, JS and Wasm compilers, analysis, core libraries, and more.
https://dart.dev
BSD 3-Clause "New" or "Revised" License
10.25k stars 1.58k forks source link

Analyzer CLI should support generated sources #34098

Closed matanlurey closed 1 year ago

matanlurey commented 6 years ago

Thanks to the work of @MichaelRFairhurst (and others!), the analysis server now understands generated sources from package:build_*. That makes the IDE experience of users much better, thanks!

The next step though is to make generated sources work for non-IDE purposes, i.e. continuous integration using the dartanalyzer CLI tool. For now we are skipping analysis, which is less than desirable (https://github.com/dart-lang/angular/issues/1508).

dentra commented 5 years ago

Any chance to get it fixed?

evanweible-wf commented 5 years ago

Is there any work planned to address this issue? And if not, would it be something you'd accept a contribution for? It's impacting us on our transition from Dart 1 to Dart 2 and our workarounds are essentially the same as yours were for angular – skip analysis during CI or add an ignore rule for the analyzer error. Unfortunately our issue is not with the uri_has_not_been_generated rule but the undefined_setter rule because of how we're using this generated code.

Skipping analysis altogether during CI is most likely a non-starter for us, and our concern is that ignoring the undefined_setter rule for whole files or for the entire package has a high chance of swallowing actual errors. For some additional context, this is related to our conversion of over_react from transformers to builders and has a significant impact on our internal consumers. It's tough for us to convert 50+ over_react consumers to Dart 2 and the builder pattern with a caveat like this.

@matanlurey @kevmoo

kevmoo commented 5 years ago

@stereotype441 @devoncarew ?

stereotype441 commented 5 years ago

@MichaelRFairhurst what would be involved in getting this to work with the command-line analyzer?

matanlurey commented 5 years ago

Of note, this will start to impact Flutter and related projects as they start to adopt builders.

/cc @jonahwilliams

... we are having to start ignoring other errors now, such as invalid_assignment, due to the lack of generated file support on the command-line.=, similar to @evanweible-wf has noted in https://github.com/dart-lang/sdk/issues/34098#issuecomment-460331317.

devoncarew commented 5 years ago

I believe Flutter will be fine here - flutter analyze is based on the analysis server, not the cli analyzer command.

For non-flutter projects, one option for analysis on CI systems is to use a CLI based on the analysis server. pub global activate tuneup and then pub global run tuneup analyze will analyze the project, should support generated code, and will return a non-zero exit code if any analysis issues are found.

evanweible-wf commented 5 years ago

@devoncarew thanks for suggesting tuneup, we've been able to use that during CI successfully.

Unfortunately, this still presents a pretty big issue for us because the pana tool that scans every uploaded package uses dartanalyzer directly and fails on our over_react package for this same reason, with the result being that our package receives a health score of 0 (and prevents it from recognizing that it is Dart 2 compatible).

Here's an example pana run: https://pub.dartlang.org/packages/over_react/versions/2.0.0-alpha#-analysis-tab-

@kevmoo is there any way to get pana to successfully analyze a package that leverages build-to-cache? We were hoping to release 2.0.0 today, but we don't want it to appear as though the package is broken.

kevmoo commented 5 years ago

@evanweible-wf – you're publishing a package that's incomplete without source gen being run? Uh...we should discuss if there is something else you could do here...

evanweible-wf commented 5 years ago

@kevmoo It sounds like our only option is to build to source, but we had originally shied away from that option because of the documentation around source vs cache:

Use build_to: source when:

  • The generated code does not depend on details outside of the containing package that might change with a different version of its dependencies. Any details from the generating package, or any other dependency used by the generated code must not change without a major version bump.
  • The generated code should be pub published with the package and used by downstream packages without running a build themselves.

Use build_to: cache when:

  • The generated code uses details from outside of the package with the generated code, including from the generating package which may change without a major version bump.
  • The builder generates lots of files that the user might not expect to see.
  • The builder targets use cases for "application" packages that are unlikely to be published rather than packages which will be published and become dependencies.

Our thought was that if we build to source, we'd be unable to propagate fixes or changes to the builder that consumers would be able to consume _just by updating over_react_ itself (which would be possible with build-to-cache). If we build to source, then we are at the mercy of all dependencies that have generated over_react code. Maybe that's fine and maybe that's realistically the only option we have. Does that sound right?

aaronlademann-wf commented 5 years ago

I would also add that, if the use of build_to: cache results in "a package that's incomplete without source gen being run" - why is it available?

To me - a package that relies on the tool that the dart team designed as a replacement for transformers - which analyzed to perfection using a transformer - should not be seen as "incomplete". It should be seen as a package that needs to be built to be complete since we can no longer rely on the transformer.

Also, committing generated files is a big bummer / overhead for consumers.

MichaelRFairhurst commented 5 years ago

if the use of build_to: cache results in "a package that's incomplete without source gen being run" - why is it available?

See the last two bullets of each. Cache is best for "application" packages that are unlikely to be published" and source is best for "code should be pub published."

There are some unknowns here about what we should do in the case of "generated code uses details from outside of the package." That's the problem you're hitting with over_react. The problem is that we don't have hardly any infrastructure here....should we issue warnings when using such packages without a proper build config, for instance? Should this affect package health at all in the meantime before we have such infrastructure (granted, a score of 0 is of course way too extreme), ie, since it relies on a not-fully-supported use case, do we want to give full health for this?

I'll add another unknown. Does anyone know if pub runs the build before analysis? Ie, even if this issue were solved, would the files exist for the CLI to pick up? I'm pretty sure pana uses the analysis server and not the CLI, so I'm pretty sure the issue here is related to the build not running before analysis rather than it being an analyzer issue in this case.

@stereotype441 Regarding the question of how long it would take to support in the analyzer -- which would have been a benefit regardless so that you didn't need to switch your CI tools to use tuneup -- there are two paths:

evanweible-wf commented 5 years ago

@MichaelRFairhurst Any update on this? Is there a separate issue for updating the analyzer CLI to use the analysis server?

srawlins commented 1 year ago

dartanalyzer (now dart analyze) has also similarly moved to DAS, like flutter analyze. This should be fixed there as well.