[PATCH] JDK-8235457: Crash when reporting a message about an annotation on a module
Jonathan Gibbons
jonathan.gibbons at oracle.com
Tue Dec 10 19:45:27 UTC 2019
FYI ...
* For a regexp solution, you can use just "\R" ... replaceAll("\\R", "\n")
* For a list-of-lines solution you can do the following:
68 List<String> actualOutput = outputWriter.toString().lines().collect(Collectors.toList());
69 List<String> expectedOutput = Files.readLines(testBasePath.resolve("ReportOnImportedModuleAnnotation.out")));
70
71 if(!actualOutput.equals(expectedOutput)) {
72 System.err.println("Expected: ["); expectedOutput.forEach(System.err::println); System.err.println("]");
73 System.err.println("Received: ["); actualOutput.forEach(System.err::println); System.err.println("]");
* For a test-library solution, use toolbox.* to run javac, collect the
results as lines, and compare the resulting lists. See the package in
test/langtools/tools/lib/toolbox.
But, for now, the lineSeparator solution is good enough.
-- Jon
On 12/10/2019 01:10 AM, Jeremy Kuhn wrote:
> 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 <mailto: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/
> <http://cr.openjdk.java.net/%7Ejjg/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 <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
>> <http://cr.openjdk.java.net/%7Ejjg/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/6d65a8e5/attachment-0001.htm>
More information about the compiler-dev
mailing list