Skip to content

Report compilation errors in exception #142

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

Open
wants to merge 2 commits into
base: ea
Choose a base branch
from
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
178 changes: 129 additions & 49 deletions src/main/java/net/openhft/compiler/CachedCompiler.java
Original file line number Diff line number Diff line change
@@ -35,12 +35,14 @@
import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.ConcurrentMap;
import java.util.function.Function;
import java.util.stream.Collectors;

import static net.openhft.compiler.CompilerUtils.*;

@SuppressWarnings("StaticNonFinalField")
public class CachedCompiler implements Closeable {
private static final Logger LOG = LoggerFactory.getLogger(CachedCompiler.class);
/** Writes to {@link System#err} */
private static final PrintWriter DEFAULT_WRITER = new PrintWriter(System.err);
private static final List<String> DEFAULT_OPTIONS = Arrays.asList("-g", "-nowarn");

@@ -57,16 +59,28 @@ public class CachedCompiler implements Closeable {

private final ConcurrentMap<String, JavaFileObject> javaFileObjects = new ConcurrentHashMap<>();

/**
* Delegates to {@link #CachedCompiler(File, File, List)} with default {@code javac} compilation
* options {@code -g} (generate debug information) and {@code -nowarn}.
*/
public CachedCompiler(@Nullable File sourceDir, @Nullable File classDir) {
this(sourceDir, classDir, DEFAULT_OPTIONS);
}

/**
* @param sourceDir where to write {@code .java} source code files to be compiled; {@code null}
* to not write them to the file system
* @param classDir where to write compiled {@code .class} files; {@code null} to not write them
* to the file system
* @param options {@code javac} compilation options
*/
public CachedCompiler(@Nullable File sourceDir, @Nullable File classDir, @NotNull List<String> options) {
this.sourceDir = sourceDir;
this.classDir = classDir;
this.options = options;
}

@Override
public void close() {
try {
for (MyJavaFileManager fileManager : fileManagerMap.values()) {
@@ -77,67 +91,58 @@ public void close() {
}
}

/**
* Delegates to {@link #loadFromJava(ClassLoader, String, String, PrintWriter, DiagnosticListener)}.
* <ul>
* <li>The class loader of {@link CachedCompiler} is used for defining and loading the class
* <li>Only error diagnostics are collected, and are written to {@link System#err}
* </ul>
*/
public Class<?> loadFromJava(@NotNull String className, @NotNull String javaCode) throws ClassNotFoundException {
return loadFromJava(getClass().getClassLoader(), className, javaCode, DEFAULT_WRITER);
}

/**
* Delegates to {@link #loadFromJava(ClassLoader, String, String, PrintWriter, DiagnosticListener)}.
* Only error diagnostics are collected, and are written to {@link System#err}.
*/
public Class<?> loadFromJava(@NotNull ClassLoader classLoader,
@NotNull String className,
@NotNull String javaCode) throws ClassNotFoundException {
return loadFromJava(classLoader, className, javaCode, DEFAULT_WRITER);
}

@NotNull
Map<String, byte[]> compileFromJava(@NotNull String className, @NotNull String javaCode, MyJavaFileManager fileManager) {
return compileFromJava(className, javaCode, DEFAULT_WRITER, fileManager);
}

@NotNull
Map<String, byte[]> compileFromJava(@NotNull String className,
@NotNull String javaCode,
final @NotNull PrintWriter writer,
MyJavaFileManager fileManager) {
Iterable<? extends JavaFileObject> compilationUnits;
if (sourceDir != null) {
String filename = className.replaceAll("\\.", '\\' + File.separator) + ".java";
File file = new File(sourceDir, filename);
writeText(file, javaCode);
if (s_standardJavaFileManager == null)
s_standardJavaFileManager = s_compiler.getStandardFileManager(null, null, null);
compilationUnits = s_standardJavaFileManager.getJavaFileObjects(file);

} else {
javaFileObjects.put(className, new JavaSourceFromString(className, javaCode));
compilationUnits = new ArrayList<>(javaFileObjects.values()); // To prevent CME from compiler code
}
// reuse the same file manager to allow caching of jar files
boolean ok = s_compiler.getTask(writer, fileManager, new DiagnosticListener<JavaFileObject>() {
@Override
public void report(Diagnostic<? extends JavaFileObject> diagnostic) {
if (diagnostic.getKind() == Diagnostic.Kind.ERROR) {
writer.println(diagnostic);
}
}
}, options, null, compilationUnits).call();

if (!ok) {
// compilation error, so we want to exclude this file from future compilation passes
if (sourceDir == null)
javaFileObjects.remove(className);

// nothing to return due to compiler error
return Collections.emptyMap();
} else {
Map<String, byte[]> result = fileManager.getAllBuffers();

return result;
}
/**
* Delegates to {@link #loadFromJava(ClassLoader, String, String, PrintWriter, DiagnosticListener)}.
* Only error diagnostics are collected, and are written to {@code writer}.
*/
public Class<?> loadFromJava(@NotNull ClassLoader classLoader,
@NotNull String className,
@NotNull String javaCode,
@Nullable PrintWriter writer) throws ClassNotFoundException {
return loadFromJava(classLoader, className, javaCode, writer, null);
}

/**
* Gets a previously compiled and loaded class, or compiles the given Java code and
* loads the class.
*
* @param classLoader class loader for defining and loading the class
* @param className binary name of the class to load, for example {@code com.example.MyClass$Nested}
* @param javaCode Java code to compile, in case the class had not been compiled and loaded before
* @param writer writer for compilation information and diagnostics (should be thread-safe);
* when {@code null} defaults to writing to {@link System#err}
* @param diagnosticListener listener for diagnostics emitted by the compiler (should be thread-safe);
* when {@code null}, error diagnostics are written to the {@code writer}, other diagnostics are ignored
* @return the loaded class
* @throws ClassNotFoundException if compiling or loading the class failed; inspect {@code writer} or
* {@code diagnosticListener} for additional details
*/
public Class<?> loadFromJava(@NotNull ClassLoader classLoader,
@NotNull String className,
@NotNull String javaCode,
@Nullable PrintWriter writer) throws ClassNotFoundException {
@Nullable PrintWriter writer,
@Nullable DiagnosticListener<? super JavaFileObject> diagnosticListener) throws ClassNotFoundException {
Class<?> clazz = null;
Map<String, Class<?>> loadedClasses;
synchronized (loadedClassesMap) {
@@ -147,17 +152,43 @@ public Class<?> loadFromJava(@NotNull ClassLoader classLoader,
else
clazz = loadedClasses.get(className);
}
PrintWriter printWriter = (writer == null ? DEFAULT_WRITER : writer);

if (clazz != null)
return clazz;

final PrintWriter writerFinal = writer == null ? DEFAULT_WRITER : writer;
final DiagnosticListener<? super JavaFileObject> diagnosticListenerFinal;
if (diagnosticListener == null) {
diagnosticListenerFinal = new DiagnosticListener<JavaFileObject>() {
@Override
public void report(Diagnostic<? extends JavaFileObject> diagnostic) {
if (diagnostic.getKind() == Diagnostic.Kind.ERROR) {
writerFinal.println(diagnostic);
}
}
};
} else {
diagnosticListenerFinal = diagnosticListener;
}

List<Diagnostic<?>> errorDiagnostics = Collections.synchronizedList(new ArrayList<>());
DiagnosticListener<JavaFileObject> wrappingDiagnosticListener = new DiagnosticListener<JavaFileObject>() {
@Override
public void report(Diagnostic<? extends JavaFileObject> diagnostic) {
if (diagnostic.getKind() == Diagnostic.Kind.ERROR) {
errorDiagnostics.add(diagnostic);
}
diagnosticListenerFinal.report(diagnostic);
}
};

MyJavaFileManager fileManager = fileManagerMap.get(classLoader);
if (fileManager == null) {
StandardJavaFileManager standardJavaFileManager = s_compiler.getStandardFileManager(null, null, null);
fileManager = getFileManager(standardJavaFileManager);
fileManagerMap.put(classLoader, fileManager);
}
final Map<String, byte[]> compiled = compileFromJava(className, javaCode, printWriter, fileManager);
final Map<String, byte[]> compiled = compileFromJava(className, javaCode, writerFinal, wrappingDiagnosticListener, fileManager);
for (Map.Entry<String, byte[]> entry : compiled.entrySet()) {
String className2 = entry.getKey();
synchronized (loadedClassesMap) {
@@ -186,11 +217,60 @@ public Class<?> loadFromJava(@NotNull ClassLoader classLoader,
}
}
synchronized (loadedClassesMap) {
loadedClasses.put(className, clazz = classLoader.loadClass(className));
try {
clazz = classLoader.loadClass(className);
} catch (ClassNotFoundException e) {
if (errorDiagnostics.isEmpty()) {
throw e;
}

// Enhance exception message with compilation errors, otherwise it will might not
// be obvious that reason why loading failed is because compilation failed
String message = "Failed to load class " + className + "\nCompilation errors:\n";
message += errorDiagnostics.stream().map(Diagnostic::toString).collect(Collectors.joining("\n"));
throw new ClassNotFoundException(message, e);
}
loadedClasses.put(className, clazz);
}
return clazz;
}

@NotNull
Map<String, byte[]> compileFromJava(@NotNull String className,
@NotNull String javaCode,
@NotNull PrintWriter writer,
@NotNull DiagnosticListener<? super JavaFileObject> diagnosticListener,
MyJavaFileManager fileManager) {
Iterable<? extends JavaFileObject> compilationUnits;
if (sourceDir != null) {
String filename = className.replaceAll("\\.", '\\' + File.separator) + ".java";
File file = new File(sourceDir, filename);
writeText(file, javaCode);
if (s_standardJavaFileManager == null)
s_standardJavaFileManager = s_compiler.getStandardFileManager(null, null, null);
compilationUnits = s_standardJavaFileManager.getJavaFileObjects(file);

} else {
javaFileObjects.put(className, new JavaSourceFromString(className, javaCode));
compilationUnits = new ArrayList<>(javaFileObjects.values()); // To prevent CME from compiler code
}
// reuse the same file manager to allow caching of jar files
boolean ok = s_compiler.getTask(writer, fileManager, diagnosticListener, options, null, compilationUnits).call();

if (!ok) {
// compilation error, so we want to exclude this file from future compilation passes
if (sourceDir == null)
javaFileObjects.remove(className);

// nothing to return due to compiler error
return Collections.emptyMap();
} else {
Map<String, byte[]> result = fileManager.getAllBuffers();

return result;
}
}

private @NotNull MyJavaFileManager getFileManager(StandardJavaFileManager fm) {
return fileManagerOverride != null
? fileManagerOverride.apply(fm)
24 changes: 10 additions & 14 deletions src/main/java/net/openhft/compiler/CompilerUtils.java
Original file line number Diff line number Diff line change
@@ -33,7 +33,7 @@
import java.lang.reflect.Field;
import java.lang.reflect.InvocationTargetException;
import java.lang.reflect.Method;
import java.nio.charset.Charset;
import java.nio.charset.StandardCharsets;
import java.util.Arrays;

/**
@@ -42,11 +42,15 @@
public enum CompilerUtils {
; // none
public static final boolean DEBUGGING = isDebug();
/**
* Singleton {@link CachedCompiler}. Uses default {@code javac} options of
* {@link CachedCompiler#CachedCompiler(File, File)}, and does not write {@code .java}
* source files and {@code .class} files to the file system.
*/
public static final CachedCompiler CACHED_COMPILER = new CachedCompiler(null, null);

private static final Logger LOGGER = LoggerFactory.getLogger(CompilerUtils.class);
private static final Method DEFINE_CLASS_METHOD;
private static final Charset UTF_8 = Charset.forName("UTF-8");
private static final String JAVA_CLASS_PATH = "java.class.path";
static JavaCompiler s_compiler;
static StandardJavaFileManager s_standardJavaFileManager;
@@ -160,7 +164,7 @@ public static void defineClass(@NotNull String className, @NotNull byte[] bytes)
*/
public static Class<?> defineClass(@Nullable ClassLoader classLoader, @NotNull String className, @NotNull byte[] bytes) {
try {
return (Class) DEFINE_CLASS_METHOD.invoke(classLoader, className, bytes, 0, bytes.length);
return (Class<?>) DEFINE_CLASS_METHOD.invoke(classLoader, className, bytes, 0, bytes.length);
} catch (IllegalAccessException e) {
throw new AssertionError(e);
} catch (InvocationTargetException e) {
@@ -173,7 +177,7 @@ private static String readText(@NotNull String resourceName) throws IOException
if (resourceName.startsWith("="))
return resourceName.substring(1);
StringWriter sw = new StringWriter();
Reader isr = new InputStreamReader(getInputStream(resourceName), UTF_8);
Reader isr = new InputStreamReader(getInputStream(resourceName), StandardCharsets.UTF_8);
try {
char[] chars = new char[8 * 1024];
int len;
@@ -187,11 +191,7 @@ private static String readText(@NotNull String resourceName) throws IOException

@NotNull
private static String decodeUTF8(@NotNull byte[] bytes) {
try {
return new String(bytes, UTF_8.name());
} catch (UnsupportedEncodingException e) {
throw new AssertionError(e);
}
return new String(bytes, StandardCharsets.UTF_8);
}

@Nullable
@@ -230,11 +230,7 @@ public static boolean writeText(@NotNull File file, @NotNull String text) {

@NotNull
private static byte[] encodeUTF8(@NotNull String text) {
try {
return text.getBytes(UTF_8.name());
} catch (UnsupportedEncodingException e) {
throw new AssertionError(e);
}
return text.getBytes(StandardCharsets.UTF_8);
}

public static boolean writeBytes(@NotNull File file, @NotNull byte[] bytes) {
194 changes: 169 additions & 25 deletions src/test/java/net/openhft/compiler/CompilerTest.java
Original file line number Diff line number Diff line change
@@ -25,13 +25,18 @@

import java.io.*;
import java.lang.reflect.Constructor;
import java.lang.reflect.InvocationTargetException;
import java.net.URL;
import java.net.URLClassLoader;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collections;
import java.util.Date;
import java.util.List;
import java.util.concurrent.Callable;
import java.util.concurrent.atomic.AtomicBoolean;

import javax.tools.Diagnostic;

public class CompilerTest extends TestCase {
static final File parent;
private static final String EG_FOO_BAR_TEE = "eg.FooBarTee";
@@ -47,8 +52,30 @@ public class CompilerTest extends TestCase {
}
}

private CachedCompiler compiler;
private URLClassLoader classLoader;

@Override
protected void setUp() throws Exception {
// Create new compiler and class loader to prevent tests from affecting each other
compiler = new CachedCompiler(null, null);
classLoader = new URLClassLoader(new URL[0]);
}

@Override
protected void tearDown() throws Exception {
compiler.close();
classLoader.close();
}

public static void main(String[] args) throws Throwable {
new CompilerTest().test_compiler();
CompilerTest compilerTest = new CompilerTest();
try {
compilerTest.setUp();
compilerTest.test_compiler();
} finally {
compilerTest.tearDown();
}
}

public void test_compiler() throws Throwable {
@@ -62,7 +89,7 @@ public void test_compiler() throws Throwable {
// this writes the file to disk only when debugging is enabled.
CachedCompiler cc = CompilerUtils.DEBUGGING ?
new CachedCompiler(new File(parent, "target/generated-test-sources"), new File(parent, "target/test-classes")) :
CompilerUtils.CACHED_COMPILER;
compiler;

String text = "generated test " + new Date();
try {
@@ -109,9 +136,7 @@ public void test_compiler() throws Throwable {
}
}

public void test_fromFile()
throws ClassNotFoundException, IOException, IllegalAccessException, InstantiationException,
NoSuchMethodException, InvocationTargetException, NoSuchFieldException {
public void test_fromFile() throws Exception {
Class<?> clazz = CompilerUtils.loadFromResource("eg.FooBarTee2", "eg/FooBarTee2.jcf");
// turn off System.out
PrintStream out = System.out;
@@ -121,7 +146,7 @@ public void test_fromFile()
public void write(int b) throws IOException {
}
}));
final Constructor stringConstructor = clazz.getConstructor(String.class);
final Constructor<?> stringConstructor = clazz.getConstructor(String.class);
long start = 0;
for (int i = -RUNS / 10; i < RUNS; i++) {
if (i == 0) start = System.nanoTime();
@@ -160,12 +185,14 @@ public void write(int b) throws IOException {
}
}));

CompilerUtils.CACHED_COMPILER.loadFromJava(
getClass().getClassLoader(), "TestClass", "clazz TestClass {}",
compiler.loadFromJava(
classLoader, "TestClass", "clazz TestClass {}",
new PrintWriter(writer));
fail("Should have failed to compile");
} catch (ClassNotFoundException e) {
// expected
// Should have been enhanced with additional details
assertTrue(e.getMessage().contains("Compilation errors"));
assertTrue(e.getCause() instanceof ClassNotFoundException);
} finally {
System.setOut(out);
System.setErr(err);
@@ -205,8 +232,8 @@ public void write(int b) throws IOException {
}
}));

CompilerUtils.CACHED_COMPILER.loadFromJava(
getClass().getClassLoader(), "TestClass", "class TestClass {}",
compiler.loadFromJava(
classLoader, "TestClass", "class TestClass {}",
new PrintWriter(writer));
} finally {
System.setOut(out);
@@ -226,7 +253,10 @@ public void test_settingPrintStreamWithWarnings() throws Exception {
final PrintStream err = System.err;
final StringWriter writer = new StringWriter();

try {
// Enable lint; otherwise compiler produces no Warning diagnostics but only Note, saying
// that `-Xlint:deprecation` should be used
final List<String> options = Arrays.asList("-Xlint:deprecation");
try (CachedCompiler compiler = new CachedCompiler(null, null, options)) {
System.setOut(new PrintStream(new OutputStream() {
@Override
public void write(int b) throws IOException {
@@ -240,10 +270,10 @@ public void write(int b) throws IOException {
}
}));

CompilerUtils.CACHED_COMPILER.loadFromJava(
getClass().getClassLoader(), "TestClass",
// definition with a mandatory warning
"class TestClass { int i = new Date().getDay(); }",
compiler.loadFromJava(
classLoader, "TestClass",
// definition with a mandatory warning for deprecated `Date.getDay()`
"import java.util.Date; class TestClass { int i = new Date().getDay(); }",
new PrintWriter(writer));
} finally {
System.setOut(out);
@@ -255,27 +285,128 @@ public void write(int b) throws IOException {
assertEquals("", writer.toString());
}

public void test_settingDiagnosticListenerWithCompilerErrors() throws Exception {
final AtomicBoolean usedSysOut = new AtomicBoolean(false);
final AtomicBoolean usedSysErr = new AtomicBoolean(false);

final PrintStream out = System.out;
final PrintStream err = System.err;
final StringWriter writer = new StringWriter();
final List<Diagnostic<?>> diagnostics = Collections.synchronizedList(new ArrayList<>());

try {
System.setOut(new PrintStream(new OutputStream() {
@Override
public void write(int b) throws IOException {
usedSysOut.set(true);
}
}));
System.setErr(new PrintStream(new OutputStream() {
@Override
public void write(int b) throws IOException {
usedSysErr.set(true);
}
}));

compiler.loadFromJava(
classLoader,
"TestClass",
"clazz TestClass {}",
new PrintWriter(writer),
diagnostics::add);
fail("Should have failed to compile");
} catch (ClassNotFoundException e) {
// Should have been enhanced with additional details
assertTrue(e.getMessage().contains("Compilation errors"));
assertTrue(e.getCause() instanceof ClassNotFoundException);
} finally {
System.setOut(out);
System.setErr(err);
}

assertFalse(usedSysOut.get());
assertFalse(usedSysErr.get());
// Diagnostics should have only been reported to listener; not written to output
assertEquals("", writer.toString());

assertEquals(1, diagnostics.size());
Diagnostic<?> diagnostic = diagnostics.get(0);
assertEquals(Diagnostic.Kind.ERROR, diagnostic.getKind());
assertEquals(1, diagnostic.getLineNumber());
}

public void test_settingDiagnosticListenerWithWarnings() throws Exception {
final AtomicBoolean usedSysOut = new AtomicBoolean(false);
final AtomicBoolean usedSysErr = new AtomicBoolean(false);

final PrintStream out = System.out;
final PrintStream err = System.err;
final StringWriter writer = new StringWriter();
final List<Diagnostic<?>> diagnostics = Collections.synchronizedList(new ArrayList<>());

// Enable lint; otherwise compiler only produces no Warning diagnostics but only Note, saying
// that `-Xlint:unchecked` should be used
final List<String> options = Arrays.asList("-Xlint:unchecked");
try (CachedCompiler compiler = new CachedCompiler(null, null, options)) {
System.setOut(new PrintStream(new OutputStream() {
@Override
public void write(int b) throws IOException {
usedSysOut.set(true);
}
}));
System.setErr(new PrintStream(new OutputStream() {
@Override
public void write(int b) throws IOException {
usedSysErr.set(true);
}
}));

compiler.loadFromJava(
classLoader,
"TestClass",
// definition with a mandatory warning for unchecked cast
"import java.util.*; class TestClass { public List<Integer> unsafe(List<?> l) { return (List<Integer>) l; } }",
new PrintWriter(writer),
diagnostics::add);
} finally {
System.setOut(out);
System.setErr(err);
}

assertFalse(usedSysOut.get());
assertFalse(usedSysErr.get());
// Diagnostics should have only been reported to listener; not written to output
assertEquals("", writer.toString());

assertEquals(1, diagnostics.size());
Diagnostic<?> diagnostic = diagnostics.get(0);
assertEquals(Diagnostic.Kind.MANDATORY_WARNING, diagnostic.getKind());
assertEquals(1, diagnostic.getLineNumber());
}

public void test_compilerErrorsDoNotBreakNextCompilations() throws Exception {
// quieten the compiler output
PrintWriter quietWriter = new PrintWriter(new StringWriter());

// cause a compiler error
try {
CompilerUtils.CACHED_COMPILER.loadFromJava(
getClass().getClassLoader(), "X", "clazz X {}", quietWriter);
compiler.loadFromJava(
classLoader, "X", "clazz X {}", quietWriter);
fail("Should have failed to compile");
} catch (ClassNotFoundException e) {
// expected
// Should have been enhanced with additional details
assertTrue(e.getMessage().contains("Compilation errors"));
assertTrue(e.getCause() instanceof ClassNotFoundException);
}

// ensure next class can be compiled and used
Class<?> testClass = CompilerUtils.CACHED_COMPILER.loadFromJava(
getClass().getClassLoader(), "S", "class S {" +
Class<?> testClass = compiler.loadFromJava(
classLoader, "S", "class S {" +
"public static final String s = \"ok\";}");

Callable callable = (Callable)
CompilerUtils.CACHED_COMPILER.loadFromJava(
getClass().getClassLoader(), "OtherClass",
Callable<?> callable = (Callable<?>)
compiler.loadFromJava(
classLoader, "OtherClass",
"import java.util.concurrent.Callable; " +
"public class OtherClass implements Callable<String> {" +
"public String call() { return S.s; }}")
@@ -286,6 +417,19 @@ public void test_compilerErrorsDoNotBreakNextCompilations() throws Exception {
assertEquals("ok", callable.call());
}

public void test_compilerErrorsButLoadingDifferentClass() throws Exception {
// quieten the compiler output
PrintWriter quietWriter = new PrintWriter(new StringWriter());

// TODO: Should this throw an exception due to the compilation error nonetheless to be less
// error-prone for users, even if loading class would succeed?
Class<?> testClass = compiler.loadFromJava(classLoader,
// Load other class which is unaffected by compilation error
String.class.getName(),
"clazz X {}", quietWriter);
assertSame(String.class, testClass);
}
Comment on lines +420 to +431
Copy link
Contributor Author

@Marcono1234 Marcono1234 Jul 21, 2024

Choose a reason for hiding this comment

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

Open question: This mirrors the existing behavior, but personally I think it would be cleaner to always have CachedCompiler throw an exception (ClassNotFoundException?) when compilation errors occur. Even if loading the class would succeed in the end, because that class was unaffected by the compilation errors.


@Test
public void testNewCompiler() throws Exception {
for (int i = 1; i <= 3; i++) {