On Thu, 25 Apr 2024 14:29:27 GMT, Nizar Benalla <d...@openjdk.org> wrote:
> Please review this PR that aims to add all the remaining needed `@since` tags > in `java.base`, and group them into a single fix. > This is related to #18934 and my work around the `@since` checker feature. > Explicit `@since` tags are needed for some overriding methods for the purpose > of the checker. > > I will only give the example with the `CompletableFuture` class but here is > the before where the methods only appeared in "Methods declared in interface > N" > > <img width="1510" alt="Screenshot 2024-05-06 at 00 06 57" > src="https://github.com/openjdk/jdk/assets/96922791/1749a355-133b-4a38-bffe-51ac415b2aac";> > > > > and after where the method has it's own javadoc, the main description is > added and the `@since` tags are added as intended. > > I don't have an account on `https://cr.openjdk.org/` but I could host the > generated docs somewhere if that is needed. > > <img width="1511" alt="Screenshot 2024-05-06 at 00 07 16" > src="https://github.com/openjdk/jdk/assets/96922791/89b92288-9b5e-48ed-8fa1-9342ea43e043";> > > <img width="1505" alt="Screenshot 2024-05-06 at 00 08 06" > src="https://github.com/openjdk/jdk/assets/96922791/9aef08ff-5030-4189-a996-582a7eef849b";> > > <img width="1050" alt="Screenshot 2024-05-06 at 00 09 03" > src="https://github.com/openjdk/jdk/assets/96922791/fdd75a26-0396-4c5e-8f59-a3717b7d7ec8";> > > > TIA Changes requested by liach (Author). Marked as reviewed by liach (Author). I think your changes mostly group in these categories: 1. New API methods provided in superclasses/superinterfaces, this class provides a more concrete implementation: Examples being `CompletableFuture`, `FileInputStream`, `DelayQueue`, `FutureTask` I don't think you should add since tags for these; without explicit javadoc, the methods inherit the superclass/superinterface docs, and appear in `Methods declared in class/interface Xxx` (supertype) section, which already have the correct since tags. There's one scenario where such addition may be meaningful, however: that's if the supertype's since version is newer than this class/interfaces's since version, so we might need to specify here. (On a side note, it would be great if we can mark the since version of an interface, notorious example being `ZipFile` retrofitted to implement `Closeable` in 1.7 and breaks compile target 1.6) 2. Remove unnecessary since tags for existing API methods with newer implementation Examples being `Reference`, `RsaPrivateKey`. These make sense. 3. API methods with different return types Examples being `ClassSignature`, `ClassDesc`. These make sense too, as older version may return different types. But problem here is should we count methods with only signature (but not descriptor) differences, like `ClassSignature::superinterfaceSignatures()`? For case 1 I mentioned, the new since tags in `CompletableFuture`, `FileInputStream`, `DelayQueue`, `FutureTask` are not necessary: their docs are carried from the superclass/superinterfaces, and those superclass/superinterface methods already have the correct since tags. Please consider this scenario where class A extends B, both from earlier versions, and there's `B::method` added in a new version X. Why are we adding a `@since` tag on the method `A::method` when it doesn't have its own doc and just refers to `B::method`, which already includes a `@since` tag? For your convenience, I will reiterate with the javadoc output API specification instead of just the source code. These are most likely caused by bugs in your analyais tool: 1. `CompletableFuture::exceptionallyComposeAsync` `CompletableFuture::resultNow` etc. already have the correct `@since` tags inherited from superclass/superinterfaces javadocs. Notice if a method doesn't have javadoc, it carries the javadoc from its overridden method and it doesn't appear in this class's Method Summary section. https://docs.oracle.com/en/java/javase/22/docs/api/java.base/java/util/concurrent/CompletableFuture.html#methods-inherited-from-class-java.util.concurrent.CompletionStage Affected classes: `CompletableFuture`, `ForkJoinTask`, `FutureTask`, and `ChoiceFormat`. 2. `FileInputStream::readNBytes` is explicitly overridden in JDK 12 for a better implementation, but it is already a valid method since JDK 9 when `InputStream::readNBytes` was added. This `@since 12` does not make sense. https://docs.oracle.com/en/java/javase/22/docs/api/java.base/java/io/FileInputStream.html#methods-inherited-from-class-java.io.InputStream Affected classes: `FileInputStream`, `DelayQueue`. All other changes look correct. FYI you can generate the documentation with `make docs` and upload it to https://cr.openjdk.org and link it for review purposes. (You just need to include the changed classes and the stylesheet.css) I don't want to reiterate again, but if a method is declared so: /** * Class One. * * @since 42 */ public class One { /** * Method. * * @since 48 */ public void method() {} } /** * Class Two. * * @since 42 */ public class Two extends One { @Override public void method() {} } The generated docs for `Two` will list `method` only in "Method declared in class One" section instead of the "Method Summary" section, and the link in "Method declared in ..." section links to the method declaration in class One where there's a correct `@since` version. What's wrong with you that you ask `Two::method` to have a redundant javadoc and since tag? I'm sorry, you have always mentioned "match the javadoc rules" "javadoc doesn't look into supertype" "go against current behavior", but there is no problem with these `@since` tags in the current output documentation because those overriding methods without javadoc are treated as if they don't exist by javadoc tool. > For overriding methods we don't look into the supertype We indeed don't because we treat it as if it does not exist, and then the current docs are right. Please, just take a look at the javadoc output once and reach your own conclusion. src/java.base/share/classes/java/lang/classfile/ClassSignature.java line 47: > 45: * > 46: * @since 23 > 47: * */ Suggestion: */ src/java.base/share/classes/java/lang/constant/ClassDesc.java line 367: > 365: > 366: /** > 367: * @since 21 Suggestion: * {@inheritDoc} * * @since 21 src/java.base/share/classes/java/lang/constant/MethodHandleDesc.java line 209: > 207: > 208: /** > 209: * @since 21 Suggestion: * {@inheritDoc} * * @since 21 src/java.base/share/classes/java/lang/constant/MethodHandleDesc.java line 210: > 208: /** > 209: * @since 21 > 210: * */ Suggestion: */ ------------- PR Review: https://git.openjdk.org/jdk/pull/18954#pullrequestreview-2023023198 PR Review: https://git.openjdk.org/jdk/pull/18954#pullrequestreview-2028193951 PR Comment: https://git.openjdk.org/jdk/pull/18954#issuecomment-2077725789 PR Comment: https://git.openjdk.org/jdk/pull/18954#issuecomment-2078313984 PR Comment: https://git.openjdk.org/jdk/pull/18954#issuecomment-2078545190 PR Comment: https://git.openjdk.org/jdk/pull/18954#issuecomment-2080002723 PR Comment: https://git.openjdk.org/jdk/pull/18954#issuecomment-2080529185 PR Review Comment: https://git.openjdk.org/jdk/pull/18954#discussion_r1579787419 PR Review Comment: https://git.openjdk.org/jdk/pull/18954#discussion_r1579821135 PR Review Comment: https://git.openjdk.org/jdk/pull/18954#discussion_r1579821029 PR Review Comment: https://git.openjdk.org/jdk/pull/18954#discussion_r1579820432
