Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

JDK-8250768: javac should be adapted to changes in JEP 12 #703

Closed
wants to merge 59 commits into from

Conversation

@lahodaj
Copy link
Contributor

@lahodaj lahodaj commented Oct 16, 2020

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


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8250768: javac should be adapted to changes in JEP 12

Reviewers

Download

$ git fetch https://git.openjdk.java.net/jdk pull/703/head:pull/703
$ git checkout pull/703

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Oct 16, 2020

👋 Welcome back jlahoda! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@mlbridge
Copy link

@mlbridge mlbridge bot commented Nov 6, 2020

Mailing list message from Chris Hegarty on kulla-dev:

On 5 Nov 2020, at 18:11, Alex Buckley <alex.buckley at oracle.com> wrote:

On 11/5/2020 4:45 AM, Jan Lahoda wrote:

FWIW, a javadoc generated with the current version of the patch:
http://cr.openjdk.java.net/~jlahoda/8250768/jdk.javadoc.01/api/index.html

Allow me to draw people's attention to the PREVIEW link in the banner of the generated javadoc. It shows all the preview APIs in the release on one page. This is very helpful for understanding the surface area of a preview feature.

For example, with Sealed Classes being the only preview feature likely to target JDK 16, the PREVIEW page shows that the feature's API is solely about reflection. It's clear that Sealed Classes do not introduce, say, a java.lang.Sealed class analogous to the java.lang.Record class introduced by Records in JDK 14/15 (and which would have appeared on the PREVIEW page had it existed then).

Very cool! ( I didn?t notice this until you pointed it out ;-) )

http://cr.openjdk.java.net/~jlahoda/8250768/jdk.javadoc.01/api/preview-list.html <http://cr.openjdk.java.net/~jlahoda/8250768/jdk.javadoc.01/api/preview-list.html>

There appears to be a very minor bug; when I click on the PREVIEW link in the banner, the page that lists the preview API points loads, but the banner does not ?highlight? that PREVIEW is ?selected?, but rather it ?highlights? DEPRECATED.

-Chris.

@jonathan-gibbons
Copy link
Contributor

@jonathan-gibbons jonathan-gibbons commented Nov 6, 2020

@lahodaj
Copy link
Contributor Author

@lahodaj lahodaj commented Nov 6, 2020

Copy link
Contributor

@jonathan-gibbons jonathan-gibbons left a comment

I have a mild queasiness about this new overloaded use of the word "Summary", since "summary tables" are normally the summary of the contents of a declaration, like fields and methods of a class.

That being said, the usage is primarily internal, and I have no overwhelmingly wonderful alternative, and (overloading aside) the term is accurate.

So, OK for now. We can change it later if we want to.

I like how you managed to merge both the builder and writer for the new summary classes.

@@ -2942,4 +2944,256 @@ public String toString() {
return first + ":" + second;
}
}

