Skip to content
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

8308715: Create a mechanism for Implicitly Declared Class javadoc #16853

Closed
wants to merge 8 commits into from

Conversation

pavelrappo
Copy link
Member

@pavelrappo pavelrappo commented Nov 28, 2023

Please review this PR to support JEP 463 Implicitly Declared Classes and Instance Main Method (Second Preview) in JavaDoc.

JEP 463 is the next iteration of JEP 445, which introduced the ability to have a source file as a launchable, "classless" method bag

% cat HelloWorld.java
/** Run me. */
void main() {
    print("Hello, world!");
}

/** Shortcut for printing strings. */
void print(String arg) {
    System.out.println(arg);
}

which the compiler interprets as a familiar class

final class HelloWorld {

    HelloWorld() {
    }

    /** Run me. */
    void main() {
        print("Hello, world!");
    }

    /** Shortcut for printing strings. */
    void print(String arg) {
        System.out.println(arg);
    }    
}

How JEP 445 works with JavaDoc today

In JDK 21, javadoc can document such a file without any changes to the javadoc tool. The only thing that the user needs to do is to make sure that the following options are present:

  • --enable-preview and --source=21
  • -package

The first pair of options tells javadoc to use preview features, which JEP 445 is one of. Without these preview-related options, javadoc will raise the following error:

% javadoc --version
javadoc 21

