RFR: JDK-8250768: javac should be adapted to changes in JEP 12 [v2]

[Hannes Wallnöfer]

On Tue, 20 Oct 2020 12:15:23 GMT, Jan Lahoda <jlahoda at openjdk.org> wrote:

>> This is an update to javac and javadoc, to introduce support for Preview APIs, and generally improve javac and javadoc behavior to more closely adhere to JEP 12.
>> 
>> The notable changes are:
>> 
>>  * adding support for Preview APIs (javac until now supported primarily only preview language features, and APIs associated with preview language features). This includes:
>>      * the @PreviewFeature annotation has boolean attribute "reflective", which should be true for reflective Preview APIs, false otherwise. This replaces the existing "essentialAPI" attribute with roughly inverted meaning.
>>      * the preview warnings for preview APIs are auto-suppressed as described in the JEP 12. E.g. the module that declares the preview API is free to use it without warnings
>>  * improving error/warning messages. Please see [1] for a list of cases/examples.
>>  * class files are only marked as preview if they are using a preview feature. [1] also shows if a class file is marked as preview or not.
>>  * the PreviewFeature annotation has been moved to jdk.internal.javac package (originally was in the jdk.internal package).
>>  * Preview API in JDK's javadoc no longer needs to specify @preview tag in the source files. javadoc will auto-generate a note for @PreviewFeature elements, see e.g. [2] and [3] (non-reflective and reflective API, respectively). A summary of preview elements is also provided [4]. Existing uses of @preview have been updated/removed.
>>  * non-JDK javadoc is also enhanced to auto-generate preview notes for uses of Preview elements, and for declaring elements using preview language features [5].
>>  
>>  Please also see the CSR [6] for more information.
>>  
>>  [1] http://cr.openjdk.java.net/~jlahoda/8250768/JEP12-errors-warnings-v6.html
>>  [2] http://cr.openjdk.java.net/~jlahoda/8250768/jdk.javadoc.00/api/java.base/java/lang/Record.html
>>  [3] http://cr.openjdk.java.net/~jlahoda/8250768/jdk.javadoc.00/api/java.compiler/javax/lang/model/element/RecordComponentElement.html
>>  [4] http://cr.openjdk.java.net/~jlahoda/8250768/jdk.javadoc.00/api/preview-list.html
>>  [5] http://cr.openjdk.java.net/~jlahoda/8250768/test.javadoc.00/
>>  [6] https://bugs.openjdk.java.net/browse/JDK-8250769
>
> Jan Lahoda has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 35 commits:
> 
>  - Merge branch 'JDK-8250768-dev' of https://github.com/lahodaj/jdk into JDK-8250768
>  - More fixing tests.
>  - Fixing tests.
>  - Using unique sections for preview warning sections, as suggested.
>  - Merge branch 'master' into JDK-8250768
>  - Reflecting review comments.
>  - Fixing tests.
>  - Various cleanup.
>  - The Preview taglet is not needed anymore.
>  - There is not jdk.internal package anymore
>  - ... and 25 more: https://git.openjdk.java.net/jdk/compare/98ec4a67...be1d8651

The javadoc code changes look reasonable, and the preview bits in the generated documentation look good as well. 

Apart from my inline comments which all address minor issues, there are a few things I don't like around `PreviewListWriter`: its code replication with `DeprecatedListWriter, and it adds things to HtmlDocletWriter and HtmlStyle that I don't think are strictly necessary. However, I wouldn't expect you to know how we like things done in javadoc, so maybe the simplest solution would be for one of us to go over the javadoc bits, either as part of this pull request or (if you prefer to get it integrated rather sooner) as a follow-up task.

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/util/Utils.java line 3164:

> 3162:     }
> 3163: 
> 3164:     public Set<ElementFlag> elementFlags(Element el) {

It seems like the sole use of this method and the `ElementFlag` enum below is to determine whether a preview warning note should be generated for an element. Is there something that speaks against simplifying it to reflect that purpose, e.g. change its name to `hasPreviewNote` or `hasPreviewContent` and change the return type to `boolean`?  Of course that's unless you foresee adding more `ElementFlag` values in the future.

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/HtmlDocletWriter.java line 125:

> 123: import static javax.lang.model.element.ElementKind.METHOD;
> 124: import static javax.lang.model.element.ElementKind.PACKAGE;
> 125: import jdk.javadoc.internal.doclets.formats.html.markup.RawHtml;

These imports as well as HtmlAttr above aren't used.

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/LinkInfoImpl.java line 247:

> 245:      * The member this link points to (if any).
> 246:      */
> 247:     public Element whereMember;

I don't think `whereMember` is a great name (and `where` above is not a great name either). What about naming this `targetMember` or `targetElement`?

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/markup/HtmlAttr.java line 47:

> 45:     CLEAR,
> 46:     COLS,
> 47:     COLSPAN,

I don't think this is used anywhere.

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/util/links/LinkFactory.java line 209:

> 207:         linkInfo.label = null;
> 208:         linkInfo.type = bound;
> 209:         ((LinkInfoImpl) linkInfo).skipPreview = false;

I guess it would be nicer to move the `skipPreview` field to `LinkInfo` to avoid that cast.

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/HtmlDocletWriter.java line 1314:

> 1312:             div = HtmlTree.DIV(HtmlStyle.block, result);
> 1313:             htmltree.add(div);
> 1314:         } else {

I notice that preview code above produces the same HTML output as non-preview code below in the `else` branch. Do we really need the `preview` parameter?

-------------

PR: https://git.openjdk.java.net/jdk/pull/703