Fix bug when using javapoet with Eclipse compiler

[marcosb]

For the following code:

public interface Interface1 {}
public interface Interface 2 {}
public T myMethod(T input) {}

a code generator (javax.annotation.processing.Processor) in eclipse will have a TypeVariable for the return type T instead of a DeclaredType.

This results in a stack overflow exception, because the return value of Collections.singletonList(upperBound) ends up returning a list containing, in essence, the input parameter.

[swankjesse]

Care to add a test case?

[octylFractal]

Also, do you know about force pushing? You could have corrected your first PR.

[marcosb]

swankjesse: unfortunately I'm not sure a test case can be added; the problem is how the internal reflective structure of eclipse vs javac (which is why I broke the test).

In eclipse, "T extends A & B" is represented as a TypeKind.TYPEVAR with multiple upper bound elements (which actually seems like the more correct way to do this).

In javac, "T extends A & B" is represented as a TypeKind.DECLARED. Interestingly, it seems to represent "T extends A" as a TypeKind.TYPEVAR; seems really wrong that it represents these differently, but c'est la vie.

My updated commit fixes the test & keeps everything working properly.

[marcosb]

kenzierocks: sorry, haven't used github in a long time; just came here to commit this change which I have locally; I think I did the right thing with my updated bug fix.

[tbroyer]

In T extends A & B, A & B is an intersection type. In Java 7, this is represented by a DeclaredType (per spec, this is explicitly called out in the javadoc), whereas Java 8 introduced a proper IntersectionType

What you're saying here, is that Eclipse has a bug and represents intersection types as type variables (whether it “seems like the more correct way to do this” or not doesn't really matter, it's wrong, per spec).

Wrt testing, that's probably possible using an "integration test" (a whole project in src/it) that would use the Eclipse compiler within the maven-compiler-plugin.

[marcosb]

Ah cool, didn't realize it was part of the spec; yah, sounds like a bug in Eclipse then (though I can understand their confusion; if an intersection type is represented as a DeclaredType, then why would TypeParameterElement have a getBounds() which returns a list?)

I will admit that my familiarity with github is limited, and I'd have no idea how to add an eclipse dependency; it seems reasonable to have TypesTest run against both javac as well as the eclipse compiler.

Is there any chance you guys could take over, as you are more familiar? I have my own test against Eclipse which I can verify is fixed by this code, I just have no idea how to set that up here.

[swankjesse]

Yeah, I'd love to get this fixed regardless.

[tbroyer]

Judging from https://github.com/eclipse/eclipse.jdt.core/blob/bbfd43e75c85d178e78e2e1f2cdd86b56e672481/org.eclipse.jdt.compiler.apt/src/org/eclipse/jdt/internal/compiler/apt/model/TypeVariableImpl.java#L54-L67 JDT will actually return a new TypeVariable around the same internal TypeVariableBinding, so another way to detect an intersection type is to check whether the asElement() for both compare equal (we cannot compare the TypeVariable instances as type mirrors should be compared using Types#isSameType rather than equals()). I think it'd be a better way than simply looking at the size of the “upper bound's bounds” as proposed here, as it clearly identifies a bug, the Eclipse bug.

An alternative would be, according to the Java 7 javadoc, to not use TypeVariable#getUpperBound() and instead use Types#directSupertypes to get the individual upper bounds, but that would mean changing the API to pass in a Types instance (and this is only needed in JDK 7 and Eclipse, as JDK 8 has proper intersection types).

FWIW, the javadoc for TypeVariable talks about capture conversion (as in Types#capture()); and I have absolutely no idea what that actually means wrt. the return values of getUpperBound and/or asElement. I actually have no idea what Types#capture() can be useful for.

[marcosb]

I'm fine with whatever approach you guys feel is best; the difficulty I have with the code before my changes is that it is completely ignoring TypeParameterElement.getBounds() - even if the eclipse compiler is using it wrong, it is a field which has pertinent data, and it's not currently being processed.

It just so happens that it also fixes the eclipse bug, but I don't think the code as I have it is nonsensical/crazy, it's simply taking this data into account.

Whatever the case, the stack overflow exception in Eclipse isn't pretty, and so fixing this ASAP would be nice.

[marcosb]

Looking @ the javadoc:
http://docs.oracle.com/javase/7/docs/api/javax/lang/model/element/TypeParameterElement.html#getBounds()

"These are the types given by the extends clause used to declare this type parameter"

So, why have we complicated the code so much? Take a look at my latest commit; it works both in javac (all tests pass) + eclipse, and simplifies things a lot.

[tbroyer]

Well, I thought about it too, but was afraid we might loose some information when going from type mirrors to elements; but I think for type variables / parameters it's probably safe.

Maybe @eamonnmcmanus knows more about the details?

(the JavaC code is hard to follow, but IIUC, when getting type parameters of an element, it actually loops over it's type's parameter types and extract their underlying type parameter; which would mean that there's indeed a one-to-one relation between type parameter elements and type variables, and it's safe to just do an asElement().getBounds(); Eclipse code is easier to follow but we know it has bugs so…)

[marcosb]

@tbroyer TypesTest does a lot of checks based on this using javac, and they are all passing; if there's any information that can be lost that's not currently covered by TypesTest, it should probably be added there (so we can make sure this solution works)

[eamonnmcmanus]

I also think that it would be safe to go through TypeParameterElement. I have a vague recollection of doing that in AutoValue at some point, though I can't find it. Currently AutoValue uses Types#directSuperTypes() in the way @tbroyer alluded to, but I agree that it would be a pity to have to pass in a Types instance just to be able to do that.

[tbroyer]

You don't need the first cast actually, and the variable should be renamed as it's not about the upper bounds. This can be simplified to:

return typeVariable.asElement().getBounds();

…and probably inlined then.

[tbroyer]

#305 has been merged. Thanks @marcosb for the bug report and ideas for fixing.

[marcosb]

Sweet, thanks @tbroyer!