[BUG] Verbose assertThatCompilation failure output when compilation error affects complete class

[Marcono1234]

Version
3.2.3

Describe the bug
When you use assertThatCompilation(...).isSuccessfulWithoutWarnings() the output for assertion failures will be pretty verbose when a compilation error affects the complete class, for example when you forgot to implement a method.
In that case every line of the file will be highlighted.

To Reproduce

var compiler = JctCompilers.newPlatformCompiler();
try (var workspace = Workspaces.newWorkspace(PathStrategy.RAM_DIRECTORIES)) {
    workspace.createSourcePathPackage()
        .createFile("Main.java").withContents("""
        public class Main implements java.util.Iterator<String> {
            public void doSomething() {
                /* some unrelated code */
            }
        }
        """);

    var compilation = compiler.compile(workspace);
    assertThatCompilation(compilation)
        .isSuccessfulWithoutWarnings();
}

If you run this, you will see that it highlighted every line of the code.

Expected behaviour
Ideally only the first line, or the class name should be highlighted.

Test case or reproduction
see above

Logs
none

Additional context

JDK version: JDK 22.0.2+9
JDK vendor: Eclipse Adoptium
Platform: Windows 10

[ascopes]

My guess here is that the compiler is reporting it covering all of these lines. I can take a look when I have a few minutes and see if I can come up with a better solution (maybe truncating the example if it gets too long?)

How does that sound?

If not, an edge case could be added for the error code, although ideally I'd like to avoid making these classes compiler specific if at all possible.

[Marcono1234]

maybe truncating the example if it gets too long?

Not sure how javac does it internally, but it reports this error only for the class declaration:

.\JavacError.java:1: error: JavacError is not abstract and does not override abstract method next() in Iterator
public class JavacError implements java.util.Iterator<String> {
       ^
1 error

Another case is having multiple methods with the same signature: JCT reports the complete method with body, whereas javac only reports the declaration:

.\JavacError.java:6: error: method doSomething() is already defined in class JavacError
public void doSomething() {
            ^
1 error

---

an edge case could be added for the error code, although ideally I'd like to avoid making these classes compiler specific if at all possible

Yes, I also think that this should be avoided if possible. There are probably multiple errors and warnings which cause this.

[ascopes]

Shall see if I can sort this out at some point this week when I have time then. Thanks!

[ascopes]

Just thinking about this...

The code to handle this is in TraceDiagnosticRepresentation.

var startLine = Math.max(1, (int) diagnostic.getLineNumber() - ADDITIONAL_CONTEXT_LINES);
    var lineStartOffset = StringUtils.indexOfLine(content, startLine);
    var endOffset = (int) diagnostic.getEndPosition();

Wondering if it is worth removing the support for underlining multiple lines in the representation. It would significantly simplify the implementation. My main concern is that for other types of diagnostic, showing the full snippet may be useful.

Another option I guess might be to allow setting the type of representation on the library globally or per assertion somehow...

static {
  JctAssertions.setDiagnosticStyle(JAVAC);
}

...then switch on the representation class we choose to use..

This implementation was vaguely inspired by Rustc-style diagnostics. I can have a look through the code for OpenJDK to see how they handle this.

Might not be until next weekend though as I am busy today (unless I find time tonight) and I am then working all week. I will see what I can do though.

[ascopes]

Link to what appears to be the OpenJDK implementation: https://github.com/openjdk/jdk/blob/master/src/jdk.compiler/share/classes/com/sun/tools/javac/util/DiagnosticSource.java

It seems to only ever handle the first line.

[Marcono1234]

Another option I guess might be to allow setting the type of representation on the library globally or per assertion somehow...

static {
  JctAssertions.setDiagnosticStyle(JAVAC);
}

Just in case you want to make this (or something else, such as the number of context lines) configurable, maybe an additional way to support this would be through JUnit Configuration Parameters, which can possibly be accessed using ExtensionContext#getConfigurationParameter. But I haven't tested this yet, so I might be wrong.

[ascopes]

Yeah, I had considered that, although I've tried to keep the assertions uncoupled from the implementation.

For now, I'm hoping to just push a strategic fix for the issue until I have more time to address it properly (happy to take PRs as well!)

[Marcono1234]

For now, I'm hoping to just push a strategic fix for the issue until I have more time to address it properly (happy to take PRs as well!)

That is totally fine for me as well. As mentioned on the other issue no problem in case you don't have that much time to work on this at the moment.

I will probably not submit a PR since I am not really familiar with the internals of this project, and I think the solution you are working on will already solve this issue well enough.

[ascopes]

Sure, no problem. For future reference if you are interested, the mechanism that handles this is the TraceDiagnosticRepresentation

The WIP I have up limits the number of lines output by this representation to a maximum of 5 lines. This enables still retaining the detailed output of generated snippets but without it totally filling the logs up with nonsense in the case you describe. I need to work out why the tests are failing in the adjusted cases though still, I believe I have made a logic error somewhere in that MR which is why it is still a draft.

I will try to find some time to finish this once I am back from my holiday and get this released for you.

[ascopes]

I've decided for now to go with implementing a simpler output format, which looks like this and is closer to what javac outputs anyway:

 - [ERROR] compiler.err.illegal.start.of.expr in com/example/greeter/Greeter.java (line 29, col 3)
   
   illegal start of expression

Will get that merged and released as an RC today so you can test.