Skip to content

CORE-57 - Surface javac diagnostics for runtime compile failures#180

Open
sam-ross wants to merge 1 commit intodevelopfrom
feature/CORE-57-surface-javac-diagnostics
Open

CORE-57 - Surface javac diagnostics for runtime compile failures#180
sam-ross wants to merge 1 commit intodevelopfrom
feature/CORE-57-surface-javac-diagnostics

Conversation

@sam-ross
Copy link
Copy Markdown

@sam-ross sam-ross commented May 7, 2026

CORE-57 - Surface javac diagnostics for runtime compile failures

Summary

Surfaces javac diagnostics directly when runtime compilation fails, instead of masking the real compile failure as a generated-class ClassNotFoundException.

Context

CachedCompiler.compileFromJava(...) returns an empty map when javac fails. Previously, CachedCompiler.loadFromJava(...) did not distinguish that failure from a successful compile with no classes to define, and still called classLoader.loadClass(className).

With module-style classloaders, this can produce a misleading ClassNotFoundException for the generated class name, hiding the actual javac diagnostics such as missing symbols or unresolved types.

Changes

  • Added an internal CompilationResult path in CachedCompiler.
  • Preserved the existing map-returning compileFromJava(...) behavior.
  • Captured javac diagnostics while continuing to write them to the supplied PrintWriter.
  • Changed loadFromJava(...) to throw diagnostic ClassNotFoundException immediately when javac compilation fails.
  • Added an explicit guard for “compilation succeeded but did not produce the requested class”.
  • Returned the cached class after successful defineClass(...) instead of falling through to classLoader.loadClass(...).
  • Added regression tests for:
    • successful generated-class loading with a module-like classloader
    • loader-only dependency compile failure surfacing javac diagnostics
    • successful compile output missing the requested class

Verification

mvn -q -Dtest=CachedCompilerModuleClassLoaderReproTest test
mvn -q -Dtest=CachedCompilerModuleClassLoaderReproTest,CompilerTest,CachedCompilerAdditionalTest test
mvn -q test

@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud Bot commented May 7, 2026

@sam-ross sam-ross requested review from a team, KealanTolandChronicle, benbonavia, chroniclekevinpowe, james-mcsherry and tgd and removed request for a team May 7, 2026 13:17
Copy link
Copy Markdown

@KealanTolandChronicle KealanTolandChronicle left a comment

Choose a reason for hiding this comment

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

Overall looks good, just one small comment about thread safety string appending, approved!

javaFileObjects.put(className, new JavaSourceFromString(className, javaCode));
compilationUnits = new ArrayList<>(javaFileObjects.values()); // To prevent CME from compiler code
}
StringBuilder diagnostics = new StringBuilder();
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This StringBuilder is captured in the DiagnosticListener lambda below, but if javac calls report() on multiple threads, then StringBuilder.append() will not be thread safe. Might be better to use StringBuffer or else explicit synchronisation, unless we can guarantee this will always be single-threaded ?

Copy link
Copy Markdown
Contributor

@benbonavia benbonavia May 8, 2026

Choose a reason for hiding this comment

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

The compile is done in this thread from the call() method, so I don't think there foresee any concurrency issues

fileManagerMap.put(classLoader, fileManager);
}
final Map<String, byte[]> compiled = compileFromJava(className, javaCode, printWriter, fileManager);
final CompilationResult compilation = compileFromJavaResult(className, javaCode, printWriter, fileManager);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is it worth having the compilation result examined from within the compileFromJavaResult() method to reduce complexity in this method?

Also this way, changing of the method signature may not be required as it can still return the Map<String, byte[]> of the compiled classes if it is a success and the className exists in the map of compiled (as its provided to the compileFromJava() method. Then to allow for diagnostics to be used, you can pass in the StringBuilder on the call - this can determine whether the caller actually cares about diagnostics and add the detail if it's asked for

Copy link
Copy Markdown
Contributor

@benbonavia benbonavia left a comment

Choose a reason for hiding this comment

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

Please reduce the complexity as per the sonar analysis

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants