Fixes #1028, try to make compiler summary message usableinstead of obscur aggregates by rmannibucau · Pull Request #1027 · apache/maven-compiler-plugin

rmannibucau · 2026-02-02T00:22:46Z

Following this checklist to help us incorporate your
contribution quickly and easily:

Your pull request should address just one issue, without pulling in other changes.
Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
Each commit in the pull request should have a meaningful subject line and body.
Note that commits might be squashed by a maintainer on merge.
Write unit tests that match behavioral changes, where the tests fail if the changes to the runtime are not applied.
This may not always be possible but is a best-practice.
Run mvn verify to make sure basic checks pass.
A more thorough check will be performed on your pull request automatically.
You have run the integration tests successfully (mvn -Prun-its verify).

If your pull request is about ~20 lines of code you don't need to sign an
Individual Contributor License Agreement if you are unsure
please ask on the developers list.

To make clear that you license your contribution under
the Apache License Version 2.0, January 2004
you have to acknowledge this by using the following check-box.

I hereby declare this contribution to be licenced under the Apache License Version 2.0, January 2004
In any other case, please file an Apache Individual Contributor License Agreement.

desruisseaux · 2026-02-02T10:14:32Z

src/main/java/org/apache/maven/plugin/compiler/ToolExecutor.java

-     * The locale for diagnostics, or {@code null} for the platform default.
-     */
-    private static final Locale LOCALE = null;
-


While this constant is null for now, its purpose is to make easier to find locale-sensitive codes in the future if we want to take locale in account. It may not happen in near term, but if it happens in the future, I would search for Locale or LOCALE on the whole code base.

I understand but as of today it is more misleading since it looks like it is supported and buggy cause injected statically whereas it is dead code so I preferred to drop it

It is neither buggy nor misleading. It is supported: if someone puts a value different then null, it will be taken in account. The fact that it is static final clearly indicates that it is not configurable yet. It is not dead code, it is a constant in the same was as if we defined a constant of 0. Please revert.

I didnt say it was buggy but it looks like that - was my first thought checking the code.

if someone puts a value different then null, it will be taken in account

then it will be a bug so we must not do it, it can be acceptable to make it not static but it would stay dead code cause it is never written - doesn't idea warns about it btw, it should?

So think it is really safer and saner to not do it until we do it cleanly cause it will also require to handle locale configuration and think about doing it globally or per execution.

then it will be a bug so we must not do it, it can be acceptable to make it not static but it would stay dead code

If someone puts Locale.JAPANESE, some texts will appear in Japanese, which is the expected behaviour, not a bug. It is not dead code neither, it is the declaration of a constant (for now), and null is a perfectly valid constant value.

Configuration may come in the future, which is precisely the reason why this constant is defined: if some day we want to support Locale configuration, we will just need to remove the static final part. The purpose of this constant is to save time in the future by avoiding the need to search where a locale is used. With the change in this pull request, LOCALE is replaced by null in method call arguments. It is not easy to guess by reading the code that the argument was a locale. With the LOCALE constant, it is obvious.

Please revert. It is not because the value of a constant is zero or null that it is dead code.

desruisseaux · 2026-02-02T10:47:22Z

src/main/java/org/apache/maven/plugin/compiler/AbstractCompilerMojo.java

+     */
+    @Parameter(property = "maven.compiler.messageLogType", defaultValue = "COMPILER")
+    protected MessageLogType messageLogType;
+


Could we use logger configuration instead of introducing a new plugin parameter? Currently, we use dependency injection like below, where Log is org.apache.maven.api.plugin.Log:

@Inject protected Log logger;

I think that each plugin should use a named logger. With apache/maven#11685 recently merged, I would check if using System.getLogger("org.apache.maven.compiler") produces the desired output. If it works, then I suggest to drop org.apache.maven.api.plugin.Log and use system loggers or java.util.logging instead. Then:

"org.apache.maven" would be the parent logger in Maven core and all plugins.

"org.apache.maven.compiler" would be the logger for Maven Compiler Plugin.

"org.apache.maven.compiler.summary" (or something else, proposal welcome) could be the logger for summary.

