Issue #3238: Java 8 Grammar: annotations on arrays and varargs #7407

esilkensen · 2019-12-27T21:15:27Z

This PR adds support for parsing annotations on arrays and varargs, with the goal of resolving:

Issue Java Grammar: annotation before variable declaration #3236: Java Grammar: annotation before variable declaration
Issue Java 8 Grammar: annotations on varargs parameters #3238: Java 8 Grammar: annotations on varargs parameters
Issue Checkstyle does not support annotations on array types #4589: Checkstyle does not support annotations on array types
potentially another related issue that hasn't been created, if it is worth distinguishing: supporting annotations in new array expressions

If this looks like a reasonable approach, I would be happy to try and split it up into separate PR's addressing arrays and varargs separately if that would be better.

Since the changes are relatively small (and kind of overlapping? if thinking of varargs as a kind of array), I thought I would open the PR as it is now for initial feedback.

Regression Reports

Ran on all the projects-to-test-on.properties, also including the latest version of Guava. The only differences in the reports should be new violations for files that previously failed to parse.

3238-annotations-class-design-2: config with annotation, block, and class design checks
3238-coding-2: config with coding checks
3238-headers-misc-2: config with header, import, javadoc, metrics, and miscellaneous checks
3238-modifiers-whitespace-2: config with modifier, naming, regexp, size, and whitespace checks

rnveach · 2019-12-27T21:50:02Z

We should expand regression on more than 1 check to ensure none are broken. I recommend all checks, separated into multiple reports to make it easier.

src/main/resources/com/puppycrawl/tools/checkstyle/grammar/java.g

esilkensen · 2019-12-27T22:05:21Z

src/main/resources/com/puppycrawl/tools/checkstyle/grammar/java.g

@@ -386,8 +394,8 @@ builtInTypeSpec[boolean addImagNode]
 // A type name. which is either a (possibly qualified and parameterized)
 // class name or a primitive (builtin) type
 type
-    :    classOrInterfaceType[false]
-    |    builtInType
+    :    ({LA(1) == AT}? annotations | )


note: type is only used in newExpression

reference: in the spec, PrimitiveType allows an annotation (similar to ClassOrInterfaceTypeToInstantiate)

PrimitiveType: {Annotation} NumericType {Annotation} boolean

but while checkstyle's classOrInterfaceType allows annotations, builtInType by itself doesn't.

My thinking was to allow the annotation here in type for either case, rather than update builtInType which is used in several other places, so hoping to minimize changes.

esilkensen · 2019-12-27T22:09:58Z

src/main/resources/com/puppycrawl/tools/checkstyle/grammar/java.g

@@ -256,6 +256,14 @@ typeSpec[boolean addImagNode]
    | builtInTypeSpec[addImagNode]
    ;

+// A type specification for a variable length parameter is a type name with
+// possible brackets afterwards that can end with an annotation.
+variableLengthParameterTypeSpec


reference: LastFormalParameter which allows zero or more Annotation just before the ...

esilkensen · 2019-12-29T16:57:48Z

I've updated the PR description with links to regression reports trying to test all checks, almost all using the default configuration. So far I have only seen differences for files that fail to parse on master because of array/vararg annotations -- they have added violations in the patch branch, which I think is what we'd want to see. Let me know if there are other reports we should run and I will be happy to do it!

pbludov · 2019-12-29T19:44:38Z

From http://esilkensen.github.io/checkstyle-tester/3238-modifiers-whitespace/guava28/index.html

There is a nuance with NoWhitespaceBefore:

For void main(String... args) a whitespace before ... looks odd.
But for void main(String @Nullable ... args) it is OK to have a whitespace here.
Perhaps we should review NoWhitespaceBefore behaviour for elipces.

esilkensen · 2019-12-29T19:51:29Z

Ah yes, nice catch, will look into it

pbludov · 2019-12-29T19:59:51Z

Same for http://esilkensen.github.io/checkstyle-tester/3238-modifiers-whitespace/openjdk10/index.html#A3

new String @B [3]; versus String[3]; for NoWhitespaceAfter

