Skip to content

JDK-8306819: Consider disabling the compiler's default active annotation processing #14432

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

Closed
wants to merge 10 commits into from

Conversation

jddarcy
Copy link
Member

@jddarcy jddarcy commented Jun 12, 2023

Change annotation processing to be opt-in rather than opt-out.


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
  • Change requires CSR request JDK-8317544 to be approved

Issues

  • JDK-8306819: Consider disabling the compiler's default active annotation processing (Enhancement - P4)
  • JDK-8317544: Consider disabling the compiler's default active annotation processing (CSR)

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 14432

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

Using diff file

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

Webrev

Link to Webrev Comment

@jddarcy jddarcy marked this pull request as draft June 12, 2023 23:50
@bridgekeeper
Copy link

bridgekeeper bot commented Jun 12, 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 12, 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 12, 2023
@bridgekeeper
Copy link

bridgekeeper bot commented Aug 8, 2023

@jddarcy This pull request has been inactive for more than 8 weeks and will be automatically closed if another 8 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!

@jddarcy
Copy link
Member Author

jddarcy commented Aug 8, 2023

Keep alive.

@jddarcy jddarcy marked this pull request as ready for review September 9, 2023 22:26
@jddarcy
Copy link
Member Author

jddarcy commented Sep 9, 2023

/csr needed

@openjdk openjdk bot added rfr Pull request is ready for review csr Pull request needs approved CSR before integration labels Sep 9, 2023
@openjdk
Copy link

openjdk bot commented Sep 9, 2023

@jddarcy has indicated that a compatibility and specification (CSR) request is needed for this pull request.

@jddarcy please create a CSR request for issue JDK-8306819 with the correct fix version. This pull request cannot be integrated until the CSR request is approved.

@jddarcy
Copy link
Member Author

jddarcy commented Sep 9, 2023

With the preparatory work of

8310061: Note if implicit annotation processing is being used

in JDK 21, please review this first cut at removing implicit annotation processing by default.

In this first iteration, I left the javac note in the properties file. In a subsequent iteration, we may decided to keep the old policy or earlier source levels, etc. Depending on the final disposition of that point, the property key should be fully expunged.

Once it is clearer what the eventual policy will be, I'll file the corresponding CSR.

@mlbridge
Copy link

mlbridge bot commented Sep 9, 2023

Webrevs

@rbygrave
Copy link

rbygrave commented Sep 10, 2023

Based on my observations, nowadays annotation processing is hardly used anymore and if it's used it mostly occurs after the actual compilation process in a build tool

Annotation processing use is fundamental to newer frameworks like Quarkus, Micronaut and Spring framework is also adopting more use of annotation processing. This is because it enables frameworks and libraries to move some work from runtime to build time for reduced memory consumption and faster startup (replacing reflection, dynamic proxies etc).

For myself I have annotation processors that:

  • Perform Dependency Injection
  • Generate JSON adapters (think Jackson without any runtime reflection, all the fastest JSON libs use annotation processing)
  • Generate Rest Http Clients (think JAX-RS Jersey client implemented as source code generation)
  • Generate Http Server controllers (think JAX-RS Jersey server implementation as source code generation)
  • Generate Validation adapters (think Jakarta Validation implemented as source code generation)
  • Generate "Builders" for record types

Its also common to use annotation processing for mapping/adapting between types (e.g. MapStruct)

I'd suggest the use of annotation processing is actually taking off in the last say 5 years as we especially look to deploy applications into cloud hosted environments where we pay money for the amount of CPU and Memory used. Annotation processing means that a LOT of libraries can move some or even most of their functionality to build time reducing the CPU required to initialise the library and the total memory consumption.

Based on my observations, nowadays annotation processing is hardly used anymore

To me I see the exact opposite happening and it's driven by cloud adoption and the desire to move work from runtime to build time.

@altrisi
Copy link
Contributor

altrisi commented Sep 10, 2023

This doesn't remove annotation processors, just disables them running by default with no input from the developer (I assume if they were simply in the compiler classpath, which could happen by accident). As mentioned, it can still be enabled by its flag, and it's likely build systems will set that flag for you if you have declared you use annotation processors for your build.

I also disagree in the hardly used anymore part though, but I have no data.

@liach
Copy link
Member

liach commented Sep 10, 2023