Logging can be configured in a logging framework specific way, but we could also add a <logging> section in the POM 4.1 model. We need to address Maven logging verbosity anyway, not just for the compiler plugin.

I thought about that and rational was that this is a behavior you want to have and not logger related, this is closer to an "output format" than a channel with a log level. Also think it would be a bad practise to abuse logger as we did abuse type in terms of design cause loggers are intended for output management and not for output formatting/content selection primarily - even if we can always workaround things.

so think we are good this way

This is not output formatting, but output filtering, which is in scope of logger's configuration.

filtering for a logger is only the level which doesn't apply well at all there

agree "formatting" is poor from a semantic point of view but still, it doesn't match logger design

also think we should stick to logger=class until we abuse MDC globally which is not needed there and would require a lot of rework

filtering for a logger is only the level

In java.util.logging, filtering is also by logger name. Filtering is also by handler (different filtering for logs sent to the console or logs sent to a file). I never use SLF4J, but I presume that it has the same capability than java.util.logging. If we use system logging, it will delegate either to java.util.logging (default), or to SLF4J, therefore is would have this capability as well.

also think we should stick to logger=class

java.util.logging uses a more generic term, "subsystem", with a recommendation to base it on the package name or class name. In other words, the logger name should indeed starts at least with the package name, but there is some flexibility for what come after the package name.

desruisseaux · 2026-02-02T10:55:47Z

src/main/java/org/apache/maven/plugin/compiler/MessageLogType.java

+    /**
+     * Only the summary.
+     */
+    SUMMARY,


This mode duplicates the -Xlint:none compiler option. More generally, this enumeration duplicates logging configuration. See above comment about an alternative that may be explored.

I dont understand that at all:

logging -> see previous answer

duplication -> it is already the case, if you dislike it I'm happy to drop summary feature at all too, it fixes current issues

With this pull request, in DiagnosticListener.report if the mode is SUMMARY, the compiler error or warning is not logged. This is equivalent to running javac with -Xlint:none option, which disable all warnings.

One difference between SUMMARY and -Xlint:none is that SUMMARY disables not only the warnings but also the errors, which is probably not desired.

it disables the details yes and if you have an error you rerun with -D to see it, think it matches more the summary use case than the opposite

I'm also not sure you're right saying it is equivalent to -Xlint:none since summary will be empty with that as well, no? 🤔

desruisseaux · 2026-02-02T11:05:35Z

src/main/java/org/apache/maven/plugin/compiler/ToolExecutor.java

    protected ToolExecutor(final AbstractCompilerMojo mojo, DiagnosticListener<? super JavaFileObject> listener)
            throws IOException {

+        this.listener = requireNonNull(listener, "DiagnosticListener can't be null in ToolExecutor");


With this change, we are forcing all callers to create the DiagnosticListener themselves instead of creating the default instance in one single place. A useOrCreateListener method was added in another change, but it forgot that ToolExecutor is extended by ToolExecutorForTest. The latter is now broken (EDIT: with a second though, it is not broken because of a duplicated call to useOrCreateListener in the private compiler method).

If the goal was to access the messageLogType field, it was not necessary to move this code. It can be accessed as mojo.messageLogType.

yes but I think it is cleaner to ensure we have a listener and avoid to rely on mojo there
the issue having the default there is that it makes the code particular to a single use case
I hesitate to drop tool executor to bring it back in the abstract mojo to keep the coupling

long story short, I think it doesn't change much thing even if we should probably clean up the tool executor coupling and keep it more generic (as plexus was before)

yes but I think it is cleaner to ensure we have a listener

DiagnosticListener is used only by javax.tools.JavaCompiler in tasks that are created only by ToolExecutor. The logic was that the ToolExecutor constructor is a mandatory passage before the listener is used, therefore creating a default listener in ToolExecutor constructor guarantees that we have a listener in the class that needs it. By contrast, AbstractCompilerMojo does nothing with the listener.

desruisseaux · 2026-02-02T11:07:02Z

src/main/java/org/apache/maven/plugin/compiler/AbstractCompilerMojo.java

    @SuppressWarnings("UseSpecificCatch")
    private void compile(final JavaCompiler compiler, final Options configuration) throws IOException {
-        final ToolExecutor executor = createExecutor(null);
+        final ToolExecutor executor = createExecutor(useOrCreateListener(null));


This call to useOrCreateListener duplicates the same call done in createExecutor.

EDIT: with a second though, it is a duplication for the main code, but not for TestCompilerMojo.

desruisseaux · 2026-02-02T11:15:10Z

src/main/java/org/apache/maven/plugin/compiler/AbstractCompilerMojo.java

+     */
+    private DiagnosticListener<? super JavaFileObject> useOrCreateListener(
+            final DiagnosticListener<? super JavaFileObject> listener) {
+        return listener == null


I prefer that we keep this construction inside ToolExecutor as today, in order to have a single place where the default listener is constructed. It would avoid my confusion in this and this comment.

just tried to deduplicate the useOrCreateListener call

desruisseaux · 2026-02-02T11:29:02Z

src/main/java/org/apache/maven/plugin/compiler/DiagnosticLogger.java

-        if (count > 1) {
-            message.append('s');
+    // we mainly know the one for javac as of today, this impl is best effort
+    private ResourceBundle tryGetCompilerBundle() {


This is very specific to the implementation details of the OpenJDK compiler, and I don't think that it will work in JPMS context (access to non-exported resources are blocked by the JVM). It is not exactly the desired properties file anyway, because the values are for example "{0} uses or overrides a deprecated API" and we have nothing to provide for the {0} part. We may have to provide our own .properties file. It would cover only a subset of the messages, but we could cover the most common ones.

I don't think that it will work in JPMS context

It does in our context, give it a try

It is not exactly the desired properties file anyway

Not sure what you mean cause it is literally the pattern message for the code you used 🤔 , so the intent and since it is an aggregation we don't want to fill the placeholders there, but at least the user sees something readable

It does in our context, give it a try

Yes but this is not a JPMS context. While Maven is not yet modularized, this is one of my long term goal.

Not sure what you mean cause it is literally the pattern message for the code you used

Yes but with parameters that we don't have when writing a summary. The "{0} uses or overrides a deprecated API" is a message reported when one particular class or method use a deprecated API, and "{0}" is the name of that class or method. But when we write the summary, we are saying that (for example) this warnings occurred 100 times. We have no particular class or method that we can insert in the "{0}" part.

While Maven is not yet modularized, this is one of my long term goal.

hmm, there is no consensus on that so tempted to say it will not happen so it is ok, worse case it would be easy to make it working too so not a blocker IMHO

hmm, there is no consensus on that so tempted to say it will not happen so it is ok, worse case it would be easy to make it working too

Agree that modularizing Maven is not on the agenda soon. However, it will not be easy at all to make the pull request works in that case. It will be blocked by the JVM, unless we put --add-opens options in the Maven launch script, which is strongly discouraged except for testing. Generally, using internal API is discouraged even in a non-JPMS world, and it is precisely for avoiding such practices that JPMS makes it hard.

Furthermore, we still have the issue of resource strings not applicable (the problem of having no value to supply in "{0}" emplacements).

try to make compiler summary message usable

fb9b27e

rmannibucau changed the title ~~try to make compiler summary message usable~~ try to make compiler summary message usableinstead of obscur aggregates Feb 2, 2026

rmannibucau changed the title ~~try to make compiler summary message usableinstead of obscur aggregates~~ Fixes #1028, try to make compiler summary message usableinstead of obscur aggregates Feb 2, 2026

desruisseaux reviewed Feb 2, 2026

View reviewed changes

avoid two calls to useOrCreateListener

38f6bba

desruisseaux mentioned this pull request Feb 2, 2026

DiagnosticListener summary message is obscur and misleading #1028

Open

Conversation

rmannibucau commented Feb 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

desruisseaux Feb 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

desruisseaux Feb 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rmannibucau Feb 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

desruisseaux Feb 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

rmannibucau commented Feb 2, 2026 •

edited

Loading

desruisseaux Feb 2, 2026 •

edited

Loading

desruisseaux Feb 2, 2026 •

edited

Loading

rmannibucau Feb 2, 2026 •

edited

Loading

desruisseaux Feb 2, 2026 •

edited

Loading