Run annotation processors when sources are unchanged

[efegokdemir]

Summary

Fixes #1006.

When <proc>only</proc> is configured, the compiler execution is dedicated to annotation processing. Incremental source-change detection must not skip that execution merely because the Java sources are unchanged.

The fix makes proc=only use the existing NONE incremental mode, so all sources remain available to javac and the processor runs on every execution. A unit regression test covers two executions with unchanged sources.

Testing

• mvn test — passed (17 tests)
• mvn -Dtest=CompilerMojoTestCase#testCompilerProcOnlyRunsWhenSourcesAreUnchanged test — passed
• mvn spotless:check — passed
• git diff --check — passed
• mvn verify — attempted; the repository's dependency analysis failed while parsing the intentionally minimal compiled.class emitted by the existing test compiler stub (Index 6 out of bounds for length 0). The test suite itself passed before that lifecycle step.

Contribution checklist

• This pull request addresses one issue.
• A regression test fails on the unchanged implementation and passes with this change.
• The commit includes a DCO sign-off and a verified SSH signature.
• The contribution is licensed under the Apache License 2.0.

AI assistance

Codex assisted with repository analysis, implementation, test authoring, and validation under human supervision. The submitted change and test results have been reviewed for correctness by the human contributor.

[gnodet-bot]

Correct fix — proc=only must bypass incremental source-change detection so annotation processors always run. The NONE aspect properly disables all incremental checks in ToolExecutor.applyIncrementalBuild(). Two minor observations below.

This review was generated by an AI agent, Hermès on behalf of @gnodet.

[gnodet-bot]

Both previous findings addressed:

• Javadoc — expanded to describe proc=only disabling incremental compilation. ✅
• Test fixture — removed hardcoded <incrementalCompilation>classes</incrementalCompilation>, now tests the default (user-reported) scenario. ✅

No new issues introduced by the follow-up commit. The proc=only guard is correct — fires early, clears aspects, sets NONE, returns before the annotation processor check. Test covers the exact regression from #1006.

This review was generated by an AI agent, Hermès on behalf of @gnodet.

[gnodet-bot]

All previous findings addressed:

• Javadoc — expanded to describe proc=only disabling incremental compilation. ✅
• Test fixture — uses default incrementalCompilation (no hardcoded override), matching the user-reported scenario from #1006. ✅
• Formatting (@desruisseaux) — return;} if replaced with } else if. Semantically identical — nothing follows the if-else block in the method body. ✅

No new issues in commit 010495c.

This review was generated by an AI agent, Hermès on behalf of @gnodet.

[desruisseaux]

The rational for disabling incremental compilation when there is a -proc only option is also true for the case of a -proc full option: the latter could be for both collecting information on all classes and recompiling a few classes. Doing something different in the two cases is an heuristic rules with no guarantees to be true.

Another inconsistency is that the current implementation modifies the configuration only when user did not specified explicitly an incrementalCompilation value (user configuration has precedence), while this pull request unconditionally discards user configuration in the -proc only case.

I propose to revert the changes in the amendincrementalCompilation method. Instead, keep the currently existing if statement with a different body: delete the two aspects.add calls and put the aspects.clear() and the new aspects.add(IncrementalBuild.Aspect.NONE) instead.

As an optimization, if the project is modular and there is no include/exclude filters, the option should be MODULES instead of NONE.

[gnodet]

Fixed CI failure: added assertCompilerStubOutputFileExists(compileMojo) at the end of the new test.

The CompilerStub creates an empty (0-byte) compiled.class file in the output directory during the test. The dependency:analyze phase then scans target/test-classes recursively and trips on that file with an ArrayIndexOutOfBoundsException in ASM (which cannot read an empty class file). All other tests using the stub already call assertCompilerStubOutputFileExists(), which both asserts the file exists and deletes it — the Javadoc even mentions this exact issue. The new test was the only one missing that cleanup call.

Fixed in 667a164.

[gnodet-bot]

Re-review after commit 667a164 (CI fix).

Previous findings status:

• ✅ Javadoc clarification — addressed (5b8f0e4)
• ✅ Test fixture uses default incrementalCompilation — addressed (5b8f0e4)
• ✅ Formatting (else if) — addressed (010495c)
• ⚠️ @desruisseaux's design suggestion (handling proc=full consistently, respecting user-explicit incrementalCompilation, alternative guard placement) — still open, awaiting author response

New commit 667a164: Adds the standard assertCompilerStubOutputFileExists(compileMojo) cleanup call at the end of the new test — all other tests in the suite follow this pattern. Prevents dependency:analyze from tripping on the empty stub compiled.class. Clean fix, no issues.