@rbygrave This patch isn't aimed to remove AP. This aims to avoid unintentional AP pollutions in classpath, like if one AP is present in a dependency and thus your classpath but you don't want it to run. Such classpath separation have already been promoted by build tools like Gradle.

After all, your build tool should be setting the -proc flag for you already, just like setting the classpath flags. You can first upgrade to Java 21, which has patch https://bugs.openjdk.org/browse/JDK-8310061 that warns you about implicit APs, before upgrading to 22 that changes the default behavior.

@SentryMan
Copy link

SentryMan commented Sep 10, 2023

@rbygrave This patch isn't aimed to remove AP.

Pretty sure he was only speaking against the statement that AP is an unused feature.

This aims to avoid unintentional AP pollutions in classpath, like if one AP is present in a dependency and thus your classpath but you don't want it to run.

In practice is this a thing that happens? Most processors I've seen (like Lombok or record builder for example) are typically added as provided or optional scope dependencies. (and so cannot be transitively pulled)

@rob-bygrave
Copy link

rob-bygrave commented Sep 10, 2023

This patch isn't aimed to remove AP. This aims to avoid unintentional AP pollutions in classpath

Yes, I understand that.

Such classpath separation have already been promoted by build tools like Gradle.

BUT it has NOT been promoted by Maven. In the bug report and in this PR I do not see any explicit recognition of how this change will impact the community that is using Maven and I am suggesting that this is a significant proportion of the community.

For example, all the libraries that use annotation processing that document maven use could be impacted along the lines that their documentation will now be at least misleading (and sometimes incorrect).

This change might indeed be a good long term change but it seems to be being made without any recognition or understanding of how impactful it will be.

@mlbridge
Copy link

mlbridge bot commented Sep 11, 2023

Mailing list message from Josiah Noel on compiler-dev:

Is it really such a common thing that people accidentally include unrelated
annotation processor dependencies transitively via maven? Most of the time,
I see processors are explicitly defined with the provided or optional scope
meaning they aren't usually pulled transitively. Perhaps the most used
annotation processor Lombok (even though it breaks the rules, it starts
with annotation processing) tells it's users to add it via provided scope.

On Sun, Sep 10, 2023, 8:58 AM altrisi <duke at openjdk.org> wrote:

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/compiler-dev/attachments/20230910/db1b9a49/attachment.htm>

@SentryMan
Copy link

BUT it has NOT been promoted by Maven. In the bug report and in this PR I do not see any explicit recognition of how this change will impact the community using Maven. I am suggesting that this is a significant proportion of the community.

