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

Mon Dec 9 23:12:16 UTC 2019

Hi Jon,

Following my previous mail, please find enclosed updated patches:
- JDK-8235457.patch provides fix for 8235457 bug
- JDK-8235458.pathc provides fix for 8235458 bug
- JDK-8235457-8235458.patch provides a test to validate both fixes: a
processor reporting a message on an imported module annotation.

JCheck doesn't complain, code is more consistent and legal header has been
added.

Please let me know if you need anything else.

Jeremy

On Sat, Dec 7, 2019 at 7:47 AM Jeremy Kuhn <jeremy.kuhn.java at gmail.com>
wrote:

> Hi Jon,
>
> I take good note of your comments and get back to you with updated patches
> asap.
>
> As for the other bug 8235458, you're actually right to say that both tests
> are quite similar: annotation module is the same, processor module is
> almost the same except that the processor doesn't report a message on the
> annotation to avoid the first issue and the mod module uses import.
>
> I did that to clearly separate the two tests, there's different approaches
> to testing, either we test one particular issue at a time (following a bug)
> or we test a wider functionality and potentially other things at the same
> time. Since I saw many directories named after a bug number I assumed the
> first approach was expected but clearly I would prefer to test
> functionalities (and reduce code)
>
> Following your comment on names I think I'll create one folder for both
> tests in test/langtools/tools/javac/modules or
> test/langtools/tools/javac/annotations defining annotation and processor
> modules once. Now I can either test both issues altogether with one
> annotated module with import statements or create separate tests to test
> the import issue separately, I think that one test here would be ok because
> if the first test fails the other would inevitably fail as well (since the
> processor report message on annotation), there's no need to report twice
> the same error and in case of error the issue would be quite clear: either
> assertion error or the annotation processor doesn't process the annotation,
> in any case you'll know what to do.
>
> So I propose to create three patches: one for each bug fix and one for the
> test if that's ok for you.
>
> Regarding annotation processor module path, what can I say I like modules
> :-P
>
> Jeremy
>
> On Sat, Dec 7, 2019 at 1:39 AM Jonathan Gibbons <
> jonathan.gibbons at oracle.com> wrote:
>
>>
>>
>> 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
>>
>>  * Copyright (c) 2019, Oracle and/or its affiliates. All rights reserved.
>>
>> 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/20191210/d8429040/attachment-0001.htm>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: JDK-8235457.patch
Type: text/x-patch
Size: 734 bytes
Desc: not available
URL: <https://mail.openjdk.java.net/pipermail/compiler-dev/attachments/20191210/d8429040/JDK-8235457-0001.patch>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: JDK-8235457-8235458-test.patch
Type: text/x-patch
Size: 14002 bytes
Desc: not available
URL: <https://mail.openjdk.java.net/pipermail/compiler-dev/attachments/20191210/d8429040/JDK-8235457-8235458-test-0001.patch>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: JDK-8235458.patch
Type: text/x-patch
Size: 1454 bytes
Desc: not available
URL: <https://mail.openjdk.java.net/pipermail/compiler-dev/attachments/20191210/d8429040/JDK-8235458-0001.patch>