[PATCH] JDK-8235457: Crash when reporting a message about an annotation on a module

Sat Dec 7 00:36:28 UTC 2019

On 12/06/2019 03:36 PM, Jeremy Kuhn wrote:
> Hi,
>
> Following previous discussions please find enclosed a patch for the 
> issue referenced in JDK-8235457.
>
> The com.sun.tools.javac.model.JavacElements#matchAnnoToTree() method 
> (line 275) is invoked when reporting a message on an annotation on an 
> element to return the tree for an annotation given the annotated 
> element, this allows the compiler to output the correct line where the 
> message should be displayed, however in the "Vis" class within the 
> method the JCModuleDecl has probably been forgotten when modules were 
> introduced in JDK-9 leading to an AssertionError crashing the compiler 
> when an annotation processor tries to report a message on a module's 
> annotation.
>
> In order to fix the issue, I've simply implemented the missing method 
> visitModuleDef() in the Vis class so that modules' annotations are 
> properly returned.
>
> You'll find a jtreg test in the patch as well, I've actually created 
> three modules for the test:
> - annotation: which defines a simple module annotation
> - processor: which provides the annotation processor reporting a 
> message on the module's annotation
> - mod: which contains a module annotated with the previous annotation
>
> The test is successful when the test run without error and the message 
> is properly displayed by the compiler.
>
> I didn't find a way to use jtreg tags to compile modules, so I've 
> decided to compile everything programmatically in the test main() method.
>
> Please let me know if I can be of any further assistance.

Hi Jeremy,

Thanks for providing the patch for the first of your issues. I've 
uploaded the patch as a webrev:
http://cr.openjdk.java.net/~jjg/8235457/webrev/index.html

Some comments ...

Some of the files contain tab characters. Code in OpenJDK should always 
use spaces instead of tabs.
This and other white-space rules (no trailing spaces;  source files end 
with newline, etc) are enforced
by a Mercurial extension called jcheck, available here:
http://openjdk.java.net/projects/code-tools/jcheck/

Although I acknowledge you'll see a lot of instances in 
test/langtools/tools/javac, we now regard the
use of a directory named after a bug number as something of an 
anti-pattern. The same for source
files and other related files. The preferred style these days is to use 
a "human-understandable" name,
perhaps as a subdirectory in a directory of functionally related tests.

Source files, including test source files, should generally have the 
"standard" legal header.
For test files, the standard is to use GPL2 without the ClassPath 
Exception. In most cases, you
can copy the header from a nearby file and just update the date to be 
the current year,  as in

I note you used /nodynamiccopyright/. This is just for use in files that 
will be compiled, and the
compilation generates output (containing line-numbers) that will be 
compared against a "golden-file".
The purpose of the comment is to mark the file to indicate it should not 
have a comment, on the
grounds that if it had a comment and at some point in the future the 
header were to be changed,
the test might break (i.e. if the number of lines in the header were 
changed, affecting line numbers in
the source file.

Because source files to be compiled in javac tests are often small, and 
because they are often
dominated in size by the legal header, it is sometimes more convenient 
(but not required) to embed
these files in strings in the main test program.

Be careful when comparing javac output against golden files. Your test 
will almost certainly fail on
Windows, because the output written by java will contain Windows-style 
line-terminators (\r\n), but
the golden file will contain Unix-style line terminators (\n). The 
recommend solutions are to either
convert one of the strings to be compared to be compatible with the 
other, or to compare them
as a "list of lines".

In addition, javac provides a hidden option -XDrawDiagnostics for use in 
cases like this, which causes
it to use just the simple name of the file, and to use resource keys 
instead of localized strings.
This helps avoid what you had to do in lines 47,48 of T8235457.java.

You may have intended an additional line break in the middle of line 34.

The code would be more concise if you just accessed the system 
properties for test.src and test.classes
once and stored the results in variables to use later. Even better is to 
store them in type-safe variables
of type File or Path instead of plain old String. Stylistically, it is 
nicer to use either File or Path, but not
a mixture unless absolutely necessary. In general, Path (as the newer 
API) is the better overall choice.

Also regarding consistency, you use a mixture of Arrays.asList and 
List.of. That's not wrong, but
consistent use of List.of would be better.

Cool for using  StandardLocation.ANNOTATION_PROCESSOR_MODULE_PATH. I 
don't think your anno
processor needed to be a module, but it is nice to see that you chose to 
make it one.
(You could have just put the processor class on the classpath and used 
the -processor option to
specify it by name ;-) )

-- Jon

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/compiler-dev/attachments/20191206/7571691b/attachment.html>