jdk icon indicating copy to clipboard operation
jdk copied to clipboard

8332103: Add missing `@since` tags to `java.desktop`

Open nizarbenalla opened this issue 1 year ago • 4 comments
trafficstars

If you're currently reviewing this PR, thank you! Most fixes here are according to the reports by the since checker tool in #18934 and are pretty simple.

To make reviewing easier

  • BasicSliderUI has the constructor public BasicSliderUI(JSlider b) for a long time so the default constructor (without parameters) didn't exist until JDK 16

For the package-info files, it is pretty hard to find source code of JDK 1-5 so I used the grep command to find the oldest instance of an @since in those packages.

I found instances of @since 1.1 in the other packages but javax/swing/plaf/synth/package-info.java might be worth checking as most classes there had no @since.


Progress

  • [ ] 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-8332103: Add missing @<!---->since tags to java.desktop (Bug - P4)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 19192

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

Using diff file

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

Webrev

Link to Webrev Comment

nizarbenalla avatar May 11 '24 17:05 nizarbenalla

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

bridgekeeper[bot] avatar May 11 '24 17:05 bridgekeeper[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:

8332103: since-checker - Add missing @ since tags to java.desktop

Reviewed-by: tr, aivanov

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 2 new commits pushed to the master branch:

  • f23295ec1dde58d239a2625c9b1645534a2bb625: 8334600: TEST java/net/MulticastSocket/IPMulticastIF.java fails on linux-aarch64
  • 741a0f39dd1fffc1caaa8d69bfe3662dad830452: 8334241: Adjust API docs side bar dimensions

Please see this link for an up-to-date comparison between the source branch of this pull request and 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 (@TejeshR13, @aivanov-jdk) 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] avatar May 11 '24 17:05 openjdk[bot]

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

  • client

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

openjdk[bot] avatar May 11 '24 17:05 openjdk[bot]

Webrevs

mlbridge[bot] avatar May 12 '24 18:05 mlbridge[bot]

I'm not sure I understand the methodology here. I think there must be hundreds of similar missing tags and this seems to be just a few random ones that aren't always correct.

prrace avatar May 14 '24 21:05 prrace

As I'm using the historical data built into javac to determine the correct @since tag to be used, I can only check code added after JDK 9. So a lot of errors in "older" code will go unnoticed, but I can make sure new tags are correct. When an @since is missing from an element added in newer JDK releases I get an accurate error message i.e

method: void javax.swing.JSlider.AccessibleJSlider.stateChanged(javax.swing.event.ChangeEvent): @since version is 9 instead of 16

but for older code you can only guess "Element: X existed before JDK 10". So I was left to check on my own, and made a mistake.

nizarbenalla avatar May 14 '24 23:05 nizarbenalla

but for older code you can only guess "Element: X existed before JDK 10". So I was left to check on my own, and made a mistake.

Why is it? There's history beyond 10 and 9, yet accessing it requires more effort. In addition to that, old JDK releases are available in an archive so that you can verify if a method existed in a certain release.

If all the methods need @since marker, it has to be accurate and represent the real picture rather than an estimate.

aivanov-jdk avatar May 29 '24 11:05 aivanov-jdk

Why is it? There's history beyond 10 and 9

As I've explained before, a program relying on the historical data built into javac can only give accurate reports for newer releases. As that data only goes back so far. I was left to investigate and couldn't find source code of JDK 1-2 internally after some digging.

To fix old code, we would need to find a new way to map elements. I don't think this would need to be added to the checker tool as checking old code doesn't need to run regularly. It can be a one time script, fix it once and be done with this. It's out of scope for my current work.

The good thing is that once these tags (in this PR) are fixed, this will no longer be an issue and the errors reported will be clear and simple. As we would only be dealing with missing/wrong information in current releases.

I agree I should've looked more, because the @since values deserve to be correct. (I am not disputing that I made a mistake)

nizarbenalla avatar May 29 '24 21:05 nizarbenalla

here is the report from the @since checker tool java.desktop

        SinceChecker java.desktop
STDERR:
method: void java.awt.geom.Path2D.Double.trimToSize(): `@since` version is 9 instead of 10
method: void java.awt.geom.Path2D.Float.trimToSize(): `@since` version is 9 instead of 10
Unable to retrieve `@since` for Package: package: java.beans
Unable to retrieve `@since` for Package: package: javax.swing
method: void javax.swing.DefaultListModel.addAll(java.util.Collection): `@since` version is 9 instead of 11
method: void javax.swing.DefaultListModel.addAll(int,java.util.Collection): `@since` version is 9 instead of 11
method: void javax.swing.JSlider.AccessibleJSlider.stateChanged(javax.swing.event.ChangeEvent): `@since` version is 9 instead of 16
method: int javax.swing.JSlider.AccessibleJSlider.getAccessibleActionCount(): `@since` version is 9 instead of 17
method: java.lang.String javax.swing.JSlider.AccessibleJSlider.getAccessibleActionDescription(int): `@since` version is 9 instead of 17
method: boolean javax.swing.JSlider.AccessibleJSlider.doAccessibleAction(int): `@since` version is 9 instead of 17
method: void javax.swing.DefaultComboBoxModel.addAll(java.util.Collection): `@since` version is 9 instead of 11
method: void javax.swing.DefaultComboBoxModel.addAll(int,java.util.Collection): `@since` version is 9 instead of 11
method: void javax.swing.plaf.basic.BasicSliderUI.<init>(): `@since` version is 9 instead of 16
Unable to retrieve `@since` for Package: package: javax.swing.plaf.synth
field: javax.swing.text.DefaultEditorKit:beginLineUpAction: `@since` version is 9 instead of 20
field: javax.swing.text.DefaultEditorKit:endLineDownAction: `@since` version is 9 instead of 20
java.lang.Exception: The `@since` checker found 16 problems
        at SinceChecker.checkModule(SinceChecker.java:262)
        at SinceChecker.main(SinceChecker.java:123)
        at java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:103)
        at java.base/java.lang.reflect.Method.invoke(Method.java:580)
        at com.sun.javatest.regtest.agent.MainActionHelper$AgentVMRunnable.run(MainActionHelper.java:333)
        at java.base/java.lang.Thread.run(Thread.java:1575)

