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-8310061: Note if implicit annotation processing is being used #14499

Closed
wants to merge 12 commits into from

Conversation

jddarcy
Copy link
Member

@jddarcy jddarcy commented Jun 15, 2023

Emit a note warning of the possibility of changing the implicit running of annotation processors in a future release.


Progress

  • Change must be properly reviewed (1 review required, with at least 1 Reviewer)
  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue

Issue

  • JDK-8310061: Note if implicit annotation processing is being used (Bug - P3)

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/14499/head:pull/14499
$ git checkout pull/14499

Update a local copy of the PR:
$ git checkout pull/14499
$ git pull https://git.openjdk.org/jdk.git pull/14499/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 14499

View PR using the GUI difftool:
$ git pr show -t 14499

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/14499.diff

Webrev

Link to Webrev Comment

@jddarcy jddarcy marked this pull request as draft June 15, 2023 18:17
@bridgekeeper
Copy link

bridgekeeper bot commented Jun 15, 2023

👋 Welcome back darcy! 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.

@openjdk
Copy link

openjdk bot commented Jun 15, 2023

@jddarcy The following label will be automatically applied to this pull request:

  • compiler

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

@openjdk openjdk bot added the compiler compiler-dev@openjdk.org label Jun 15, 2023
Copy link
Contributor

@briangoetz briangoetz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New diagnostic messages look reasonable

@jddarcy jddarcy marked this pull request as ready for review June 16, 2023 18:35
@openjdk openjdk bot added the rfr Pull request is ready for review label Jun 16, 2023
@mlbridge
Copy link

mlbridge bot commented Jun 16, 2023

Webrevs

@jddarcy
Copy link
Member Author

jddarcy commented Jun 16, 2023

The goal of the change in this PR is to emit a message to users who may be unknowingly relying on default implicit annotation processing that the long-standing default may be changing in a future release. The message is a Note rather than a Warning to avoid running afoul of build setups using configurations like "-Xlint:all -Werror".

Semantically, the note about implicit processors is treated as on by default. The note is not emitted if there are explicit configuration options related to annotation processing, including the new-in-JDK-21 "-proc:full". and not emitted if the "options" lint check is explicitly disabled, either through "-Xlint:-options" or "-Xlint:none".

A targeted regression test for the change to follow.

@jddarcy
Copy link
Member Author

jddarcy commented Jun 16, 2023

PS To be clear, the goal is to get this change into JDK 22 and then backport it into JDK 21.

@mlbridge
Copy link

mlbridge bot commented Jun 17, 2023

Mailing list message from Alex Buckley on compiler-dev:

On 6/16/2023 11:48 AM, Joe Darcy wrote:

On Thu, 15 Jun 2023 18:16:57 GMT, Joe Darcy <darcy at openjdk.org> wrote:

Emit a note warning of the possibility of changing the implicit running of annotation processors in a future release.

The goal of the change in this PR is to emit a message to users who may be unknowingly relying on default implicit annotation processing that the long-standing default may be changing in a future release. The message is a Note rather than a Warning to avoid running afoul of build setups using configurations like "-Xlint:all -Werror".

I looked at the man page --
https://download.java.net/java/early_access/jdk21/docs/specs/man/javac.html#how-annotation-processing-works
-- to learn about javac's default search path for processors.

(`-proc:full` isn't mentioned, but maybe it's coming soon?)

The title of this mail is about "implicit annotation processing", that
is, the fact that annotation processing is enabled by default ... but
the focus of the new message is about a different kind of default,
namely the default search path for locating annotation processors. I read:

One or more annotation processors found under the default policy of
searching the class path or module path for processors.

as saying:

Let's try to be more specific about where javac should search for
processors -- look at --processor-path or --processor-module-path

(The man page doesn't mention searching the module path for processors.
It only searches the "user class path".)

Consider this text, which sidesteps "implicit" in favor of
"enabled"/"disabled" to align with how the man page starts: "Unless
annotation processing is disabled with the -proc:none option ..."