If this is the new reality I can deal with it, but it would indeed be nice to know that the full gravity of this breaking change was acknowledged and taken into account when making this decision. (The initial proposal seems to make the assertion
that annotation processing is a niche tool that isn't used much)

@liach
Copy link
Member

liach commented Sep 11, 2023

For whoever wondering about the impact of this breaking change, in JDK 21, the previous release and the previous LTS release, there's a new warning when the default active AP is used: #14499 https://bugs.openjdk.org/browse/JDK-8310061

@jddarcy
Copy link
Member Author

jddarcy commented Sep 11, 2023

For whoever wondering about the impact of this breaking change, in JDK 21, the previous release and the previous LTS release, there's a new warning when the default active AP is used: #14499 https://bugs.openjdk.org/browse/JDK-8310061

To expand on and reiterate points already made by @liach , the goal here is only to change whether or not annotation processing is run implicitly in a case like

javac -cp $CLASSPATH file

Accomplishing this is being broken up over (at least) two releases. In JDK 21, there were two related changes:

JDK-8308245: Add -proc:full to describe current default annotation processing policy
[JDK-8310061] (https://bugs.openjdk.org/browse/JDK-8310061): Note if implicit annotation processing is being used

The -proc:full option corresponds to the long-standing old default policy on annotation processing. Quoting the note added in the second bug,

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.
Use -proc:none to disable annotation processing.

The message is a note rather than a waning to avoid tripping up people who run their builds with -Werror. Besides a release note in JDK 21, we did extra outreach on the quality discuss alias to publicize this change (https://mail.openjdk.org/pipermail/quality-discuss/2023-July/001122.html).

So, starting in JDK 21, if you want to get the old look-on-the-classpath behavior, it is sufficient to use -proc:full. This also will work in JDK 22 after the changes in this PR go back. Breaking the change up over multiple release helps avoid a more disruptive flag day since -proc:full can be used before and after to request the desired behavior. HTH

@dmlloyd
Copy link
Contributor

dmlloyd commented Sep 11, 2023

This aims to avoid unintentional AP pollutions in classpath, like if one AP is present in a dependency and thus your classpath but you don't want it to run.

In practice is this a thing that happens? Most processors I've seen (like Lombok or record builder for example) are typically added as provided or optional scope dependencies. (and so cannot be transitively pulled)

In fact this is a thing that can cause problems. At present, annotation processor authors have to be cautious about including dependencies because commonly used libraries (such as Guava or Eclipse Collections) can conflict with other processors and/or the user's project with surprising (and sometimes subtle) results.

I'm not sure about other build tools, but Maven does supply a means to include an annotation processor on a (relatively) isolated class path, i.e. separately from the application's class path. Though this is somewhat more cumbersome, it is certainly safer.

@SentryMan
Copy link

SentryMan commented Sep 11, 2023

I'm not sure about other build tools, but Maven does supply a means to include an annotation processor on a (relatively) isolated class path, i.e. separately from the application's class path. Though this is somewhat more cumbersome, it is certainly safer.

Maven's annotationProcessorPaths has its own CP issues when compared to using implicit processing. Even now I have code in our processor to check if the processor is currently running in annotationProcessorPaths since the cp can get wonky when using it. (especially when dealing with JPMS)

The message is a note rather than a waning to avoid tripping up people who run their builds with -Werror. Besides a release note in JDK 21, we did extra outreach on the quality discuss alias to publicize this change (https://mail.openjdk.org/pipermail/quality-discuss/2023-July/001122.html).

I do appreciate this, thanks. I've added myself to this mailing list.

@jddarcy
Copy link
Member Author

jddarcy commented Sep 11, 2023

This aims to avoid unintentional AP pollutions in classpath, like if one AP is present in a dependency and thus your classpath but you don't want it to run.

In practice is this a thing that happens? Most processors I've seen (like Lombok or record builder for example) are typically added as provided or optional scope dependencies. (and so cannot be transitively pulled)

In fact this is a thing that can cause problems. At present, annotation processor authors have to be cautious about including dependencies because commonly used libraries (such as Guava or Eclipse Collections) can conflict with other processors and/or the user's project with surprising (and sometimes subtle) results.

I'm not sure about other build tools, but Maven does supply a means to include an annotation processor on a (relatively) isolated class path, i.e. separately from the application's class path. Though this is somewhat more cumbersome, it is certainly safer.

Yes, in terms of good build hygiene, I recommend separately specifying the path for annotation processors and the path for general compiler dependencies, even if they happen to be the same path.

Likewise,, specifying --release $N is a good practice too.

@@ -67,7 +67,7 @@ enum ImplicitType {

enum AnnoType {
NONE, // no annotation processing
SERVICE, // implicit annotation processing, via ServiceLoader
// SERVICE, // implicit annotation processing, via ServiceLoader
Copy link
Contributor

Choose a reason for hiding this comment

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

why keeping this if it won't be used?

Copy link
Member Author

Choose a reason for hiding this comment

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

why keeping this if it won't be used?

Sure; I can delete that before the changeset goes back.

@jddarcy
Copy link
Member Author

jddarcy commented Oct 6, 2023

/integrate

@openjdk
Copy link

openjdk bot commented Oct 6, 2023

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

  • a4e9168: 8309130: x86_64 AVX512 intrinsics for Arrays.sort methods (int, long, float and double arrays)
  • 6c6beba: 8317128: java/nio/file/Files/CopyAndMove.java failed with AccessDeniedException
  • b62e774: 8317515: Unify the code of the parse*() families of methods in j.l.Integer and j.l.Long
  • a64794b: 8317560: Change to Xcode 14.3.1 for building on macOS at Oracle
  • b3cc0c8: 8317318: Serial: Change GenCollectedHeap to SerialHeap in whitebox
  • 691db5d: 8317592: Serial: Remove Space::toContiguousSpace
  • ec9ba5d: 8317660: [BACKOUT] 8269393: store/load order not preserved when handling memory pool due to weakly ordered memory architecture of aarch64
  • 7162624: 8269393: store/load order not preserved when handling memory pool due to weakly ordered memory architecture of aarch64
  • f0d66d1: 8317502: Add asserts to check for non-null in ciInstance::java_lang_Class_klass
  • 991ce84: 4964430: (spec) missing IllegalStateException exception requirement for javax.crypto.Cipher.doFinal
  • ... and 9 more: https://git.openjdk.org/jdk/compare/3105538de5569845547b40f243a994a95a84b48f...master

Your commit was automatically rebased without conflicts.

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

openjdk bot commented Oct 6, 2023

@jddarcy Pushed as commit dc4bc4f.

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

@DanielThomas
Copy link

DanielThomas commented Oct 8, 2023

After much head scratching attempting to run the benchmarks for 8309130, I realised this change broke microbenchmarks:

$ make test TEST="micro:java.lang.ArraysSort"
Building target 'test' in configuration 'linux-x86_64-server-release'
...
Test selection 'micro:java.lang.ArraysSort', will run:
* micro:java.lang.ArraysSort

Running test 'micro:java.lang.ArraysSort'
Exception in thread "main" java.lang.RuntimeException: ERROR: Unable to find the resource: /META-INF/BenchmarkList
	at org.openjdk.jmh.runner.AbstractResourceReader.getReaders(AbstractResourceReader.java:98)
	at org.openjdk.jmh.runner.BenchmarkList.find(BenchmarkList.java:124)
	at org.openjdk.jmh.runner.Runner.internalRun(Runner.java:252)
	at org.openjdk.jmh.runner.Runner.run(Runner.java:208)
	at org.openjdk.jmh.Main.main(Main.java:71)
Finished running test 'micro:java.lang.ArraysSort'
Test report is stored in build/linux-x86_64-server-release/test-results/micro_java_lang_ArraysSort

@forax
Copy link
Member

forax commented Oct 8, 2023

Yes, JMH is using an annotation processor which now does not run anymore

@jaikiran
Copy link
Member

Hello @DanielThomas, thank you reporting and narrowing down the microbenchmark failures. This is now being addressed in https://bugs.openjdk.org/browse/JDK-8317802.

@jddarcy
Copy link
Member Author

jddarcy commented Oct 19, 2023

Hello @DanielThomas, thank you reporting and narrowing down the microbenchmark failures. This is now being addressed in https://bugs.openjdk.org/browse/JDK-8317802.

Thanks for taking care of that issue @jaikiran .

@jddarcy
Copy link
Member Author

jddarcy commented Oct 21, 2023

This patch isn't aimed to remove AP. This aims to avoid unintentional AP pollutions in classpath

Yes, I understand that.

Such classpath separation have already been promoted by build tools like Gradle.

BUT it has NOT been promoted by Maven. In the bug report and in this PR I do not see any explicit recognition of how this change will impact the community that is using Maven and I am suggesting that this is a significant proportion of the community.

For example, all the libraries that use annotation processing that document maven use could be impacted along the lines that their documentation will now be at least misleading (and sometimes incorrect).

This change might indeed be a good long term change but it seems to be being made without any recognition or understanding of how impactful it will be.

Maven is listed as a participant in the OpenJDK Quality Outreach effort (https://wiki.openjdk.org/display/quality/Quality+Outreach).

The prior change of the javac emitting the note was explicitly raised on the quality discuss alias in July 2023 (https://mail.openjdk.org/pipermail/quality-discuss/2023-July/thread.html), ahead of the GA of JDK 21 in September.

The change in this PR is likewise publicized this the October 2023 quality discuss message (https://mail.openjdk.org/pipermail/quality-discuss/2023-October/001129.html), months ahead of the expected GA of JDK 23 in March 2024.

@SentryMan
Copy link

Just out of curiosity though, why didn't this (pretty sizable breaking change) get a JEP? The dynamic agents being disabled by default got one.

@jddarcy
Copy link
Member Author

jddarcy commented Oct 26, 2023

Just out of curiosity though, why didn't this (pretty sizable breaking change) get a JEP? The dynamic agents being disabled by default got one.

The short answer is despite the Sturm und Drang of any discussions on Reddit, I don't agree with the assessment that this PR constitutes a "pretty sizable breaking change."

To summarize: the capabilities of javac with respect to annotation processing are not changing in any way. The only aspect that is changing is what is the default policy if no explicit annotation processing configuration options are given.

If a build environment is already using any of the javac options -processor, --processor-path, --processor-module-path, or -proc:only nothing changes.

If a build environment does not have annotation processors on the class path, nothing changes.

In JDK 21, javac prints a note -- not a warning for a reason I'll explain below -- to identify the situations where implicit annotation processing is being relied on. Now it is true that users might not see this note, but communication about the JDK 21 change was publicized on the quality-discuss list to help make sure interested parties were informed of the change. The reason the javac message is a note and not a warning is so that build environments running under -Werror will not (spuriously) fail solely due to communicating the possibility of a future policy change.

Starting in JDK 21, a build environment or other javac invocation can be explicitly configured to request the old default policy by using -proc:full. All the other existing annotation processing configuration options (-processor, -processor-path, ...) are still supported and mean exactly the same thing as before.

Only in JDK 22 will the default policy actually be change to not look on the class path for annotation processors (unless -proc:full is also given). However, there need not be any flag day for changing to JDK 22 specifically since the option to request the old policy is accepted in JDK 21.

@rbygrave
Copy link

there need not be any flag day for changing to JDK 22 specifically since the option to request the old policy is accepted in JDK 21

Just to say, most of our libs build against 11, 17, 21 and EA/22. So for this reason our projects we can't trivially make use of -proc:full introduced in 21 as that fails the builds using 11 and 17 and instead we needed to introduce build profiles for this.

@jddarcy
Copy link
Member Author

jddarcy commented Oct 26, 2023

there need not be any flag day for changing to JDK 22 specifically since the option to request the old policy is accepted in JDK 21

Just to say, most of our libs build against 11, 17, 21 and EA/22. So for this reason our projects we can't trivially make use of -proc:full introduced in 21 as that fails the builds using 11 and 17 and instead we needed to introduce build profiles for this.

Noted; thanks.

@mlbridge
Copy link

mlbridge bot commented Nov 1, 2023

Mailing list message from Ethan McCue on compiler-dev:

I'm still unclear why this change is not going through the same general
process as JEP 451.

On Fri, Oct 20, 2023 at 9:45?PM Joe Darcy <darcy at openjdk.org> wrote:

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/compiler-dev/attachments/20231031/7714b571/attachment.htm>

@shipilev
Copy link
Member

I have learned very late (about a month ago, and through JMH bug reports!) about this change.

Looking at incoming bug reports, I can see this is looking to break significant amount of day-to-day things that rely on annotation processing to work by default. This includes existing JMH benchmarks, existing JCStress tests, and that is only OpenJDK tools so far! The proposed fix, "enable the annotation processing in build" seems to underestimate how many already existing build configs are there, and how widespread annotation processing is. So this is a harbinger of a widespread breakage when updating to JDK 22 and beyond.

Yet, I see only tiny discussion about this in either here or in the CSR. It mostly just states that disabling annotation processors is the right thing to do to solve the externally reported problematic scenario. Normally, given the impact for changing a policy like this, I would have expected to see a JEP-sized discussion that weighs pros and cons for mitigation strategies, polls what problems real projects would run into, discusses to what extent we want to deal with currently supported JDK releases, etc.

After more digging, the only (?) discussions I could find is Reddit post and this PR comments, which contain some valid questions, concerns, and scenarios the discussion for a change like this should consider.

Yes, there was a warning message. This only highlights that hardly anyone reads the warning messages, especially buried somewhere in CI logs. This also does not capture the unfortunate reality that JDK 21 only starts to see major use, so most users did not even had the opportunity to see the warning message! In other words, warning messages are inefficient tools to bring quick attention to the issue that proposes to change the JDK behavior in a considerable way.

IMO, JEPs are done for this reason, even for less impactful things. So I suggest we revert JDK-8306819 from JDK 22, and start over with the JEP targeting JDK 23 or even 24. I believe this would be a right thing to do at this point.

@jddarcy
Copy link
Member Author

jddarcy commented Nov 30, 2023

I have learned very late (about a month ago, and through JMH bug reports!) about this change.

Looking at incoming bug reports, I can see this is looking to break significant amount of day-to-day things that rely on annotation processing to work by default. This includes existing JMH benchmarks, existing JCStress tests, and that is only OpenJDK tools so far! The proposed fix, "enable the annotation processing in build" seems to underestimate how many already existing build configs are there, and how widespread annotation processing is. So this is a harbinger of a widespread breakage when updating to JDK 22 and beyond.

Yet, I see only tiny discussion about this in either here or in the CSR. It mostly just states that disabling annotation processors is the right thing to do to solve the externally reported problematic scenario. Normally, given the impact for changing a policy like this, I would have expected to see a JEP-sized discussion that weighs pros and cons for mitigation strategies, polls what problems real projects would run into, discusses to what extent we want to deal with currently supported JDK releases, etc.

After more digging, the only (?) discussions I could find is Reddit post and this PR comments, which contain some valid questions, concerns, and scenarios the discussion for a change like this should consider.

Yes, there was a warning message. This only highlights that hardly anyone reads the warning messages, especially buried somewhere in CI logs. This also does not capture the unfortunate reality that JDK 21 only starts to see major use, so most users did not even had the opportunity to see the warning message! In other words, warning messages are inefficient tools to bring quick attention to the issue that proposes to change the JDK behavior in a considerable way.

IMO, JEPs are done for this reason, even for less impactful things. So I suggest we revert JDK-8306819 from JDK 22, and start over with the JEP targeting JDK 23 or even 24. I believe this would be a right thing to do at this point.

Thanks for the input @shipilev ; I'm looking into options to amend this change later in JDK 22.

@jddarcy
Copy link
Member Author

jddarcy commented Dec 6, 2023

I have learned very late (about a month ago, and through JMH bug reports!) about this change.
Looking at incoming bug reports, I can see this is looking to break significant amount of day-to-day things that rely on annotation processing to work by default. This includes existing JMH benchmarks, existing JCStress tests, and that is only OpenJDK tools so far! The proposed fix, "enable the annotation processing in build" seems to underestimate how many already existing build configs are there, and how widespread annotation processing is. So this is a harbinger of a widespread breakage when updating to JDK 22 and beyond.
Yet, I see only tiny discussion about this in either here or in the CSR. It mostly just states that disabling annotation processors is the right thing to do to solve the externally reported problematic scenario. Normally, given the impact for changing a policy like this, I would have expected to see a JEP-sized discussion that weighs pros and cons for mitigation strategies, polls what problems real projects would run into, discusses to what extent we want to deal with currently supported JDK releases, etc.
After more digging, the only (?) discussions I could find is Reddit post and this PR comments, which contain some valid questions, concerns, and scenarios the discussion for a change like this should consider.
Yes, there was a warning message. This only highlights that hardly anyone reads the warning messages, especially buried somewhere in CI logs. This also does not capture the unfortunate reality that JDK 21 only starts to see major use, so most users did not even had the opportunity to see the warning message! In other words, warning messages are inefficient tools to bring quick attention to the issue that proposes to change the JDK behavior in a considerable way.
IMO, JEPs are done for this reason, even for less impactful things. So I suggest we revert JDK-8306819 from JDK 22, and start over with the JEP targeting JDK 23 or even 24. I believe this would be a right thing to do at this point.

Thanks for the input @shipilev ; I'm looking into options to amend this change later in JDK 22.

Upon further reflection, we'll be deferring the policy change to disable implicit annotation processing from JDK 22 to JDK 23. The changeset to re-enable the old policy is out for review as #16988.

In addition, we plan to backport support for the -proc:full option to our update releases: https://bugs.openjdk.org/browse/JDK-8308245

@rmannibucau
Copy link

Think there is some reasoning shortcut taken there.
The will to avoid to enable annotation processor when not needed is sane and I agree we regularly see apps enabling some of them without needing due to the dependency game/abuse.

That said it would be welcomed to take into account 3 points and not only postpone because the community kind of reject this shift - otherwise it will happen again in a few years:

  1. Discovery is needed and enable a loose coupling between processor and applications (referencing the class is not great and couple the provider of the processor with all apps setup, this is undesired until a notion of alias is added to annot proc but not sure it is needed).
  2. Warning undesired setup (enablement when not needed) is a good thing, if there is a too high cost (ServiceLoader one is not since any run will likely trigger a lot of getResources and it is fine, even just plain ToolProvider to get javac) then defaulting to a mode where run fails is better than disabling blindly (-proc:failIfOverhead - indeed a better name is welcomed). That said, due to javac code this is hardly explanable so it would better be "fail if explicitly enabled and unused, warn if unused and implicitly enabled" so pretty close to what we have for years and everyone was happy with but a bit stricter for future uses.
  3. Annotation processing is a great Javac feature and I don't think it is an option to make it "hard"/verbose to setup (2 maven executions of compiler plugin for example which would make people using it even slower and harder to setup - incremental support is not trivial for people not understanding what happens under the hood).
  4. Annotation processing is far to be dead, debatable probably but lombok, immutables, JPA, dagger, (google) auto, jmh, mapstruct, fusion, ... are good example of very living libraries with some serious user base (records don't solve all issues ;)).
  5. Annotation processing is a neat way to solve the reflection challenges native-image (graalvm) brings.
  6. If the generation is not part of the main compilation cycle it enforces to use 2 "load the model" cycle which is often slower (due to the model loading but also the fact it will not use javac internals to rely on a stable API so in terms of classloading it will be worse, in particular in modular systems like maven).
  7. All the point about disabling it are security related but this is the wrong way to fix the security of the build for most builds (all using a dependency manager like ivy/gradle, maven/aether cause these builds will keep all the mentionned issues if they don't use the dependency validation which must be done at build level (by design it cant be done at javac level).

So overall the original statements should be (in)validated and if there is really an issue using ServiceLoader maybe switch to a better SPI option (better to request tools/producers to adjust than consumers) rather than disabling the discovery feature.

Side note: not great for users but acceptable if the will is really to disable the discovery, keeping the full flag could be an accepable compromise IMHO.

@jddarcy
Copy link
Member Author

jddarcy commented Feb 7, 2024

there need not be any flag day for changing to JDK 22 specifically since the option to request the old policy is accepted in JDK 21

Just to say, most of our libs build against 11, 17, 21 and EA/22. So for this reason our projects we can't trivially make use of -proc:full introduced in 21 as that fails the builds using 11 and 17 and instead we needed to introduce build profiles for this.

Noted; thanks.

PS Support for -proc:full has been backported to Oracle's 17u, 11u, and 8u release trains (JDK-8321416, JDK-8321418, JDK-8321419) as well as the OpenJDK 17u and 11u release trains (JDK-8324670, JDK-8324804). The feature is on track to appear in the April 2024 set of releases.

@jddarcy
Copy link
Member Author

jddarcy commented May 24, 2024

FYI, PR to implement the change in annotation processing default policy in JDK 23 out for review:

#19331

see also discussion thread

https://mail.openjdk.org/pipermail/jdk-dev/2024-May/009028.html

@rmannibucau
Copy link

Any hope it got reverted to the good old behavior since gain for the jdk is negligible and impact for end user is huge, plus it breaks annot proc cycle generation?

@jddarcy jddarcy deleted the JDK-8306819 branch October 27, 2024 18:35
iappleth pushed a commit to iappleth/iAppleCore that referenced this pull request Oct 31, 2024
…void warnings

Until Java<21 the java compiler implicitely looked for and ran any
annotation processor found on the classpath.
Since Java21 the following warning is shown to inform users about upcoming change of
default for this policy.

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

- Add a new ant property javac.proc to specify value to be used via
  <compilerarg line> for all our various javac invocations
- For the value use -proc:none to explicitely disable annotation processing
  during compilation
  <compilearg line> was chosne as <compiler arg> value run into some
  cornercase issues in case value was empty.
- Reasoning:
  - Upstream recommends to explcitely specify and control annotation
    processors instead of them being 'found' implicitely
    openjdk/jdk#14432 (comment)
  - Checking all jar libraries in mods/pmods (if they ship a
    META-INF/avax.annotation.processing.Processor) file found 1 single library
    shipping an annotation processor: log4j for its plugin system
    - However we do not use any shipped extra plugin
  - We have a few custom plugins ourself in src-test having @plugin
    - Annotation processing was running before for those as shown on
      compile output
    [javac] Note: Processing Log4j annotations
    [javac] Note: Annotations processed
  - However not running it seems to have no impact on the functionality
    - Tested via existing DalTest which assert's on output of content of
      testLogAppender configured via those plugins
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
Development

Successfully merging this pull request may close these issues.