This is strange, the doc says

If the annotation is between the type and the array, the check will skip validation for spaces.

esilkensen · 2019-12-30T06:04:51Z

@pbludov for ... and NoWhitespaceBefore, I agree, it seems like it could make sense for that check to have a special case similar to NoWhitespaceAfter that says: space before ... is OK if what precedes it is an annotation.

What do you think about

update NoWhitespaceAfter in this PR to extend the logic to new array expressions, which previously failed to parse, since the check already mentions it will skip validation "if the annotation is between the type and the array" (edit: I just pushed a change for this, and ran a regression report for NoWhitespaceAfter that now shows no differences http://esilkensen.github.io/checkstyle-tester/3238-nowhitespaceafter/index.html)
update NoWhitespaceBefore for ... in a separate PR/issue, since that logic doesn't exist there yet / annotations aren't mentioned as a special case?

src/main/resources/com/puppycrawl/tools/checkstyle/grammar/java.g

esilkensen · 2020-01-01T01:43:28Z

Updated the PR description with new regression reports using the latest code

src/test/resources/com/puppycrawl/tools/checkstyle/grammar/InputRegressionJava8Class1Ast.txt

esilkensen

Thanks for your comments @pbludov - I will be happy to run regression reports again if this looks good, or else continue iterating on the implementation

src/test/resources/com/puppycrawl/tools/checkstyle/grammar/InputRegressionJava8Class1Ast.txt

src/main/resources/com/puppycrawl/tools/checkstyle/grammar/java.g

pbludov

I'm ok to merge. Regression with NoWhitespaceBefore can be fixed in separate issue.

@rnveach please do second review

src/test/resources/com/puppycrawl/tools/checkstyle/grammar/InputRegressionJava8Class1Ast.txt

rnveach · 2020-01-06T19:33:49Z

@esilkensen @pbludov There is alot of review comments that are still open and not clicked as resolved. Are they all completed?

pbludov · 2020-01-06T19:52:17Z

@esilkensen @pbludov There is alot of review comments that are still open and not clicked as resolved. Are they all completed?

Done. I marked my comments as resolved.

src/main/java/com/puppycrawl/tools/checkstyle/checks/whitespace/NoWhitespaceAfterCheck.java

src/main/resources/com/puppycrawl/tools/checkstyle/grammar/java.g

esilkensen · 2020-01-08T19:09:34Z

I think the Travis failures are connectivity issues / unrelated to the changes in this PR

esilkensen · 2020-01-09T02:23:14Z

Sounds good, I will run the regression tonight.

esilkensen · 2020-01-09T07:22:42Z

New regression report: http://esilkensen.github.io/checkstyle-tester/3238-errors/index.html

<module name="Checker">
    <property name="charset" value="UTF-8"/>
    <property name="severity" value="warning"/>
    <property name="haltOnException" value="false"/>
    <module name="BeforeExecutionExclusionFileFilter">
        <property name="fileNamePattern" value="module\-info\.java$" />
    </module>
    <module name="TreeWalker">
        <module name="FinalLocalVariable">
            <property name="severity" value="ignore"/>
        </module>
    </module>
</module>

I think this report shows what we want?

The parse errors addressed by this PR go away (in openjdk8+ and guava28).
Because the code generated by antlr is different in master vs. patch, stack traces for parse errors that would otherwise be the same show up as diffs in the report (e.g. referencing different line numbers in GeneratedJavaRecognizer.java)

rnveach · 2020-01-09T14:56:20Z

Because the code generated by antlr is different in master vs. patch, stack traces for parse errors that would otherwise be the same show up as diffs in the report

Yes, this is a sad side effect, and the main reason these exceptions are suppressed in the default regression config.

I think this report shows what we want?

I agree. Exceptions going away and no new ones being generated. This was the main focus of this type of report.

rnveach · 2020-01-09T14:59:24Z

@romani This is a big change to antlr, so please finalize review.

esilkensen · 2020-01-19T10:13:18Z

Hi @romani, just checking in on this PR, if you get a chance to review, let me know if there’s anything I can do :)

