[PATCH] JDK-8235457: Crash when reporting a message about an annotation on a module
Jonathan Gibbons
jonathan.gibbons at oracle.com
Tue Dec 10 02:30:43 UTC 2019
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
> <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/20191209/ab286882/attachment-0001.htm>
More information about the compiler-dev
mailing list