The implementation remains correct for the proc=only case. The outstanding design discussion with @desruisseaux about proc=full consistency and the MODULES optimization are reasonable follow-ups but do not block this fix for #1006.

This review was generated by an AI agent, Hermès on behalf of @gnodet.

[desruisseaux]

@gnodet-bot : no, at least the consistency point of above discussion is blocking. Doing that later would be a behavioural change for users and likely to cause "regression" bug reports. The comment was saying exactly what to do, please apply.

The MODULES point should at least be considered, again for avoiding the risk of regression in the future.

(being rude toward the bot, not toward the human)

[efegokdemir]

Applied the blocking consistency feedback in 05525cf. amendincrementalCompilation now keeps the existing incrementalCompilation-absent condition, clears the default aspects, and selects NONE for annotation processors, so proc=only and proc=full behave consistently without overriding explicit user configuration. I evaluated the MODULES optimization; this method does not have the modular-project context needed to select it without broadening the change.

Added coverage for default proc=full and explicit incrementalCompilation with proc=only; the existing proc=only regression and CompilerStub cleanup remain intact. mvn -Denforcer.skip=true -Dspotless.check.skip=true -Dspotless.apply.skip=true -Dspotless.skip=true -Dcheckstyle.skip=true -DskipITs -Dtest=CompilerMojoTestCase test passed (18 tests), and git diff --check passed. The normal Spotless path remains incompatible with this local JDK due the existing palantir-java-format JCTree$JCCompilationUnit.endPositions error.

[gnodet-bot]

All previous findings addressed in commit 05525cf:

• @desruisseaux consistency concern (blocking) — ✅ amendincrementalCompilation now uses aspects.clear() + aspects.add(NONE) inside the existing isAbsent(incrementalCompilation) guard. hasAnnotationProcessor returns true for both proc=only and proc=full (they fall through past the "none" check), so both cases are handled consistently.
• Explicit user configuration respected — ✅ The isAbsent(incrementalCompilation) condition ensures explicit incrementalCompilation settings are never overridden. New test testCompilerProcOnlyRespectsExplicitIncrementalCompilation validates this.
• MODULES optimization — Reasonably deferred; amendincrementalCompilation doesn't have modular-project context to select MODULES without broadening the change.
• Javadoc — ✅ Accurately describes the new behavior: "Amends the default configuration... does not amend an explicitly configured value."
• Test coverage — ✅ Three new tests: proc=only (default), proc=full (default), and proc=only with explicit incrementalCompilation=classes.
• Formatting (else if) — ✅ Addressed in 010495c, now superseded by the simplified method body.

No new issues introduced by commit 05525cf.

This review was generated by an AI agent, Hermès on behalf of @gnodet.

[efegokdemir]

Adjusted the explicit incrementalCompilation=classes regression assertion in 218661a. The second proc=only execution performs the expected incremental compile, so it must not emit “Nothing to compile - all classes are up to date.” This matches the neighboring processor-only regression conventions.

Validation: git diff --check passed. The focused Maven test could not run locally because this environment has no Java runtime and the checkout has no Maven wrapper; CI is the executable validation for this commit.

[gnodet-bot]

Re-review after commit 218661a (test assertion fix).

Previous findings status — all addressed:

• ✅ Javadoc clarification (5b8f0e4)
• ✅ Test fixture uses default incrementalCompilation (5b8f0e4)
• ✅ Formatting else if per @desruisseaux (010495c)
• ✅ CI stub cleanup assertCompilerStubOutputFileExists (667a164)
• ✅ proc=full consistency and explicit config precedence per @desruisseaux (05525cf)

New commit 218661a: Corrects the assertion in testCompilerProcOnlyRespectsExplicitIncrementalCompilation. With explicit incrementalCompilation=classes, amendincrementalCompilation does not fire (isAbsent guard), so the CLASSES aspect applies. The first execution generates class files, which the CLASSES aspect detects as changed on the second run — the second proc=only execution correctly recompiles. The previous verify(log).info("Nothing to compile...") was wrong; verify(log, never()) is correct.

This review was generated by an AI agent, Hermès on behalf of @gnodet.

[desruisseaux]

Some integration tests are failing (logs there):

• default-incremental-disable/pom.xml
• mcompiler-182/pom.xml — Case sensitivity changes in class names not detected by compiler
• MCOMPILER-192/pom.xml — No error reporting with forked compiler when Compiler includes non java files
• MCOMPILER-500-package-info-incr/pom.xml — add a parameter to exclude some pattern from incr stale source file calculation

Two of them seem related to incremental builds and may need adjustment for the new behaviour. The two others seem unrelated (judging by their title only) and would need some investigation.

[efegokdemir]

