-
Notifications
You must be signed in to change notification settings - Fork 5.8k
8326951: since-checker - missing @ since tags #18055
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
Conversation
Hi @nizarbenalla, welcome to this OpenJDK project and thanks for contributing! We do not recognize you as Contributor and need to ensure you have signed the Oracle Contributor Agreement (OCA). If you have not signed the OCA, please follow the instructions. Please fill in your GitHub username in the "Username" field of the application. Once you have signed the OCA, please let us know by writing If you already are an OpenJDK Author, Committer or Reviewer, please click here to open a new issue so that we can record that fact. Please use "Add GitHub user nizarbenalla" as summary for the issue. If you are contributing this work on behalf of your employer and your employer has signed the OCA, please let us know by writing |
@nizarbenalla The following labels will be automatically applied to this pull request:
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. |
|
You are already a known contributor! |
@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:
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 294 new commits pushed to the
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 (@jaikiran) but any other Committer may sponsor as well. ➡️ To flag this PR as ready for integration with the above commit message, type |
Webrevs
|
@@ -65,7 +65,6 @@ public interface DHPublicKey extends DHKey, java.security.PublicKey { | |||
* The default implementation returns {@code null}. | |||
* | |||
* @return {@inheritDoc java.security.AsymmetricKey} | |||
* @since 22 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello Nizar, are the removal of @since
in this PR intentional? I haven't checked all of them, but this specific removal appears incorrect, since this method was indeed introduced in Java 22 as part of https://bugs.openjdk.org/browse/JDK-8318096.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello Jaikiran,
in jdk21 DHPPublicKey did have a getParams() method, so it is not new in jdk 22. It also existed before that
If you haven't looked at the other cases, I was about to group the changes related to the Key interfaces in a separate PR if that's fine. let me know what you think
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello,
While the override of getParams
in DHPublicKey
was added, the getParams
method has been inherited to DHPublicKey
for a long time from DHKey
. E.g., I can take this:
import javax.crypto.interfaces.DHPublicKey;
public class DhkeyTest {
public static void main(DHPublicKey key) {
System.err.println(key.getParams());
}
}
and compile using JDK 8 without any compile-time errors. So, it would make more sense to me to not add the @since
for it.
I believe Nizar will separate the changes to the Key interfaces into a separate PR, so we can discuss in more detail there.
Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello Jan, interesting. I hadn't noticed that javax.crypto.interfaces.DHPublicKey
already was exposing getParams()
in earlier versions because javax.crypto.interfaces.DHPublicKey
extends from javax.crypto.interfaces.DHKey
which has the getParams()
method.
I believe Nizar will separate the changes to the Key interfaces into a separate PR, so we can discuss in more detail there.
That sounds fine to me.
@since
Tags
@since
tags@since
tags
@since
tags
Here is a snippet from the report of the checker with the relevant bits for this PR
Here is the full report
|
@@ -679,8 +679,6 @@ private void implWrite(byte[] buf, int off, int len) throws IOException { | |||
* | |||
* @see #writeBytes(byte[]) | |||
* @see #write(byte[],int,int) | |||
* | |||
* @since 14 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The @since 14
was added here as part of https://bugs.openjdk.org/browse/JDK-8187898. The CSR explains what the changes were https://bugs.openjdk.org/browse/JDK-8230625. As noted in that CSR's specification (and attached images), the change for this method in that issue ended up being just API clarification (through a @apiNote
) and no other change to this specific method. It continued to have the same signature including the throws clause that was previously available on this class through the super FilterOutputStream#write(byte[])
method.
So removing this @since 14
looks correct to me.
@@ -7919,6 +7919,8 @@ private static void tryFinallyChecks(MethodHandle target, MethodHandle cleanup) | |||
* handles is not {@code int}, or if the types of | |||
* the fallback handle and all of target handles are | |||
* not the same. | |||
* | |||
* @since 17 | |||
*/ | |||
public static MethodHandle tableSwitch(MethodHandle fallback, MethodHandle... targets) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method was introduced in Java 17 through https://bugs.openjdk.org/browse/JDK-8263087. So this addition of @since 17
looks fine to me.
@@ -50,6 +50,8 @@ public MalformedParameterizedTypeException() { | |||
* Constructs a {@code MalformedParameterizedTypeException} with | |||
* the given detail message. | |||
* @param message the detail message; may be {@code null} | |||
* | |||
* @since 10 | |||
*/ | |||
public MalformedParameterizedTypeException(String message) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both this and the explicit no-arg constructor were introduced in this class through https://bugs.openjdk.org/browse/JDK-8183175 in Java 10. Since an (implicit) no-arg constructor was always available even before that change, it makes sense to add a @since 10
to only this explicit constructor. This change looks good.
@@ -397,6 +397,8 @@ public final MappedByteBuffer rewind() { | |||
* {@code force()} on the returned buffer, will only act on the sub-range | |||
* of this buffer that the returned buffer represents, namely | |||
* {@code [position(),limit())}. | |||
* | |||
* @since 17 | |||
*/ | |||
@Override | |||
public abstract MappedByteBuffer slice(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This and the other 3 methods on which this @since 17
is being added here were introduced in Java 17, through https://bugs.openjdk.org/browse/JDK-4833719. Before that change, these methods would have been available on this MappedByteBuffer
through the super ByteBuffer
class and those methods have been specified to return a ByteBuffer
. After that change in JDK-4833719, these methods now return a MappedByteBuffer
which is an observable change. So adding @since 17
to these methods looks correct to me.
@@ -185,6 +185,7 @@ public Properties() { | |||
* accommodate this many elements | |||
* @throws IllegalArgumentException if the initial capacity is less than | |||
* zero. | |||
* @since 10 | |||
*/ | |||
public Properties(int initialCapacity) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This constructor was introduced in Java 10 through https://bugs.openjdk.org/browse/JDK-8189319, so adding @since 10
here looks correct to me.
@@ -506,6 +506,8 @@ public float nextFloat() { | |||
* {@inheritDoc} | |||
* @throws IllegalArgumentException {@inheritDoc} | |||
* @implNote {@inheritDoc} | |||
* | |||
* @since 17 | |||
*/ | |||
@Override | |||
public float nextFloat(float bound) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These 2 nextFloat(...)
methods which take parameters were introduced in Java 17 https://bugs.openjdk.org/browse/JDK-8248862, whereas the no-arg nextFloat()
method existed prior to that. So adding @since 17
to these 2 methods looks correct to me.
@@ -336,6 +336,8 @@ public void setDictionary(byte[] dictionary) { | |||
* @param dictionary the dictionary data bytes | |||
* @see Inflater#inflate | |||
* @see Inflater#getAdler() | |||
* | |||
* @since 11 | |||
*/ | |||
public void setDictionary(ByteBuffer dictionary) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method was introduced in Java 11 through https://bugs.openjdk.org/browse/JDK-6341887. It appears to be an oversight that a @since 11
wasn't added to this method in that change, because other new methods introduced in that change did come with a @since 11
. So this addition of @since 11
now looks fine to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These changes look good to me. Thank you Nizar for the work on the @since
checker and these PRs in individual areas.
Thanks for the thorough review! /integrate |
/integrate |
1 similar comment
/integrate |
@nizarbenalla |
/sponsor |
@nizarbenalla |
@nizarbenalla |
Going to push as commit e0bab78.
Your commit was automatically rebased without conflicts. |
@jaikiran @nizarbenalla Pushed as commit e0bab78. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
I added
@since
tags for methods/constructors that do not match the@since
of the enclosing class.The
write
method already existed inPrintStream
in earlier versions and instances of it could always call this method, since it extendsFilterOutputStream
which has the method.for
MappedByteBuffer slice()
andMappedByteBuffer slice(int index, int length)
, the return type changed fromByteBuffer
toMappedByteBuffer
. And the checker tool differentiates between them because of that.This is similar to #18032 and #18373
For context, I am writing tests to check for accurate use of
@since
tags in documentation comments in source code.We're following these rules for now:
Rule 1: Introduction of New Elements
@since N
.@since
if their version matches the value specified for the enclosing class or interface.Rule 2: Existing Elements in Subsequent JDK Versions
@since N
.Rule 3: Handling Missing
@since
Tags in methods if there is no@since
When inspecting methods, prioritize the
@since
annotation of the supertype's overridden method.If unavailable or if the enclosing class's
@since
is newer, use the enclosing element's@since
.I.e. if A extends B, and we add a method to B in JDK N, and add an override of the method to A in JDK M (M > N), we will use N as the effective
@since
for the method.Progress
Warning
8326951: since-checker - missing @ since tags
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/18055/head:pull/18055
$ git checkout pull/18055
Update a local copy of the PR:
$ git checkout pull/18055
$ git pull https://git.openjdk.org/jdk.git pull/18055/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 18055
View PR using the GUI difftool:
$ git pr show -t 18055
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/18055.diff
Webrev
Link to Webrev Comment