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

JDK-8261079: Fix support for @hidden in classes and interfaces #2406

Closed
wants to merge 6 commits into from

Conversation

hns
Copy link
Member

@hns hns commented Feb 4, 2021

This change improves support for the @hidden tag to suppress documentation for specific elements, especially in the context of classes, interfaces and inheritance/method overriding.

The important changes are in Utils and in VisibleMemberTable. (There is also a number of non-related small code cleanup changes, sorry about that, I know it makes review a bit harder but I couldn't resist.)

In Utils the most important change are:

  • Consider types as "undocumented enclosures" if they are marked with a @hidden tag
  • Check for @hidden tags even in non-included elements as they may be included via undocumented enclosures
  • To counter the above change, only run doclint on elements that are either included or contained in an included type, as we don't want to report warnings or errors for non-included code.

In VisibleMemberTable there is a subtle change to not consider an overriding method as a "simple override" if the overridden method is hidden or otherwise invisible but in turn is a non-simple override of a method further up the inheritance chain. This resulted in methods which should have been documented as declared locally to be documented as declared in supertypes. I also did a bit of renaming in VisibleMemberTable to make the purpose of things a bit clearer.

Other than that, most of the changes consist of added calls to utils.hasHiddenTag(Element), usually with the purpose of not generating links to things that are not there.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

Reviewers

Download

$ git fetch https://git.openjdk.java.net/jdk pull/2406/head:pull/2406
$ git checkout pull/2406

@bridgekeeper
Copy link

bridgekeeper bot commented Feb 4, 2021

👋 Welcome back hannesw! 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 Feb 4, 2021

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

  • javadoc

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 openjdk bot added the javadoc javadoc-dev@openjdk.org label Feb 4, 2021
@hns hns marked this pull request as ready for review February 4, 2021 16:07
@openjdk openjdk bot added the rfr Pull request is ready for review label Feb 4, 2021
@mlbridge
Copy link

mlbridge bot commented Feb 4, 2021

Webrevs

@hns
Copy link
Member Author

hns commented Feb 5, 2021

Since I changed the code for handling method overrides in VisibleMemberTable I compared the generated documentation for current JDK 17 with and without this patch. There were no changes in the generated documentation.

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.

Two primary areas:

  1. I think the changes to Utils to determine if a tag is hidden can be improved/simplified
  2. I'm trying to understand the changes to VisibleMemberTable

if (isOverviewElement(e) || isIncluded(e)) {
return true;
}
// Run doclint on non-incuded members of included type elements.
Copy link
Contributor

Choose a reason for hiding this comment

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

typo incuded

Comment on lines 2729 to 2731
if (shouldRunDocLint(element)) {
configuration.runDocLint(path);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm disappointed there is so much disruption to the code because of the enhanced treatment of @hidden.Maybe it's necessary, but I was hoping/expecting that we could add a separate code path for the specific purpose of seeing if a non-selected(?) class has a @hidden tag.

For example, leave all the code for block tags and doc comment tree alone unchanged, and then provide isHidden(Element) by just using getDocCommentInfo, which gives cached access to the DocCommentTree then scan the block tags for a HIDDEN tree.

return found.overrider;
return found.overridden;
Copy link
Contributor

Choose a reason for hiding this comment

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

changing from overrider to overridden seems like a pretty big change, and not just a terminology cleanup.

Is this related to the change you described in the introduction?

Comment on lines 1563 to 1567
// prevent needless tests on elements which are not included
if (!isIncluded(e)) {
// prevent needless tests on elements which are neither included nor selected.
// Non-included members may still be visible via "transclusion" from undocumented enclusure
if (!isIncluded(e) && !configuration.docEnv.isSelected(e)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment applies to the whole of this method body.

I think the @hidden (or @treatAsprivate) tag is sufficiently special that it should be handled separately, especially in the cases where the containing comment would not otherwise be read, because the element is not included/selected. See other/earlier comments about avoiding getDocComment and going to the underlying getDocCommentInfo, which will (should?) completely bypass getDocComment and (importantly) the use of doclint.

 - Use separate unchecked/uncached code path when checking for @hidden tag on non-incuded elements
 - Rename OverridingMethodInfo to OverriddenMethodInfo to better reflect its purpose
@openjdk
Copy link

openjdk bot commented Feb 11, 2021

@hns this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout JDK-8261079
git fetch https://git.openjdk.java.net/jdk master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

@openjdk openjdk bot added the merge-conflict Pull request has merge conflict with target branch label Feb 11, 2021
@openjdk openjdk bot removed the merge-conflict Pull request has merge conflict with target branch label Feb 11, 2021
@hns
Copy link
Member Author

hns commented Feb 11, 2021

Thanks for the review, Jon!

I pushed a new commit that should address your concerns. For the lookup of @hidden tags in non-included elements I added a new hasBlockTagUnchecked method that avoids caching and doclint checking by using getDocCommentInfo as you suggested. Note that this isn't used for the javafx "treatAsPrivate" tag as changing the semantics of an undocumented/internal tag didn't seem like a good idea on second thought.

Regarding the renaming of overrider to overridden: the old name was a misnomer, the new name actually reflects the meaning of the field as it contains the overridden method, not the overriding one. I actually went the whole way and renamed the nested class as well, from OverridingMethodInfo to OverriddenMethodInfo.

@mlbridge
Copy link

mlbridge bot commented Feb 11, 2021

Mailing list message from Jonathan Gibbons on javadoc-dev:

On 2/11/21 4:10 AM, Hannes Walln?fer wrote:

Thanks for the review, Jon!

I pushed a new commit that should address your concerns. For the lookup of `@hidden` tags in non-included elements I added a new `hasBlockTagUnchecked` method that avoids caching and doclint checking by using `getDocCommentInfo` as you suggested. Note that this isn't used for the javafx "treatAsPrivate" tag as changing the semantics of an undocumented/internal tag didn't seem like a good idea on second thought.

I agree that there is no need and that we should not change the
operation of the older `treatAsPrivate` flag.

Regarding the renaming of `overrider` to `overridden`: the old name was a misnomer, the new name actually reflects the meaning of the field as it contains the overridden method, not the overriding one. I actually went the whole way and renamed the nested class as well, from `OverridingMethodInfo` to `OverriddenMethodInfo`.

Understood.

(For my own satisfaction, I may go back and look at the old code, to
better understand the provenance of the name. That is independent of
this review.)

-- Jon

@mlbridge
Copy link

mlbridge bot commented Feb 11, 2021

Mailing list message from Jonathan Gibbons on javadoc-dev:

On 2/11/21 4:10 AM, Hannes Walln?fer wrote:

- JDK-8261079: Updates to review comments
- Use separate unchecked/uncached code path when checking for @hidden tag on non-incuded elements

I think you'll find that if you dig deep enough, it is not so much
"uncached" as "less cached"!

The comments read from files are cached deep down in `jdk.compiler`?
(JCTrees:533, DocCommentTable) ... the additional level of caching in
javadoc is to support caching of synthetic comments, such as for the
mandated methods on enum and record classes.

-- Jon

@openjdk
Copy link

openjdk bot commented Feb 12, 2021

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

8261079: Fix support for @hidden in classes and interfaces

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 1 new commit pushed to the master branch:

  • 9c0ec8d: 8260941: Remove the conc_scan parameter for CardTable

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.

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

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Feb 12, 2021
@hns
Copy link
Member Author

hns commented Feb 12, 2021

/integrate

@openjdk
Copy link

openjdk bot commented Feb 12, 2021

@hns Unknown command integrated - for a list of valid commands use /help.

@openjdk openjdk bot closed this Feb 12, 2021
@openjdk openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Feb 12, 2021
@openjdk
Copy link

openjdk bot commented Feb 12, 2021

@hns Since your change was applied there has been 1 commit pushed to the master branch:

  • 9c0ec8d: 8260941: Remove the conc_scan parameter for CardTable

Your commit was automatically rebased without conflicts.

Pushed as commit 3210095.

💡 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
2 participants