Re: RFR: JDK-8250768: javac should be adapted to changes in JEP 12 [v4]
On Tue, 27 Oct 2020 16:09:45 GMT, Jan Lahoda wrote:
>> 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.
>
> Note this method returns the same link for deprecate or preview - it just was
> named "getDeprecatedLink", and when using it work preview, I renamed it
> "getDeprecatedOrPreviewLink". We may need to think of a better name.
This name is OK for now. Maybe we'll have m ore insight into a better name
if/when we add a third variant ;-)
-
PR: https://git.openjdk.java.net/jdk/pull/703
Re: RFR: JDK-8250768: javac should be adapted to changes in JEP 12 [v4]
On Thu, 29 Oct 2020 09:26:05 GMT, Jan Lahoda wrote: >> 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. > > HtmlDocletWriter.getPreviewNotes analyzes classes and their members, to see > if some are using aspects that are under preview. When looking at the > members, it uses utils.isIncluded on the member, and that eventually gets > here. And if the member is a RECORD_COMPONENT, it would fail here. But we can > avoid asking for RECORD_COMPONENTS, if needed. ok - PR: https://git.openjdk.java.net/jdk/pull/703
Re: RFR: JDK-8250768: javac should be adapted to changes in JEP 12 [v4]
On Thu, 29 Oct 2020 09:43:56 GMT, Jan Lahoda wrote: >> I don't think there should be much interaction with -source . >> We don't support preview features from previous version or preview class >> files from previous versions, so I think it should be enough to handle the >> currently present preview features. > > We don't support preview features from previous releases, AFAIK. So javadoc > for JDK 16 should not accept e.g. record class when running with -source 15. Yeah, my recollection is that I was wondering whether preview-related code needs to be "guarded" to only work in the current release. But, I guess we may get the right effect (of forbidding preview features in older code) from the javac front end, so that in javadoc we can be assured that there are no instances of what may still be preview features in older code (i.e with older --source/--rlease options) - PR: https://git.openjdk.java.net/jdk/pull/703
Re: RFR: JDK-8250768: javac should be adapted to changes in JEP 12 [v4]
On Tue, 27 Oct 2020 16:11:18 GMT, Jan Lahoda wrote:
>> 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?
>
> I don't think there should be much interaction with -source .
> We don't support preview features from previous version or preview class
> files from previous versions, so I think it should be enough to handle the
> currently present preview features.
We don't support preview features from previous releases, AFAIK. So javadoc for
JDK 16 should not accept e.g. record class when running with -source 15.
-
PR: https://git.openjdk.java.net/jdk/pull/703
Re: RFR: JDK-8250768: javac should be adapted to changes in JEP 12 [v4]
On Fri, 23 Oct 2020 18:19:13 GMT, Jonathan Gibbons wrote: >> Jan Lahoda has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Removing unnecessary cast. > > src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/util/Utils.java > line 2848: > >> 2846: UnknownInlineTagTree previewTag = (UnknownInlineTagTree) t; >> 2847: List 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. This was intended to allow Preview APIs to provide their own text instead of the generic one. But, looking again at JEP 12, this shouldn't be supported, so removing. - PR: https://git.openjdk.java.net/jdk/pull/703
Re: RFR: JDK-8250768: javac should be adapted to changes in JEP 12 [v4]
On Fri, 23 Oct 2020 17:58:42 GMT, Jonathan Gibbons wrote: >> Jan Lahoda has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Removing unnecessary cast. > > 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. I believe the link is in the navigation bar on all pages (as DEPRECATED is). - PR: https://git.openjdk.java.net/jdk/pull/703
Re: RFR: JDK-8250768: javac should be adapted to changes in JEP 12 [v4]
On Fri, 23 Oct 2020 18:28:12 GMT, Jonathan Gibbons wrote: >> Jan Lahoda has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Removing unnecessary cast. > > 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. HtmlDocletWriter.getPreviewNotes analyzes classes and their members, to see if some are using aspects that are under preview. When looking at the members, it uses utils.isIncluded on the member, and that eventually gets here. And if the member is a RECORD_COMPONENT, it would fail here. But we can avoid asking for RECORD_COMPONENTS, if needed. - PR: https://git.openjdk.java.net/jdk/pull/703
Re: RFR: JDK-8250768: javac should be adapted to changes in JEP 12 [v4]
On Fri, 23 Oct 2020 17:22:33 GMT, Jonathan Gibbons wrote:
>> Jan Lahoda has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Removing unnecessary cast.
>
> 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.
Note this method returns the same link for deprecate or preview - it just was
named "getDeprecatedLink", and when using it work preview, I renamed it
"getDeprecatedOrPreviewLink". We may need to think of a better name.
> 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?
I don't think there should be much interaction with -source . We
don't support preview features from previous version or preview class files
from previous versions, so I think it should be enough to handle the currently
present preview features.
> 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?
Right, this is likely to go away soon.
-
PR: https://git.openjdk.java.net/jdk/pull/703
Re: RFR: JDK-8250768: javac should be adapted to changes in JEP 12 [v4]
On Fri, 23 Oct 2020 16:21:53 GMT, Jan Lahoda 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. Build changes look good. - Marked as reviewed by ihse (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/703
Re: RFR: JDK-8250768: javac should be adapted to changes in JEP 12 [v4]
On Fri, 23 Oct 2020 16:21:53 GMT, Jan Lahoda 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
Re: RFR: JDK-8250768: javac should be adapted to changes in JEP 12 [v4]
> 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. - Changes: - all: https://git.openjdk.java.net/jdk/pull/703/files - new: https://git.openjdk.java.net/jdk/pull/703/files/caa4fd34..461e7d15 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=703&range=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=703&range=02-03 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/703.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/703/head:pull/703 PR: https://git.openjdk.java.net/jdk/pull/703