JavaTest Message: Test threw exception: java.lang.Exception
JavaTest Message: shutting down test


TEST RESULT: Failed. Execution failed: `main' threw exception: java.lang.Exception: The `@since` checker found 16 problems

nizarbenalla avatar Jun 03 '24 23:06 nizarbenalla

method: void java.awt.geom.Path2D.Double.trimToSize(): @since version is 9 instead of 10 method: void java.awt.geom.Path2D.Float.trimToSize(): @since version is 9 instead of 10

In JDK 10, a new method trimToSize was added to Path2D. It is marked with @since 10.

Path2D.Float and Path2D.Double override the method or rather implement it. Does this require an explicit @since tag? I'm unsure about it.

aivanov-jdk avatar Jun 04 '24 16:06 aivanov-jdk

Referring to the discussion about BasicSliderUI() constructor:

Hmm, the explicit default constructor was added in JDK 16, but it was implicit before then.

I believe there was no default constructor in BasicSliderUI() because there was a constructor with a parameter BasicSliderUI(JSlider b).

Thus, this case seem to be correct BasicSliderUI() is available since 16.

It seems that BasicSliderUI() was added by the mistake? it was not mentioned in the bug report... [JDK-8250852] Seems it is too late to delete it?

I agree. It shouldn't have been added.

Instead of adding @since, the constructor should be removed. It requires a CSR.

The longer it exists, the more chances there are that it's used.

Taking all these points into account all these points above, adding @since 16 to the no-argument constructor of BasicSliderUI() is correct.

How do we remove this constructor? Can it be removed right away? Should it be deprecated for several releases before it's removed?

@prrace @prsadhuk @mrserb

aivanov-jdk avatar Jun 12 '24 11:06 aivanov-jdk

How do we remove this constructor? Can it be removed right away? Should it be deprecated for several releases before it's removed?

Just delete it in all versions of 17+?

mrserb avatar Jun 12 '24 20:06 mrserb

How do we remove this constructor? Can it be removed right away? Should it be deprecated for several releases before it's removed?

Just delete it in all versions of 17+?

Now it is part of Java 17 and 21. It can't be removed from these releases, I believe.

Can it be removed quickly from the upcoming JDK 23? Probably, not as well.

aivanov-jdk avatar Jun 18 '24 17:06 aivanov-jdk

How do we remove this constructor? Can it be removed right away? Should it be deprecated for several releases before it's removed?

Just delete it in all versions of 17+?

Now it is part of Java 17 and 21. It can't be removed from these releases, I believe.

Can it be removed quickly from the upcoming JDK 23? Probably, not as well.

I've submitted the follow-up bugs to deal with the no-arg constructor BasicSliderUI():

  1. JDK-8334580: Deprecate no-arg constructor BasicSliderUI() for removal
  2. JDK-8334581: Remove no-arg constructor BasicSliderUI()

The plan is to deprecate BasicSliderUI() for removal in 23 and then to remove it in 25 or, if possible, in 24.

aivanov-jdk avatar Jun 20 '24 13:06 aivanov-jdk

Thank you Alexey and Tejesh for your reviews!

/integrate

nizarbenalla avatar Jun 24 '24 13:06 nizarbenalla

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

openjdk[bot] avatar Jun 24 '24 13:06 openjdk[bot]

Let me add a new commit to remove the sponsor label

nizarbenalla avatar Jun 24 '24 19:06 nizarbenalla

Let me add a new commit to remove the sponsor label

Thank you!

aivanov-jdk avatar Jun 24 '24 20:06 aivanov-jdk

Thank you Aleksei!

/Integrate

nizarbenalla avatar Jun 26 '24 10:06 nizarbenalla

/integrate

nizarbenalla avatar Jun 26 '24 11:06 nizarbenalla

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

openjdk[bot] avatar Jun 26 '24 11:06 openjdk[bot]

/sponsor

aivanov-jdk avatar Jun 26 '24 14:06 aivanov-jdk

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

  • 8374d16504503c7441346c99045736b7ac72233f: 8335006: C2 SuperWord: add JMH benchmark VectorLoadToStoreForwarding.java
  • 4ffc5e60776353b03e9a557c39148e378b1690e2: 8326705: Test CertMsgCheck.java fails to find alert certificate_required
  • efb905e57ab7a5299952419fa9961316541056c2: 8334618: ubsan: support setting additional ubsan check options
  • b2ac7259c96f154ba0ca54fd47b37caaa8c8647b: 8327380: Add tests for Shenandoah barrier expansion optimization
  • f23295ec1dde58d239a2625c9b1645534a2bb625: 8334600: TEST java/net/MulticastSocket/IPMulticastIF.java fails on linux-aarch64
  • 741a0f39dd1fffc1caaa8d69bfe3662dad830452: 8334241: Adjust API docs side bar dimensions

Your commit was automatically rebased without conflicts.

openjdk[bot] avatar Jun 26 '24 14:06 openjdk[bot]

@aivanov-jdk @nizarbenalla Pushed as commit 8591eff78dbc9770b8d0a16e05040ac35c99881a.

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

openjdk[bot] avatar Jun 26 '24 14:06 openjdk[bot]