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

Don't throw ClassCastException on certain compile errors. #258

Merged
merged 2 commits into from May 28, 2013

Conversation

@cgruber
Copy link
Collaborator

commented May 26, 2013

Handle the failure of compilation better, so we don't find out because of a ClassCastException on a TypeElement.

This can happen when the element fails compilation in certain ways (say, missing symbols from using the wrong android SDK version, etc.). The compiler hands the Processor a bum AST which seems to put @Provides annotations structurally on the class (normally quite impossible). The resulting error is obscure and doesn't show which class the problem is occurring on.

This change simply validates the structure (and the implicit assumption of the cast) before performing the cast, so an error is trapped and reported, not thrown within the compilation job.

I haven't been able to replicate this in a simple consistent test, sadly, or I would include it here, but this code, in the real error inside google, resulted in the correct error being obvious. If nothing else, we should never be in a position where we throw out of a compilation/processing job, as that shuts down error reporting.

…e of a ClassCastException on a TypeElement.
@@ -118,13 +118,16 @@ private void error(String msg, Element element) {

Map<String, List<ExecutableElement>> result = new HashMap<String, List<ExecutableElement>>();
for (Element providerMethod : providesMethods(env)) {
switch (providerMethod.getEnclosingElement().getKind()) {
case CLASS:
break; // valid, move along

This comment has been minimized.

Copy link
@swankjesse

swankjesse May 26, 2013

Member

nit: why +4?

This comment has been minimized.

Copy link
@cgruber

cgruber May 28, 2013

Author Collaborator

Habit? I'll fix.

@swankjesse

This comment has been minimized.

Copy link
Member

commented May 26, 2013

LGTM

break; // valid, move along
default:
// TODO(tbroyer): pass annotation information
error("Unexpected @Provides on " + providerMethod, providerMethod);

This comment has been minimized.

Copy link
@tbroyer

tbroyer May 26, 2013

Collaborator

IIRC, the compiler validates (or at least reports errors for) the annotations' @Target after the annotation processors are called; so how about just ignoring annotations that we don't know how to handle and let the compiler report the error later on?

This comment has been minimized.

Copy link
@cgruber

cgruber May 28, 2013

Author Collaborator

I'm not opposed, except that this isn't an annotation we can't handle, this is the annotation being reported back on an impossible type. I still think reporting the extra weird is more helpful than harmful.

@cgruber

This comment has been minimized.

Copy link
Collaborator Author

commented May 28, 2013

@swankjesse - Etiquette check: you LGTM'ed it - have we moved to a self-merge-with-an-LGTM approach, or were you just opining and someone other than me should still merge it? I don't want the "douchebag-self-merger" bit flipped on me. :)

@swankjesse

This comment has been minimized.

Copy link
Member

commented May 28, 2013

An LGTM from me means, "address the comments in the obvious way, then merge it yourself."

Anything else puts an unreasonably high cost on fixing spelling mistakes and other nitpicky things, in which we all delight in pointing out.

(If I didn't trust you, you wouldn't have commit privileges!)

@cgruber

This comment has been minimized.

Copy link
Collaborator Author

commented May 28, 2013

I inferred no lack of trust - just wanted to check etiquette. Thanks.
Shall do.

cgruber added a commit that referenced this pull request May 28, 2013
Don't throw ClassCastException on certain compile errors.
@cgruber cgruber merged commit c1a80cd into square:master May 28, 2013
1 check passed
1 check passed
default The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants
You can’t perform that action at this time.