Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 14 commits into from

Conversation

nizarbenalla
Copy link
Member

@nizarbenalla nizarbenalla commented Apr 25, 2024

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"

Screenshot 2024-05-06 at 00 06 57

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.

Screenshot 2024-05-06 at 00 07 16 Screenshot 2024-05-06 at 00 08 06 Screenshot 2024-05-06 at 00 09 03

TIA


Progress

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

Issue

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

Reviewers

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
Copy link

bridgekeeper bot commented Apr 25, 2024

👋 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
Copy link

openjdk bot commented Apr 25, 2024

@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:

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
Copy link

openjdk bot commented Apr 25, 2024

@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
Copy link
Member

liach commented Apr 25, 2024

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
Copy link
Member Author

nizarbenalla commented Apr 25, 2024

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
Copy link
Member

liach commented Apr 25, 2024

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
Copy link
Member Author

nizarbenalla commented Apr 26, 2024

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
Copy link
Member

liach commented Apr 26, 2024

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
Copy link
Member Author

nizarbenalla commented Apr 26, 2024

  • 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
Copy link
Member

liach commented Apr 26, 2024

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
Copy link
Member

liach commented Apr 26, 2024

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
Copy link
Member Author

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
Copy link
Member

liach commented Apr 27, 2024

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
Copy link
Member

liach commented Apr 27, 2024

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
Copy link
Member Author

nizarbenalla commented Apr 27, 2024

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.

Comment on lines 343 to 345
* @throws IllegalArgumentException {@inheritDoc}
* @throws IOException {@inheritDoc}
* @throws OutOfMemoryError {@inheritDoc}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re: inheriting unchecked exception documentation here and elsewhere in this PR

This PR's title suggests that the PR has nothing to do with exception documentation. If you feel that it should be addressed, please file a separate bug and move these changes there.

FWIW, I agree that @throws ... {@inheritDoc} for such exceptions are at least worth being considered. It's a common misconception that all exception documentation is inherited automatically.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is because the tool from #18934 has no easy way to fetch the doc comment from a superclass/superinterface overridden method when this class only has a plain override. Javadoc handles this in complex logic in VisibleMemberTable; it's hard for other clients to try to emulate the behavior of doc finding, and the tool just incorrectly assumes such methods (what I have been talking about before) are reusing @since from the class docs.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pavel, can I simply change the PR/issue title to be more descriptive?
As I want to include the the inherit doc because of the unchecked exceptions, rather than clean this up in a different PR

Copy link
Member

@pavelrappo pavelrappo Apr 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pavel, can I simply change the PR/issue title to be more descriptive? As I want to include the the inherit doc because of the unchecked exceptions, rather than clean this up in a different PR

I suggest dropping all the changes that are irrelevant to @since from this PR, unless you have PRs with similar mixed changes integrated. As far as I know, @since is not a normative part of documentation, whereas @throws is. So the latter is a bigger change and would require more scrutiny and more area experts during review.

In the future, we might want to have yet another aid/tool: a "@throws checker" that finds all overriding methods that do not declare some of the unchecked exceptions that the methods they override do.

@pavelrappo
Copy link
Member

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
Copy link
Member

liach commented Apr 29, 2024

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.

@@ -1,5 +1,5 @@
/*
* Copyright (c) 1998, 2023, Oracle and/or its affiliates. All rights reserved.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This diff is redundant but no-op. You should merge openjdk/jdk's master branch to your PR branch, so the diff displayed by GitHub is up-to-date and this will go away.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't want to merge or rebase on an active PR. It should get fixed once this is integrated.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, this comment serves as a note to reviewers that these 2 header changes have been committed in other changes and thus can be safely ignored.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no harm in merging the upstream master, using git merge, to bring your branch more up-to-date. And it can be helpful in cases like this where it might help reviewers avoid extraneous information. It is also recommended in other cases (e.g., a long-running PR where there is a lot of drift between the PR source branch and the upstream target branch).

Skara will squash all commits anyway when integrating so the results will be identical.

Btw, one thing you should not do is rebase a source branch of an active PR.

@nizarbenalla nizarbenalla changed the title 8330954: Fix remaining @since tags in java.base 8330954: since-checker - Fix remaining @since tags in java.base Jun 3, 2024
@jonathan-gibbons
Copy link
Contributor

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.

@nizarbenalla nizarbenalla changed the title 8330954: since-checker - Fix remaining @since tags in java.base 8330954: since-checker - Fix remaining @ since tags in java.base Jun 4, 2024
@@ -41,7 +41,10 @@ public sealed interface ClassSignature
/** {@return the type parameters of this class} */
List<Signature.TypeParam> typeParameters();

/** {@return the instantiation of the superclass in this signature} */
/** {@return the instantiation of the superclass in this signature}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/** {@return the instantiation of the superclass in this signature}
/**
* {@return the instantiation of the superclass in this signature}

I think this is our preference if we have multi-line specs.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 91df97f, Thanks.

Comment on lines +589 to +591
/**
* @since 23
*/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIUC, this addition will add & inherit the javadoc from NumberFormat, which is not the case before. The description for NumberFormat does not fit with ChoiceFormat. Probably that needs to be addressed with a different issue.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My review is for the ChoiceFormat class only. So asking for more Reviews for other area
/reviewers 2 reviewer

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I will leave this open for a few more days to let more people from the relevant areas review it.

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Jun 26, 2024
@naotoj
Copy link
Member

naotoj commented Jun 26, 2024

/reviewers 2 reviewer

@openjdk
Copy link

openjdk bot commented Jun 26, 2024

@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).

@openjdk openjdk bot removed the ready Pull request is ready to be integrated label Jun 26, 2024
@naotoj
Copy link
Member

naotoj commented Jul 3, 2024

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

@openjdk
Copy link

openjdk bot commented Jul 3, 2024

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

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Jul 3, 2024
@nizarbenalla
Copy link
Member Author

Thank you Naoto for checking, and thanks Chen!

/integrate

@openjdk openjdk bot added the sponsor Pull request is ready to be sponsored label Jul 4, 2024
@openjdk
Copy link

openjdk bot commented Jul 4, 2024

@nizarbenalla
Your change (at version afca07b) is now ready to be sponsored by a Committer.

@liach
Copy link
Member

liach commented Jul 4, 2024

/sponsor

@openjdk
Copy link

openjdk bot commented Jul 4, 2024

Going to push as commit f4fa35e.
Since your change was applied there have been 73 commits pushed to the master branch:

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label Jul 4, 2024
@openjdk openjdk bot closed this Jul 4, 2024
@openjdk openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review sponsor Pull request is ready to be sponsored labels Jul 4, 2024
@openjdk
Copy link

openjdk bot commented Jul 4, 2024

@liach @nizarbenalla Pushed as commit f4fa35e.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

@nizarbenalla nizarbenalla deleted the 8330954 branch September 19, 2024 00:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

7 participants