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

2.12: Eclipse compilation errors #949

Closed
DelRed2 opened this Issue Nov 14, 2017 · 13 comments

Comments

Projects
None yet
5 participants
@DelRed2
Copy link

DelRed2 commented Nov 14, 2017

Hello, this bug is the same as #896
Actually, it is not a bug of dagger, but of eclipse.

Another bug of eclipse, that can be reproduced with dagger codegen (#380) was reported to eclipse (https://bugs.eclipse.org/bugs/show_bug.cgi?id=513567) but it was not fixed for 8 months. This bug can be simply avoided.

The reported bug can not be simply fixed without fixing generated code. (or changing @BINDS to @provides, but it is not a right way)

I think, it will be right to change dagger codegen of Component - it should always cast Factory to Provider like this:
this.someProvider = DoubleCheck.provider((Provider)Some_Factory.create((Provider) dependency));
instead of this:
this.someProvider = DoubleCheck.provider(Some_Factory.create((Provider) dependency));
All Eclipse users will say thank you :)

@DelRed2

This comment has been minimized.

Copy link

DelRed2 commented Nov 14, 2017

@stephan-herrmann

This comment has been minimized.

Copy link

stephan-herrmann commented Nov 14, 2017

There may or may not be a bug in the Eclipse compiler. Unfortunately, just answering this question requires significant efforts in each individual case (the specification is sufficiently complex, and comparison against javac is not reliable, either). IMHO, development effort is much better invested in developing and polishing new language features, rather than trying to repair, what was intended only as a short-term, interim workaround for migration of Java 1.4 code. Raw types are a broken concept in Java 8+, and should be removed from the language as it was announced right from the beginning. For those reasons I beg the dagger developers, to avoid raw types in generated code (and thus avoid the impression you are generating code for the year 2003 or earlier ;p ).

@ronshapiro

This comment has been minimized.

Copy link

ronshapiro commented Nov 14, 2017

Raw types are unfortunately necessary in some cases - consider when you have a package-private binding in package pkg.a and your component is in pkg.b. If we need a Provider for that binding, it must be a rawtypes provider, otherwise the code won't compile. When we can, we do avoid raw types. If you're curious, read through some of our tests and you'll see when we do/don't avoid them.

Raw types are a broken concept in Java 8+, and should be removed from the language as it was announced right from the beginning.

I don't know what you're referring to.

@stephan-herrmann

This comment has been minimized.

Copy link

stephan-herrmann commented Nov 14, 2017

... consider when you have a package-private binding ...

I know close to nothing about dagger, but this sounds like a strange design to me. If visibility prohibits the use of generics then the problem must be solved at the level of visibility, not by relying on tricks to avoid mentioning the inaccessible type. I don't have sufficient background to judge the importance of supporting package-private bindings that need to be accessed outside the package, though.

I don't know what you're referring to.

Regarding "broken": raw types defeat the improved type inference in Java 8 (and obviously raw types imply that "type-correct" code can throw type errors at runtime, which is totally against the spirit of a statically typed language like Java).

Regarding removal, see JLS §4.8: "The use of raw types is allowed only as a concession to compatibility of legacy code. The use of raw types in code written after the introduction of generics into the Java programming language is strongly discouraged. It is possible that future versions of the Java programming language will disallow the use of raw types."

@ronshapiro

This comment has been minimized.

Copy link

ronshapiro commented Nov 14, 2017

The simple case is something like this:

pkg a;

class Foo {
  @Inject Foo(Set<Object> multiboundObjects) {}
  @Override public String toString() { return multiboundObjects.toString() + getClass().getName(); }
}
pkg a;

public class Bar {
  @Inject Bar(Provider<Foo> dependencyOnFoo) {}

  public void doStuff() {
    System.out.println(dependencyOnFoo.get().toString());
  }
}

Bar is exposed API, so it can be referred to code outside of it's package. Foo is package private, an implementation detail of Bar, but can have dependencies of it's own that come from other public types. So we can't generate everything relative to Foo's package.

