8330954: since-checker - Fix remaining @ since tags in java.base

[nizarbenalla]

Please review this PR that aims to add all the remaining needed @since tags in java.base, and group them into a single fix. This is related to #18934 and my work around the @since checker feature. Explicit @since tags are needed for some overriding methods for the purpose of the checker.

I will only give the example with the CompletableFuture class but here is the before where the methods only appeared in "Methods declared in interface N"

and after where the method has it's own javadoc, the main description is added and the @since tags are added as intended.

I don't have an account on https://cr.openjdk.org/ but I could host the generated docs somewhere if that is needed.

TIA

---

Progress

• [x] Change must be properly reviewed (1 review required, with at least 1 Reviewer)
• [x] Change must not contain extraneous whitespace
• [x] Commit message must refer to an issue

Issue

• JDK-8330954: since-checker - Fix remaining @ since tags in java.base (Sub-task - P4)

Reviewers

• Chen Liang (@liach - Committer)
• Naoto Sato (@naotoj - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/18954/head:pull/18954
$ git checkout pull/18954

Update a local copy of the PR:
$ git checkout pull/18954
$ git pull https://git.openjdk.org/jdk.git pull/18954/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 18954

View PR using the GUI difftool:
$ git pr show -t 18954

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/18954.diff

Webrev

Link to Webrev Comment

[bridgekeeper[bot]]

:wave: Welcome back nizarbenalla! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[openjdk[bot]]

@nizarbenalla This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8330954: since-checker - Fix remaining @ since tags in java.base

Reviewed-by: liach, naoto

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 73 new commits pushed to the master branch:

• 3050ba017687ac13e1bbccdd1544d25f8eb2a747: 8335654: Remove stale hyperlink in divnode.cpp
• da0ffa8b7ff04eb5cbc0fcbe4b858f20d7e46405: 8334031: Generated JfrNativeSettings seems off
• b0efd7740243916ba22178524ab2ede9e5436d94: 8314653: Metaspace: remove allocation guard feature
• 6a472797a410a6fa27f50371b255054af0cd3c99: 8332072: Convert package.html files in java.naming to package-info.java
• 7e378fccd8a4601c8b8e86aa2862c61e469c3a04: 8335667: Fix simple -Wzero-as-null-pointer-constant warnings in compiler code
• ced99066354fc6a32c587b9e3c35b07e26d3452e: 8334371: [AIX] Beginning with AIX 7.3 TL1 mmap() supports 64K memory pages
• 916db07e533cdc0fca2010751f7ebe54e6ada7b9: 8335532: [JVMCI] Export VM_Version::L1_line_size in JVMCI
• c0604fb823d9f3b2e347a9857b11606b223ad8ec: 8334890: Missing unconditional cross modifying fence in nmethod entry barriers
• cf1be87279ddfb2a9fd272e0b245fccd7ec10972: 8335663: Fix simple -Wzero-as-null-pointer-constant warnings in C2 code
• 0bb9c76288b5f63fe965c3276bb566cef5f51c50: 8324089: Fix typo in the manual page for "jcmd" (man jcmd)
• ... and 63 more: https://git.openjdk.org/jdk/compare/c798316bc4cb33fd902f926030d8a0b6870d661a...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@liach, @naotoj) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

[openjdk[bot]]

@nizarbenalla The following labels will be automatically applied to this pull request:

• core-libs
• i18n
• security

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

[liach]

I think your changes mostly group in these categories:

1. New API methods provided in superclasses/superinterfaces, this class provides a more concrete implementation: Examples being CompletableFuture, FileInputStream, DelayQueue, FutureTask I don't think you should add since tags for these; without explicit javadoc, the methods inherit the superclass/superinterface docs, and appear in Methods declared in class/interface Xxx (supertype) section, which already have the correct since tags. There's one scenario where such addition may be meaningful, however: that's if the supertype's since version is newer than this class/interfaces's since version, so we might need to specify here. (On a side note, it would be great if we can mark the since version of an interface, notorious example being ZipFile retrofitted to implement Closeable in 1.7 and breaks compile target 1.6)
2. Remove unnecessary since tags for existing API methods with newer implementation Examples being Reference, RsaPrivateKey. These make sense.
3. API methods with different return types Examples being ClassSignature, ClassDesc. These make sense too, as older version may return different types. But problem here is should we count methods with only signature (but not descriptor) differences, like ClassSignature::superinterfaceSignatures()?

[nizarbenalla]

I think your changes mostly group in these categories:

1. New API methods provided in superclasses/superinterfaces, this class provides a more concrete implementation: Examples being CompletableFuture, FileInputStream, DelayQueue, FutureTask I don't think you should add since tags for these; without explicit javadoc, the methods inherit the superclass/superinterface docs, and appear in Methods declared in class/interface Xxx (supertype) section, which already have the correct since tags. There's one scenario where such addition may be meaningful, however: that's if the supertype's since version is newer than this class/interfaces's since version, so we might need to specify here. (On a side note, it would be great if we can mark the since version of an interface, notorious example being ZipFile retrofitted to implement Closeable in 1.7 and breaks compile target 1.6)
2. Remove unnecessary since tags for existing API methods with newer implementation Examples being Reference, RsaPrivateKey. These make sense.
3. API methods with different return types Examples being ClassSignature, ClassDesc. These make sense too, as older version may return different types. But problem here is should we count methods with only signature (but not descriptor) differences, like ClassSignature::superinterfaceSignatures()?

@liach

• I am only looking at code added in JDK 9-current and do not plan on checking old code for now (in case there are questions on why certain methods weren't affected)
• I want generify-ing methods to be fine, so I am leaving ClassSignature::superinterfaceSignatures(). It will be changed eventually once the class goes out of Preview

[liach]

For case 1 I mentioned, the new since tags in CompletableFuture, FileInputStream, DelayQueue, FutureTask are not necessary: their docs are carried from the superclass/superinterfaces, and those superclass/superinterface methods already have the correct since tags.

[nizarbenalla]

Recent [offline] discussions have showed that dealing with @since in overriding methods is complicated. The solution is to add an explicit @since to some overriding methods that do not have any javadoc as the only @since we can infer is that of the enclosing class. The positive part is that these cases are very rare, and would help the checker have precise rules and match those of javadoc.

[liach]

Please consider this scenario where class A extends B, both from earlier versions, and there's B::method added in a new version X. Why are we adding a @since tag on the method A::method when it doesn't have its own doc and just refers to B::method, which already includes a @since tag?

For your convenience, I will reiterate with the javadoc output API specification instead of just the source code. These are most likely caused by bugs in your analyais tool:

1. CompletableFuture::exceptionallyComposeAsync CompletableFuture::resultNow etc. already have the correct @since tags inherited from superclass/superinterfaces javadocs. Notice if a method doesn't have javadoc, it carries the javadoc from its overridden method and it doesn't appear in this class's Method Summary section. https://docs.oracle.com/en/java/javase/22/docs/api/java.base/java/util/concurrent/CompletableFuture.html#methods-inherited-from-class-java.util.concurrent.CompletionStage Affected classes: CompletableFuture, ForkJoinTask, FutureTask, and ChoiceFormat.

2. FileInputStream::readNBytes is explicitly overridden in JDK 12 for a better implementation, but it is already a valid method since JDK 9 when InputStream::readNBytes was added. This @since 12 does not make sense. https://docs.oracle.com/en/java/javase/22/docs/api/java.base/java/io/FileInputStream.html#methods-inherited-from-class-java.io.InputStream Affected classes: FileInputStream, DelayQueue.

All other changes look correct.

[nizarbenalla]

• We will effectively enforce javadoc comment for some method overrides with the checker (we want to match the rules for javadoc tool which doesn't have any special handing for @since tags in inherited methods), and for that we need to fix some of the existing tags. But turns out it's not too many as we are only checking JDK 9-current.

• Regarding FileInputStream::readNBytes, the method int java.io.FileInputStream.readNBytes(byte[],int,int) was available since JDK 9. The one I added an @since 11 is a different method byte[] java.io.FileInputStream.readNBytes(int) with different return type and parameters. Link to JDK 9 docs with the method

[liach]

We will effectively enforce javadoc comment for some method overrides with the checker

Those overriding methods don't even appear on the javadoc output. If you go to search for CompletableFuture.resultNow on https://docs.oracle.com/en/java/javase/22/docs/api/ you will find nothing. Why are we fixing "broken since tags" that don't even exist in the first place?

Also have you looked at the output documentation? Without the @inheritDoc tags the content will only have a since tag, which is definitely wrong.

[liach]

FYI you can generate the documentation with make docs and upload it to https://cr.openjdk.org and link it for review purposes. (You just need to include the changed classes and the stylesheet.css)

I don't want to reiterate again, but if a method is declared so:

/**
 * Class One.
 *
 * @since 42
 */
public class One {
    /**
     * Method.
     *
     * @since 48
     */
    public void method() {}
}

/**
 * Class Two.
 *
 * @since 42
 */
public class Two extends One {
    @Override
    public void method() {}
}

The generated docs for Two will list method only in "Method declared in class One" section instead of the "Method Summary" section, and the link in "Method declared in ..." section links to the method declaration in class One where there's a correct @since version.

What's wrong with you that you ask Two::method to have a redundant javadoc and since tag?

[nizarbenalla]

We will effectively enforce javadoc comment for some method overrides with the checker

Those overriding methods don't even appear on the javadoc output. If you go to search for CompletableFuture.resultNow on https://docs.oracle.com/en/java/javase/22/docs/api/ you will find nothing. Why are we fixing "broken since tags" that don't even exist in the first place?

Also have you looked at the output documentation? Without the @inheritDoc tags the content will only have a since tag, which is definitely wrong.

For overriding methods we don't look into the supertype because that's what javadoc (the tool) is doing, javadoc doesn't look into the supertype and has no special handling or support for @since tags in inherited methods. If javadoc changes how it deals with method overriding in the future we will match it's behavior and look into the supertype for overriding methods. What is important is that we have to match the rules used in javadoc. You can't have both, either you add explicit javadoc comments to these methods or use rules that go against the current behavior or javadoc

[liach]

I'm sorry, you have always mentioned "match the javadoc rules" "javadoc doesn't look into supertype" "go against current behavior", but there is no problem with these @since tags in the current output documentation because those overriding methods without javadoc are treated as if they don't exist by javadoc tool.

For overriding methods we don't look into the supertype

We indeed don't because we treat it as if it does not exist, and then the current docs are right. Please, just take a look at the javadoc output once and reach your own conclusion.

[liach]

Recent [offline] discussions have showed that dealing with @since in overriding methods is complicated. The solution is to add an explicit @since to some overriding methods that do not have any javadoc as the only @since we can infer is that of the enclosing class.

If possible, I wish the others who joined the offline discussion can take part here. It seems you have some trouble understanding that overriding methods without explicit documentation are ignored by javadoc, and this ignorance makes the @since tags correct...

[nizarbenalla]

When I said "For overriding methods we don't look into the supertype" I meant my checker tool doesn't do that.

It seems you have some trouble understanding that overriding methods without explicit documentation are ignored by javadoc

This is still a Draft PR, I meant to add the {@inheritDoc} or other needed content to the javadoc and cleanup before opening. Should fix the issue you have with this change.

Others can join the discussion if they want, I'll let them know on monday but you might just get the same answer. For now the decision is that the every time an overload like the ones in this Draft PR is added, we would need to add explicit javadoc comment as we can't infer the @since from the overriden method.

[pavelrappo]

Also have you looked at the output documentation? Without the @inheritDoc tags the content will only have a since tag, which is definitely wrong.

This is not how I remember it. Unless written around, {@inheritDoc} in a main description is redundant. Let me clarify what I mean. Suppose we have a method:

/** Foos. */
foo()

and a couple of other methods that override that method:

@Override
foo()

/**
 */
@Override
foo()

/**
 * {@inheritDoc}
 */
@Override
foo()

Now, as far as the main description goes, the above three are equivalent, and the main description is inherited. While the third variant is arguably the most readable, its {@inheritDoc} is unnecessary. Explicit {@inheritDoc} is only necessary if we want to "write around" the inherited part. For example:

/**
 * Foos with extra steps.
 * 
 * {@inheritDoc}
 * 
 * Also bars if baz is true.
 */
@Override
foo()

In this case, the generated documentation would be as follows:

Foos with extra steps.
Foos.
Also bars if baz is true.

@since is a block tag. Block tags do not belong to the main description. So, if the goal is to only add a block tag, one does not need to implicitly {@inheritDoc} the main description. Does it make sense, Chen?

[liach]

Thanks for the explanation @pavelrappo. Also I recall inheritDoc has weird interactions with block tags like user-defined @apiNote in the JDK, would be nice if you can share more about these (also about the behaviors of inheriting @throws etc.

[pavelrappo]

would be nice if you can share more about these (also about the behaviors of inheriting @throws etc.

I hope this document explains it well; if it doesn't, we should fix it: https://docs.oracle.com/en/java/javase/22/docs/specs/javadoc/doc-comment-spec.html

[nizarbenalla]

I will run make docs-jdk-api before and after making changes, and compare the directories before opening. Might include the info in the PR body.

[mlbridge[bot]]

Webrevs

• 06: Full - Incremental (afca07bf)
• 05: Full - Incremental (91df97f7)
• 04: Full - Incremental (dbef517a)
• 03: Full - Incremental (b3574b97)
• 02: Full - Incremental (be91ab80)
• 01: Full - Incremental (d38a1c7d)
• 00: Full (62e623fa)

[liach]

For account on cr.openjdk.org, just noticed that you aren't listed on the census https://openjdk.org/census yet. Once you have 2 patches integrated into the JDK, you should ask lead of JDK project Mark Reinhold to become an author (See https://openjdk.org/guide/#becoming-an-author and https://openjdk.org/projects/#project-author) and then you can use cr to host files for review.

Also, I am sorry for being too aggressive in the conversations with you before.

[nizarbenalla]

Thanks, I read those two links yesterday and was thinking of which project lead to mail to be added to the census. Will do so.

[wangweij]

/label remove security

[openjdk[bot]]

@wangweij The security label was successfully removed.

[liach]

Just curious, what's the rationale for reverting the since tags on security default methods?

Edit: Just noticed they are covered by #18373 instead.

[mlbridge[bot]]

Mailing list message from Jonathan Gibbons on core-libs-dev:

While `@since` might not be considered a normative part of the specification, (it's effectively a cache of derived meta-data) it is part of the generated documentation, and as such deserves to be correct.

-- Jon

On 5/5/24 4:33 PM, Pavel Rappo wrote:

[jonathan-gibbons]

I disagree somewhat with the statements in the comments that the checker should follow the javadoc rules, whatever they are.

The important thing is to decide what the rules for @since should be, in terms of changes to the set of signatures in the class. Generally, I think the rule should be that every declaration should have @since except that members need not have the tag if it would be the same as for the enclosing class or interface. As far as adding an overriding method is concerned, if it has the same VM descriptor as the overridden method, it is not a "new" method in the class; if it has a covariant return type, that is a significant change to the descriptor and so such methods should have @since.

As a practical rule for deciding whether any declaration is new or not, imagine writing a test program that refers to the most specific form of the declaration. If that test program does not compile on JDK version N-1 and does compile on version N, then it warrants having @since N. Put another way, @since N should identify the first release in which the declaration can be used in the given form.

Note, these rules are stated without reference to what javadoc does. javadoc should follow these rules as well; it is a bug if the tool generates incorrect documentation based on @since tags following these rules.

Also, while the proposed new Since Checker should follow these rules when analysing declarations, it may go further when making suggestions to correct errors that it finds. For example, instead of simply saying No @since tag found here, it might analyze the history and say No @since tag found here; the declaration was introduced in X for an appropriate X.

[naotoj]

/reviewers 2 reviewer

[openjdk[bot]]

@naotoj The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 2 Reviewers).

[naotoj]

OK, I look at other areas and I believe they are fine. /reviewers 1 reviewer