Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Dagger generates invalid code #3091

Open
h908714124 opened this issue Dec 3, 2021 · 7 comments
Open

Dagger generates invalid code #3091

h908714124 opened this issue Dec 3, 2021 · 7 comments

Comments

@h908714124
Copy link

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

  • AutoAnnotation is not in artifact auto-value but in auto-value-annotations
  • Having AutoAnnotation on the classpath does not imply that auto-value is also working as an annotation processor

See demo

@bcorso
Copy link

bcorso commented Dec 14, 2021

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
Copy link
Author

h908714124 commented Dec 14, 2021

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:

  • Defer processing until the referenced auto-value class actually exists, OR
  • Stop relying on auto-value and make dagger generate the necessary code itself, OR
  • Otherwise remove or deprecate the unwrapValue option in the MapKey annotation, if its flawless operation can't be guaranteed.

copybara-service bot pushed a commit that referenced this issue Dec 16, 2021
This CL updates the error message to reference the new auto-value artifacts.

Fixes #3091

RELNOTES=Fixes #3091: Update MapKey error message to reference new auto-value artifacts.
PiperOrigin-RevId: 416180376
@bcorso bcorso reopened this Dec 16, 2021
@bcorso
Copy link

bcorso commented Dec 16, 2021

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.

@bcorso bcorso closed this as completed Dec 16, 2021
@h908714124
Copy link
Author

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

@bcorso
Copy link

bcorso commented Dec 17, 2021

The error message was updated in eeb93c9 (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
Copy link
Author

h908714124 commented Dec 17, 2021

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

@bcorso
Copy link

bcorso commented Dec 17, 2021

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.

@bcorso bcorso reopened this Dec 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants