On Wed, 24 Apr 2024 14:10:29 GMT, Nizar Benalla <d...@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.
Various comments inline. Overall, a great start.
Consider adding more explanatory comments for the bigger items, like classes
and long/important methods. Imagine a future-person looking over your shoulder,
asking what the more important items do or are for.
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.
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`.
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.
test/jdk/tools/sincechecker/SinceChecker.java line 87:
> 85: public class SinceChecker {
> 86: private final Map<String, Set<String>> LEGACY_PREVIEW_METHODS = new
> HashMap<>();
> 87: private final Map<String, IntroducedIn> classDictionary = new
> HashMap<>();
See comment lower down about `class IntroducedIn`
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`.
test/jdk/tools/sincechecker/SinceChecker.java line 135:
> 133: private void analyzeClassRecord(TypeElement te, String version,
> Types types, Elements elements) {
> 134: Set<Modifier> classModifiers = te.getModifiers();
> 135: if (!(classModifiers.contains(Modifier.PUBLIC) ||
> classModifiers.contains(Modifier.PROTECTED))) {
Insert comment:
/// JDK documentation only contains public and protected declarations
which is 99% true. (The serialization page can contain `private`
declarations...)
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)
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;
};
}
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
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)`
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");
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`.
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`
test/jdk/tools/sincechecker/SinceChecker.java line 309:
> 307: .map(TypeElement.class::cast)
> 308: .forEach(nestedClass -> analyzeClassCheck(nestedClass,
> currentjdkVersion, javadocHelper, types, elementUtils));
> 309: }
more possible applications of those utility predicate methods I mentioned
earlier.
test/jdk/tools/sincechecker/SinceChecker.java line 330:
> 328: mappedVersion.introducedStable;
> 329: } catch (Exception e) {
> 330: wrongTagsList.add("For element " + element + "mappedVersion"
> + mappedVersion + " is null" + e + "\n");
Put a space e or colon between `is null` and `e`
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
test/jdk/tools/sincechecker/SinceChecker.java line 375:
> 373: if (mappedVersion.equals("9")) {
> 374: message = "For " + elementSimpleName +
> 375: " Wrong `@since` version " + sinceVersion + " But
> the element exists before JDK 10\n";
lower case "but"
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.
test/jdk/tools/sincechecker/SinceChecker.java line 423:
> 421: }
> 422:
> 423: //these were preview in before the introduction of the
> @PreviewFeature
Just curious: where do you find this information?
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.
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
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
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
-------------
PR Review: https://git.openjdk.org/jdk/pull/18934#pullrequestreview-2025970944
PR Review Comment: https://git.openjdk.org/jdk/pull/18934#discussion_r1578567066
PR Review Comment: https://git.openjdk.org/jdk/pull/18934#discussion_r1578573782
PR Review Comment: https://git.openjdk.org/jdk/pull/18934#discussion_r1578579654
PR Review Comment: https://git.openjdk.org/jdk/pull/18934#discussion_r1578592510
PR Review Comment: https://git.openjdk.org/jdk/pull/18934#discussion_r1578583705
PR Review Comment: https://git.openjdk.org/jdk/pull/18934#discussion_r1578584959
PR Review Comment: https://git.openjdk.org/jdk/pull/18934#discussion_r1578587585
PR Review Comment: https://git.openjdk.org/jdk/pull/18934#discussion_r1578588555
PR Review Comment: https://git.openjdk.org/jdk/pull/18934#discussion_r1578601911
PR Review Comment: https://git.openjdk.org/jdk/pull/18934#discussion_r1578593616
PR Review Comment: https://git.openjdk.org/jdk/pull/18934#discussion_r1578597558
PR Review Comment: https://git.openjdk.org/jdk/pull/18934#discussion_r1578600689
PR Review Comment: https://git.openjdk.org/jdk/pull/18934#discussion_r1578604639
PR Review Comment: https://git.openjdk.org/jdk/pull/18934#discussion_r1578605090
PR Review Comment: https://git.openjdk.org/jdk/pull/18934#discussion_r1578605648
PR Review Comment: https://git.openjdk.org/jdk/pull/18934#discussion_r1581605988
PR Review Comment: https://git.openjdk.org/jdk/pull/18934#discussion_r1581605289
PR Review Comment: https://git.openjdk.org/jdk/pull/18934#discussion_r1578592255
PR Review Comment: https://git.openjdk.org/jdk/pull/18934#discussion_r1581606328
PR Review Comment: https://git.openjdk.org/jdk/pull/18934#discussion_r1581603329
PR Review Comment: https://git.openjdk.org/jdk/pull/18934#discussion_r1581605077
PR Review Comment: https://git.openjdk.org/jdk/pull/18934#discussion_r1581604207
PR Review Comment: https://git.openjdk.org/jdk/pull/18934#discussion_r1578057928
