[PATCH] JDK-8235457: Crash when reporting a message about an annotation on a module
Jonathan Gibbons
jonathan.gibbons at oracle.com
Wed Dec 11 21:09:44 UTC 2019
To close out this thread, I have pushed the changeset:
https://hg.openjdk.java.net/jdk/jdk/rev/8086ccbe445d
Jeremy, thanks again for your contribution.
-- Jon
On 12/10/2019 11:45 AM, Jonathan Gibbons wrote:
>
> 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/20191211/68370325/attachment-0001.htm>
More information about the compiler-dev
mailing list