Skip to content

Conversation

@pavelrappo
Copy link
Member

@pavelrappo pavelrappo commented Sep 5, 2024

This fixes some of the recently discovered issues with the block variants of the specification tags. While reviewing, please check the proposed changes against the actual specifications. Since the specifications for JDK 23 are not yet available in the HTML format, you can use the specifications for JDK 22, which are reasonably up-to-date:

Note that this PR does NOT address any issues with the inline variants of the said tags. Those are harder to check. Even flagging suspicious tags requires a human. If you have some time, consider performing similar checks for inline @jls and @jvms tags in your area. Thanks.


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-8339631: Fix block @jls and @jvms tags (Enhancement - P4)

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 20879

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

Using diff file

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

Webrev

Link to Webrev Comment

@bridgekeeper
Copy link

bridgekeeper bot commented Sep 5, 2024

👋 Welcome back prappo! 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 Sep 5, 2024

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

8339631: Fix block @jls and @jvms tags

Reviewed-by: liach, darcy, jjg

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

  • 615a24f: 8338902: CDS flags are reported with wrong flag category
  • 347d572: 8339387: ZGC: Synchronize medium page allocation
  • 4ff72dc: 8339487: ProcessHandleImpl os_getChildren sysctl call - retry in case of ENOMEM and enhance exception message
  • cb5c60b: 8339591: Mark jdk/jshell/ExceptionMessageTest.java intermittent
  • b45fe17: 8339710: Avoid initializing AccessFlag related classes in write-only cases
  • a18d9d8: 8326616: tools/javac/patterns/Exhaustiveness.java intermittently Timeout signalled after 480 seconds
  • 79d7613: 8338153: java/awt/Checkbox/CheckboxCheckerScalingTest.java test failed on linux machine
  • f0e84b7: 8339703: Problem list serviceability/sa/TestJhsdbJstackUpcall.java for generational ZGC
  • deeb09a: 8339307: jhsdb jstack could not trace FFM upcall frame
  • fbe2629: 8339635: StringConcatFactory optimization for CompactStrings off
  • ... and 14 more: https://git.openjdk.org/jdk/compare/48d79431c95759954f6dd283de78fe9f9fe9370a...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.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk openjdk bot added the rfr Pull request is ready for review label Sep 5, 2024
@openjdk
Copy link

openjdk bot commented Sep 5, 2024

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

  • compiler
  • core-libs
  • kulla

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.

@openjdk openjdk bot added core-libs core-libs-dev@openjdk.org compiler compiler-dev@openjdk.org kulla kulla-dev@openjdk.org labels Sep 5, 2024
Copy link
Member

@jddarcy jddarcy left a comment

Choose a reason for hiding this comment

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

Looks fine; thanks.

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Sep 5, 2024
@mlbridge
Copy link

mlbridge bot commented Sep 5, 2024

Webrevs

Copy link
Contributor

@jonathan-gibbons jonathan-gibbons left a comment

Choose a reason for hiding this comment

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

possibly for later, a separate pass might be to review and make consistent the use of {@code } in the tags, such as in The ... Attribute

@liach
Copy link
Member

liach commented Sep 5, 2024

possibly for later, a separate pass might be to review and make consistent the use of {@code } in the tags, such as in The ... Attribute

For example, the addition @jls 15.20.2 The instanceof Operator in this patch does not wrap instanceof in a code tag.

@pavelrappo
Copy link
Member Author

possibly for later, a separate pass might be to review and make consistent the use of {@code } in the tags, such as in The ... Attribute

For example, the addition @jls 15.20.2 The instanceof Operator in this patch does not wrap instanceof in a code tag.

Typography issues are less severe than those fixed in this PR, but I can fix them too.

8.5.1 was removed in JDK 16. 8.1.3 seems an appropriate substitution.
Alternatively, the link can be deleted altogether.
@openjdk openjdk bot removed the ready Pull request is ready to be integrated label Sep 6, 2024
@pavelrappo
Copy link
Member Author

I understand that a PR might not be the best place for a comment like this. Still, it's highly relevant to the review feedback I've got so far. Also, a PR comment will be seen by more people sooner than a JBS comment. This is by the virtue of being fanned out by the mailing lists.

Some reviewers suggested fixing the typography. I could do it, but I think it will pay us more if we pause and think what the broken typography tells us, which I think is that the design of these tags is not entirely what it should be.

Currently, these tags are really a specialised form of @see and @linkplain. The block variant corresponds to @see and the inline variant corresponds to @linkplain.

Since the tag links to a specific resource, it saves the documentation author keystrokes on typing the link to that resource. Along with the visual consistency of the generated documentation, the author also gets some integrity: the tag links to the same version of the specification as that of the documentation.

Unfortunately, mismatching versions is not the only source of broken integrity. Even if initially the tag is accurate, it may degrade over time by various means. This PR is a good evidence that specification sections can be reordered and that their text can be changed. The problem is not the degradation per se, but that it stays unnoticed indefinitely.

In similar situations, we use external checkers as a post-build documentation test. While it's much better than nothing and is more flexible than changing the tags, failing tests are prone to stay problem-listed indefinitely. Which gets us back to square one.

If like me you think that a broken or irrelevant link is bad, we could make these tags provide more integrity. The tags could check their contents against the actual specification and report an error if the contents are not the same (subject to some typographic and formatting leeway). As for the visual consistency, the tags could also make sure the text they generate is of a particular form by automatically carrying over (some) typography from the specification to the generated documentation.

In my mind, this will require having (an extract from) the specification accessible to the tag. Because build system don't typically have access to the Internet, the specification could be stored in the repo and updated every release, which happens every 6 months.

