RFR: 8331051: Add an `@since` checker test for `java.base` module [v14]

[Jonathan Gibbons]

On Thu, 19 Sep 2024 16:38:54 GMT, Nizar Benalla <nbenalla at openjdk.org> wrote:

>> This checker checks the values of the `@since` tag found in the documentation comment for an element against the release in which the element first appeared.
>> 
>> Real since value of an API element is computed as the oldest release in which the given API element was introduced. That is:
>> - for modules, classes and interfaces, the release in which the element with the given qualified name was introduced
>> - for constructors, the release in which the constructor with the given VM descriptor was introduced
>> - for methods and fields, the release in which the given method or field with the given VM descriptor became a member of its enclosing class or interface, whether direct or inherited
>> 
>> Effective since value of an API element is computed as follows:
>> - if the given element has a `@since` tag in its javadoc, it is used
>> - in all other cases, return the effective since value of the enclosing element
>> 
>> The since checker verifies that for every API element, the real since value and the effective since value are the same, and reports an error if they are not.
>> 
>> Preview method are handled as per JEP 12, if `@PreviewFeature` is used consistently going forward then the checker doesn't need to be updated with every release. The checker has explicit knowledge of preview elements that came before `JDK 14` because they weren't marked in a machine understandable way and preview elements that came before `JDK 17` that didn't have `@PreviewFeature`.
>> 
>> Important note : We only check code written since `JDK 9` as the releases used to determine the expected value of `@since` tags are taken from the historical data built into `javac` which only goes back that far
>> 
>> The intial comment at the beginning of `SinceChecker.java` holds more information into the program.
>> 
>> I already have filed issues and fixed some wrong tags like in #18640, #18032, #18030, #18055, #18373, #18954, #18972.
>
> Nizar Benalla has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 28 commits:
> 
>  - Change "found" to "should be" in the error messages
>  - Merge remote-tracking branch 'upstream/master' into source-based-since-checker
>  - avoid using `continue`, fix mistake in last commit
>  - extend SinceChecker to now skip some packages
>  - Merge remote-tracking branch 'upstream/master' into source-based-since-checker
>  - Merge remote-tracking branch 'upstream/master' into source-based-since-checker
>  - Merge remote-tracking branch 'upstream/master' into source-based-since-checker
>  - Skip over files and packages that aren't found
>  - Move the tests to module/module_name directory for clearer naming
>  - (C)
>  - ... and 18 more: https://git.openjdk.org/jdk/compare/bc36ace7...d355222e

Generally very good. It's getting to be a really nice piece of functionality.

I added one suggestion for a minor improvement, about setting up the `EXCLUDE_LIST in a more module-specific way.  The intent is that if we need to make module-specific changes to the exclude list in times going forward, we should do that in the module-specific test invocation, and not in the main module-independent code.

test/jdk/tools/sincechecker/SinceChecker.java line 114:

> 112:     private static final Set<String> EXCLUDE_LIST = Set.of(
> 113:             "java.lang.classfile"
> 114:     );

This should not really be here, in the general code for a SinceChecker for all modules.

It would be better if this info was provided in command-line options, given when the test is run from the module-specific test (i.e. `CheckSince_javaBase.java`). 

This means you will have to write some additional simple option decoding in the `main` method, starting line 21.

test/jdk/tools/sincechecker/modules/java_base/CheckSince_javaBase.java line 33:

> 31:  *          jdk.compiler/com.sun.tools.javac.util
> 32:  *          jdk.compiler/com.sun.tools.javac.code
> 33:  * @run main SinceChecker java.base

See the comment about init-ing the EXCLUDE_LIST in the main SinceChecker, at about line 112.

You are already paying a module name as an option to the main test. Maybe the command here on line 33 could be something like

@run main SinceChecker java.base --exclude java.lang.classfile

where the argument after `--exclude` is (logically) a comma-separated list of items to exclude. You could also do a whitespace-separated list up to the next option (i.e. item beginning with `-`) or end of the list. In other words make it future-proof against adding more options in future.

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

Changes requested by jjg (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/18934#pullrequestreview-2316850250
PR Review Comment: https://git.openjdk.org/jdk/pull/18934#discussion_r1767666131
PR Review Comment: https://git.openjdk.org/jdk/pull/18934#discussion_r1767669975