Analyzer regression around diagnostics when imported files are not generated

bwilkerson commented 4 years ago

The analyzer used to notice when an import, export or part directive referenced a generate file that does not exist and silenced follow-on diagnostics. That behavior appears to have been broken. For example, a file containing the following:

import 'doesNotExist.g.dart';

void f(A a) {
  var b = A.b;
}

Now produces 3 diagnostics when it ought to produce a single "generated file does not exist" diagnostic.

This has serious implications for internal users.

bwilkerson commented 4 years ago

@scheglov

scheglov commented 4 years ago

Can you point me at a document that specifies the required behavior?

What I see implemented, and what I vaguely remember hearing when this feature was discussed, is that we ignore errors reported on identifiers that are either (1) imported with a prefix; (2) imported without a prefix but with an explicit show combinator. In both cases the library must be synthetic. meaning that its file is missing.

The example provided in the first comment does not satisfy these conditions, so we report multiple errors. But for the variants we report only one.

import 'doesNotExist.g.dart' show A;

void f(A a) {
  A.b;
}

or

import 'doesNotExist.g.dart' as p;

void f(p.A a) {
  p.A.b;
}

In both cases we get CompileTimeErrorCode.URI_HAS_NOT_BEEN_GENERATED [7, 21, Target of URI hasn't been generated: 'doesNotExist.g.dart'.].

We have a disjoined check for reporting a special error for a missing file when it looks like it might be generated. But I don't remember changes there, it has always been private, since it was added in 2017.

bwilkerson commented 4 years ago

Can you point me at a document that specifies the required behavior?

Unfortunately, no. I don't believe that we documented the desired behavior.

... we ignore errors reported on identifiers that are either (1) imported with a prefix; (2) imported without a prefix but with an explicit show combinator.

I don't remember those restrictions, but it's quite possible that we decided to do that in order to reduce the number of false negatives.

If that's the case, there is no regression. But I think we should consider broadening the support because using show combinators and prefixes won't work with part directives, which are what's used with most of the code generators. I suspect that users would even be happy having this loosening apply only to part directives at this point.

scheglov commented 4 years ago

OK, then this is not a bug, but an enhancement.

Without additional information we cannot distinguish an identifier that should come from an imported library, and identifier that should come from a part. But I can imagine that there might be a correspondence between a generated part name, and the pattern of names of generated entities in the part.

Do you have examples of such parts and generated identifiers? Maybe google3 targets which I could look at?

bwilkerson commented 4 years ago

@jwren

greglittlefield-wf commented 4 years ago

Related: https://github.com/dart-lang/sdk/issues/42977 (see note at the bottom of the issue)

Do you have examples of such parts and generated identifiers?

I have some examples here for over_react, if they're worth considering 🙂:

Source file: example/builder/src/basic.dart
Generated file: example/builder/src/basic.dart.over_react.g.dart

(There are other examples under example/builder/src) as well.

(Note that in this repo the generated files use a build-to-source builder and are checked in, but all consumer repos use build-to-cache.)

To summarize:

All of our generated members have names that start with either $ or a series of _ and $
Names in the generated file are derived from prefixing and/or postfixing names in the source file, but a single source member may result in multiple generated members
- For example: BasicProps results in $BasicProps, _$BasicProps, _$$BasicProps$PlainMap, etc. being generated
- However, we only support referencing a few of those members in the source file

devoncarew commented 3 years ago

Hey @greglittlefield-wf - to follow up on your last comment (and, cc'ing in @robbecker-wf):

We're looking at expanding the file name patterns for what we consider probably generated code, and, how we then identify the following undefined symbols in that file, to know which missing symbol errors to ignore.

We currently support two patterns for this - import 'doesNotExist.g.dart' show A; and import 'doesNotExist.g.dart' as p;. The nice things about these imports is that they encode the information in them to let us know which errors to suppress (the shown symbols or the ones referenced by the given prefix).

For the imports of the form part 'part_name.g.dart';, we don't have an easy way to know which following errors to suppress.

One possible solution to this would be to use part 'part_name.g.dart'; to identify probably generated code, and _$GeneratedSymbol to then suppress following issues. From @kevmoo, this matches the codegen pattern used by several libraries. Would this generally work for your use case? Note that this would catch _$$Symbol names but not $Symbol names.

One follow-on concern we have here is that this could unintentionally catch user code which is not using codegen.

alanknight-wk commented 3 years ago

If you allowed user to specify the form of generated symbols, would that help with the concern for false negatives. e.g. @ExpectGeneratedNamesToStartWith('_$')

bwilkerson commented 3 years ago

Yes, that would help.

It would be better if users didn't need to repeat that pattern in every library with a generated part, so we could consider adding a list of generatedNames patterns to the analysis options file. Even better would be some way for generators to publish the patterns that they produce so that analyzer could compute the list by looking at the dependencies, but maybe that would be too much magic.

greglittlefield-wf commented 3 years ago

@devoncarew Yes, I believe that would work for our use-case!

We currently have a subset of boilerplate that references generated $Symbol names (example reference and generated code), but we could definitely update those to be _$Symbol instead.

scheglov commented 3 years ago

https://dart-review.googlesource.com/c/sdk/+/166820

alanknight-wk commented 3 years ago

If it only applies when there's a missing generated file then it will still cause errors while people are editing a file that uses a code generator.

bwilkerson commented 3 years ago

Do you mean: in the case where the file was generated but the generated file is not out of date?

alanknight-wk commented 3 years ago

Is not up to date, yes.

bwilkerson commented 3 years ago

How important is it to support that use case (not reporting diagnostics when a generated file is out of date)?

alanknight-wk commented 3 years ago

Hard to say. It depends a lot on how much the code generator is doing. If, as soon as you touch anything in the file, it fills up with false errors, that seems bad - makes analysis useless for that file. But I think a lot of generators try to minimize how much of the original file is affected. Is it difficult to detect when the generated file is older than the current file and treat that the same as if it was missing?

natebosch commented 3 years ago

Hard to say. It depends a lot on how much the code generator is doing. If, as soon as you touch anything in the file, it fills up with false errors, that seems bad - makes analysis useless for that file.

If the content was this unstable I would expect that the // ignore also go stale on a change.

The only new errors that should surface here would be, I think, the same places you'd need to add a new // ignore using the old strategy. It would be good to get some real world experience trying this approach and see how it feels in practice.

bwilkerson commented 3 years ago

Is it difficult to detect when the generated file is older than the current file and treat that the same as if it was missing?

No, but that means that any change to the current file, such as adding a space, would block analysis support until the generated file is regenerated. I don't think that's a good UX either.

greglittlefield-wf commented 3 years ago

At least for the over_react use-case, the majority of the time, the code that references generated members isn't often edited after it's introduced. Usually, it's one UI component per file of the same name. For example, this isn't very common:

part 'foo.over_react.g.dart';

-UiFactory<FooProps> Foo = _$Foo;
+UiFactory<BarProps> Bar = _$Bar;

...and in those cases the solution from that CL would work well.

However, there's a newer case that isn't very common yet, but I suspect could become more common, where you have multiple, smaller UI components per file. This means new code can be added to existing files that references new generated members. For example:

part 'foo.over_react.g.dart';

UiFactory<FooProps> Foo = _$Foo;

...

+UiFactory<BarProps> Bar = _$Bar;

...and that would be an issue, both when users are adding that new code themselves, or when switching to branches that contain added code.

As a middle ground between only ignoring errors when the generated files are missing vs. only erroring when the source file hasn't been modified, could we update the error messages for undefined members that match that _$ pattern to suggest rerunning the code generator?

Since the situation is less common, my main concern would be more around users being confused about how to resolve the issue, as opposed to being around them having to rerun a build to resolve it.

For example, instead of just saying

error: Undefined name '_$Foo'. Try correcting the name to one that is defined, or defining that name.

...it also said something like:

If the name is generated, try checking that the generated code is up to date, and that the name is expected to be generated based on the source code and generator configuration.

greglittlefield-wf commented 3 years ago

@scheglov @bwilkerson I tried out that CL locally and it works great except for one thing: I'm seeing that errors related to disabled implicit casts aren't ignored.

For example:

part 'foo.g.dart';

class Foo {}
methodCall(Foo foo) {}

// error: A value of type 'dynamic' can't be assigned to a variable of type 'Foo'. (invalid_assignment)
Foo foo = _$foo;

// error: The argument type 'dynamic' can't be assigned to the parameter type 'Foo'. (argument_type_not_assignable)
var result = methodCall(_$foo);

Are there plans to silence invalid_assignment/argument_type_not_assignable as well? We could work around it with explicit casts, but it would be really nice to not have to.

scheglov commented 3 years ago

Well, that's a complication. Any unresolved identifier gets the dynamic type. And later we don't know if something was declared as dynamic, or was given dynamic because of a previous error. This is not different from Foo foo = resolved.unresolved();.

bwilkerson commented 3 years ago

At one point we had an "undefined" type that was distinct from dynamic so that we could distinguish between these two cases. I seem to remember that we removed it because it caused complications in the implementation, but I don't remember what kind of complications specifically.

greglittlefield-wf commented 3 years ago

So, I assume re-implementing a distinction between the two cases is out of the question?

That aside, just to make sure I have a correct understanding of what the path forward for us currently looks like...

We'd be going from what we have now:

UiFactory<FooProps> Foo = _$Foo; // ignore: undefined_identifier, invalid_assignment

...to utilizing the latest implementation added in that commit and either:

// A: Adding an implicit cast.
UiFactory<FooProps> Foo = _$Foo as UiFactory<FooProps>;

// B: Adding an implicit cast and removing the LHS typing to avoid
//    having to repeat the type.
final Foo = _$Foo as UiFactory<FooProps>;

// C: Using a helper method that performs a cast without having to repeat the type.
T cast<T>(dynamic value) => value as T;
UiFactory<FooProps> Foo = cast(_$Foo);

old incorrect option B for posterity

```dart // B: Adding an implicit cast and removing the LHS typing to avoid // having to repeating the type. // But, this would conflict with the type_annotate_public_apis Effective Dart lint, // so we probably wouldn't pick it. var Foo = _$Foo as UiFactory; ```

(It might be worth noting that having the cast isn't ideal because it prevents static type-checking from occurring once the generated field is present.)

While having to perform that explicit cast isn't ideal, if we can eventually replace them with external variables like what was proposed in this comment https://github.com/dart-lang/sdk/issues/42977#issuecomment-671555938, then having to live with this until then seems acceptable.

And sorry for bringing up the invalid_assignment issue last-minute! We haven't turned off implicit casts in most of our repos, so it wasn't top of mind 😓 .

kevmoo commented 3 years ago

Are you sure about B: ?

For top-level final fields, I believe this is fixed!

On Thu, Oct 15, 2020 at 4:39 PM Greg Littlefield notifications@github.com wrote:

So, I assume re-implementing a distinction between the two cases is out of the question?

That aside, just to make sure I have a correct understanding of what the path forward for us currently looks like...

We'd be going from what we have now:

UiFactory Foo = _$Foo; // ignore: undefined_identifier, invalid_assignment

...to utilizing the latest implementation added in that commit and either:

// A: Adding an implicit cast. UiFactory Foo = _$Foo as UiFactory;

// B: Adding an implicit cast and removing the LHS typing to avoid // having to repeating the type. // But, this would conflict with the type_annotate_publicapis Effective Dart lint, // so we probably wouldn't pick it. var Foo = $Foo as UiFactory;

// C: Using a helper method that performs a cast without having to repeat the type. T cast(dynamic value) => value as T; UiFactory Foo = cast(_$Foo);

(It might be worth noting that having the cast isn't ideal because it prevents static type-checking from occurring once the generated field is present.)

While having to perform that explicit cast isn't ideal, if we can eventually replace them with external variables like what was proposed in this comment #42977 (comment) https://github.com/dart-lang/sdk/issues/42977#issuecomment-671555938, then having to live with this until then seems acceptable.

And sorry for bringing up the invalid_assignment issue last-minute! We haven't turned off implicit casts in most of our repos, so it wasn't top of mind 😓 .

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/dart-lang/sdk/issues/42832#issuecomment-709645003, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAEFCW2RBN6CIKN7DRE5P3SK6B4ZANCNFSM4PHCHT6A .

greglittlefield-wf commented 3 years ago

Oh you're right: making it final gets rid of the type_annotate_public_apis lint! Nice!! I'll update that example

-var Foo = _$Foo as UiFactory<FooProps>;
+final Foo = _$Foo as UiFactory<FooProps>;

dart-lang / sdk

Analyzer regression around diagnostics when imported files are not generated #42832