% javadoc HelloWorld.java -d /tmp/throwaway
Loading source file HelloWorld.java...
HelloWorld.java:2: error: unnamed classes are a preview feature and are disabled by default.
void main() {
^
  (use --enable-preview to enable unnamed classes)
1 error

The second option, -package, tells javadoc to document classes that are public, protected, or declared with package access (colloquially known as "package private"). Without this option, javadoc will only document public and protected classes, which do not include the interpreted class:

% javadoc --enable-preview --source=21 HelloWorld.java -d /tmp/throwaway
Loading source file HelloWorld.java...
Constructing Javadoc information...
error: No public or protected classes found to document.
1 error

When those additional options are present, javadoc does its job:

% javadoc --enable-preview --source=21 -package HelloWorld.java -d /tmp/throwaway
Loading source file HelloWorld.java...
Constructing Javadoc information...
Creating destination directory: "/tmp/throwaway/"
Building index for all the packages and classes...
Standard Doclet version 21+35-LTS-2513
Building tree for all the packages and classes...
Generating /tmp/throwaway/HelloWorld.html...
HelloWorld.java:7: warning: no @param for arg
void print(String arg) {
     ^
HelloWorld.java:2: warning: no comment
void main() {
     ^
HelloWorld.java:2: warning: use of default constructor, which does not provide a comment
void main() {
     ^
Generating /tmp/throwaway/package-summary.html...
Generating /tmp/throwaway/package-tree.html...
Generating /tmp/throwaway/overview-tree.html...
Building index for all classes...
Generating /tmp/throwaway/allclasses-index.html...
Generating /tmp/throwaway/allpackages-index.html...
Generating /tmp/throwaway/index-all.html...
Generating /tmp/throwaway/search.html...
Generating /tmp/throwaway/index.html...
Generating /tmp/throwaway/help-doc.html...
3 warnings

However, the result does not feel quite right. Firstly, -package is too coarse. It includes all top-level classes and their elements, not just the implicit class from HelloWorld.java, its default constructor and methods, which are all declared with package access. Secondly, HelloWorld.java isn't a first-class citizen in javadoc. That latter fact can be seen from examining stdout and the output directory:

  1. DocLint (compiler and javadoc) as well as javadoc itself issue unjust warnings: neither the implicit class nor its default constructor can be documented. The author either does not know about classes and constructors yet (on-ramp audience) or does not care about them (scripts/utilities audience).

    Additionally, because the class' AST node is at the same position as that of the first method declaration, the warning about the undocumented class can be confused with a warning on the first method being undocumented.

  2. While such a file is documented as if it were an explicitly declared (normal) class, we might want to dispense with the documentation for the default constructor as it lacks a comment and is an artefact.

What this PR proposes for JEP 463

  1. Leave --enable-preview and --source as correct and unavoidable until the feature is standardised.
  2. "Drill a hole" in javadoc access control to automatically allow implicit classes and their public, protected or declared with package access members in documentation.
  3. Do not emit warnings for an implicit class and its deault constructor.
  4. Do not document an implicit class' default constructor.

Progress

  • Change must be properly reviewed (1 review required, with at least 1 Reviewer)
  • Change requires CSR request JDK-8321030 to be approved
  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue

Issues

  • JDK-8308715: Create a mechanism for Implicitly Declared Class javadoc (Enhancement - P4)
  • JDK-8321030: Create a mechanism for Implicitly Declared Class javadoc (CSR)

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 16853

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

Using diff file

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

Webrev

Link to Webrev Comment

@bridgekeeper
Copy link

bridgekeeper bot commented Nov 28, 2023

👋 Welcome back prappo! A progress list of the required criteria for merging this PR into pr/16461 will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk openjdk bot added the rfr Pull request is ready for review label Nov 28, 2023
@openjdk
Copy link

openjdk bot commented Nov 28, 2023

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

  • build
  • hotspot
  • javadoc

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 javadoc javadoc-dev@openjdk.org build build-dev@openjdk.org hotspot hotspot-dev@openjdk.org labels Nov 28, 2023
@mlbridge
Copy link

mlbridge bot commented Nov 28, 2023

@pavelrappo
Copy link
Member Author

Reviewers, please ignore changes to the following files as well as commits that brought them:

  • .github/workflows/main.yml
  • src/hotspot/share/utilities/nativeCallStack.cpp

Those changes seem to be transient artefacts of the workflow and should disappear on their own, eventually.

The reason those changes appeared in this PR is that this PR's branch is based on a more recent master than that of the PR that this PR depends on, #16461.

@pavelrappo
Copy link
Member Author

/label remove hotspot

@openjdk openjdk bot removed the hotspot hotspot-dev@openjdk.org label Nov 28, 2023
@openjdk
Copy link

openjdk bot commented Nov 28, 2023

@pavelrappo
The hotspot label was successfully removed.

@pavelrappo
Copy link
Member Author

/label remove build

@openjdk openjdk bot removed the build build-dev@openjdk.org label Nov 28, 2023
@openjdk
Copy link

openjdk bot commented Nov 28, 2023

@pavelrappo
The build label was successfully removed.

@jddarcy
Copy link
Member

jddarcy commented Nov 28, 2023

@pavelrappo , please create a CSR for this behavioral change.

@pavelrappo
Copy link
Member Author

/csr

@openjdk openjdk bot added the csr Pull request needs approved CSR before integration label Nov 28, 2023
@openjdk
Copy link

openjdk bot commented Nov 28, 2023

@pavelrappo has indicated that a compatibility and specification (CSR) request is needed for this pull request.

@pavelrappo please create a CSR request for issue JDK-8308715 with the correct fix version. This pull request cannot be integrated until the CSR request is approved.

@openjdk-notifier openjdk-notifier bot changed the base branch from pr/16461 to master November 30, 2023 12:53
@openjdk-notifier
Copy link

The parent pull request that this pull request depends on has now been integrated and the target branch of this pull request has been updated. This means that changes from the dependent pull request can start to show up as belonging to this pull request, which may be confusing for reviewers. To remedy this situation, simply merge the latest changes from the new target branch into this pull request by running commands similar to these in the local repository for your personal fork:

git checkout 8308715
git fetch https://git.openjdk.org/jdk.git master
git merge FETCH_HEAD
# if there are conflicts, follow the instructions given by git merge
git commit -m "Merge master"
git push

@pavelrappo
Copy link
Member Author

@jonathan-gibbons I pushed a few updates.

While I agree with your stance on Utils here, I don't think it's for JavaDoc to define anything language-related. Have a look and tell me if what you see in those updates addresses your concerns.

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.

TL;DR: approved.

"One of these days" we should try and split the jdk.javadoc module in two: one for the "tool" part, and another for the "standard doclet" part.

In that split world, the "tool" part is deeply intertwined with the javac front end, and gets to access javac front-end internals almost without restriction. As such, it does not need to use WorkArounds. In module terms, it would be given qualified access into jdk.compiler internals. And, in that split world", the doclet part would/could/should be a pure client of the Language Model API, to reflect over the declarations, and the Compiler Tree API, to access the documentation comments. And, for that module, WorkArounds is used to workaround shortcomings of those public API. So it would reluctantly be given "temporary" access to javac internals, until the workarounds can be eliminated.

All of which is to say that ElementsTable will have an unnecessary forward dependency on WorkArounds, which belongs in that hypothetical "other" module. But at least the method the method has a well placed comment. DocLint is using access to javac internals, but DocLint is currently something of an orphan stepchild of javac and javadoc and deserves more significant remedial attention of its own, so gets a pass for now.

@openjdk
Copy link

openjdk bot commented Dec 4, 2023

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

8308715: Create a mechanism for Implicitly Declared Class javadoc

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

  • fddc02e: 8321225: [JVMCI] HotSpotResolvedObjectTypeImpl.isLeafClass shouldn't create strong references
  • 640d7f3: 8314327: Issues with JShell when using "local" execution engine
  • db5613a: 8317288: [macos] java/awt/Window/Grab/GrabTest.java: Press on the outside area didn't cause ungrab
  • b1cb374: 8320349: Simplify FileChooserSymLinkTest.java by using single-window testUI
  • 18c7922: 8321224: ct.sym for JDK 22 contains references to internal modules
  • 83ffc1a: 8320303: Allow PassFailJFrame to accept single window creator
  • fd31f6a: 8321183: Incorrect warning from cds about the modules file
  • 027b5db: 8321215: Incorrect x86 instruction encoding for VSIB addressing mode
  • 61d0db3: 8318468: compiler/tiered/LevelTransitionTest.java fails with -XX:CompileThreshold=100 -XX:TieredStopAtLevel=1
  • 87516e2: 8320943: Files/probeContentType/Basic.java fails on latest Windows 11 - content type mismatch
  • ... and 16 more: https://git.openjdk.org/jdk/compare/65be5e0c547d74ca7de288b164aa9bd6d6855685...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 ready Pull request is ready to be integrated and removed csr Pull request needs approved CSR before integration labels Dec 4, 2023
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.

Latest version approved

@pavelrappo
Copy link
Member Author

/integrate

@openjdk
Copy link

openjdk bot commented Dec 5, 2023

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

  • c8fa758: 8320860: add-opens/add-exports require '=' in JAVA_TOOL_OPTIONS
  • 9e57010: 8315149: Add hsperf counters for CPU time of internal GC threads
  • b0d1450: 8321053: Use ByteArrayInputStream.buf directly when parameter of transferTo() is trusted
  • acaf2c8: 8318590: JButton ignores margin when painting HTML text
  • d3df3eb: 8294699: Launcher causes lingering busy cursor
  • fddc02e: 8321225: [JVMCI] HotSpotResolvedObjectTypeImpl.isLeafClass shouldn't create strong references
  • 640d7f3: 8314327: Issues with JShell when using "local" execution engine
  • db5613a: 8317288: [macos] java/awt/Window/Grab/GrabTest.java: Press on the outside area didn't cause ungrab
  • b1cb374: 8320349: Simplify FileChooserSymLinkTest.java by using single-window testUI
  • 18c7922: 8321224: ct.sym for JDK 22 contains references to internal modules
  • ... and 21 more: https://git.openjdk.org/jdk/compare/65be5e0c547d74ca7de288b164aa9bd6d6855685...master

Your commit was automatically rebased without conflicts.

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

openjdk bot commented Dec 5, 2023

@pavelrappo Pushed as commit 430564c.

💡 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
integrated Pull request has been integrated javadoc javadoc-dev@openjdk.org
3 participants