On Thu, 31 Oct 2024 13:18:44 GMT, Nizar Benalla <[email protected]> wrote:
>> Please review this patch to prevent links to private and package-private
>> members to be generated.
>> The bug happens when you link to private/package-private members, and
>> javadoc used to generated links to them (assuming they were inherited
>> because the holder is unreachable).
>>
>> Taking the code path I changed is very rare, as it only used by 4 anchors in
>> 4 classes in all the JDK.
>>
>> if (refSignature.trim().startsWith("#") &&
>> ! (utils.isPublic(containing) ||
>> utils.isLinkable(containing))
>>
>>
>> The classes that used it are `StringBuilder`/`StringBuffer` with
>> `#append(java.lang.String)` and `ZipEntry`/`ZipOutputStream` with `#CENHDR`
>>
>>
>> I've expanded the test to check whether the links are created when they
>> should be.
>>
>> The generated documentation before and after the change are identical.
>
> Nizar Benalla has updated the pull request incrementally with one additional
> commit since the last revision:
>
> whitespace
Sorry, my initial feedback was wrong. `utils.isIncluded` is indeed not what we
want to use, because `refMem` and `containing` are in fact not included (which
is why we want to link to the inherited documentation in the current class).
So I had to refresh my knowledge of [doclet terminology], because what we want
to know is whether the referenced member is _selected_ rather than _included_.
In other words, we want to check if selection control allows the member to be
documented in the current class.
I'm adding an inline comment with the change I suggest to take this into
account.
[doclet terminology]:
https://docs.oracle.com/en/java/javase/23/docs/api/jdk.javadoc/jdk/javadoc/doclet/package-summary.html#terminology-heading
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/taglets/LinkTaglet.java
line 253:
> 251: if (refSignature.trim().startsWith("#") &&
> 252: ! (utils.isPublic(containing) ||
> utils.isLinkable(containing)) &&
> 253: ! (utils.isPrivate(refMem) ||
> utils.isPackagePrivate(refMem))) {
While this fixes the problem in the common case, it hard-codes the
accessibility level for linked members to protected and public, and also
prevents us from printing a warning for private/package private members a few
lines below.
What I suggest to do instead of this is to add a check if `refMem` is
**selected** in line 257/258:
if (utils.configuration.docEnv.isSelected(refMem)
&& htmlWriter instanceof ClassWriter cw) {
This takes care of the selected access level (`-private`, `-package`,
`-protecdted` etc) and also should cause a warning to be generated for
unselected private members.
test/langtools/jdk/javadoc/doclet/5093723/T5093723.java line 78:
> 76: }
> 77: """);
> 78:
Kudos for "modernizing" the test!
-------------
Changes requested by hannesw (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/21802#pullrequestreview-2415876139
PR Review Comment: https://git.openjdk.org/jdk/pull/21802#discussion_r1829502655
PR Review Comment: https://git.openjdk.org/jdk/pull/21802#discussion_r1829524202
