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

[Jonathan Gibbons]

On Thu, 7 Jan 2021 20:23:16 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 57 commits:
> 
>  - Fixing tests after a merge.
>  - Merging master into JDK-8250768
>  - Merging recent master changes into JDK-8250768
>  - Fixing navigator for the PREVIEW page.
>  - Fixing typo.
>  - Removing obsolette @PreviewFeature.
>  - Merging master into JDK-8250768
>  - Removing unnecessary property keys.
>  - Cleanup - removing unnecessary code.
>  - Merging master into JDK-8250768-dev4
>  - ... and 47 more: https://git.openjdk.java.net/jdk/compare/81c06242...a8046dde

I've looked at all the files that were marked as changed since I last looked at them.

There's one suggested enhancement to reduce string bashing between `Utils` and `ClassWriterImpl` that could be done now or later.

There's a pending conflict with a PR of mine to change to use a new type `HtmlId` for HTML ids. This JEP12 work has been in progress for a while, and so it would be good to get it in before the `HtmlId` work, and I'll deal with the merge conflict in due course.

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/ClassWriterImpl.java line 194:

> 192: 
> 193:     @Override @SuppressWarnings("preview")
> 194:     public void addClassSignature(String modifiers, Content classInfoTree) {

It seems less than ideal for this method to take a `String` and to then have to take it apart and reassemble it. It looks like it should be possible (and conceptually better) to change the signature to `List<String>` and to make the corresponding change to `utils.modifiersToString`, probably renaming it as `utils.modifiersToStrings` or something like that, and dropping the always-`true` argument for `trailingSpace`.  

But, the code is OK as is, and the suggestion is just for an enhancement, so is OK to defer it, if you would prefer.

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/SummaryListWriter.java line 2:

> 1: /*
> 2:  * Copyright (c) 1997, 2020, Oracle and/or its affiliates. All rights reserved.

(minor) now 2021

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/SummaryListWriter.java line 61:

> 59: public abstract class SummaryListWriter<L extends SummaryAPIListBuilder> extends SubWriterHolderWriter {
> 60: 
> 61:     private String getAnchorName(SummaryElementKind kind) {

Heads-up: at some point this will conflict with another change in progress/review, to introduce a new type `HtmlId`  to use instead of `String` for ids.

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/SummaryListWriter.java line 227:

> 225:      * @param contentTree the content tree to which the summary table will be added
> 226:      */
> 227:     protected void addSummaryAPI(SortedSet<Element> apiList, String id,

Heads-up, here's another occurrence of where there will be a conflict for ids.

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/resources/doclets.properties line 154:

> 152: doclet.Errors=Errors
> 153: doclet.Classes=Classes
> 154: doclet.Records=Records

I guess I'm mildly surprised to see all these items being removed...

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/util/SummaryAPIListBuilder.java line 2:

> 1: /*
> 2:  * Copyright (c) 1998, 2020, Oracle and/or its affiliates. All rights reserved.

Year

test/langtools/jdk/javadoc/doclet/testPreview/TestPreview.java line 2:

> 1: /*
> 2:  * Copyright (c) 2003, 2020, Oracle and/or its affiliates. All rights reserved.

2021. I assume you will do a bulk update for affected files.

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

Marked as reviewed by jjg (Reviewer).

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