On Fri, 23 Oct 2020 16:21:53 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 incrementally with one additional > commit since the last revision: > > Removing unnecessary cast. I have primarily gone through all the javadoc changes. There are three main areas of feedback: 1. The ElementFlag question. We have gone from one kind of element with special treatment (deprecated) to two (deprecated, preview), and there are signs of a third kind coming soon, maybe as early as next year, for work currently being discussed in the Panama project. As the saying goes, three would be one too many. So, I agree `ElementFlag` is underutilized today and could reasonably be removed, but it does seem worthwhile to keep, and it would even be worth increasing its usage soon, such as to combine methods and classes for deprecated elements and preview elements. I'm not sure I can go back into looking at files while typing this message, but if we are to keep `ElementFlag` we should at a minimum provide a better description of its purpose. For example, can/should it be used for all predicates on elements, or just the elements that get "special" handling when generating docs. 2. The use of strings containing HTML, and use of `RawHtml`, instead of working in terms of `Content` nodes such as `HtmlTree` and `StringContent`. 3. Track recent updates to the repo, regarding Conditional Pages. See how we set up conditional pages for the deprecated list, and do the same for preview items. This is probably a must-do item, once you merge with mainline. -- Finally, I realize this is a big chunk of work (well done!), across many areas, and that getting through a review is hard. If it is too hard to address some of the comments here, I'm OK if you file follow-up bugs of at least the same priority and Fix Version as for the main bug here: JDK-8250768. (That applies to #1, #2 above; not #3). src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/WorkArounds.java line 526: > 524: return (sym.flags() & Flags.PREVIEW_REFLECTIVE) != 0; > 525: } > 526: Generally, hacking your way into `Symbol` is undesirable (though accepted if there is no realistic alternative.) Adding new code into the `WorkArounds` class should be seen as a means of last resort when the required information cannot be obtained from public API ... which may require updating the public API as well. For example, should these methods be predicates on the Language Model `Elements` class? 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 deprecatedLinkContent = new ContentBuilder(); name does not match usage. I suggest simplifying it to just "content". src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/AbstractExecutableMemberWriter.java line 88: > 86: > 87: @Override > 88: protected Content getDeprecatedOrPreviewLink(Element member) { This name is a side-effect of the `ElementFlag` question. We should either use explicit field and method names, or we should use `ElementFlag` more consistently. This method name works OK for now, but if if ever gets to have another `orFoo` component in the name, the signature should be parameterized with something like `ElementFlag` or its equivalent. src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/AllClassesIndexWriter.java line 172: > 170: description.add(getDeprecatedPhrase(klass)); > 171: List<? extends DocTree> tags = > utils.getDeprecatedTrees(klass); > 172: if (!tags.isEmpty()) { There is potential for future parameterization using `ElementFlag` or its equivalent. src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/AnnotationTypeRequiredMemberWriterImpl.java line 205: > 203: protected Content getDeprecatedOrPreviewLink(Element member) { > 204: String name = utils.getFullyQualifiedName(member) + "." + > member.getSimpleName(); > 205: return > writer.getDocLink(LinkInfoImpl.Kind.MEMBER_DEPRECATED_PREVIEW, member, name); I suspect the MEMBER_DEPRECATED_PREVIEW will become more general in future src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/ClassWriterImpl.java line 210: > 208: pre.add(modifiersPart); > 209: pre.add(new > HtmlTree(TagName.SUP).add(links.createLink(getPreviewSectionAnchor(typeElement), > 210: > contents.previewMark))); Possible future enhancement: use a set of modifiers that need flags src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/ClassWriterImpl.java line 281: > 279: pre.add(DocletConstants.NL); > 280: pre.add("permits"); > 281: pre.add(new > HtmlTree(TagName.SUP).add(links.createLink(getPreviewSectionAnchor(typeElement), @hns is it better to use `<sup>` or CSS? Either way is OK in this review. src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/HtmlDoclet.java line 187: > 185: PreviewListWriter.generate(configuration); > 186: } > 187: This may need to be updated, by comparing against similar code for DEPRECATED, and/or you need to take `HtmlDocletWriter.ConditionalPage` into account, again by comparing with latest code for deprecated items. src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/HtmlDocletWriter.java line 112: > 110: import jdk.javadoc.internal.doclets.toolkit.util.VisibleMemberTable; > 111: import jdk.javadoc.internal.doclets.toolkit.util.Utils; > 112: import > jdk.javadoc.internal.doclets.toolkit.util.Utils.DeclarationPreviewLanguageFeatures; Uuugh on the long class name ;-) src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/HtmlDocletWriter.java line 1039: > 1037: } else if (utils.isVariableElement(element) || > utils.isTypeElement(element)) { > 1038: return getLink(new LinkInfoImpl(configuration, context, > typeElement) > 1039: > .label(label).where(links.getName(element.getSimpleName().toString())).targetMember(element)); Note to self (@jonathan-gibbons ) links.getName should accept a `CharSequence` to avoid the need for `toString()` src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/HtmlDocletWriter.java line 2219: > 2217: if (previewTree != null) { > 2218: div.add(HtmlTree.SPAN(HtmlStyle.previewLabel, > 2219: new > RawHtml(utils.getPreviewTreeSummaryOrDetails(previewTree, true)))); There's a big cringe here that there is a method in Utils returning HTML. src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/HtmlDocletWriter.java line 2281: > 2279: RawHtml note2 = > 2280: new > RawHtml(resources.getText("doclet.PreviewTrailingNote2", > 2281: name)); This is a deviation from existing practice to allow HTML in resource files. That doesn't seem like the sort of stuff that should be localizable. In other situations, (e.g. the Help page) we put the plain-text contents in the resource file and generate the markup in the code. Elsewhere, I said that `WorkArounds` is a means of last resort. The same is true for `RawHtml`. Use it if you must, but it's better to use other forms of `Content`, like `HtmlTree`. src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/HtmlDocletWriter.java line 2322: > 2320: feature.features > 2321: .stream() > 2322: .map(f -> "<code>" + f + "</code>") Ugh for using string constants for HTML. Try and use `Content` instead, src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/HtmlDocletWriter.java line 2329: > 2327: featureDisplayName, > 2328: featureCodes); > 2329: result.add(new RawHtml(text)); General ugh for many of the aforementioned reasons, all related to handling HTML in strings. src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/HtmlDocletWriter.java line 2351: > 2349: .map(this::toLink) > 2350: .map(link -> getLink(link).toString()) > 2351: .collect(Collectors.joining(", ")))); More of the same ... `RawHtml` and building HTML in strings src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/LinkInfoImpl.java line 64: > 62: * Indicate that the link appears in member documentation on the > Deprecated or Preview page. > 63: */ > 64: MEMBER_DEPRECATED_PREVIEW, Will need to be generalized, eventually src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/Navigation.java line 228: > 226: addTreeLink(tree); > 227: addDeprecatedLink(tree); > 228: addPreviewLink(tree); It's OK to put the link here, I guess, but it should also be on the INDEX page. See also recent updates for conditional pages. src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/Navigation.java line 888: > 886: > 887: private void addPreviewLink(Content tree) { > 888: if > (configuration.getIncludedModuleElements().stream().anyMatch(m -> > m.getQualifiedName().contentEquals("java.base"))) { This becomes simpler with recent support for conditional pages. src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/PackageWriterImpl.java line 39: > 37: import com.sun.source.doctree.TextTree; > 38: import com.sun.source.doctree.UnknownInlineTagTree; > 39: import java.util.stream.Collectors; why these imports? src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/resources/standard.properties line 288: > 286: doclet.Declared_Using_Preview.SEALED_PERMITS=Sealed Classes > 287: doclet.PreviewPlatformLeadingNote=<code>{0}</code> is a preview API of > the Java platform. > 288: doclet.ReflectivePreviewPlatformLeadingNote=<code>{0}</code> is a > reflective preview API of the Java platform. HTML in resource files is bad. It would be marginally better to move the HTML to the value being substituted (using strings from Content nodes) and even better to use a method that substitutes Content nodes into a resource string (Not sure if that method exists, but it could/should). src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/ConstructorWriter.java line 79: > 77: * @param annotationDocTree content tree to which the preview > information will be added > 78: */ > 79: void addPreview(ExecutableElement member, Content contentTree); Note to javadoc-team: Consider using default methods to provide an empty implementation. src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/builders/ConstructorBuilder.java line 28: > 26: package jdk.javadoc.internal.doclets.toolkit.builders; > 27: > 28: import static java.lang.invoke.ConstantBootstraps.enumConstant; Really? If it is required, it is in the wrong place. src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/util/PreviewAPIListBuilder.java line 46: > 44: * deletion without notice.</b> > 45: */ > 46: public class PreviewAPIListBuilder { OK for now, but it might be worth eventually trying to merge this with `DeprecatedListBuilder` src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/util/PreviewAPIListBuilder.java line 79: > 77: for (PreviewElementKind kind : PreviewElementKind.values()) { > 78: previewMap.put(kind, > 79: new > TreeSet<>(utils.comparators.makeDeprecatedComparator())); The use of `makeDeprecatedComparator` is not awful, but does smell a bit. src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/util/PreviewAPIListBuilder.java line 86: > 84: /** > 85: * Build the sorted list of all the deprecated APIs in this run. > 86: * Build separate lists for deprecated modules, packages, classes, > constructors, "d-word", twice src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/util/PreviewAPIListBuilder.java line 89: > 87: * methods and fields. > 88: */ > 89: private void buildDeprecatedAPIInfo() { D-word src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/util/PreviewAPIListBuilder.java line 133: > 131: } > 132: } > 133: > composeDeprecatedList(previewMap.get(PreviewElementKind.FIELD), I suggest you grep this file for all uses of the d-word src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/util/PreviewAPIListBuilder.java line 158: > 156: * @param members members to be added in the list. > 157: */ > 158: private void composeDeprecatedList(SortedSet<Element> sset, List<? > extends Element> members) { Last time d-word comment. Consider all such uses to be flagged. src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/util/Utils.java line 2848: > 2846: UnknownInlineTagTree previewTag = (UnknownInlineTagTree) t; > 2847: List<? extends DocTree> previewContent = > previewTag.getContent(); > 2848: String previewText = ((TextTree) > previewContent.get(0)).getBody(); This looks unreasonably fragile. And, I thought the tag was going away... at least according to earlier files in this review. src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/util/Utils.java line 2984: > 2982: SEALED(List.of("sealed")), > 2983: SEALED_PERMITS(List.of("sealed", "permits")), > 2984: RECORD(List.of("record")); I'm guessing this is about to go away soon? src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/util/Utils.java line 2980: > 2978: } > 2979: > 2980: public enum DeclarationPreviewLanguageFeatures { General thinking aloud question ... how does all this interact with source or release options for an earlier release? src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/util/Utils.java line 3025: > 3023: case MODULE, PACKAGE -> { > 3024: } > 3025: default -> throw new IllegalStateException("Unexpected: " + > el.getKind()); Should be `IllegalArgumentException` not `IllegalStateException` src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/util/Utils.java line 3145: > 3143: * Checks whether the given Element should be marked as a preview > API. > 3144: * > 3145: * Note that is a type is marked as a preview, its members are not. probable typo: "is" -> "if" src/jdk.javadoc/share/classes/jdk/javadoc/internal/tool/ElementsTable.java line 1288: > 1286: case FIELD: case INSTANCE_INIT: case LOCAL_VARIABLE: > case PARAMETER: > 1287: case RESOURCE_VARIABLE: case STATIC_INIT: case > TYPE_PARAMETER: > 1288: case RECORD_COMPONENT: I'm not saying this is wrong, but I'd like to understand why it is necessary. ------------- PR: https://git.openjdk.java.net/jdk/pull/703
