RFR: 8331051: Add an `@since` checker test for `java.base` module [v5]
Jan Lahoda
jlahoda at openjdk.org
Fri May 17 15:06:05 UTC 2024
On Sun, 5 May 2024 15:22:15 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 incrementally with one additional commit since the last revision:
>
> - Not checking elements enclosed within a record, I doubt a record class will change after being created
> - Added a null check as `toString` can return an exception
Overall seems pretty good, great job!
A few comments inline, to make the code cleaner.
Thanks for working on this!
test/jdk/tools/sincechecker/SinceChecker.java line 224:
> 222: }
> 223:
> 224: return LEGACY_PREVIEW_METHODS.containsKey(currentVersion)
Nit: could probably be `LEGACY_PREVIEW_METHODS.getOrDefault(currentVersion, Map.of()).contains(uniqueId)`
test/jdk/tools/sincechecker/SinceChecker.java line 309:
> 307: error("module-info.java not found or couldn't be opened AND this module has no unqualified exports");
> 308: } catch (NullPointerException e) {
> 309: error("module-info.java does not contain an `@since`");
Catching a NPE like this feels like a code smell to me. I assume it may happen when `extractSinceVersionFromText(moduleInfoContent)` returns `null` - so just store that in a variable, and check for `null` there.
test/jdk/tools/sincechecker/SinceChecker.java line 331:
> 329: error(ed.getPackage().getQualifiedName() + ": package-info.java not found or couldn't be opened");
> 330: } catch (NullPointerException e) {
> 331: error(ed.getPackage().getQualifiedName() + ": package-info.java does not contain an `@since`");
Please see the comment on NPE catching in `getModuleVersionFromFile`.
test/jdk/tools/sincechecker/SinceChecker.java line 352:
> 350: }
> 351: checkElement(te.getEnclosingElement(), te, types, javadocHelper, version, elementUtils);
> 352: if( te.getKind() == ElementKind.RECORD){
This seems a bit too broad. Ignoring all members of a record type may lead to missed members. `Elements.getOrigin` may be unusable here, but I wonder what exactly is the problem which this condition is trying to solve?
-------------
PR Review: https://git.openjdk.org/jdk/pull/18934#pullrequestreview-2063646309
PR Review Comment: https://git.openjdk.org/jdk/pull/18934#discussion_r1605134919
PR Review Comment: https://git.openjdk.org/jdk/pull/18934#discussion_r1605140507
PR Review Comment: https://git.openjdk.org/jdk/pull/18934#discussion_r1605143505
PR Review Comment: https://git.openjdk.org/jdk/pull/18934#discussion_r1605150261
More information about the core-libs-dev
mailing list