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

Propagate @GwtIncompatible to generated Factory classes? #121

Closed
cpovirk opened this issue Feb 10, 2015 · 3 comments
Closed

Propagate @GwtIncompatible to generated Factory classes? #121

cpovirk opened this issue Feb 10, 2015 · 3 comments

Comments

@cpovirk
Copy link
Member

cpovirk commented Feb 10, 2015

(This is the Dagger equivalent to google/auto#187)

Motivation: I am trying to get Guava to start relying on the GWT compiler's new automatic stripping, rather than doing its own @GwtIncompatible stripping. This means that the Java compilation that we do for GWT is now seeing @GwtIncompatible classes. The GWT compiler proper then ignores then. But because javac saw them, Dagger generated Factory classes for those with @Inject constructors. Those Factory classes refer back to the @GwtIncompatible original, which the GWT compiler can't see. We get a compilation error:

[ERROR] Errors in 'blahblah.jar/com/google/common/time/DefaultSleeper$$Factory.java'
   [ERROR] Line 12: The constructor DefaultSleeper() is not visible

(Google developers can check out the full results.)

A solution (I think) is for Dagger to annotate the Factory class as @GwtIncompatible if the original class (or any enclosing class) is so annotated. Does this sound reasonable?

@cgruber
Copy link

cgruber commented Mar 26, 2015

This does sound reasonable as an approach.

@cgruber
Copy link

cgruber commented Mar 26, 2015

I'm going to mark this as a 2.1 milestone though, but we can chat about it if we need it for our 2.0 release shortly. I think the solution here is sort of subtle - we could special case gwt, but i'm hesitant to do that, since we have the same problem with j2objc, etc. But a comprehensive solution seems like it requires some semantic clarity I not fully certain we can get in time to fix it for 2.0.

2.1 should follow fairly soon after, though.

@cgruber cgruber added this to the Dagger 2.1 Release milestone Mar 26, 2015
@gk5885 gk5885 removed this from the Dagger 2.1 Release milestone Nov 9, 2015
@gk5885
Copy link

gk5885 commented Nov 9, 2015

This is pretty trivial and we should absolutely do that.

ronshapiro added a commit that referenced this issue May 18, 2017
Fixes #121 (finally)

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=156005530
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants