gnodet-bot commented on code in PR #1143:
URL:
https://github.com/apache/maven-compiler-plugin/pull/1143#discussion_r4228411402
##########
src/main/java/org/apache/maven/plugin/compiler/AbstractCompilerMojo.java:
##########
@@ -642,21 +642,26 @@ final Charset charset() {
*
* <h4>Default value</h4>
* The default value depends on the context.
- * If there is no annotation processor, then the default is {@code
"options,dependencies,sources"}.
+ * If there is no annotation processor, then the default is {@code
"options,dependencies,sources,rebuild-on-change"}.
* It means that a full rebuild will be done if the compiler options or
the dependencies changed,
- * or if a source file has been deleted. Otherwise, only the modified
source files will be recompiled.
- *
- * <p>If an annotation processor is present (e.g., {@link #proc} set to a
value other than {@code "none"}),
- * then the default value is same as above with the addition of {@code
"rebuild-on-add,rebuild-on-change"}.
- * It means that a full rebuild will be done if any kind of change is
detected.</p>
- *
- * <p>Whether an annotation processor is considered present depends on the
Java version when {@link #proc} is unset,
+ * or if a source file has been deleted, or if any source file has been
modified.
+ * This ensures correctness: if a method signature changes in {@code
Foo.java}, all classes that depend
+ * on {@code Foo} will be recompiled together with it, preventing stale
{@code .class} files that could
+ * cause {@link NoSuchMethodError} at runtime.
+ * Users who prefer faster (but potentially unsafe) incremental builds can
set
+ * {@code "options,dependencies,sources"} explicitly.
+ *
+ * <p>When an annotation processor is confirmed to be present (e.g.,
{@link #proc} set to a value other
+ * than {@code "none"}, or Java ≥ 23 with explicit processor paths),
incremental compilation is
+ * disabled entirely ({@code "none"}) because annotation processors can
generate or modify classes in
+ * ways the timestamp strategy cannot track, risking invalid compiled
output.</p>
+ *
+ * <p>When the presence of an annotation processor is uncertain (Java <
23, {@link #proc} unset,
+ * no explicit processor path), the default is
+ * {@code "options,dependencies,sources,rebuild-on-change,rebuild-on-add"}.
+ * This is a conservative middle ground: a full rebuild is done if any
kind of change is detected,
* because {@code javac} enables annotation processing by default before
Java 23 ({@code -proc:full})
- * but disables it since Java 23 ({@code -proc:none} unless a processor is
configured).
- * Consequently, on Java versions prior to 23 the plugin conservatively
assumes that a processor may be present
- * — since {@code javac} would discover processors on the compile
classpath —
- * and therefore applies {@code "rebuild-on-add,rebuild-on-change"} by
default,
- * doing a full rebuild on any change even when no processor is actually
present.
+ * and would discover processors on the compile classpath.
* Projects on Java < 23 that use no annotation processor can restore
per-file recompilation
* by setting {@link #proc} to {@code "none"} (or by setting this {@code
incrementalCompilation} property explicitly).</p>
Review Comment:
⚠️ **Stale Javadoc — `proc=none` no longer restores per-file recompilation**
This paragraph survived unchanged from the old code, but is now incorrect
after the base-default change at line 645.
Previously, `proc=none` caused `hasAnnotationProcessor()` to return `false`,
which selected the no-processor default of `options,dependencies,sources` — no
`rebuild-on-change`, so per-file recompilation worked.
With this PR, the no-processor default is now
`options,dependencies,sources,rebuild-on-change` (line 645). Setting
`proc=none` still picks the no-processor branch, but that branch now includes
`rebuild-on-change`, so a full rebuild is triggered on every source change.
The IT diff confirms this: `incremental-proc-none-per-file` was updated to
**explicitly** set
`<incrementalCompilation>options,dependencies,sources</incrementalCompilation>`
because `proc=none` alone is no longer sufficient, with the IT description
stating: *"users who want faster (but potentially unsafe) per-file compilation
must opt in explicitly."*
The paragraph should be updated to remove the `proc=none` shortcut and only
recommend the explicit `incrementalCompilation` property:
```suggestion
* Projects on Java < 23 that use no annotation processor can restore
per-file recompilation
* by setting this {@code incrementalCompilation} property explicitly to
{@code "options,dependencies,sources"}.</p>
```
##########
src/main/java/org/apache/maven/plugin/compiler/AbstractCompilerMojo.java:
##########
@@ -642,21 +642,26 @@ final Charset charset() {
*
* <h4>Default value</h4>
* The default value depends on the context.
- * If there is no annotation processor, then the default is {@code
"options,dependencies,sources"}.
+ * If there is no annotation processor, then the default is {@code
"options,dependencies,sources,rebuild-on-change"}.
* It means that a full rebuild will be done if the compiler options or
the dependencies changed,
- * or if a source file has been deleted. Otherwise, only the modified
source files will be recompiled.
- *
- * <p>If an annotation processor is present (e.g., {@link #proc} set to a
value other than {@code "none"}),
- * then the default value is same as above with the addition of {@code
"rebuild-on-add,rebuild-on-change"}.
- * It means that a full rebuild will be done if any kind of change is
detected.</p>
- *
- * <p>Whether an annotation processor is considered present depends on the
Java version when {@link #proc} is unset,
+ * or if a source file has been deleted, or if any source file has been
modified.
+ * This ensures correctness: if a method signature changes in {@code
Foo.java}, all classes that depend
+ * on {@code Foo} will be recompiled together with it, preventing stale
{@code .class} files that could
+ * cause {@link NoSuchMethodError} at runtime.
+ * Users who prefer faster (but potentially unsafe) incremental builds can
set
+ * {@code "options,dependencies,sources"} explicitly.
+ *
+ * <p>When an annotation processor is confirmed to be present (e.g.,
{@link #proc} set to a value other
+ * than {@code "none"}, or Java ≥ 23 with explicit processor paths),
incremental compilation is
+ * disabled entirely ({@code "none"}) because annotation processors can
generate or modify classes in
+ * ways the timestamp strategy cannot track, risking invalid compiled
output.</p>
+ *
+ * <p>When the presence of an annotation processor is uncertain (Java <
23, {@link #proc} unset,
+ * no explicit processor path), the default is
+ * {@code "options,dependencies,sources,rebuild-on-change,rebuild-on-add"}.
+ * This is a conservative middle ground: a full rebuild is done if any
kind of change is detected,
* because {@code javac} enables annotation processing by default before
Java 23 ({@code -proc:full})
- * but disables it since Java 23 ({@code -proc:none} unless a processor is
configured).
- * Consequently, on Java versions prior to 23 the plugin conservatively
assumes that a processor may be present
- * — since {@code javac} would discover processors on the compile
classpath —
- * and therefore applies {@code "rebuild-on-add,rebuild-on-change"} by
default,
- * doing a full rebuild on any change even when no processor is actually
present.
+ * and would discover processors on the compile classpath.
* Projects on Java < 23 that use no annotation processor can restore
per-file recompilation
* by setting {@link #proc} to {@code "none"} (or by setting this {@code
incrementalCompilation} property explicitly).</p>
Review Comment:
⚠️ **Stale Javadoc — `proc=none` no longer restores per-file recompilation**
This paragraph survived unchanged from the old code, but is now incorrect
after the base-default change at line 645.
Previously, `proc=none` caused `hasAnnotationProcessor()` to return `false`,
which selected the no-processor default of `options,dependencies,sources` — no
`rebuild-on-change`, so per-file recompilation worked.
With this PR, the no-processor default is now
`options,dependencies,sources,rebuild-on-change` (line 645). Setting
`proc=none` still picks the no-processor branch, but that branch now includes
`rebuild-on-change`, so a full rebuild is triggered on every source change.
The IT diff confirms this: `incremental-proc-none-per-file` was updated to
**explicitly** set
`<incrementalCompilation>options,dependencies,sources</incrementalCompilation>`
because `proc=none` alone is no longer sufficient, with the IT description
stating: *"users who want faster (but potentially unsafe) per-file compilation
must opt in explicitly."*
The paragraph should be updated to remove the `proc=none` shortcut and only
recommend the explicit `incrementalCompilation` property:
```suggestion
* Projects on Java < 23 that use no annotation processor can restore
per-file recompilation
* by setting this {@code incrementalCompilation} property explicitly to
{@code "options,dependencies,sources"}.</p>
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