romani · 2020-01-20T22:17:02Z

@esilkensen , please pull latest code of contribution repo, reports now are good at mobile browsers.
I am reviewing your PR ...

romani

items:

src/test/resources/com/puppycrawl/tools/checkstyle/grammar/InputRegressionJava8Class1Ast.txt

romani · 2020-01-22T22:23:03Z

@esilkensen , thanks a lot for such big update and sorry for delay in review.

ATTENTION: I rebased and pushed new commit that will use contribution repo (special) with latest guava. We will not merge this commit to master, we just need to know that all working.

related PR: checkstyle/contribution#444 that will be merged after this PR.

sevntu Checks looks like ok with such modification (tests are passed).

✔ ~/java/github/sevntu-checkstyle/sevntu.checkstyle/sevntu-checks [795-eclipse-cs-8.28.0 L|✚ 1] 
14:44 $ git diff
diff --git a/sevntu-checks/pom.xml b/sevntu-checks/pom.xml
index 5ce24e4..8723834 100644
--- a/sevntu-checks/pom.xml
+++ b/sevntu-checks/pom.xml
@@ -16,7 +16,7 @@
   <properties>
     <project.build.sourceEncoding>iso-8859-1</project.build.sourceEncoding>
     <!-- It is compile dependency to checkstyle, this version has to be the same as eclipse-cs depends on -->
-    <checkstyle.eclipse-cs.version>8.28</checkstyle.eclipse-cs.version>
+    <checkstyle.eclipse-cs.version>8.29-SNAPSHOT</checkstyle.eclipse-cs.version>

romani · 2020-01-22T22:52:50Z

wercker is failed at ./.ci/wercker.sh no-warning-imports-guava looks like a bit unrelated .... but still need to be fixed probably expectation should be corrected.

romani · 2020-01-23T20:52:37Z

no-warning-imports-guava

should be fixed by checkstyle/contribution@35d455d

romani

Please remove first commit .... CI all green, so we a good to go further.

minor update:

src/main/java/com/puppycrawl/tools/checkstyle/checks/whitespace/NoWhitespaceAfterCheck.java

romani · 2020-01-24T22:23:28Z

Just notes of my review to show AST tree of new items:

public void m1(@MyAnnotation String @MyAnnotation ... vararg) {}


