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

Tue Dec 10 09:10:52 UTC 2019

Hi,

I'm Ok with the changes you suggest, it makes sense. I googled a bit to
find the regexp solution but using line.separator property is cleaner
indeed.

Thanks!

On Tue, Dec 10, 2019 at 3:30 AM Jonathan Gibbons <
jonathan.gibbons at oracle.com> wrote:

> Jeremy,
>
> Thank you for your updated patches.
>
> I've taken your patches and posted them in a single webrev, with the
> intent to eventually push them as a single changeset.
>
> http://cr.openjdk.java.net/~jjg/8235457-8235458/webrev.00/
>
> While it is not common to post a single changeset for multiple issues, I
> think it is warranted in this case because the bug fixes are small and the
> one test covers both fixes. (And, I don't think it is worth splitting the
> test in two, one for each fix.)  There's a somewhat stronger rule that says
> that fixes should have associated tests, and so it would be inappropriate
> to push disjoint fixes and tests.
>
> I've taken the liberty of applying some trivial cleanup:
>
> * I moved the test into test/langtools/tools/javac/processing instead of
> test/langtools/tools/javac/modules. The primary reason for this is that the
> fixes themselves are in code that is more strongly associated with
> annotation processing.  Moving the test does not need any corresponding
> code changes.
>
> * I fixed the use of /nodynamiccopyright/. I'm sorry I didn't explain it
> better. It should appear in a comment in files that appear as messages with
> line numbers in a golden file, and so which should not have a legal header.
> In this case, it is mods-src2/mod/module-info.java that appears in the
> golden file, and so that is the file that should have /nodynamiccopyright/
> instead of the legal header.   Making this change meant that I had to make
> a corresponding update in the line number in the golden file.  By the same
> reasoning, I removed /nodynamiccopyright/ from the test file, since that
> file is not mentioned in the golden file.
>
> --
>
> I'm not sure quite what you're trying to do with those regular expressions
> trying to normalize newlines. On Unix, they'll be no-ops, but on Windows,
> they will completely remove newlines. Not wrong, but sorta weirdly
> inconsistent.  I'd suggest to avoid regexes here and just doing a literal
> replace of System.getProperty("line.separator") with "\n", as in
>
>   68         String actualOutput = outputWriter.toString();
>   69         String expectedOutput = Files.readString(testBasePath.resolve("ReportOnImportedModuleAnnotation.out")));
>   70
>              String lineSep = System.getProperty("line.separator");
>   71         if(!actualOutput.replace(lineSep, "\n").equals(expectedOutput.replace(lineSep, "\n"))) {
>
> There are other ways you can write this, but that is the simplest
> adjustment to what you have here, and without getting into using test
> library code.
>
> If you're OK, I can make this change before running the patch through our
> internal build-and-test system.
>
> -- Jon
>
>
> On 12/09/2019 03: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>
> 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/c536cb56/attachment-0001.htm>