kohsuke / metainf-services

Generates META-INF/services files automatically
http://metainf-services.kohsuke.org/
69 stars 26 forks source link

Annotation processors should always report the latest version... #4

Closed dmlloyd closed 11 years ago

dmlloyd commented 11 years ago

as this compat mechanism was ill-conceived and impossible to use reliably in a forward-compatible manner.

It is better to risk (the very unlikely possibility of) something not working right in the far-flung future than to have to fix the supported version every time a JDK is released (thereby breaking compatibility with previous JDKs at the same time).

kohsuke commented 11 years ago

So what happens when you try to run annotation processor with @SupportedSourceVersion(SourceVersion.RELEASE_6) on -source 1.7 mode? Does it just flat out refuse to use the processor?

dmlloyd commented 11 years ago

It prints a big ugly warning that it isn't going to work (but then it works).

jglick commented 11 years ago

Already covered by #3. Using .latest is potentially unsafe but there are alternatives.

dmlloyd commented 11 years ago

I disagree with the assertion that "Using .latest is potentially unsafe". According to Joe Darcy at Oracle (see http://mail.openjdk.java.net/pipermail/compiler-dev/2013-December/008272.html):

If you have taken the trouble to make a processor robust in the face of unknown future language versions, then returning latest is the right thing to do.

I think that it is completely, 100% safe to assert that this processor is robust in the face of future language versions. There is no good reason to introduce a large complex reflection blob to solve this problem; in addition, I agree completely with Martin Buchholz' argument in that not using latest is essentially not useful unless you're specifically, consciously targeting one language and not any future versions.

In the highly unlikely event that the language fundamentally changes in a way which makes simple class annotations no longer function, I think that this bridge can be crossed when it is encountered. It is far less likely than the certainty of the choice between warnings plus compatibility annoyances, or complex reflection blobs where a simple solution will do.

jglick commented 11 years ago

The reflection blob is actually quite short. And in fact no one has taken the trouble to make this particular processor robust at all; there are not even tests, and there are some obvious things it could be checking which it does not. (Compare SezPoz which performs a roughly similar task but has fairly thorough processor tests.) I am sure that simple class annotations will continue to function; the question is more whether incorrect annotations would be correctly reported.

Probably in the future when it is cleaned up and thoroughly tested, SourceVersion.latest() would be sensible. In the meantime, the slow pace of JDK releases (and the even slower adoption of code such as Jenkins plugins using new source versions) makes this a pretty low priority.

kohsuke commented 11 years ago

Should we at least bump it up to RELEASE_8 then? Since we know this works with Java8?

I guess doing that breaks people trying to use this with Java6 or 7, since no such constant is defined? Isn't that a bigger problem here? Namely that we can't claim conformance with Java N without breaking compatibility with earlier versions of JDKs?

dmlloyd commented 11 years ago

Yes, it would, which is why I contend that just using latest is the best option - it's simple and practical, and does not necessitate a new release (for what is honestly otherwise a very simple project) for every major Java version unless it is shown that there is a real issue.

jglick commented 11 years ago

Should we at least bump it up to RELEASE_8 then?

Already did, in #3 (after testing it in JDK 8).

I guess doing that breaks people trying to use this with Java6 or 7, since no such constant is defined?

No, see patch.