[BUG] Cannot use source with intentional compile errors for ToolsExtension

[toolforger]

Describe the bug
Compiler errors will make ToolsExtension believe that the compiler didn't start, failing the test run.

To Reproduce
Set up a test class with @ExtendWith(ToolsExtension.class) and feed it source code that makes the annotation processor report errors.
The test run will fail, with the somewhat misleading message Failed to start javac (javac actually runs, it merely returns a compilation error)

Expected behavior

DaemonCompiler.run should not throw a CompilationException. (Though it's weird that the test will hang indefinitely if I make the Results object a success to prevent the run() function from throwing that exception. I didn't look deeper at what's happening there.)

I also have been hoping that the Java compiler will return a best-effort AST if it hits compilation errors, and that I can inspect it.
Maybe that hope was misguided though and you can't use ToolsExtension with intentionally-failing Java code.

[Pante]

If I understand correctly you tried to feed the extension source code which contained compilation errors. In that case, I think JavacExtension will be more appropriate. See the javac extension.

Due to how ToolExtension works internally, I don't think it's possible for it to parse uncompilable code.

[toolforger]

It's just the annotation processor that throws errors, javac can parse if no problems.

To the very least, the error message shouldn't say "could not start the java compiler", that's just misleading: The Java compiler started, spat out error messages, terminated normally, and ToolsExtension even constructs a Results object.

[toolforger]

But yeah I see what you mean; expecting javac to return an AST in case of errors, whether from an annotation processor or not, is likely going beyond what the TCK demands.

[Pante]

Could attach a short snippet of code that demonstrates this?

It seems strange that DaemonCompiler includes an annotation processor when running. I think what may be happening is that DaemonCompiler is unintentionally picking up the annotation processor on the classpath.

I can't think of how it might otherwise compile while including the annotation processor under test.

On another note, if an annotation processor does throw an exception, it does mean that compilation has failed since it is assumed that the annotation processor has errored out unintentionally. For expected errors, i.e. annotation processor expected some other type, it should log a failure using the given Messager instead.

[toolforger]

Nah, it's the example project that's erroring out.
The compiler messages also have just the annotation processor's error message.

Example project is at https://github.com/toolforger/elementary-demo .
I put a breakpoint in DaemonCompiler.run(), on the if (!results.success) { line, so it was easy to check the logs and see what actually happened.

[toolforger]

On a tangent, I think I can work around the issue with erroring-out annotation processor - just leave out the annotations, make the tests not check that the annotation processor errors out but just that the conditions hold that will make it error out, in those code snippets that we believe should trigger specific error conditions.
Still, the error message is misleading. I think it's this code that needs a bit of a rewrite:

        try {
            var results = compiler.compile(files);
            if (!results.success) {
                throw new CompilationException(results.find().diagnostics());
            }
            
        } catch (Throwable e) {
            processor.environment.completeExceptionally(new CompilationException("Failed to start javac", e));
        }

I think it should be something like this:

        Results results;
        try {
            var results = compiler.compile(files);
        } catch (Throwable e) {
            processor.environment.completeExceptionally(new CompilationException("javac did not start, or crashed", e));
            return;
        }
        if (!results.success) {
            processor.environment.completeExceptionally(new CompilationException(results.find().diagnostics()));
        }

This also avoids wrapping a CompilationException in another CompilationException, so the exception stack trace will have a better noise-to-signal ratio.
I wanted to insert a "javac completed with error messages:" line before the results.find().diagnostics() messages, but it would have complicated the suggested code so I left it out for better clarity.

[Pante]

I think I get what you mean, but can't the if-statement be placed in the try-block? Also, any chance you would be willing to submit a PR for this on master heh?

EDIT: Okay I think I derped up here, the throw should definitely be outside.

[toolforger]

I have found over the years that nesting tends to couple code blocks, which can hinder refactoring. It also creates a bias towards keeping the code path joined, even if it shouldn't be.
The exact decision is pretty subjective though, so I don't care much about the exact flow.

The subjectivity is also why I didn't create a PR: I don't know what your priorities are, so any PR would probably be rejected and require more work from both of us.

[Pante]

Fair enough, I'll create a PR later then, thanks for reporting this issue regardless!

[Pante]

Fixed in b320fd0