[PATCH] JDK-8235457: Crash when reporting a message about an annotation on a module
Jonathan Gibbons
jonathan.gibbons at oracle.com
Mon Dec 9 23:15:11 UTC 2019
Hi Jeremy,
I'll go through this.
My inclination to combine all three into a single changeset to push into
the repos. The fixes themselves are simple; the changeset is dominated
by the test.
-- Jon
On 12/9/19 3:12 PM, Jeremy Kuhn wrote:
> 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
> <mailto: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 <mailto: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/20191209/d8f1d42a/attachment.htm>
More information about the compiler-dev
mailing list