RFR: JDK-8267936: PreserveAllAnnotations option isn't tested

[Peter Levart]

On Tue, 1 Jun 2021 09:30:40 GMT, Jaroslav Tulach <github.com+26887752+JaroslavTulach at openjdk.org> wrote:

> There doesn't seem to be much support for the complete changes in #4245. To get at least something useful from that endeavor I have extracted the test for existing behavior of `-XX:+PreserveAllAnnotations` and I am offering it in this pull request without any changes to the JVM behavior.

> _Mailing list message from [Brian Goetz](mailto:brian.goetz at oracle.com) on [core-libs-dev](mailto:core-libs-dev at mail.openjdk.java.net):_
> 
> It seems pretty clear that this "feature" is a leftover from an early
> implementation, doesn't clearly say what it is supposed to do, is more
> complicated than it looks, and is buggily implemented.?

I agree that this "feature" is poorly documented and buggily implemented, but I can't agree that it was not created for a reason. At least I can't so easily classify it as a "leftover". In particular because it makes sense.

> While I
> understand the temptation to "fix" it, at this point we'd realistically
> be adding a basically entirely new reflection feature that hasn't gone
> through any sort of design analysis.

That "feature" is there already from JDK 5 days. We would not be adding it. The reason why we are dealing with it here is very simple. It was/is useful for solving a real world problem and a user (looking a Jaroslav) tried to use it, but because it was not documented, the user miss-interpreted its meaning and tried to propose a "patch" that would solve his problem in the wrong way. Through the discussion we established a more correct way to use the "feature" (he just has to modify the annotation). Jaroslav can use it. Nothing has to be done to it.
Now because the feature wasn't documented and didn't even have a unit test, Jaroslav volunteered to create a test for it. Reviewing that test, I found an inconsistency and discovered a corner case where the "feature" behaves inconsistently. Now at this point we can either fix this inconsistency or not. I guess not many were affected by it since nobody officially complained and from that we can predict that it is highly unlikely that anybody will.

> -- we'd just be guessing about what
> the original intent of this vestigial feature is.?

The feature makes sense to me. But would it help if we invited original authors (Joshua Bloch and/or Neal Gafter) to explain? Of course, it they're willing to participate and if they remember what the heck they were doing :-) ...

> It seems the
> reasonable options are to either leave it alone, deprecate it, or engage
> in a much more deliberate redesign.? (And given the fact that I can
> think of at least 1,000 things that are higher priority, I'd not be
> inclined to pursue the third option.)

I see deprecating a feature so easily on the terms of "it is not documented and it is buggy" and without even trying to understand the feature and the consequences of its deprecation as a more "ruthless" act than trying to fix its corner-case inconsistency by carefully paying attention to compatibility. The feature is clearly useful (a user is using it to solve the problem). Deprecating it and later removing it would leave such user(s) without a means to solve such problem(s). So I'm at least for leaving it alone at this point.

Regards, Peter

-------------

PR: https://git.openjdk.java.net/jdk/pull/4280