One problem though is what to do when (not if) a spec is not up-to-date. For example, consider these @jls tags in JDK 23:

package java.lang.runtime;
   ...
 * @jls 5.7.1 Exact Testing Conversions
 * @jls 5.7.2 Unconditionally Exact Testing Conversions
   ...
 * @since 23
 */
public final class ExactConversionsSupport {

These tags relate to a preview feature, which is documented not in JLS but in an annex to it, which is currently missing due to a build error. If the tags are to strictly check their integrity, a problem like this would prevent a documentation build.

Thoughts?

@jonathan-gibbons
Copy link
Contributor

jonathan-gibbons commented Sep 6, 2024

Thoughts?

My initial thought is, at least we're working with specific tags (@jls and @jvms) rather than generic HTML links (<a href=...>...</a>), since that permits the kind of detailed analysis and checking you are doing. And yes, any and all checking we can do to ensure the accuracy of our documentation is to be commended. Broken links are bad.

Preview-ness is a pervasive feature of JDK these days, on the command-line and in APIs. Perhaps the tags here can be augmented to indicate that a feature is a preview feature, for example, @jls preview 5.7.2 Unconditionally Exact Testing Conversions. This could be used not only to affect the generated output, perhaps with PREVIEW, but could be used to help both determine the target for such links, and to review such links in subsequent releases. Such management should be part of the responsibilities of the preview-feature owner.

Copy link
Member

@liach liach left a comment

Choose a reason for hiding this comment

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

I think we can ignore the title style aspect for now; the key of these tags has always been to provide a correct link to the specs, so fixing trailing period or section number is more important.

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Sep 6, 2024
Note: any commit hashes below might be outdated due to subsequent
history rewriting (e.g. git rebase).

 + update src/java.base/share/classes/java/lang/ClassLoader.java due to 51ffa5e
 + update src/java.base/share/classes/java/lang/Record.java due to 51ffa5e
 + update src/java.base/share/classes/java/lang/StackWalker.java due to 51ffa5e
 + update src/java.base/share/classes/java/lang/reflect/AccessFlag.java due to 51ffa5e
 + update src/java.base/share/classes/java/lang/reflect/InvocationHandler.java due to 51ffa5e
 + update src/java.compiler/share/classes/javax/lang/model/element/NestingKind.java due to 51ffa5e
 + update src/java.compiler/share/classes/javax/lang/model/type/NullType.java due to 51ffa5e
 + update src/jdk.compiler/share/classes/com/sun/source/tree/ClassTree.java due to 51ffa5e
 + update src/jdk.compiler/share/classes/com/sun/source/tree/InstanceOfTree.java due to 51ffa5e
 + update src/jdk.compiler/share/classes/com/sun/source/tree/LiteralTree.java due to 51ffa5e
 + update src/jdk.compiler/share/classes/com/sun/source/tree/MethodTree.java due to 51ffa5e
 + update src/jdk.compiler/share/classes/com/sun/source/tree/ModifiersTree.java due to 2c3d47a
 + update src/jdk.compiler/share/classes/com/sun/source/tree/StatementTree.java due to 51ffa5e
 + update src/jdk.compiler/share/classes/com/sun/source/tree/SwitchExpressionTree.java due to 51ffa5e
 + update src/jdk.compiler/share/classes/com/sun/source/tree/VariableTree.java due to 51ffa5e
@pavelrappo
Copy link
Member Author

/integrate

@openjdk openjdk bot removed the ready Pull request is ready to be integrated label Sep 9, 2024
@openjdk
Copy link

openjdk bot commented Sep 9, 2024

@pavelrappo This pull request has not yet been marked as ready for integration.

@pavelrappo
Copy link
Member Author

Ugh... The most recent change to copyright years caused the bot to remove the "ready" label. Please re-review; thanks.

@pavelrappo
Copy link
Member Author

/integrate

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Sep 9, 2024
@openjdk
Copy link

openjdk bot commented Sep 9, 2024

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

  • 615a24f: 8338902: CDS flags are reported with wrong flag category
  • 347d572: 8339387: ZGC: Synchronize medium page allocation
  • 4ff72dc: 8339487: ProcessHandleImpl os_getChildren sysctl call - retry in case of ENOMEM and enhance exception message
  • cb5c60b: 8339591: Mark jdk/jshell/ExceptionMessageTest.java intermittent
  • b45fe17: 8339710: Avoid initializing AccessFlag related classes in write-only cases
  • a18d9d8: 8326616: tools/javac/patterns/Exhaustiveness.java intermittently Timeout signalled after 480 seconds
  • 79d7613: 8338153: java/awt/Checkbox/CheckboxCheckerScalingTest.java test failed on linux machine
  • f0e84b7: 8339703: Problem list serviceability/sa/TestJhsdbJstackUpcall.java for generational ZGC
  • deeb09a: 8339307: jhsdb jstack could not trace FFM upcall frame
  • fbe2629: 8339635: StringConcatFactory optimization for CompactStrings off
  • ... and 14 more: https://git.openjdk.org/jdk/compare/48d79431c95759954f6dd283de78fe9f9fe9370a...master

Your commit was automatically rebased without conflicts.

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

openjdk bot commented Sep 9, 2024

@pavelrappo Pushed as commit 88cccc1.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

compiler compiler-dev@openjdk.org core-libs core-libs-dev@openjdk.org integrated Pull request has been integrated kulla kulla-dev@openjdk.org

Development

Successfully merging this pull request may close these issues.

4 participants