AnnotationValues.getProbableFQTypes does not work for class literals not on CP

[lombokissues]

Migrated from Google Code (issue 88)

[lombokissues]

👤 petr.jiricka   🕗 Dec 16, 2009 at 19:57 UTC

I have an annotation that uses a Class parameter, so it is used like this:
@ MyAnnotation(forClass=some.pkg.MyClass.class)

In the Javac handler for this annotation, I call
annotation.getProbableFQType("forClass")

However, this does not work, as the implementation calls
Class.forName("some.pkg.MyClass"), and this class is not on Javac's
classpath (it is part of the user code being compiled).

I am attaching a patch that fixes this problem for me - though I am not a
Javac expert so I don't know if this is the correct patch.

(It also fixes one other minor problem that the compiled source's package
is not correctly populated if the package name is just one level without
any dots.)

[lombokissues]

👤 petr.jiricka   🕗 Dec 16, 2009 at 19:57 UTC

🔗 patch1.txt

[lombokissues]

👤 reinierz   🕗 Dec 17, 2009 at 11:59 UTC

Thanks for the patch, it showed me this is a bit more involve than I at first thought when I read the issue report.

The biggest problem I have with your (very nicely designed) setup of letting each concrete implementation of lombok (eclipse, and javac) set up the job of arriving at an FQN will
invariably result in only two possible scenarios:

1. We specify exactly what FQN resolution should do, and we enforce that each implementation does no less, but also no more than this.

2. We don't specify exactly, or we are lax about enforcing it, and the inevitable result is that compiling code on eclipse is going to give you a different result compared to compiling
   with javac. This is not an acceptable scenario.

Thus, we must go with the first scenario, but therein lies a problem. Javac is fairly nicely designed and uses round-based compiling consistently, and will thus give you excellent
resolution. Eclipse doesn't do this; at the phase lombok (currently) runs in eclipse, resolution just isn't available. Therefore, speccing that FQN resolution should do what the javac-
specific implementation in your patch does means a massive amount of work on eclipse to completely change how and where lombok plugs into the eclipse architecture. We actually
want to go there, because having resolution available allows for some awesome transformations, but it's a lot of work and we're not ready to do this yet.

I'll make sure that when we do go there, that getProbableFQTypes is rewritten. It'll also be renamed; there's no more 'probable' about it once resolution is available to us. You won't
get the probable type, you'll get the type, properly resolved.

Thus, unfortunately, I can't accept your patch. However, I can rejigger how it works.

Separate from that there's the bugfix to "package foo;" which I did accept, and it's in the master branch on git now as 61b054d.

How's this:

Instead of trying to use Class.forName, which admittedly is not a particularly good idea, how about this more complex but more powerful algorithm:

1. Walk through the type hierarchy in the local file. If you have a field in an inner class annotated with "v=OuterName.Foo.class", we'll get that right and stick the local Compilation
   Unit's package in front, and return that. So, "package.OuterName.Foo" would be returned.
2. Walk through the import statements. Any non-star match is returned. Thus, if you write v=Foo.class and your source file includes "import bar.baz.Foo;", we'll return bar.baz.Foo.
3. for any star import (code presumes there's an import java.lang.* at the end of your import declarations), IF it starts with "java.", we'll use Class.forName. Such a class should be
   available to the compiler classloader.
4. If all these strategies failed, and the type name includes no dot, stick the CU's package name in front, and return that (so v=Bar.class, where we have no idea what Bar is referring
   to, if it occurs in a file with "package x.y", we return "x.y.Bar". It's also possible this class is imported via a star import, but then, that's why its called "probable" - this is a
   guesstimate, not an infallible algorithm.
5. If the type name does include a dot, and the type name DOES NOT start with a titlecase or uppercase character, we presume this is a fully qualified name, and we return it
   verbatim. If the first character IS titlecase or uppercase (ex: Foo.Bar.Baz), then we presume this is a reference to a nested class in a sibling class (a class in the same package) and we
   use the same strategy as ﹟4: We prefix this type with the local CU's package name and return that.

This ISNT the proper java resolution algorithm. There are edge cases where the proper resolution gives you a different class than this hack, but the only ways I can think of that this
is not going to work is if you either star-import non-java stuff, or if you throw the conventions to the wind and start your package names with capitals or your class names with
lowercase letters.

This concept is now in the master branch (though not tested very well): 31d7d8e

It's not as good as using the true resolution, but I think it'll do until we upgrade lombok to run with full resolution support.

[lombokissues]

👤 petr.jiricka   🕗 Dec 17, 2009 at 13:07 UTC

Thanks for a quick fix - I will test it.

I did not look into the Eclipse support area at all, and I did not know type
resolution was not possible there.

[lombokissues]

👤 reinierz   🕗 Dec 17, 2009 at 16:33 UTC

If eclipse just ceased to exist overnight, lombok could do a lot more than it does now. Eclipse's internal AST and
compilation code is a disaster. It's unfortunate in the extreme eclipse doesn't just dump the entire JDT core and
swap it over to a javac based solution. NetBeans proves that this can work rather well.

[lombokissues]

👤 reinierz   🕗 Dec 21, 2009 at 13:33 UTC

You can download a direct binary of this fix in the edge build: https://projectlombok.org/download-edge.html

[lombokissues]

End of migration