Addressed the implicit pre-Java-23 proc regression in 55c4389. When proc is unset on Java <23, the compiler default only means processing is enabled; it does not establish that a processor is present. That path now retains the previous REBUILD_ON_ADD / REBUILD_ON_CHANGE behavior instead of clearing all aspects to NONE. Explicit proc=only and proc=full still use NONE, and the explicit incrementalCompilation guard remains unchanged.

Validation: focused proc=only and proc=full unit tests passed (2/2) on local JDK 27; git diff --check passed. The full CompilerMojoTestCase run had 1 local JDK-27-only failure in the explicit-incremental expectation (17 passed); the remote JDK-17 CI run before this commit showed that test passing and failed in MCOMPILER-500-package-info-incr. I could not execute the reported JDK-17 integration cases locally. Spotless is blocked by the repository formatter’s JCCompilationUnit.endPositions incompatibility with JDK 27. No checks are reported for the new remote head yet.

[gnodet-bot]

Re-review after commit 55c4389 (fix implicit proc default incremental behavior).

Previous findings status — all addressed:

• ✅ Javadoc clarification (5b8f0e4) — updated again in this commit to describe three-way behavior
• ✅ Test fixture default incrementalCompilation (5b8f0e4)
• ✅ Formatting else if per @desruisseaux (010495c)
• ✅ CI stub cleanup assertCompilerStubOutputFileExists (667a164)
• ✅ proc=full consistency and explicit config precedence per @desruisseaux (05525cf)
• ✅ IT failures reported by @desruisseaux — addressed by this commit
• ⏳ MODULES optimization — reasonably deferred (author evaluated and explained)

Commit 55c4389 analysis: Correctly restores original master behavior for the implicit-proc pre-Java-23 case (REBUILD_ON_ADD + REBUILD_ON_CHANGE) while preserving the #1006 fix for explicit proc=only/proc=full (NONE). The condition isAbsent(proc) && !isVersionEqualOrNewer(RELEASE_23) properly distinguishes the three cases:

proc	Java	Result
absent	<23	REBUILD_ON_ADD + REBUILD_ON_CHANGE (unchanged from master)
absent	≥23	NONE (processor detected → disable incremental)
only/full	any	NONE (fixes #1006)

The unit tests (proc=only, proc=full, explicit incrementalCompilation) are JDK-version-independent since they set proc explicitly, always taking the else branch. No new issues.

Note: CI shows action_required — likely needs a maintainer to approve the workflow run for this contributor.

This review was generated by an AI agent, Hermès on behalf of @gnodet.

[gnodet-bot]

Re-review after commit a31963f (deterministic test assertion).

Previous findings status — all addressed (unchanged from last review at 55c4389).

New commit a31963f: Replaces the full mojo-execution test (testCompilerProcOnlyRespectsExplicitIncrementalCompilation) with a direct unit assertion on amendincrementalCompilation(). The old test was non-deterministic — the CLASSES aspect caused incremental detection to fire on the second execute() because generated class files from the first run were detected as changes. The new test directly verifies that an explicitly configured incrementalCompilation is never overridden: creates EnumSet.of(CLASSES), calls amendincrementalCompilation with PROCESSOR_CLASSES, and asserts the set is unchanged. Correct and deterministic.

This review was generated by an AI agent, Hermès on behalf of @gnodet.

[gnodet-bot]

Re-review after commit cc9c788 (remove obsolete test fixture).

@desruisseaux's request addressed: The compiler-proc-only-explicit-incremental-test fixture directory is deleted, and the test method now uses direct CompilerMojo instantiation with field assignment — no mojo injection or test resources needed. The assertion is identical and deterministic.

Previous findings status — all addressed (unchanged from last review at a31963f).

No new issues. CI still needs maintainer workflow approval for this external contributor.

This review was generated by an AI agent, Hermès on behalf of @gnodet.

[desruisseaux]

Thanks, it looks fine to me. I would just like two comments to be added for helping the reader to understand why the amend method is doing a special case with isAbsent(proc) && !isVersionEqualOrNewer(RELEASE_23), then I think we can merge.

[gnodet-bot]

Re-review after commit ea009b6 (docs: clarify annotation processor detection).

@desruisseaux's final requests — both addressed:

• ✅ Comment in amendincrementalCompilation below the isAbsent(proc) && !isVersionEqualOrNewer(RELEASE_23) guard — added verbatim as requested
• ✅ Javadoc addition in hasAnnotationProcessor — added verbatim as requested

Previous findings status — all addressed (unchanged from last review at cc9c788).

No code changes in this commit — documentation only. All 8 rounds of review findings are resolved.

This review was generated by an AI agent, Hermès on behalf of @gnodet.

[github-actions]

@desruisseaux Please assign appropriate label to PR according to the type of change.