allow use of dollar signs in processed class names

[jhump]

@swankjesse @JakeWharton

[swankjesse]

That wasn't as easy as I was hoping it to be! I assume you didn't see something dumb we were doing that could have made this easier?

[jhump]

@swankjesse, my first approach I ended up tossing because it seemed riskier: it was to change the format of the key strings so that type names get encoded with slashes in the package (instead of dots) and nested classes are separated with dots (instead of dollars). For example com/squareup/Outer.Inner.Foo$Bar.Baz. That way we can unambiguously convert the string into either binary names (replace dots with dollars then replace slashes with dots) or canonical names (replace slashes with dots).

But this was a bigger and more complicated change which had a couple of big downsides:

1. More work to decode keys. It looks like Keys has had some level of optimization to make it faster (for example, using String#concat instead of + operator). The new encoding would be slower.
2. The runtime must still support code that was generated with an older version of the compiler. So the encoding must further be unambiguous about whether it was in a new vs. legacy format, and then put multiple paths in all of the decoding logic.

The latter point seemed riskiest because it doesn't seem easy to test. I suppose I could have added support for a system property to force it to generate keys in the legacy format. Then a unit test could run the compiler both ways and verify that the runtime works regardless of the key format that appears in generated code.

(Easier to do, but less thorough, would be to just have unit tests for all of the methods in Keys. But, since it's just a string vs. a Key type [with encapsulation/hiding], I'd be worried that there are places outside of Keys that directly operate on the string.)

So, all said and done, this seemed a lot easier. It does more work in pathological cases (like a class name with a silly number of dollar signs), but in practice should be the same as without the fix since it does at least first check the version of the name with all dollars replaced with dots.

Also, this is not unlike the work that javac must do to resolve a class use into a binary name (it does the inverse work; naively it must do more since there are many more dots that may potentially need to be converted to dollars, but it can employ techniques to prune the search space that I don't think can apply here).

[jhump]

BTW, any tips on getting travis-ci to go green -- is it flaky? That test passes fine locally, and the error message in travis-ci is cryptic enough that I don't know where to start debugging.

[JakeWharton]

Hmm master built successfully on Travis CI 24 days ago when the last PR was merged. Let me try locally.

[JakeWharton]

@jhump it fails for me locally when using JDK 7 but not JDK 8 (which has other problems... #425)

[JakeWharton]

Full exception is:

java.lang.ClassCastException: com.sun.tools.javac.comp.Resolve$SymbolNotFoundError cannot be cast to com.sun.tools.javac.code.Symbol$ClassSymbol
    at com.sun.tools.javac.comp.Attr$IdentAttributer.visitMemberSelect(Attr.java:313)
    at com.sun.tools.javac.comp.Attr$IdentAttributer.visitMemberSelect(Attr.java:302)
    at com.sun.tools.javac.tree.JCTree$JCFieldAccess.accept(JCTree.java:1683)
    at com.sun.tools.javac.comp.Attr.attribIdent(Attr.java:298)
    at com.sun.tools.javac.main.JavaCompiler.resolveIdent(JavaCompiler.java:672)
    at com.sun.tools.javac.model.JavacElements.nameToSymbol(JavacElements.java:162)
    at com.sun.tools.javac.model.JavacElements.getTypeElement(JavacElements.java:144)
    at com.sun.tools.javac.model.JavacElements.getTypeElement(JavacElements.java:61)
    at dagger.internal.codegen.GraphAnalysisLoader.resolveType(GraphAnalysisLoader.java:90)
    at dagger.internal.codegen.GraphAnalysisLoader.resolveType(GraphAnalysisLoader.java:91)
    at dagger.internal.codegen.GraphAnalysisLoader.resolveType(GraphAnalysisLoader.java:72)
    at dagger.internal.codegen.GraphAnalysisLoader.getAtInjectBinding(GraphAnalysisLoader.java:44)
    at dagger.internal.Linker.createBinding(Linker.java:230)
    at dagger.internal.Linker.linkRequested(Linker.java:142)
    at dagger.internal.Linker.linkAll(Linker.java:109)
    at dagger.internal.codegen.GraphAnalysisProcessor.processCompleteModule(GraphAnalysisProcessor.java:277)
    at dagger.internal.codegen.GraphAnalysisProcessor.process(GraphAnalysisProcessor.java:124)

[jhump]

The issue in CI was a bug in javac. I put together a trivial repro case and filed a bug with Oracle. Under some conditions, it can throw instead of return null when asking Elements#getTypeElement for an unknown type.

I hacked in a work-around here.