On Mon, 2 Nov 2020 18:15:09 GMT, Jan Lahoda <jlah...@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 46 commits: > > - Removing trailing whitespace. > - Merging master into JDK-8250768. > - Updating tests after records are a final feature. > - Fixing tests. > - Finalizing removal of record preview hooks. > - Merging master into JDK-8250768 > - Reflecting review comments. > - Merge branch 'master' into JDK-8250768 > - Removing unnecessary cast. > - Using a more correct way to get URLs. > - ... and 36 more: > https://git.openjdk.java.net/jdk/compare/d93e3a7d...2e403900 I have read all the files. I have added a n umber of various minor non-blocking comments (no need for re-review( to fix these. But I have a couple of comments/questions before finally giving approval. There's a comment in `PreviewListWriter` about annotation members that needs too be addressed, and I wonder is RECORD and RECORD_COMPONENT need to be added into PreviewElementKind. src/java.base/share/classes/jdk/internal/javac/PreviewFeature.java line 75: > 73: * A key for testing. > 74: */ > 75: TEST, Slightly weird src/jdk.compiler/share/classes/com/sun/tools/javac/api/JavacTaskPool.java line 257: > 255: //when patching modules (esp. java.base), it may be > impossible to > 256: //clear the symbols read from the patch path: > 257: polluted |= > get(JavaFileManager.class).hasLocation(StandardLocation.PATCH_MODULE_PATH); OK, but looks unrelated to primary work src/jdk.compiler/share/classes/com/sun/tools/javac/code/Preview.java line 218: > 216: return Errors.PreviewFeatureDisabledClassfile(classfile, > majorVersionToSource.get(majorVersion).name); > 217: } > 218: Up above in isPreview, lines 185-188, I'm supervised it's not a `switch` statement. (Can't annotate those lines directly) src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/AbstractExecutableMemberWriter.java line 89: > 87: @Override > 88: protected Content getDeprecatedOrPreviewLink(Element member) { > 89: Content content = new ContentBuilder(); Yeah the shorter name is good here and more in keeping with the code style src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/AbstractExecutableMemberWriter.java line 93: > 91: if (!utils.isConstructor(member)) { > 92: content.add("."); > 93: content.add(member.getSimpleName()); this is OK, but generally FYI, `Content` is now set up to allow chained method calls. src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/ClassUseWriter.java line 436: > 434: configuration, LinkInfoImpl.Kind.CLASS_USE_HEADER, > typeElement) > 435: .label(resources.getText("doclet.Class")) > 436: .skipPreview(true)); @hns General comment, not directly related to this review: is it worth a cleanup to fold the sequence `getLink(new LinkInfoImpl(...))` into a hypothetical new `LinkBuilder` ? src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/ClassWriterImpl.java line 221: > 219: if (modifiers.endsWith(" ")) { > 220: pre.add(" "); > 221: } Obligatory ugh that `modifiers` is a string and that it might end with a space. This is a possibility for future cleanup. src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/ClassWriterImpl.java line 283: > 281: if (isFirst) { > 282: pre.add(DocletConstants.NL); > 283: pre.add("permits"); Note for javadoc-dev folk: we should have a better more consistent way of handling keywords src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/HelpWriter.java line 254: > 252: contentTree.add(section); > 253: } > 254: 👍 src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/HtmlConfiguration.java line 145: > 143: // which performs a somewhat similar role > 144: public enum ConditionalPage { > 145: CONSTANT_VALUES, DEPRECATED, PREVIEW, SERIALIZED_FORM, > SYSTEM_PROPERTIES 👍 src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/LinkFactoryImpl.java line 120: > 118: title, > 119: classLinkInfo.target)); > 120: if (flags.contains(ElementFlag.PREVIEW)) { I see a lot of `new HtmlTree(TagName.SUP).add(...)` ... enough to warrant a new factory method ``HtmlTree.SUP(Content)` or similar. src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/ModuleIndexWriter.java line 39: > 37: import jdk.javadoc.internal.doclets.formats.html.markup.HtmlStyle; > 38: import jdk.javadoc.internal.doclets.formats.html.markup.HtmlTree; > 39: import jdk.javadoc.internal.doclets.formats.html.markup.RawHtml; Are these imports still required? src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/PreviewListWriter.java line 40: > 38: > 39: import com.sun.source.doctree.DocTree; > 40: import javax.lang.model.element.ElementKind; unordered imports src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/PreviewListWriter.java line 223: > 221: case CONSTRUCTOR -> new ConstructorWriterImpl(this); > 222: case ENUM_CONSTANT -> new EnumConstantWriterImpl(this); > 223: case ANNOTATION_TYPE_MEMBER -> new > AnnotationTypeOptionalMemberWriterImpl(this, null); Hmmm. Not sure about this one. For better or worse, javadoc current handles optional and required members differently (i.e. with different classes). That's arguably something that should be cleaned up at some point, but in the meantime, I'm surprised to not see the distinction here. I guess if we're not likely to see an interaction between preview-ness and annotation-members, this will not likely show up in practice. src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/PreviewListWriter.java line 67: > 65: public class PreviewListWriter extends SubWriterHolderWriter { > 66: > 67: private String getAnchorName(PreviewElementKind kind) { 1. I'm mildly surprised to not see anything record-related here. 2. I'm mildly surprised you haven't used the new switch-expression syntax. src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/util/PreviewAPIListBuilder.java line 67: > 65: ENUM_CONSTANT, > 66: ANNOTATION_TYPE_MEMBER // no ElementKind mapping > 67: }; Should there be RECORD and RECORD_COMPONENT in this list? src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/markup/HtmlTree.java line 932: > 930: switch (tagName) { > 931: case A: case BUTTON: case BR: case CODE: case EM: case I: > case IMG: > 932: case LABEL: case SMALL: case SPAN: case STRONG: case SUB: > case SUP: See comment elsewhere about possibly adding a factory method for `SUP` trees. src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/resources/standard.properties line 290: > 288: doclet.Declared_Using_Preview.SEALED_PERMITS=Sealed Classes > 289: doclet.PreviewPlatformLeadingNote={0} is a preview API of the Java > platform. > 290: doclet.ReflectivePreviewPlatformLeadingNote={0} is a reflective preview > API of the Java platform. Question: is the following "inconsistency" deliberate: 1. Java platform 2. Java language 3. Java SE (in javac diagnostics) src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/FieldWriter.java line 75: > 73: > 74: /** > 75: * Add the preview output for the given member. (minor) Should be "Adds" src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/MethodWriter.java line 75: > 73: > 74: /** > 75: * Add the preview output for the given member. (minor) should be "Adds" src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/builders/AnnotationTypeRequiredMemberBuilder.java line 156: > 154: buildSignature(annotationDocTree); > 155: buildDeprecationInfo(annotationDocTree); > 156: buildPreviewInfo(annotationDocTree); (Just checking) I presume this behavior is inherited into `AnnotationTypeOptionalMemberBuilder` so no changes required there. src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/builders/EnumConstantBuilder.java line 166: > 164: writer.addPreview(currentElement, enumConstantsTree); > 165: } > 166: Note to self: I regret that this duplication of adding this method everywhere seems to be necessary. This would be good to clean up at some point. src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/resources/doclets.properties line 213: > 211: doclet.Preview=Preview. > 212: doclet.Properties=Properties > 213: doclet.constructors=constructors Is the period after `Preview` intentional? It seems inconsistent. src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/util/Comparators.java line 131: > 129: /** > 130: * Returns a Comparator for deprecated items listed on deprecated > list page, by comparing the > 131: * fully qualified names, and if those are equal the names of the > enclosing modules. Comment does not match method name src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/util/Utils.java line 132: > 130: import static com.sun.source.doctree.DocTree.Kind.*; > 131: import javax.lang.model.AnnotatedConstruct; > 132: import javax.lang.model.util.SimpleAnnotationValueVisitor14; unordered imports test/langtools/jdk/javadoc/doclet/testPreview/TestPreview.java line 75: > 73: </div>"""; > 74: String expected = MessageFormat.format(expectedTemplate, > zero, one, two, three); > 75: checkOutput("m/pkg/TestPreviewDeclaration.html", true, > expected); Interesting technique... Future: it might be interesting to consider folding the bundle access on line 59 into a utility call in JavadocTester test/langtools/jdk/javadoc/doclet/testPreview/api/preview/Core.java line 28: > 26: import jdk.internal.javac.PreviewFeature.Feature; > 27: > 28: @PreviewFeature(feature=Feature.TEST) Yeah, I remember `Feature.TEST` from earlier. I guess it's OK for now, as a workaround for a testing a feature which is inherently, by design, a moving target across releases. These days, javadoc tests are trending towards generating small sample test programs, instead of having small static side-files dominated by a legal header. I wonder if there is a possibility of having a "generator class" in the `javadoc.tester` package that can generate sample code using one or more of the current set of preview features, as a way of reducing the need for the TEST feature. test/langtools/tools/javac/patterns/BindingsTest2.out line 54: > 52: - compiler.note.preview.filename: BindingsTest2.java, DEFAULT > 53: - compiler.note.preview.recompile > 54: 51 errors (issue unrelated to review) it seems wrong that we encourage/allow the use of files that do not end in newline. test/langtools/tools/javac/preview/PreviewAutoSuppress.java line 25: > 23: > 24: /* > 25: * @test no `@bug`? test/langtools/tools/javac/preview/PreviewErrors.java line 38: > 36: * @build toolbox.ToolBox toolbox.JavacTask > 37: * @build combo.ComboTestHelper > 38: * @compile PreviewErrors.java do you need `@compile` here: is `@build` not good enough? ------------- PR: https://git.openjdk.java.net/jdk/pull/703
