dart-lang / source_gen

Automatic source code generation for Dart
https://pub.dev/packages/source_gen
BSD 3-Clause "New" or "Revised" License
484 stars 105 forks source link

GeneratorForAnnotation throws if there is an unresolved annotation in the file #682

Closed rrousselGit closed 1 year ago

rrousselGit commented 1 year ago

Hello!

It appears that GeneratorForAnnotation uses library.annotatedWith(typeChecker). But it does not specify throwOnUnresolved: false.
So if the file contains an unresolved annotation, that causes an error to be thrown and shown to users in the terminal.

jakemac53 commented 1 year ago

I am not sure exactly why we chose to throw by default here, but it was an explicit choice, so I don't know that we should swap it without understanding why it was set that way originally. For the majority of use cases I wouldn't expect to see unresolved annotations. Are these code generated later on? Can we just change the builder ordering to resolve it?

We definitely should at least provide an option to pass a custom value for throwOnUnresolved though either way.

rrousselGit commented 1 year ago

Users of my packages saw it happen a few times while editing files.

I think the unexpected behavior is the error message. It likely feels like the generator failed. Whereas it's likely that the user is missing an import.

jakemac53 commented 1 year ago

Whereas it's likely that the user is missing an import.

They should have an error in their IDE in that case?

jakemac53 commented 1 year ago

My main concern with making this the default is just that we could be skipping over annotations that should have been targeted silently. So that will lead to a different kind of confusion. Presumably that is why the default is to throw?

rrousselGit commented 1 year ago

I think my users complained about a race condition.

I know that there are cases where previously generated code is picked up by the analyzer even though it should be deleted. If there's an annotation in there, it could possibly cause this error

jakemac53 commented 1 year ago

I know that there are cases where previously generated code is picked up by the analyzer even though it should be deleted.

This shouldn't be the case. It is possible to be able to resolve certain identifiers that you shouldn't be able to though. This could lead to something being an error in one build and not the next in theory, where the error was essentially missed in one of the builds? This particular error could throw somewhat unreliably in this case.

rrousselGit commented 1 year ago

I think that's the case yes. My users mentioned that the error goes away when restarting generation

jakemac53 commented 1 year ago

cc @kevmoo @natebosch wdyt about changing the default here?

kevmoo commented 1 year ago

🤷

rrousselGit commented 1 year ago

I actually have a reliable way of reproducing this error. It seems to be introduced by a recent change in my generated code, which now generates an @annotation for custom linting purposes.

TL'DR, when a user defines:

@riverpod
Object? example(ExampleRef ref, int arg) => 0;

This generates (amongs many things):

@ProviderFor(example) // Pass the user-defined symbol to help the linter find the user code
const exampleProvider = ExampleFamily();

The problem: Whenever the user renames example to something else, that annotation ends-up being unresolved (since the symbol example no-longer exists).

This forces folks to delete the .dart_tool folder and restart, at which point things work again.

jakemac53 commented 1 year ago

Whenever the user renames example to something else, that annotation ends-up being unresolved (since the symbol example no-longer exists).

Just so I understand correctly - is what you are seeing that declarations from the previously generated part file (which shouldn't exist yet in this new build) - are still present? So then it fails on those unresolved annotations, which shouldn't actually be there at all?

rrousselGit commented 1 year ago

Correct. The error is about to go away as soon as generation completes. And the error happens before generation, while also preventing builders from fixing the issue.

So the only way out is to restart from a blank state

rrousselGit commented 1 year ago

For what it's worth, I've already forked GeneratorForAnnotation a while ago anyway. So this isn't critical to me and I am able to fix this on my own. If you feel like that's not important, I don't mind closing this as I wouldn't have the issue anymore.

jakemac53 commented 1 year ago

This looks like a legitimate bug, so I do appreciate the time you have spent diagnosing and giving a repro case and I do have a fix pending 👍