Annotation processing is enabled, and one or more annotation
processors were found. [Maybe -processorpath was specified, maybe it
wasn't, but either way, something was found.]

They will be called [the word from the man page] in this compilation,
but a future release of javac may choose to disable annotation
processing by default. It will then be necessary to enable annotation
processing with `-proc:only` or `-proc:full`.

Use -Xlint:-options to suppress this message.

Alex

@mlbridge
Copy link

mlbridge bot commented Jun 17, 2023

Mailing list message from Joseph D. Darcy on compiler-dev:

On 6/16/2023 12:20 PM, Alex Buckley wrote:

On 6/16/2023 11:48 AM, Joe Darcy wrote:

On Thu, 15 Jun 2023 18:16:57 GMT, Joe Darcy <darcy at openjdk.org> wrote:

Emit a note warning of the possibility of changing the implicit
running of annotation processors in a future release.

The goal of the change in this PR is to emit a message to users who
may be unknowingly relying on default implicit annotation processing
that the long-standing default may be changing in a future release.
The message is a Note rather than a Warning to avoid running afoul of
build setups using configurations like "-Xlint:all -Werror".

I looked at the man page --
https://download.java.net/java/early_access/jdk21/docs/specs/man/javac.html#how-annotation-processing-works
-- to learn about javac's default search path for processors.

(`-proc:full` isn't mentioned, but maybe it's coming soon?)

Getting -proc:full in the man page is being addressed under JDK-8308456.

The title of this mail is about "implicit annotation processing", that
is, the fact that annotation processing is enabled by default ... but
the focus of the new message is about a different kind of default,
namely the default search path for locating annotation processors. I
read:

? One or more annotation processors found under the default policy of
? searching the class path or module path for processors.

as saying:

? Let's try to be more specific about where javac should search for
? processors -- look at --processor-path or --processor-module-path

(The man page doesn't mention searching the module path for
processors. It only searches the "user class path".)

My mistake; I verified the implementation does not look for annotation
processors on the --module-path, just on --processor-module-path. I'll
update the note accordingly.

Consider this text, which sidesteps "implicit" in favor of
"enabled"/"disabled" to align with how the man page starts: "Unless
annotation processing is disabled with the -proc:none option ..."

? Annotation processing is enabled, and one or more annotation
? processors were found. [Maybe -processorpath was specified, maybe it
? wasn't, but either way, something was found.]

To give more context around the situations where the note appears and
the motivation behind it. Using a command line like

??? $ javac --class-path JarFileWithAnnotationProcessor.java Foo.java

will look for and find the annotation processor inside
JarFileWithAnnotationProcessor and run it per the process outlined in
the man page, etc. There are no strictly annotation processing related
configuration options in this situation; these are the sort of implicit
cases where the warning note will be reported under this PR.

In contrast, none of these other command lines would generate a note:

??? $ javac --processor-path JarFileWithAnnotationProcessor.java Foo.java
??? $ javac --class-path???? JarFileWithAnnotationProcessor.java
-processor NameOfProcessorInJarFile Foo.java
??? $ javac --class-path???? JarFileWithAnnotationProcessor.java
-proc:full????????????????????????? Foo.java

??? $ javac --class-path JarFileWithAnnotationProcessor.java
-Xlint:-options Foo.java
??? $ javac --class-path JarFileWithAnnotationProcessor.java
-Xlint:none???? Foo.java

The first three have explicit annotation processor related configuration
optoins, hence opting-in to annotation processing, and the last two
explicitly disable warnings related to option handling.

Thanks,

-Joe

? They will be called [the word from the man page] in this compilation,
? but a future release of javac may choose to disable annotation
? processing by default. It will then be necessary to enable annotation
? processing with `-proc:only` or `-proc:full`.

? Use -Xlint:-options to suppress this message.

Alex

@mlbridge
Copy link

mlbridge bot commented Jun 17, 2023

Mailing list message from Alex Buckley on compiler-dev:

On 6/16/2023 6:49 PM, Joseph D. Darcy wrote:

To give more context around the situations where the note appears and
the motivation behind it. Using a command line like

??? $ javac --class-path JarFileWithAnnotationProcessor.java Foo.java

will look for and find the annotation processor inside
JarFileWithAnnotationProcessor and run it per the process outlined in
the man page, etc. There are no strictly annotation processing related
configuration options in this situation; these are the sort of implicit
cases where the warning note will be reported under this PR.

In contrast, none of these other command lines would generate a note:

??? $ javac --processor-path JarFileWithAnnotationProcessor.java Foo.java
??? $ javac --class-path???? JarFileWithAnnotationProcessor.java
-processor NameOfProcessorInJarFile Foo.java
??? $ javac --class-path???? JarFileWithAnnotationProcessor.java
-proc:full????????????????????????? Foo.jav

A new suggestion:

Annotation processing is enabled because one or more processors
were found on the class path. A future release of javac may disable
annotation processing unless at least one processor is specified by
name (`-processor`), or a search path is specified
(`--processor-path`, `--processor-module-path`), or annotation
processing is enabled explicitly (`-proc:only`, `-proc:full`).
Use -Xlint:-options to suppress this message.

Alex

@openjdk openjdk bot added rfr Pull request is ready for review and removed rfr Pull request is ready for review labels Jun 21, 2023
@openjdk
Copy link

openjdk bot commented Jun 28, 2023

@jddarcy This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8310061: Note if implicit annotation processing is being used

Reviewed-by: briangoetz, vromero, jjg

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 3 new commits pushed to the master branch:

  • da0f832: 8310606: Fix signed integer overflow, part 3
  • f0c2f09: 8296972: [macos13] java/awt/Frame/MaximizedToIconified/MaximizedToIconified.java: getExtendedState() != 6 as expected.
  • 9f46fc2: 8310906: Fix -Wconversion warnings in runtime, oops and some code header files.

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Jun 28, 2023
Copy link
Contributor

@vicente-romero-oracle vicente-romero-oracle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks sensible

@@ -232,6 +236,10 @@ else if (option.equals("class"))
*/
public Log log;

/** Whether or not the options lint category was initially disabled
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You've copied my pattern of misuse. "Whether or not" is not recommended use, whether or not you like it.
The consensus is to use just whether; whether or not means regardless

FWIW, I still like the whether or not form!

Comment on lines +435 to +437
optionsCheckingInitiallyDisabled =
options.isSet(Option.XLINT_CUSTOM, "-options") ||
options.isSet(Option.XLINT_CUSTOM, "none");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You have a context, so you can use get a Lint object with Lint.instance(context) and then use isEnabled(LintCategory)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm slightly abusing the existing lint options category here. I want to have the effect that a new options-related link check is on by default. There are lint categories that are enabled by default, but options isn't one of them.

I'm implementing that policy by treating the implicit annotation processing check as enabled unless options checking has been explicitly disabled, which is what the code above does and is different than the standard is-options-lint-category enabled check.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PS Will add a clarifying comment that the idiom in question is intentional.

unless at least one processor is specified by name (-processor), or a search\n\
path is specified (--processor-path, --processor-module-path), or annotation\n\
processing is enabled explicitly (-proc:only, -proc:full).\n\
Use -Xlint:-options to suppress this message.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You ought to be able to suppress the message by addressing the underlying cause, not just by disabling the check.
For example, I would expect -proc:none to disable the message as well.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That observation is true and I'll add -proc:none to the test case.

I'll add a sentence that -proc:none will disable processing.

Comment on lines 216 to 217
compiler.err.annotation.unrecognized.attribute.name No newline at end of file
compiler.err.annotation.unrecognized.attribute.name
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it just me? I cannot see any difference in these lines... Is there a difference in trailing whitespace?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRC, it was an end-of-file difference, but it isn't being reflected in the webrev either.

processorName);

// write out processor source file
tb.writeFile(processorName + ".java",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you use writeJavaFiles, it will infer the filename from the source code.


// Compile the processor
new JavacTask(tb)
.files(processorName + ".java")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there's a utility method ToolBox.findJavaFiles which is generally useful in this sort of context

Copy link
Contributor

@jonathan-gibbons jonathan-gibbons left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved, with some optional suggestions for stylistic improvement

@jddarcy
Copy link
Member Author

jddarcy commented Jun 28, 2023

/integrate

@openjdk
Copy link

openjdk bot commented Jun 28, 2023

Going to push as commit 3df36c4.
Since your change was applied there have been 3 commits pushed to the master branch:

  • da0f832: 8310606: Fix signed integer overflow, part 3
  • f0c2f09: 8296972: [macos13] java/awt/Frame/MaximizedToIconified/MaximizedToIconified.java: getExtendedState() != 6 as expected.
  • 9f46fc2: 8310906: Fix -Wconversion warnings in runtime, oops and some code header files.

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label Jun 28, 2023
@openjdk openjdk bot closed this Jun 28, 2023
@openjdk openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Jun 28, 2023
@openjdk
Copy link

openjdk bot commented Jun 28, 2023

@jddarcy Pushed as commit 3df36c4.

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

@jddarcy
Copy link
Member Author

jddarcy commented Jun 28, 2023

/backport jdk21

@openjdk
Copy link

openjdk bot commented Jun 28, 2023

@jddarcy the backport was successfully created on the branch jddarcy-backport-3df36c4f in my personal fork of openjdk/jdk21. To create a pull request with this backport targeting openjdk/jdk21:master, just click the following link:

➡️ Create pull request

The title of the pull request is automatically filled in correctly and below you find a suggestion for the pull request body:

Hi all,

This pull request contains a backport of commit 3df36c4f from the openjdk/jdk repository.

The commit being backported was authored by Joe Darcy on 28 Jun 2023 and was reviewed by Brian Goetz, Vicente Romero and Jonathan Gibbons.

Thanks!

If you need to update the source branch of the pull then run the following commands in a local clone of your personal fork of openjdk/jdk21:

$ git fetch https://github.com/openjdk-bots/jdk21.git jddarcy-backport-3df36c4f:jddarcy-backport-3df36c4f
$ git checkout jddarcy-backport-3df36c4f
# make changes
$ git add paths/to/changed/files
$ git commit --message 'Describe additional changes made'
$ git push https://github.com/openjdk-bots/jdk21.git jddarcy-backport-3df36c4f

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler compiler-dev@openjdk.org integrated Pull request has been integrated
4 participants