|   |   `--PARAMETER_DEF -> PARAMETER_DEF [21:19]
|   |       |--MODIFIERS -> MODIFIERS [21:19]
|   |       |   `--ANNOTATION -> ANNOTATION [21:19]
|   |       |       |--AT -> @ [21:19]
|   |       |       `--IDENT -> MyAnnotation [21:20]
|   |       |--TYPE -> TYPE [21:33]
|   |       |   |--IDENT -> String [21:33]
|   |       |   `--ANNOTATIONS -> ANNOTATIONS [21:40]
|   |       |       `--ANNOTATION -> ANNOTATION [21:40]
|   |       |           |--AT -> @ [21:40]
|   |       |           `--IDENT -> MyAnnotation [21:41]
|   |       |--ELLIPSIS -> ... [21:54]
|   |       `--IDENT -> vararg [21:58]



public String m2() @MyAnnotation [] @MyAnnotation [] { return null; }

|   |--TYPE -> TYPE [22:54]
|   |   `--ARRAY_DECLARATOR -> [ [22:54]
|   |       |--ARRAY_DECLARATOR -> [ [22:37]
|   |       |   |--IDENT -> String [22:11]
|   |       |   |--ANNOTATIONS -> ANNOTATIONS [22:23]
|   |       |   |   `--ANNOTATION -> ANNOTATION [22:23]
|   |       |   |       |--AT -> @ [22:23]
|   |       |   |       `--IDENT -> MyAnnotation [22:24]
|   |       |   `--RBRACK -> ] [22:38]
|   |       |--ANNOTATIONS -> ANNOTATIONS [22:40]
|   |       |   `--ANNOTATION -> ANNOTATION [22:40]
|   |       |       |--AT -> @ [22:40]
|   |       |       `--IDENT -> MyAnnotation [22:41]
|   |       `--RBRACK -> ] [22:55]


Map.@MyAnnotation Entry e;

|       |--VARIABLE_DEF -> VARIABLE_DEF [26:11]
|       |   |--MODIFIERS -> MODIFIERS [26:11]
|       |   |--TYPE -> TYPE [26:11]
|       |   |   `--DOT -> . [26:11]
|       |   |       |--IDENT -> Map [26:8]
|       |   |       |--ANNOTATIONS -> ANNOTATIONS [26:12]
|       |   |       |   `--ANNOTATION -> ANNOTATION [26:12]
|       |   |       |       |--AT -> @ [26:12]
|       |   |       |       `--IDENT -> MyAnnotation [26:13]
|       |   |       `--IDENT -> Entry [26:26]
|       |   `--IDENT -> e [26:32]


String str = (@MyAnnotation String) "";

|       |   `--ASSIGN -> = [27:19]
|       |       `--EXPR -> EXPR [27:21]
|       |           `--TYPECAST -> ( [27:21]
|       |               |--TYPE -> TYPE [27:22]
|       |               |   |--ANNOTATIONS -> ANNOTATIONS [27:22]
|       |               |   |   `--ANNOTATION -> ANNOTATION [27:22]
|       |               |   |       |--AT -> @ [27:22]
|       |               |   |       `--IDENT -> MyAnnotation [27:23]
|       |               |   `--IDENT -> String [27:36]
|       |               |--RPAREN -> ) [27:42]
|       |               `--STRING_LITERAL -> "" [27:44]


Object arr = new @MyAnnotation String @MyAnnotation [3];

|       |   |--IDENT -> arr [29:15]
|       |   `--ASSIGN -> = [29:19]
|       |       `--EXPR -> EXPR [29:21]
|       |           `--LITERAL_NEW -> new [29:21]
|       |               |--ANNOTATIONS -> ANNOTATIONS [29:25]
|       |               |   `--ANNOTATION -> ANNOTATION [29:25]
|       |               |       |--AT -> @ [29:25]
|       |               |       `--IDENT -> MyAnnotation [29:26]
|       |               |--IDENT -> String [29:39]
|       |               `--ARRAY_DECLARATOR -> [ [29:60]
|       |                   |--ANNOTATIONS -> ANNOTATIONS [29:46]
|       |                   |   `--ANNOTATION -> ANNOTATION [29:46]
|       |                   |       |--AT -> @ [29:46]
|       |                   |       `--IDENT -> MyAnnotation [29:47]
|       |                   |--EXPR -> EXPR [29:61]
|       |                   |   `--NUM_INT -> 3 [29:61]
|       |                   `--RBRACK -> ] [29:62]

String a @MyAnnotation []

|       |   |   |--VARIABLE_DEF -> VARIABLE_DEF [30:36]
|       |   |   |   |--MODIFIERS -> MODIFIERS [30:36]
|       |   |   |   |--TYPE -> TYPE [30:36]
|       |   |   |   |   `--ARRAY_DECLARATOR -> [ [30:36]
|       |   |   |   |       |--IDENT -> String [30:13]
|       |   |   |   |       |--ANNOTATIONS -> ANNOTATIONS [30:22]
|       |   |   |   |       |   `--ANNOTATION -> ANNOTATION [30:22]
|       |   |   |   |       |       |--AT -> @ [30:22]
|       |   |   |   |       |       `--IDENT -> MyAnnotation [30:23]
|       |   |   |   |       `--RBRACK -> ] [30:37]
|       |   |   |   `--IDENT -> a [30:20]

Object arr2 = new @MyAnnotation int[3];

|       |   |--IDENT -> arr2 [31:15]
|       |   `--ASSIGN -> = [31:20]
|       |       `--EXPR -> EXPR [31:22]
|       |           `--LITERAL_NEW -> new [31:22]
|       |               |--ANNOTATIONS -> ANNOTATIONS [31:26]
|       |               |   `--ANNOTATION -> ANNOTATION [31:26]
|       |               |       |--AT -> @ [31:26]
|       |               |       `--IDENT -> MyAnnotation [31:27]
|       |               |--LITERAL_INT -> int [31:40]
|       |               `--ARRAY_DECLARATOR -> [ [31:43]
|       |                   |--EXPR -> EXPR [31:44]
|       |                   |   `--NUM_INT -> 3 [31:44]
|       |                   `--RBRACK -> ] [31:45]

romani

ok to merge ones CI pass (we should merge PR in contribution too)

@esilkensen , thanks a lot for your work on this.

esilkensen marked this pull request as ready for review December 27, 2019 21:51

esilkensen commented Dec 27, 2019

View reviewed changes

src/main/resources/com/puppycrawl/tools/checkstyle/grammar/java.g Outdated Show resolved Hide resolved

esilkensen commented Dec 27, 2019

View reviewed changes

esilkensen force-pushed the array-annotations branch from 0122845 to f7f9af5 Compare December 29, 2019 07:59

pbludov approved these changes Dec 29, 2019

View reviewed changes

esilkensen force-pushed the array-annotations branch from f7f9af5 to e768767 Compare December 30, 2019 06:56

pbludov suggested changes Dec 31, 2019

View reviewed changes

src/main/resources/com/puppycrawl/tools/checkstyle/grammar/java.g Outdated Show resolved Hide resolved

esilkensen force-pushed the array-annotations branch from e768767 to b986a1b Compare December 31, 2019 18:01

pbludov suggested changes Jan 1, 2020

View reviewed changes

src/test/resources/com/puppycrawl/tools/checkstyle/grammar/InputRegressionJava8Class1Ast.txt Outdated Show resolved Hide resolved

esilkensen force-pushed the array-annotations branch from b986a1b to 5150e7f Compare January 1, 2020 23:24

esilkensen commented Jan 1, 2020

View reviewed changes

src/test/resources/com/puppycrawl/tools/checkstyle/grammar/InputRegressionJava8Class1Ast.txt Outdated Show resolved Hide resolved

src/main/resources/com/puppycrawl/tools/checkstyle/grammar/java.g Show resolved Hide resolved

pbludov approved these changes Jan 2, 2020

View reviewed changes

src/test/resources/com/puppycrawl/tools/checkstyle/grammar/InputRegressionJava8Class1Ast.txt Outdated Show resolved Hide resolved

pbludov requested a review from rnveach January 2, 2020 04:24

pbludov assigned rnveach Jan 2, 2020

rnveach requested changes Jan 8, 2020

View reviewed changes

src/main/java/com/puppycrawl/tools/checkstyle/checks/whitespace/NoWhitespaceAfterCheck.java Outdated Show resolved Hide resolved

src/main/resources/com/puppycrawl/tools/checkstyle/grammar/java.g Show resolved Hide resolved

esilkensen force-pushed the array-annotations branch 4 times, most recently from 746b70c to d9df42d Compare January 8, 2020 18:10

rnveach approved these changes Jan 9, 2020

View reviewed changes

rnveach requested a review from romani January 9, 2020 14:59

rnveach assigned romani and unassigned rnveach Jan 9, 2020

esilkensen force-pushed the array-annotations branch from d9df42d to cfc4404 Compare January 20, 2020 05:59

romani requested changes Jan 22, 2020

View reviewed changes

src/test/resources/com/puppycrawl/tools/checkstyle/grammar/InputRegressionJava8Class1Ast.txt Outdated Show resolved Hide resolved

romani added a commit to checkstyle/contribution that referenced this pull request Jan 22, 2020

checkstyle/checkstyle#7407: enable latest guava

aa5ad93

romani force-pushed the array-annotations branch from cfc4404 to d8882be Compare January 22, 2020 22:21

romani mentioned this pull request Jan 22, 2020

checkstyle/checkstyle#7407: enable latest guava checkstyle/contribution#444

Merged

esilkensen force-pushed the array-annotations branch from d8882be to 3b749ba Compare January 23, 2020 21:54

romani requested changes Jan 24, 2020

View reviewed changes

src/main/java/com/puppycrawl/tools/checkstyle/checks/whitespace/NoWhitespaceAfterCheck.java Outdated Show resolved Hide resolved

Issue checkstyle#3238: Java 8 Grammar: annotations on arrays and varargs

5419e22

esilkensen force-pushed the array-annotations branch from 3b749ba to 5419e22 Compare January 24, 2020 22:22

romani approved these changes Jan 24, 2020

View reviewed changes

romani merged commit 89b4dcd into checkstyle:master Jan 24, 2020

romani added a commit to checkstyle/contribution that referenced this pull request Jan 24, 2020

checkstyle/checkstyle#7407: enable latest guava

adfb333

This was referenced Jan 25, 2020

Java Grammar: annotation before variable declaration #3236

Closed

Checkstyle does not support annotations on array types #4589

Closed

esilkensen deleted the array-annotations branch January 25, 2020 17:22

esilkensen mentioned this pull request Jan 25, 2020

config: remove TypeUseTarget from excludes checkstyle/contribution#445

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue #3238: Java 8 Grammar: annotations on arrays and varargs #7407

Issue #3238: Java 8 Grammar: annotations on arrays and varargs #7407

esilkensen commented Dec 27, 2019 •

edited

rnveach commented Dec 27, 2019

esilkensen Dec 27, 2019 •

edited

esilkensen Dec 27, 2019 •

edited

esilkensen commented Dec 29, 2019

pbludov commented Dec 29, 2019

esilkensen commented Dec 29, 2019

pbludov commented Dec 29, 2019

esilkensen commented Dec 30, 2019 •

edited

esilkensen commented Jan 1, 2020

esilkensen left a comment

pbludov left a comment

rnveach commented Jan 6, 2020

pbludov commented Jan 6, 2020

esilkensen commented Jan 8, 2020

esilkensen commented Jan 9, 2020

esilkensen commented Jan 9, 2020 •

edited

rnveach commented Jan 9, 2020 •

edited

rnveach commented Jan 9, 2020

esilkensen commented Jan 19, 2020

romani commented Jan 20, 2020

romani left a comment

romani commented Jan 22, 2020 •

edited

romani commented Jan 22, 2020

romani commented Jan 23, 2020

romani left a comment

romani commented Jan 24, 2020

romani left a comment

Issue #3238: Java 8 Grammar: annotations on arrays and varargs #7407

Issue #3238: Java 8 Grammar: annotations on arrays and varargs #7407

Conversation

esilkensen commented Dec 27, 2019 • edited

rnveach commented Dec 27, 2019

esilkensen Dec 27, 2019 • edited

Choose a reason for hiding this comment

esilkensen Dec 27, 2019 • edited

Choose a reason for hiding this comment

esilkensen commented Dec 29, 2019

pbludov commented Dec 29, 2019

esilkensen commented Dec 29, 2019

pbludov commented Dec 29, 2019

esilkensen commented Dec 30, 2019 • edited

esilkensen commented Jan 1, 2020

esilkensen left a comment

Choose a reason for hiding this comment

pbludov left a comment

Choose a reason for hiding this comment

rnveach commented Jan 6, 2020

pbludov commented Jan 6, 2020

esilkensen commented Jan 8, 2020

esilkensen commented Jan 9, 2020

esilkensen commented Jan 9, 2020 • edited

rnveach commented Jan 9, 2020 • edited

rnveach commented Jan 9, 2020

esilkensen commented Jan 19, 2020

romani commented Jan 20, 2020

romani left a comment

Choose a reason for hiding this comment

romani commented Jan 22, 2020 • edited

romani commented Jan 22, 2020

romani commented Jan 23, 2020

romani left a comment

Choose a reason for hiding this comment

romani commented Jan 24, 2020

romani left a comment

Choose a reason for hiding this comment

esilkensen commented Dec 27, 2019 •

edited

esilkensen Dec 27, 2019 •

edited

esilkensen Dec 27, 2019 •

edited

esilkensen commented Dec 30, 2019 •

edited

esilkensen commented Jan 9, 2020 •

edited

rnveach commented Jan 9, 2020 •

edited

romani commented Jan 22, 2020 •

edited