/**
* Return the set of preview language features used to declare the given element.

This comment has been minimized.

@jonathan-gibbons

jonathan-gibbons Nov 11, 2020
Contributor

"Returns"

@magicus
magicus approved these changes Dec 9, 2020
Copy link
Member

@magicus magicus left a comment

Build changes still good.

@openjdk
Copy link

@openjdk openjdk bot commented Dec 9, 2020

@lahodaj this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout JDK-8250768
git fetch https://git.openjdk.java.net/jdk master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push
@openjdk openjdk bot added merge-conflict and removed ready labels Dec 9, 2020
@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Jan 6, 2021

@lahodaj This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

@openjdk openjdk bot added ready and removed merge-conflict labels Jan 7, 2021
@lahodaj
Copy link
Contributor Author

@lahodaj lahodaj commented Jan 7, 2021

I've merged the PR with the recent mainline, and I'd like to integrate sometime soon. Please let me know if there's any issue with that. Thanks!

Copy link
Contributor

@jonathan-gibbons jonathan-gibbons left a comment

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.

@@ -188,15 +192,34 @@ protected TypeElement getCurrentPageElement() {

@Override @SuppressWarnings("preview")
public void addClassSignature(String modifiers, Content classInfoTree) {

This comment has been minimized.

@jonathan-gibbons

jonathan-gibbons Jan 8, 2021
Contributor

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.

@@ -0,0 +1,303 @@
/*
* Copyright (c) 1997, 2020, Oracle and/or its affiliates. All rights reserved.

This comment has been minimized.

@jonathan-gibbons

jonathan-gibbons Jan 8, 2021
Contributor

(minor) now 2021

*/
public abstract class SummaryListWriter<L extends SummaryAPIListBuilder> extends SubWriterHolderWriter {

private String getAnchorName(SummaryElementKind kind) {

This comment has been minimized.

@jonathan-gibbons

jonathan-gibbons Jan 8, 2021
Contributor

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.

* @param headerKey table header key for the summary table
* @param contentTree the content tree to which the summary table will be added
*/
protected void addSummaryAPI(SortedSet<Element> apiList, String id,

This comment has been minimized.

@jonathan-gibbons

jonathan-gibbons Jan 8, 2021
Contributor

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

@@ -151,8 +151,6 @@ doclet.AnnotationInterfaces=Annotation Interfaces
doclet.Exceptions=Exceptions
doclet.Errors=Errors
doclet.Classes=Classes
doclet.packages=packages

This comment has been minimized.

@jonathan-gibbons

jonathan-gibbons Jan 8, 2021
Contributor

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

This comment has been minimized.

@lahodaj

lahodaj Jan 8, 2021
Author Contributor

These were used from DeprecatedListWriter.getSummaryKey which is removed by this patch (and is unused in the current mainline, as far as I know).

@@ -0,0 +1,225 @@
/*
* Copyright (c) 1998, 2020, Oracle and/or its affiliates. All rights reserved.

This comment has been minimized.

@@ -0,0 +1,123 @@
/*
* Copyright (c) 2003, 2020, Oracle and/or its affiliates. All rights reserved.

This comment has been minimized.

@jonathan-gibbons

jonathan-gibbons Jan 8, 2021
Contributor

  1. I assume you will do a bulk update for affected files.
@openjdk openjdk bot added merge-conflict and removed ready labels Jan 8, 2021
@jonathan-gibbons
Copy link
Contributor

@jonathan-gibbons jonathan-gibbons commented Jan 8, 2021

Jan Lahoda added 2 commits Jan 8, 2021
Jan Lahoda
Jan Lahoda
@openjdk openjdk bot added ready and removed merge-conflict labels Jan 8, 2021
@lahodaj
Copy link
Contributor Author

@lahodaj lahodaj commented Jan 11, 2021

/integrate

@openjdk openjdk bot closed this Jan 11, 2021
@openjdk openjdk bot added integrated and removed ready rfr labels Jan 11, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Jan 11, 2021

@lahodaj Since your change was applied there have been 22 commits pushed to the master branch:

  • 18a37f9: 8259368: Zero: UseCompressedClassPointers does not depend on UseCompressedOops
  • a03e22b: 8253910: UseCompressedClassPointers depends on UseCompressedOops in vmError.cpp
  • e0d748d: 8258606: os::print_signal_handlers() should resolve the function name of the handlers
  • bd34418: 8258445: Move make/templates to make/data
  • d21a0ea: 8258449: Move make/hotspot/symbols to make/data
  • 3974fd4: 8259451: Zero: skip serviceability/sa tests, set vm.hasSA to false
  • bb247b0: 8259392: Zero error reporting is broken after JDK-8255711
  • 2806bf2: 8259475: Fix bad merge in compilerOracle
  • b72de3c: 8259385: Cleanup unused assignment
  • 9154f64: 8254973: CompletableFuture.ThreadPerTaskExecutor does not throw NPE in #execute
  • ... and 12 more: https://git.openjdk.java.net/jdk/compare/090bd3afc37555582bde61d03e2e32350ae5d714...master

Your commit was automatically rebased without conflicts.

Pushed as commit 2354882.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment