undefined_identifier and other errors are no longer ignorable, causing issues for generated parts

[greglittlefield-wf]

As of Dart 2.9.0, the undefined_identifier error is no longer ignorable, along with mixin_of_non_class, undefined_class, undefined_identifier, and invalid_assignment .

This is problematic for the over_react use-case, in which we generate part files via a build-to-cache builder (meaning the files aren't checked in), and reference some generated members in those files from the original file.

For example:

// source file: foo.dart

import 'package:over_react/over_react.dart';

part 'foo.over_react.g.dart';

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

...

// generated file: foo.g.dart

part of 'foo.dart';

UiFactory<FooProps> _$Foo = ...; 

...

We use // ignore: undefined_identifier (as well as project-wide ignores of uri_has_not_been_generated) so that the source files can analyze cleanly when the generated file doesn't exist or when the generated file is out of date.

In Dart 2.9.0, these ignore comments no longer work, so over_react projects cannot analyze cleanly without first running a build, which is a pretty big hit to the dev experience in IDEs.

We also have some other deprecated boilerplate code that references generated members in other ways:

class FooProps extends _$FooProps
    with
        // ignore: mixin_of_non_class, undefined_class
        _$FooPropsAccessorsMixin {
  // ignore: undefined_identifier, undefined_class, const_initialized_with_non_constant_value
  static const PropsMeta meta = _$metaForFooProps;
}

We're in the process of migrating off of this pattern and moving toward that single undefined_identifier ignore, but we still have a bit of it in our code bases.

This is also closely related to #42832; if the analyzer knew these APIs were generated and could suppress errors without needing ignore comments, that might help solve this problem. However, we'd still have the issue of warnings in outdated generated files.

[greglittlefield-wf]

I did some more research and found that the issue is that 2.9.0 made it so that errors can no longer be ignored... #27218

[robbecker-wf]

@kevmoo @pq @devoncarew @greglittlefield-wf
Look like @zoechi was right about removing the ability to ignore errors causing problems with generated code (link #27218 (comment))

I'm not sure what our path forward is here. Right now we're blocked from upgrading to 2.9.0. Possible options / workarounds to discuss:

• Allow ignoring errors in generated files 🤔
• Allow downgrading errors to warnings in analysis_options.yaml 😒
• Revert the change of not being able to ignore errors 😬
• We change over_react builders to build to source 😱
• ... something else .. [insert idea here]

Edit: I'll add this isn't a hair on fire emergency .. we can take some time to get the right fix since we don't need to update to 2.9 today or anything like that.

[kevmoo]

CC @natebosch and @jakemac53 –

[devoncarew]

Allow downgrading errors to warnings in analysis_options.yaml

Does this work for you? If you change the severity of undefined_identifier to a warning, does that work - does the analysis server respect the new severity, or does it stay as an error?

https://dart.dev/guides/language/analysis-options#changing-the-severity-of-rules

[natebosch]

so that the source files can analyze cleanly when the generated file doesn't exist or when the generated file is out of date.

How feasible is it to tolerate errors when the generated files are out of date? Just to make sure we're looking at the whole problem - what are the critical places where there is no tolerance for false positives for diagnostics and can we ensure that the generate files are use in those places? Is there something we can do to make it easier to analyze with generated to cache files on c.i. infrastructure?

[robbecker-wf]

@devoncarew Currently it does not work to try to downgrade to non-error. That would require a code change in dart-land. I'm guessing this solution isn't great because it technically opens up a loophole for people to ignore legit errors.

[greglittlefield-wf]

From @robbecker-wf:

... something else .. [insert idea here]

Another potential long-term solution if we can't ignore errors moving forward would be to utilize namespaced imports, which are treated as dynamic accesses when the file has not been generated.

final Foo = generated_lib.Foo as UiFactory<FooProps>;

We'd probably end up still using parts so that we'd be able to access private members of the source file, so we'd have both a generated part and a generated import.

We'd also need casts to get rid of errors, and we'd still have the same problem when the generated file is outdated

So, this probably isn't the best path forward, but I thought I'd throw it out as an option

[greglittlefield-wf]

How feasible is it to tolerate errors when the generated files are out of date?

@natebosch I think it wouldn't be critical for CI, since we could just always regenerate files before we perform analysis.

I think the bigger issue is when developing locally; if a consumer is authoring some code and there's an error resulting from out of date generated code, that could get pretty confusing. This never used to be an issue for this builder, and the generated files use build_to: cache, so it might not be immediately obvious that the solution is to rerun a build.

[jakemac53]

Longest term, this might be a good use case for external. I had worked up a bit of a proposal and even a prototype that allowed external to be used by any library, and then you would generate patch files which have the actual implementation and are read in by kernel.

Is that something that would work for over_react?

As an example of the first type of error you listed:

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

would become:

external UiFactory<FooProps> Foo;

This doesn't reference any unknown symbols, and the analyzer is already totally ok with it without any changes. As long as you can describe your generated things in terms of external fields, methods, etc, then I think it actually solves a lot of potential issues.

[aaronlademann-wf]

Forgive my directness, but in reading through #27218 - I don't come away with a clear understanding of what value is added for consumers by not allowing errors to be ignored. Is there some critical roadmap feature that necessitated this change?

To me, this seems like something that should be a build flag - since it is mostly a build-time concern IMO.

[jakemac53]

It doesn't appear like code generation was seriously considered in the original thread as a potential valid use case for ignoring errors, it was brought up briefly but not really discussed.

[greglittlefield-wf]

The external solution would be perfect for this use-case!

[kealjones-wk]

I also really like the sound of that external solution. Seems like it has a lot of potential to help cases of generated code that have felt awkward for a long time.

[eernstg]

@jakemac53 wrote:

I had worked up a bit of a proposal and even a prototype
that allowed external to be used by any library

Note that declarations like external UiFactory<FooProps> Foo; are just about to be supported: #42560. Basically, an external variable is just a shorthand for an external getter and possibly a setter.

The mechanism that binds an external declaration to an implementation (for the external variable Foo it would be a getter and a setter) is implementation specific, so it could be used by a compiler to associate the external entity with Dart declarations (rather than JavaScript or native code). Patches already use external for this purpose, and they specify the binding in a different library using @patch and native, so there's a need to specify the binding of external declarations to Dart declarations, and a need to specify the library L containing the target external (or we could simply require that those bindings must be in a part of L). So I believe that the new external variable feature would be able to coexist just fine with a patch-like use of external.

[aaronlademann-wf]

@jakemac53 @devoncarew @natebosch @eernstg

The external solution sounds promising - but it doesn't sound imminent as a viable fix for this breakage - and would still require us to manually migrate many hundreds of components at Workiva.

As far as the immediate future is concerned, I'd very much like to understand more about what the driver behind the change was. The change was breaking for certain generated code use cases, and then released in a minor release after the generated code implications were "brought up briefly but not really discussed".

To aid in our planning efforts at Workiva for upgrading past Dart 2.9.x, it would be helpful to know where the Dart team stands on this change:

---

1. The change was very intentional - with long term implications for core features on the roadmap.

   The change cannot, and will not be reverted ever, nor can it be made optional with something like a compiler flag.

2. The change is widely viewed as a resounding improvement to the language, but doesn't underpin any future roadmap changes.

   The change could be reverted or be made optional - but it is unlikely.

3. The change doesn't underpin any future roadmap changes, and in hindsight was possibly made too quickly without proper considerations for all use-cases.

   The change could, and probably should be rolled back - and either re-implemented in a major release, or along with a compiler flag to disable/enable it.

---

Knowing where the core team stands on this will help us a great deal to know what we should do now and in the near future - before something like external is supported for our use-case - and we have the time and resources to roll out a change like this to our rather large Dart ecosystem at Workiva.

cc/ @robbecker-wf @greglittlefield-wf @evanweible-wf @alan-knight @kevmoo

[jakemac53]

Another potential approach to making external work with regards to codegen specifically, is to support doing the actual patching in the build system(s). We could add an explicit api for emitting patches to an existing Dart asset, and apply the patch ourselves before invoking the compilers.

This is something that could be done without I think any language specification or compiler effort.

EDIT: This is probably a horrible idea as it would require all consumers of packages using codegen in this way to run build_runner themselves, which isn't the case today for build to source builders.

[devoncarew]

Hi @aaronlademann-wf - this change is likely closest to your 2nd bullet above. It was made intentionally; we had some use cases internally where we did not want to allow users to ignore errors, mostly around some security audits. In rolling out the change, we found other places where people had ignored errors that were expedient at the time but caused problems down the road (things like allowing two different implementations of dart:ui to have different public APIs). Additionally, I think philosophically we don't want errors to be ignorable anymore (and, wished they hadn't been originally); I think this allows tech. debt to build up downstream in various parts of the ecosystem.

It's definitely true that we didn't think through all the implications of this for use cases like code generation; sorry for the breakage here.

in which we generate part files via a build-to-cache builder

I think I'm less familiar with the toolchain you're using to generate code. package:build_runner / the source gen tool chain won't emit code if there are errors in the source files?

We change over_react builders to build to source

It does sounds like there are some options to handle the breaking change, even if they're less appealing.

I think it would also be possible to exclude targeted files from analysis. That's a pretty big hammer in this case, so hopefully a tool of last resort.

[jakemac53]

We change over_react builders to build to source

It does sounds like there are some options to handle the breaking change, even if they're less appealing.

This is likely not a real option, fwiw. At least for angular it definitely would not be - it would require either maintaining compatibility between different versions of the generated code.

[aaronlademann-wf]

@devoncarew thank you for your response!

I think I'm less familiar with the toolchain you're using to generate code.

This is our builder we use for React component factories: https://github.com/Workiva/over_react/tree/master/lib/src/builder

@greglittlefield-wf also commented on #42832 with some additional context about how we name / reference generated members.

I think philosophically we don't want errors to be ignorable anymore

Can we agree however that - making them not ignorable is a truly breaking change (with no workaround or prior deprecation to give us a heads up) better suited for a major release? Is it truly not feasible to make this optional in the analyzer / builder?

[bwilkerson]

I think it would also be possible to exclude targeted files from analysis.

See #42832 for a request to improve analyzer's handling of ungenerated code, which is a related topic.

[aaronlademann-wf]

@devoncarew also, for some background/context: at Workiva - @greglittlefield-wf and myself (and others) work on libs consumed by many product teams. Certainly not at the scale of the entire Dart SDK - but its a large set of heavily used libraries.

Philosophically we don't ever want to take on tech debt as a team in our libs - but we also can't just remove a ton of it when its public API without first deprecating it, and communicating clear paths forward for the consumers.

IMO, there's a balance that needs to be struck between philosophy and practicality. We're extremely empathetic to how hard it can be to strike that balance - but feel that this change in particular falls short of a reasonable expectation that breaking changes won't appear without migration paths in minor releases.

[natebosch]

@devoncarew - would it be feasible to add a configuration in analysis_options.yaml that goes back to the old behavior?

[kwalrath]

The dart.dev infrastructure relies on code that has known errors, so this change broke our build and (because it reduces the amount of code we can test) might impact our migration to null safety.

We use bad code in a few ways:

• Sometimes the code is an example of what not to do.
• Sometimes the code is an example of something that used to work, but doesn't anymore. In this case, we sometimes include the up-to-date analyzer output, as well as the code.
• Sometimes it's just a shortcut; we want a snippet that would be valid as part of a larger code sample, but we don't want to show (or implement) the whole sample.

See dart-lang/site-www#2575 for the work we're doing to try to make analysis work again.

[mraleph]

/fyi @leafpetersen

[aaronlademann-wf]

@devoncarew any update on a path forward here? Even just an answer to @natebosch's question about feasibility would help us with some planning items on our end.

[devoncarew]

Hey @aaronlademann-wf, here are my thoughts:

• we (for a few reasons mentioned above) don't want to support the use case of ignore error level severity items, so this change is highly unlikely to be reverted
• I'd prefer not to add a way to re-enable this behavior (via user opt-in, say in the analysis options file). A few reasons for that are that we don't want to support the use case generally; I'd prefer not to add more complexity to our analysis - each little feature can add up, and interact with other features, and require real effort to support and maintain; and, we'd want to deprecate this at some point, which itself would be a breaking change

I'm still not sure of the exact part of your workflow where this is breaking - are you seeing analysis errors in IDEs in code that had previously been error free? Is that an issue for developers or for your CI? Does build_runner not generate code when it encounters analysis errors? I'm not sure if this is affecting the IDE experience primarily, the CI one, or code gen.

I do think you have a few options going forward. You might be able to generate to source instead of generate to cache. You could adjust the analysis options files to not analyze files w/ certain path name matches. You could adjust the severity of the undefined_identifier analysis issue from an error to a warning or info. That last one is likely the least disruptive; from experimentation, you still wouldn't be able to ignore the analysis issue, but it would help w/ IDE presentation.

analyzer:
  errors:
    undefined_identifier: warning

Thanks for your patience here! I understand this was a breaking change for you; in hindsight - now knowing that people would be non-trivially impacted - we would have communicated this change better (ala https://github.com/dart-lang/sdk/blob/master/docs/process/breaking-changes.md).

[aaronlademann-wf]

@devoncarew thanks for your reply.

The problem with downgrading an error like this:

analyzer:
  errors:
    undefined_identifier: warning

Is that it downgrades it everywhere - not just in the places where we specifically know that the identifier will be generated. Thus our current use of // ignore comments.

... "we don't want to support the use case generally; I'd prefer not to add more complexity to our analysis"

I'm curious how allowing the error to be downgraded project-wide is different from a feature-support standpoint than continuing to support the inline ignore comments?

Also - I would submit that allowing errors to be downgraded project wide represents a far bigger "anti-pattern" for project authors than targeted // ignore comments do. To me, keeping support for the project-wide downgrades while disabling the comment-based ignores feels... arbitrary.

[jakemac53]

If the analyzer were a compiler, I would totally agree that we should not allow ignoring of errors. But, it isn't a compiler. It does not produce machine code or translate dart code into any runnable format. Therefore, ignoring these errors does not make code any less safe. It only allows users who know that some error will be resolved before it actually gets to a compiler to tell the analyzer "hey, I know better than you please ignore this". That seems pretty reasonable to me as a development time feature, and we have had listed here several legitimate use cases for doing exactly this.

I think it is also telling that if you look through this issue and the original one that lead to this change, you will not find a single external user in favor of this change. Not one. And plenty of them that think it is a bad idea.

Can somebody list the concrete failure scenarios where allowing users to ignore errors in the analyzer only would cause a problem?

[robbecker-wf]

So, there seems like there's still some disagreement and unresolved issues on this thread.

• Looking at some of the tickets, it looks like some of the external work has landed. Is it ready to use to work around this issue? @jakemac53
• Maybe I missed it but it seems like @kwalrath has an unresolved use case here as well.
• If external isn't a viable workaround I think we'll need to revisit and resolve the question of whether or not to allow the ability to ignore errors (in the analyzer only)

[jakemac53]

external is still not usable for code generators - it is still something that only the core implementations (compilers, runtimes) can fill in. We did recently extend I think where the external keyword can appear to support some ffi use cases or something like that, but it was unrelated to this.

[robbecker-wf]

I thought I'd recap where we are so that this issue doesn't get ignored. First off, we need a solution here as this issue is preventing us from upgrading to newer Dart versions.
The issue is with the analyzer and stems from the fact that the generated sourcecode does not exist yet. When the generated file is imported and a class is referenced, that class doesn't exist yet, so the analyzer now sees that as an error. We got around this previously by adding an ignore comment. Here's an example:

// ignore: undefined_identifier
UiFactory<ExampleProps> Example = _$Example; // _$Example comes from the generated file

The issue does not affect CI or building for production, since we run a build and build runner generates (to cache) the code needed to avoid the error. I'll list the Possible solutions that were presented and respond inline with where I think we're at.

• extend the ability of external to apply to our use case

I think this is a possibility but the work is behind finishing null safety and thus half a year away, which makes it not a viable solution to get us unstuck.

• Change the builder to generate to source instead of generate to cache

While this is technically possible it is extremely undesirable as it would limit our ability to easily roll out updated generated code by updating the builder. It would also be a large amount of work to rollout, which we would be difficult to prioritize. Basically, this isn't a viable solution either.

• Adjust the analysis options files to not analyze files w/ certain path name matches.

I don't think this would work since where these errors occur is in regular .dart files that don't adhere to a common naming pattern.

• Adjust the severity of the undefined_identifier analysis issue from an error to a warning or info.

analyzer:
  errors:
    undefined_identifier: warning

I verified that this does allow us to lower these to warnings (on Dart 2.10) and it does remove them as errors. It's less than ideal since

1. The analyzer now reports LOTS of warning that were not there before
2. other places where we do want the analyzer to catch this as an error also now show up only as warnings. Example:
   

This solution does sort of work, but it isn't ideal and would certainly leave a bad impression on our hundreds of Dart developers.

• Update the analyzer to allow ignoring errors only for the analyzer. When compiling if the identifier truly does not exist, it will still be an error.

This is our preferred solution since it would introduce no extra work. I think either a revert to how it was or an escape hatch via a config option to allow ignoring like before would be ok. Or maybe there's some other more elegant solution that both allows us to operate as we did before AND doesn't impose too much tech debt on the analyzer team.

/cc @kevmoo

[bwilkerson]

Update the analyzer to allow ignoring errors only for the analyzer.

That's the state we were in before the feature was restricted. The ignore comments have never been used by any tool other than the analyzer as far as I know.

One other possible idea: allow errors to be ignored only in files that import generated files. That assumes that we can tell from the URI that a file is generated, but analyzer already has a hardcoded list of patterns it uses for that purpose. If necessary we could extend that list, or we could make it possible to extend the list in the analysis options file.

/cc @devoncarew

[robbecker-wf]

It might be worth reiterating what the implications are for us if there are no changes to the SDK. We could:

• Remain stuck on 2.7.2 and have to use the workaround in 2.9.3 for Chrome 86 indefinitely
• Require developers to build after fresh clones, branch switches, and after making changes to over_react source files to avoid analysis errors
• Rethink our boilerplate undefined_identifier and other errors are no longer ignorable, causing issues for generated parts #42977 (comment) , likely making it more verbose and requiring a large effort on our end

[scheglov]

With https://dart-review.googlesource.com/c/sdk/+/166820 the analyzer will not report additional errors when there is a missing part with the URI that ends with .g.dart, and when the undefined identifier starts with _$. It should solve issues for fresh clones when parts are not generated.

[Hixie]

I think https://dart-review.googlesource.com/c/sdk/+/166820 is the strongest possible argument for why preventing errors from being ignored is a bad idea. We are actively taking on technical debt, making our codebase more complicated and subtle with obscure undocumented behavior, to work around the limitation we have put into our tool.

[bwilkerson]

I think https://dart-review.googlesource.com/c/sdk/+/166820 is the strongest possible argument for why preventing errors from being ignored is a bad idea.

I agree that we don't need to prohibit users from ignoring diagnostics with a severity of 'error'. If someone wants to suppress the error from the analyzer they're still not going to be able to run their code, so ultimately ignoring it is harmless.

I also understand the desire to disallow ignoring some diagnostics, especially critical security related diagnostics. I would propose that if we want to allow some diagnostics to be un-ignorable we should add a way to specify that in the analysis options file. That way groups of users can have the assurance they want / need without it being universally applied.

But the reason for the referenced CL is not because these errors can't be ignored it's because these errors proliferate when a generated file doesn't yet exist. Essentially, it was an attempt to reduce the noise without loosing the signal. I'm not convinced that we've gotten that trade off right yet either, but it is a problem for many users that needs to be addressed and ignoring these extraneous diagnostics isn't a tenable solution, hence the CL.

[Hixie]

Fair.