google / dagger

A fast dependency injector for Android and Java.
https://dagger.dev
Apache License 2.0
17.45k stars 2.02k forks source link

Dagger generates invalid code #3091

Open h908714124 opened 2 years ago

h908714124 commented 2 years ago

Dagger generates invalid code if auto-value annotations are present, but auto-value processor is not configured as an annotation processor. This can lead to compile failures that are hard to understand.

Also the error message @AutoAnnotation is a necessary dependency if @MapKey(unwrapValue = false). Add a dependency on com.google.auto.value:auto-value:<current version> in MapKeyValidator is misleading, because

See demo

bcorso commented 2 years ago

Thanks, those instructions are from a while ago when auto-value was a single artifact and auto-value-annotations didn't exist yet. Looks like we need to update this error message with the new artifacts.

I'll send out a fix soon.

h908714124 commented 2 years ago

Hi bcorso, good idea. The error message should be changed, but changing the error message alone would not fix this issue. Dagger would still generate invalid code.

The demo project would still fail with a "cannot find symbol" error like this:

/workspace/print-annotation-mirrors/build/generated/sources/annotationProcessor/java/main/mapkeys/MapKeys_ComplexKeyCreator.java:24: error: cannot find symbol
    return new AutoAnnotation_MapKeys_ComplexKeyCreator_createComplexKey(manyClasses, oneClass, annotation);
               ^
  symbol:   class AutoAnnotation_MapKeys_ComplexKeyCreator_createComplexKey
  location: class MapKeys_ComplexKeyCreator

The issue here is that the error message is not printed at all, and invalid code is generated instead, and this would still be the case after changing the error message.

To fix this issue, consider one of the following options:

bcorso commented 2 years ago

The issue here is that the error message is not printed at all, and invalid code is generated instead, and this would still be the case after changing the error message.

In this case, the reason you don't see the error message is because your demo project adds the auto-value-annotations dependency but not the processor. Dagger assumes that if it detects the Auto-Value annotations in the classpath then you've properly setup the processor for it as well.

Assuming that you initially have neither dependency you should see the Dagger error message, which should now tell you to add both the auto-value-annotations dependency and the auto-value processor dependency.

Fwiw, we may also follow up with a more automatic fix that either adds the auto-value-annotations/auto-value dependencies to Dagger's pom files or shades them into Dagger, but we're still deciding on that.

h908714124 commented 2 years ago

So is this fixed now? What's the fix version?

bcorso commented 2 years ago

The error message was updated in https://github.com/google/dagger/commit/eeb93c920c7f30ce5b5e2f480a4c5ea6834c9bde (you can scroll up to see related commits).

The changes are not yet in a release, but you can test out the change with the HEAD-SNAPSHOT artifacts if you want.

h908714124 commented 2 years ago

Seems like this ticket was closed, but the problem was not fixed.

bcorso commented 2 years ago

Sorry, I considered this more of an issue with improper setup of auto-value (on the user side) due to a misleading error message (on the Dagger side). Understandably, you could argue that the user shouldn't have to add these dependencies manually, but this is currently working as intended to avoid adding the auto-value dependencies to all users who aren't using the feature.

That said, I'm currently discussing with the team on ways we can improve this experience, e.g. by reevaluating adding the auto-value dependency (which should be pretty small now that the annotations have their own artifact), shading auto-value into Dagger, or just reimplementing that part ourselves.

I'm happy to leave this issue open until we make a decision on that.