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

[Brian Goetz]

Since the discussion happened over the holiday weekend, I didn't get a 
chance to respond until now, but I think that this came to a good 
outcome.  As Alan's archaeology discovered, this flag appears to be a 
leftover from the original implementation, and I could find no signs of 
its usage.  We might consider deprecating it (though, we might also 
leave sleeping dogs alone), but its good to have a test for the current 
behavior.

Allow me to make a few meta-comments about how we got here, though, 
before we wrap up.

> they are not visible via java.lang.reflect API. I assume that's just an omission.

While omissions do sometimes happen, it is usually not the best first 
theory in a situation like this.  More often than not, there is a good, 
but often non-obvious, explanation.

More importantly, though, the PR-first approach is not how we like to 
approach such a change, especially when it involves the behaviors of 
such fundamental APIs such as reflection, because it jumps over the most 
important steps:

  - What problem are we trying to solve?
  - Is it really a problem that needs to be solved?
  - Is it really a problem that needs to be solved in the JDK?
  - What solutions are there, besides the obvious one?
  - What are the pros, cons, and tradeoffs of the various solutions?
  - Of the proposed solution, what future downsides and risks could we 
imagine?

Going straight to the code of a specific solution is likely to be 
unsatisfying in both the short term (because its the wrong conversation) 
or the long term (because it might not be the right solution to the 
right problem.)  Instead, starting with a discussion of the problem, and 
of potential solutions and tradeoffs, is likely to yield a better result 
on both counts.

(As an example of the last one, suppose we committed this patch.  One 
could easily imagine some library later saying "must be run with 
-Xx:PreserveAllAnnotations" because that's what the author had to do to 
make it work.  But a behavior change like non-runtime annotations 
showing up unexpectedly in reflection could confuse existing code, which 
might not work as expected with the new behavior.  Which might then call 
for "can we please make the PreserveAllAnnotations flag more selective 
(e.g., per-class/module/package)" or calls for new flags or ... yuck.  
We try to avoid today's solutions becoming tomorrow's problems.  This is 
not an exact science, of course, but this is the sort of stewardship we 
strive for.)





On 6/1/2021 5:39 AM, Jaroslav Tulach 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.
>
> -------------
>
> Commit messages:
>   - Test long time existing behavior of -XX:+PreserveAllAnnotations
>
> Changes: https://git.openjdk.java.net/jdk/pull/4280/files
>   Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=4280&range=00
>    Issue: https://bugs.openjdk.java.net/browse/JDK-8267936
>    Stats: 172 lines in 1 file changed: 172 ins; 0 del; 0 mod
>    Patch: https://git.openjdk.java.net/jdk/pull/4280.diff
>    Fetch: git fetch https://git.openjdk.java.net/jdk pull/4280/head:pull/4280
>
> PR: https://git.openjdk.java.net/jdk/pull/4280