build
build copied to clipboard
dart-lang
Resolver should expose errors from Dart analysis
From https://github.com/dart-lang/sdk/issues/29783#issuecomment-305810992.
AnalysisContext (and I assume the driver replacement) has the following API:
List<AnalysisError> computeErrors(Source source);
AnalysisErrorInfo getErrors(Source source);
Additions would be to Resolver:
abstract class Resolver {
bool get hasErrors;
List<?> get errors;
}
We probably don't want to expose AnalysisError|Info directly, so we could wrap it.
/cc @bwilkerson and @jakemac53 WDUT?
AnalysisDriver isn't public API (we're working on getting a public API for it), but yes, it has a getErrors method (with a different but similar return type).
We probably don't want to expose
AnalysisErrorInfodirectly ...
Correct. It won't be part of the new public API.
What is the best way to proceed here? I imagine:
- Add the API to
abstract class Resolver(unfortunately, breaking change)
- Include a
ResolverError|Infoclass that loosely maps toAnalysisError|Info
- Implement in
build_runner,build_barback, and_bazel_codegen.
I really could use this for AngularDart, where errors are really painful because no resolution occurs in many cases and you get strange null constant values that aren't really null, etc.
I don't know enough about the errors api but I wonder if it makes more sense to associate errors with Library instead of the general Resolver?
My understanding is Context|Driver has the errors, not library, but maybe you're right.
Let me play around a bit.
We could potentially wrap it in any case and expose the errors per library. I don't know for sure if that makes sense but it seems like it might be more useful.
Would it be sensible to hold off on this until the new API is available in package:analyzer?
Or at least to hold off on this until we've done the migration from AnalysisContext to AnalysisDriver?
@natebosch I'm not confident we know when that will happen, and lack of this API makes the AngularDart compiler very brittle (and unrecoverable) in some cases.
Makes sense. We can take on a bit of duplicated effort to avoid blocking this in the short term.
I think it'll involve touching code_transformers and build_barback for one of the implementations of Resolver, as well as _bazel_codegen for another - both use AnalysisContext today.
I'd like to validate that whatever we interface we build on AnalysisContext can also be built on AnalysisDriver so that we don't have to make breaking changes during that transition.
+1 for validating. I think this is somewhere we probably need all the packages edited at once, with dependency overrides, to validate this all works e2e, and someone from the analyzer team to chime in and make sure it works similarly with the driver.
Background issue for Angular specifically: https://github.com/dart-lang/angular/issues/706
While this is P1, I imagine we can't tackle this with the current priorities yet.
I'd like to re-state the priority of this. We're seeing an increasing number of errors both internally and externally that cause our compiler to crash, but would have been totally prevented if we didn't try and analyze invalid code.
I'm worried with the move to Kernel, this will probably be overlooked indefinitely :(
@kevmoo
Should we refuse to allow access to the LibraryElement if there are errors? Or should we add a new API to query for errors and leave it up to builder authors to check it?
Doesn't look like this came up before, but there is also the problem of generating files that are imported by the current library. In that case it is fully expected that there will be errors, but it would likely be difficult to figure out which errors actually matter.
@natebosch:
Should we refuse to allow access to the
LibraryElementif there are errors?
Probably not, though that is an interesting question. The particular issue here is defining "error", for example if someone sets unused_import: error in analysis_options.yaml, will that block a build then? If we could limit to "the compiler will not be able to run"-errors, I could get on board.
Otherwise we probably want to at least, at first, expose it (resolve.errors / resolver.hasErrors), and at least builder authors (like Angular) could check to see if there were errors before continuing to compile, or if we hit a NPE we could check to see if there is an error first before printing a "Something bad has happened" error.
@jakemac53:
Doesn't look like this came up before, but there is also the problem of generating files that are imported by the current library.
I guess we'd have to ignore URI_NOT_GENERATED. Yes this is tricky, though :( Ideas?
URI_NOT_GENERATED is not a safe thing to check for - it's hardcoded by certain file patterns and won't work in general for codegen that the analyzer isn't set up for in advance. More likely we'll need to allow missing imports, exports, and parts entirely, not only generated ones.
If we could limit to "the compiler will not be able to run"-errors, I could get on board.
I don't think we can know which are breaking to any given code generator. Missing (potentially generated) assets is one we can assume most builders might hit, but even some of those could be breaking for some builders if the missing imports aren't generated. In build_runner we could potentially check but in bazel we can't.
More likely we'll only be able to expose all errors and leave it up to builder authors to decipher which are fatal.
That's unfortunate :(. I guess step one is just expose the errors and we can always figure out what else we need to do to make the data more useful.
Exposing the errors seems like a good start. Once we start to figure out how generators use the information maybe we can provide further help.
e.g. with built_value the declaration of Foo typically references a symbol that doesn't exist yet, FooBuilder. This will be generated later. So it's not just an unused import, but an undefined symbol. Having an undefined symbol might mean you run into further problems: type mismatches, unexpected @override tags, etc.
Syntax errors, at least, we know are always errors. Currently (some of?) these are not caught, which leads to some really weird results.
On Wed, 15 Aug 2018 at 22:46 Matan Lurey [email protected] wrote:
Another option is find a way not to hard code URI_NOT_GENERATED ;)
— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/dart-lang/build/issues/295#issuecomment-413330479, or mute the thread https://github.com/notifications/unsubscribe-auth/AFR8mDKgROd-jLN5p5fUVPklM1ntnY-wks5uRIidgaJpZM4Nuj05 .
I'd be happy to write the plumbing for this feature.
Mockito's code-generation would really benefit from this, as we need to know when there are unresolved types. E.g.
// foo.dart:
////////////////
import 'bar.g.dart' show Bar;
abstract class Foo {
Bar m();
}
// foo_test.dart
////////////////
import 'foo.dart' show Foo;
import 'package:mockito/annotations.dart' show GenerateMocks;
@GenerateMocks([Foo]);
void main() {}
If build.yaml is not set up correctly, then Mockito's code generation may run before Bar's code generation. When this happens, Mockito's builder has no idea that there is a problem. It can see the ClassElement for Foo, and see that there is a method m() with a returnType of dynamic. Mockito has no way of knowing that the return type of m() is incorrect.
If mockito had access to the static errors, it could see that they are non-empty and throw.
I think this ultimately boils down to exposing a way to get the ResolvedLibraryResult and/or ResolvedUnitResult objects? Definitely open to contributions here, but reach out as it will require a complicated rollout.
cc @kevmoo - this would potentially solve the angular use case - I think the discussion was around an API to get a ResolvedLibraryResult.