You're right that using raw types is not ideal, but we don't have much of a choice. I would be very surprised if that statement in the JLS was actually enacted - if for nothing else, it supports things like Collections.emptyList(), at least without a replacement that would be suitable.

@stephan-herrmann

This comment has been minimized.

Copy link

stephan-herrmann commented Nov 14, 2017

What's wrong with <T> List<T> Collections.emptyList()?
You mean the implementation using List EMPTY_LIST? That guy should simply use a wildcard to get rid of the raw type :)

To be fair, there is one case where raw types are unavoidable, unfortunately: class literals of generic classes.

@ronshapiro

This comment has been minimized.

Copy link

ronshapiro commented Nov 14, 2017

To get back on track: we only test with javac and we assume that if it works there it should work everywhere. I'm also not an expert on the type system wrt raw types, but from my understanding this should all work.

We're not inclined to start including extra casts all over the place to support a compiler that we can't easily test, and we especially don't want multiple compilation modes.

In the past we've seen other issues with ECJ and given that jack is no longer supported by Android, we assume that it's not worth the effort.

It's possible that we could start using wildcards everywhere, but that would be an enormous undertaking that we don't have time for, nor is it clear that it will in fact work.

Another workaround is making your types public - it's obviously not ideal but if you put a comment it should allow you to move on.

@ronshapiro ronshapiro closed this Nov 14, 2017

@DelRed2

This comment has been minimized.

Copy link

DelRed2 commented Nov 15, 2017

@ronshapiro it is not a problem of publicity (it is not a workaround), it is another problem:
let class B implements IB
and make binding B for IB, and inject IB into A:
dagger generates A_Factory::create method as:
Factory<A> create(Provider<IB> bProvider)
but in DaggerContainer there is a field Provider<B> bProvider
so, you can not pass bProvider to A_Factory.create() without cast to RawType Provider

The better fix is to make Factory method create interface like create(Provider<? extends IB> bProvider)

@DelRed2

This comment has been minimized.

Copy link

DelRed2 commented Nov 15, 2017

@ronshapiro will you accept pull-request with fixing Factory::create method interface with replacing Provider to Provider<? extends A>?
If you will, I can try to fix it myself.

@ronshapiro

This comment has been minimized.

Copy link

ronshapiro commented Nov 15, 2017

You're welcome to try. I imagine it will be more difficult than you perceive, but I'm happy to review it if you can get it working.

@rluble

This comment has been minimized.

Copy link

rluble commented Nov 16, 2017

FYI, this is the bug in JDT related to the situation above.

https://bugs.eclipse.org/bugs/show_bug.cgi?id=513567

facboy added a commit to facboy/dagger that referenced this issue Nov 17, 2017

facboy added a commit to facboy/dagger that referenced this issue Nov 17, 2017

facboy added a commit to facboy/dagger that referenced this issue Nov 22, 2017

@olibye

This comment has been minimized.

Copy link

olibye commented Dec 1, 2017

Thanks @facboy your 2 line fix in PR #957 solves this problem for me in Eclipse Oxygen 1A JDT 2.13.2.v20171122-0652 running in Java8.

I hope the package maintainers merge and release it soon.

@rluble

This comment has been minimized.

Copy link

rluble commented Dec 8, 2017

Updated https://bugs.eclipse.org/bugs/show_bug.cgi?id=513567 to include the workaround proposed in PR #957.

@stephan-herrmann, could you comment if you see any problem with this workaround?

ronshapiro added a commit that referenced this issue Dec 13, 2017

Use more general type for DoubleCheck.provider() (and SingleCheck) to…
… satisfy Java 9 Eclipse JDT type inference quirks

Fixes #949
Closes #957

RELNOTES=Fixed a compilation issue with eclipse

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=178773557

ronshapiro added a commit that referenced this issue Dec 14, 2017

Use more general type for DoubleCheck.provider() (and SingleCheck) to…
… satisfy Java 9 Eclipse JDT type inference quirks

Fixes #949
Closes #957

RELNOTES=Fixed a compilation issue with eclipse

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