On Wed, 24 Apr 2024 21:59:02 GMT, Jonathan Gibbons <j...@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.
>>
>> 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.
>
> test/jdk/tools/sincechecker/SinceChecker.java line 49:
>
>> 47: import java.util.regex.Matcher;
>> 48: import java.util.regex.Pattern;
>> 49: import java.util.stream.Collectors;
>
> The imports are somewhat unordered. I recommend the follooing order
>
> * `java.*` imports first,
> * then `javax.*` imports
> * then `com.sun.*` imports
> * and finally the `jtreg` import.
>
> And, alphabetically sorted in each group.
Thanks, fixed in 32edb4d811978f87bd2f850c214191e8fc7b8c88
> test/jdk/tools/sincechecker/SinceChecker.java line 52:
>
>> 50:
>> 51: /*
>> 52: The `@since` checker works as a two-step process:
>
> Insert an initial sentence or paragraph, such as the following:
>
>
> 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.
>
> The source code containing the documentation comments is read from `src.zip`
> in the release of
> JDK used to run the test. The releases used to determine the expected value
> of `@since` tags
> are taken from the historical data built into `javac`.
Added in 32edb4d811978f87bd2f850c214191e8fc7b8c88
> test/jdk/tools/sincechecker/SinceChecker.java line 79:
>
>> 77: - When an element is still marked as preview, the `@since` should be the
>> first JDK release where the element was added.
>> 78: - If the element is no longer marked as preview, the `@since` should be
>> the first JDK release where it was no longer preview.
>> 79:
>
> Add a comment about need for special treatment of early preview features.
Thanks, added it
> test/jdk/tools/sincechecker/SinceChecker.java line 116:
>
>> 114: }
>> 115:
>> 116: private void processModuleRecord(ModuleElement moduleElement,
>> String releaseVersion, JavacTask ct) {
>
> for this method, and similarly named methods, the use of `Record` seems
> confusing, if only because there do not seem to be any record classes being
> processed or analyzed. Consider dropping `Record` or changing it to
> `Element`. Same for `analyzePackageRecord`, `analyzeClassRecord`.
>
> Also, consider being consistent with `process` or analyze`.
Fixed in 32edb4d811978f87bd2f850c214191e8fc7b8c88
> test/jdk/tools/sincechecker/SinceChecker.java line 140:
>
>> 138: persistElement(te.getEnclosingElement(), te, types, version);
>> 139: elements.getAllMembers(te).stream()
>> 140: .filter(element ->
>> element.getModifiers().contains(Modifier.PUBLIC) ||
>> element.getModifiers().contains(Modifier.PROTECTED))
>
> Consider writing and using a predicate method:
>
>
> boolean isDocumented(Element e) {
> var mods = e.getModifiers();
> return mods.contains(Modifier,.PUBLIC) ||
> mods.contains(Modifier.PROTECTED);
> }
>
>
> You could then use that method in the obvious way for the class
> declaration(line 134) and in a lambda as follows:
>
> .filter(this::isDocumented)
Added in 32edb4d811978f87bd2f850c214191e8fc7b8c88
> test/jdk/tools/sincechecker/SinceChecker.java line 143:
>
>> 141: .filter(element -> element.getKind().isField()
>> 142: || element.getKind() == ElementKind.METHOD
>> 143: || element.getKind() == ElementKind.CONSTRUCTOR)
>
> Consider writing and using another predicate method,
>
> boolean isMember(Element e) {
> var k = e.getKind();
> return k.isField() || switch (k) {
> case CONSTRUCTOR, METHOD -> true;
> default -> false;
> };
> }
Added in 32edb4d811978f87bd2f850c214191e8fc7b8c88
> test/jdk/tools/sincechecker/SinceChecker.java line 178:
>
>> 176: }
>> 177:
>> 178: private void testThisModule(String moduleName) throws Exception {
>
> The word `this` is a bit distracting/unnecessary here. It's enough to say
> `testModule(String moduleName)` although `checkModule(String moduleName)`
> would be even better
Thanks, renamed it to `CheckModule`
> test/jdk/tools/sincechecker/SinceChecker.java line 190:
>
>> 188: if (!f.exists() && !f.isDirectory()) {
>> 189: throw new SkippedException("Skipping Test because src.zip
>> wasn't found");
>> 190: }
>
> For modern/new code I would recommend using `java.nio.file.Path` instead of
> `java.io.File`.
>
> I see you actually have a `Path` in `srcZip`, so use `Files.exists(srcZip)`
> and `Files.isDirectory(srcZip)`
Fixed in 32edb4d811978f87bd2f850c214191e8fc7b8c88
> test/jdk/tools/sincechecker/SinceChecker.java line 214:
>
>> 212: }
>> 213: if (!wrongTagsList.isEmpty()) {
>> 214: throw new Exception(wrongTagsList.toString());
>
> This looks like it might be the exception to end the test, right, and the
> wrongTags could be long.
> I'd suggest writing a heading to `System.err`,
> then`wrongTagsList.forEach(System.err::println);` and then throw the
> exception with a summary string, like `throw new
> Exception(wrongTagsList.size() + " problems found");
Done. Added in 32edb4d811978f87bd2f850c214191e8fc7b8c88
> test/jdk/tools/sincechecker/SinceChecker.java line 223:
>
>> 221: private void processModuleCheck(ModuleElement moduleElement,
>> JavacTask ct, Path packagePath, EffectiveSourceSinceHelper javadocHelper) {
>> 222: if (moduleElement == null) {
>> 223: wrongTagsList.add("Module element: was null because
>> `elements.getModuleElement(moduleName)` returns null." +
>
> This doesn't look like a "Wrong tag" so much as a general error message.
>
> Consider this as a different paradigm -- instead of saving up strings in
> `wrongTagsList`, consider having a method to report (and count) error
> messages as you find them:
>
>
> private int errorCount;
> void error(String message) {
> System.err.println(message);
> errorCount++;
> }
>
>
> then at the end of `testThisModule`, just check if `errorCount>0` to decide
> whether to throw the exception.
>
> Note, in this model, you need not have a `\n` at the end of every string
> argument, the way you do for add strings added into `wrongTagsList`.
Changed it and removed '\n'
> test/jdk/tools/sincechecker/SinceChecker.java line 274:
>
>> 272: try {
>> 273: byte[] packageAsBytes = Files.readAllBytes(pkgInfo);
>> 274: String packageContent = new String(packageAsBytes,
>> StandardCharsets.UTF_8);
>
> Easier to just use `Files.readString`
Fixed it, thanks
> test/jdk/tools/sincechecker/SinceChecker.java line 358:
>
>> 356: private void checkEquals(String sinceVersion, String mappedVersion,
>> String id) {
>> 357: if (sinceVersion == null || mappedVersion == null) {
>> 358: wrongTagsList.add("For " + id + " NULL value for either
>> mapped or real `@since` . mapped version is="
>
> Here and elsewhere, instead of `"For " + name + ....` consider using the
> format `name + ": " + ...` for the format of the messages
Fixed it.
> test/jdk/tools/sincechecker/SinceChecker.java line 421:
>
>> 419: public String introducedPreview;
>> 420: public String introducedStable;
>> 421: }
>
> Suggest move this nearer the map at the top of the class -- either move this
> class up, or those members down.
Thanks, moved it up
> test/jdk/tools/sincechecker/SinceChecker.java line 543:
>
>> 541: }
>> 542:
>> 543: private final class EffectiveSourceSinceHelper implements
>> AutoCloseable {
>
> Consider putting a doc comment describing the purpose of this class. The name
> hints at something useful, but is not specific enough by itself.
Added it, thanks
> test/jdk/tools/sincechecker/SinceChecker.java line 559:
>
>> 557: fm.close();
>> 558: } catch (IOException closeEx) {
>> 559: }
>
> Consider using `ex.addSuppressed(closeEx);` instead of just dropping the
> exception
Fixed in 32edb4d811978f87bd2f850c214191e8fc7b8c88
> test/jdk/tools/sincechecker/SinceChecker.java line 716:
>
>> 714: }
>> 715:
>> 716: private static final class PatchModuleFileManager
>
> provide a short doc comment describing the purpose/use of this class
Added it, thanks
> test/jdk/tools/sincechecker/testjavabase/SinceChecker.java line 37:
>
>> 35:
>> 36: public class SinceChecker {
>> 37: }
>
> You don't need this class, do you?
> The comment should be enough to run the validator
Thanks, didn't know that.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/18934#discussion_r1582576708
PR Review Comment: https://git.openjdk.org/jdk/pull/18934#discussion_r1582576954
PR Review Comment: https://git.openjdk.org/jdk/pull/18934#discussion_r1582577120
PR Review Comment: https://git.openjdk.org/jdk/pull/18934#discussion_r1582577505
PR Review Comment: https://git.openjdk.org/jdk/pull/18934#discussion_r1582578042
PR Review Comment: https://git.openjdk.org/jdk/pull/18934#discussion_r1582578390
PR Review Comment: https://git.openjdk.org/jdk/pull/18934#discussion_r1582585880
PR Review Comment: https://git.openjdk.org/jdk/pull/18934#discussion_r1582579802
PR Review Comment: https://git.openjdk.org/jdk/pull/18934#discussion_r1582580425
PR Review Comment: https://git.openjdk.org/jdk/pull/18934#discussion_r1582580782
PR Review Comment: https://git.openjdk.org/jdk/pull/18934#discussion_r1582582577
PR Review Comment: https://git.openjdk.org/jdk/pull/18934#discussion_r1582583030
PR Review Comment: https://git.openjdk.org/jdk/pull/18934#discussion_r1582578684
PR Review Comment: https://git.openjdk.org/jdk/pull/18934#discussion_r1582581513
PR Review Comment: https://git.openjdk.org/jdk/pull/18934#discussion_r1582581661
PR Review Comment: https://git.openjdk.org/jdk/pull/18934#discussion_r1582581840
PR Review Comment: https://git.openjdk.org/jdk/pull/18934#discussion_r1578128902
