Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for enhanced instanceof #7290

Closed
strkkk opened this issue Nov 29, 2019 · 13 comments
Closed

Add support for enhanced instanceof #7290

strkkk opened this issue Nov 29, 2019 · 13 comments

Comments

@strkkk
Copy link
Member

strkkk commented Nov 29, 2019

$ cat TestClass.java
public class TestClass {
 {
 	Object o = "";
	if (o instanceof String s) { // jep 305
		System.out.println(s.toLowerCase());
	}
 }
}

/var/tmp $ java  -jar checkstyle-8.20-all.jar -t TestClass.java
com.puppycrawl.tools.checkstyle.api.CheckstyleException: IllegalStateException 
occurred while parsing file TestClass.java
Caused by: TestClass.java:4:26: expecting RPAREN, found 's'

Link to JEP
It is for java 14

https://docs.oracle.com/en/java/javase/14/docs/specs/patterns-instanceof-jls.html#jls-6.3.1


UPDATE: here is copy of discussion that explain why selected final variant of AST.
telegram-discussion.txt

@romani
Copy link
Member

romani commented Nov 29, 2019

This syntax usage might result in bunch of regressions in existing Checks that try to analyze scope of variables.

@nrmancuso
Copy link
Member

Is this what you would expect?

➜  Issue-7290 cat TestClass.java                                          
public class TestClass {
 {
 	Object o = "";
	if (o instanceof String s) { // jep 305
		System.out.println(s.toLowerCase());
	}
 }
}
➜  Issue-7290 java -jar checkstyle-8.31-SNAPSHOT-all.jar -t TestClass.java
CLASS_DEF -> CLASS_DEF [1:0]
|--MODIFIERS -> MODIFIERS [1:0]
|   `--LITERAL_PUBLIC -> public [1:0]
|--LITERAL_CLASS -> class [1:7]
|--IDENT -> TestClass [1:13]
`--OBJBLOCK -> OBJBLOCK [1:23]
    |--LCURLY -> { [1:23]
    |--INSTANCE_INIT -> INSTANCE_INIT [2:1]
    |   `--SLIST -> { [2:1]
    |       |--VARIABLE_DEF -> VARIABLE_DEF [3:2]
    |       |   |--MODIFIERS -> MODIFIERS [3:2]
    |       |   |--TYPE -> TYPE [3:2]
    |       |   |   `--IDENT -> Object [3:2]
    |       |   |--IDENT -> o [3:9]
    |       |   `--ASSIGN -> = [3:11]
    |       |       `--EXPR -> EXPR [3:13]
    |       |           `--STRING_LITERAL -> "" [3:13]
    |       |--SEMI -> ; [3:15]
    |       |--LITERAL_IF -> if [4:1]
    |       |   |--LPAREN -> ( [4:4]
    |       |   |--EXPR -> EXPR [4:7]
    |       |   |   `--LITERAL_INSTANCEOF -> instanceof [4:7]
    |       |   |       |--IDENT -> o [4:5]
    |       |   |       |--TYPE -> TYPE [4:18]
    |       |   |       |   `--IDENT -> String [4:18]
    |       |   |       `--IDENT -> s [4:25]
    |       |   |--RPAREN -> ) [4:26]
    |       |   `--SLIST -> { [4:28]
    |       |       |--EXPR -> EXPR [5:20]
    |       |       |   `--METHOD_CALL -> ( [5:20]
    |       |       |       |--DOT -> . [5:12]
    |       |       |       |   |--DOT -> . [5:8]
    |       |       |       |   |   |--IDENT -> System [5:2]
    |       |       |       |   |   `--IDENT -> out [5:9]
    |       |       |       |   `--IDENT -> println [5:13]
    |       |       |       |--ELIST -> ELIST [5:34]
    |       |       |       |   `--EXPR -> EXPR [5:34]
    |       |       |       |       `--METHOD_CALL -> ( [5:34]
    |       |       |       |           |--DOT -> . [5:22]
    |       |       |       |           |   |--IDENT -> s [5:21]
    |       |       |       |           |   `--IDENT -> toLowerCase [5:23]
    |       |       |       |           |--ELIST -> ELIST [5:35]
    |       |       |       |           `--RPAREN -> ) [5:35]
    |       |       |       `--RPAREN -> ) [5:36]
    |       |       |--SEMI -> ; [5:37]
    |       |       `--RCURLY -> } [6:1]
    |       `--RCURLY -> } [7:1]
    `--RCURLY -> } [8:0]

I haven't ran regression on this yet, but if this is the expected tree, I can start on this. mvn clean verify builds no problem with my changes.

@strkkk
Copy link
Member Author

strkkk commented Mar 29, 2020

@nmancus1 thank you.
Can you please show what is the difference between usual instanceof and enhanced one?
Is it only last ident?

@nrmancuso
Copy link
Member

That's correct, I've just modified the relationalExpression rule to catch an optional IDENT after the typeSpec. I though that would be the simplest way to implement this with the least amount of other changes.

nrmancuso added a commit to nrmancuso/checkstyle that referenced this issue Mar 29, 2020
@strkkk
Copy link
Member Author

strkkk commented Mar 29, 2020

May be it will be better to make this part as VARIABLE_DEF token?
Usually, code like String s; is parsed as variable definition.
I seems better, but I guess there is gonna be more regression in this case.

@nrmancuso
Copy link
Member

@strkkk How would this be:

CLASS_DEF -> CLASS_DEF [1:0]
|--MODIFIERS -> MODIFIERS [1:0]
|   `--LITERAL_PUBLIC -> public [1:0]
|--LITERAL_CLASS -> class [1:7]
|--IDENT -> TestClass [1:13]
...
    |       |--LITERAL_IF -> if [4:1]
    |       |   |--LPAREN -> ( [4:4]
    |       |   |--EXPR -> EXPR [4:7]
    |       |   |   `--LITERAL_INSTANCEOF -> instanceof [4:7]
    |       |   |       |--IDENT -> o [4:5]
    |       |   |       `--VARIABLE_DEF -> VARIABLE_DEF [4:18]
    |       |   |           |--MODIFIERS -> MODIFIERS [4:18]
    |       |   |           |--TYPE -> TYPE [4:18]
    |       |   |           |   `--IDENT -> String [4:18]
    |       |   |           `--IDENT -> s [4:25]
    |       |   |--RPAREN -> ) [4:26]
    |       |   `--SLIST -> { [4:28]
...
    `--RCURLY -> } [8:0]



@strkkk
Copy link
Member Author

strkkk commented Mar 29, 2020

I like this approach more.
@romani @rnveach @pbludov what do you think, how AST should look like?

@nrmancuso
Copy link
Member

nrmancuso commented Apr 1, 2020

Here's the results of the normal regression tests, using checktstyle_checks.xml, broken into three parts:
https://nmancus1.github.io/Issue-7290/diff_1/index.html
https://nmancus1.github.io/Issue-7290/diff_2/index.html
https://nmancus1.github.io/Issue-7290/diff_3/index.html

ANTLR grammar regression test here:
https://nmancus1.github.io/Issue-7290/ANTLR_diff/index.html

@pbludov
Copy link
Member

pbludov commented Apr 3, 2020

how AST should look like?

Note that the docs says this is a pattern rather than a variable def:

RelationalExpression:
     ...
     RelationalExpression instanceof ReferenceType
     RelationalExpression instanceof Pattern

Pattern:
     ReferenceType Identifier

So the tree should be more complicated:

|--EXPR -> EXPR [4:7]
|   `--LITERAL_INSTANCEOF -> instanceof [4:7]
|       |--IDENT -> o [4:5]
|       `--PARRERN_DEF -> PARRERN_DEF [4:18]
|           `--VARIABLE_DEF -> VARIABLE_DEF [4:18]
|               |--MODIFIERS -> MODIFIERS [4:18]
|               |--TYPE -> TYPE [4:18]
|               |   `--IDENT -> String [4:18]
|               `--IDENT -> s [4:25]

In the future, the pattern matching may grow to something like

if (o instanceof Point(x,y) && x > 0)

in this case there still be the PATTERN_DEF node and some kind of RECORD_DEF of something like that.

@nrmancuso
Copy link
Member

Note that the docs says this is a pattern rather than a variable def

This is a good point. Regarding RECORD_DEF; that should then replace VARIABLE_DEF, in the tree you've constructed, correct?

@pbludov
Copy link
Member

pbludov commented Apr 4, 2020

This is a good point. Regarding RECORD_DEF; that should then replace VARIABLE_DEF, in the tree you've constructed, correct?

I'm not sure. Either replace it or wrap it. From a possible future point of view, the wrap is better.

In this case, we have the ability to extend the patterns. And all existing checks like VariableName will handle such variables. Especially HiddenField - it is important to catch code like this:

class Foo {
  String name;
  void method(Object obj) {
     if (obj instanceof String name) {
         bar(name); // (String)obj
     } else {
         bar(name); // this.name
     }
  }
}

On the other hand, the check VisibilityModifier (and, possible other) need to be updated to ignore matching variables as well.

The bad news is that we can't do regression tests, because we're going to test code that doesn't exist yet.

nrmancuso added a commit to nrmancuso/checkstyle that referenced this issue Jun 1, 2020
nrmancuso added a commit to nrmancuso/checkstyle that referenced this issue Jun 1, 2020
nrmancuso added a commit to nrmancuso/checkstyle that referenced this issue Jun 1, 2020
nrmancuso added a commit to nrmancuso/checkstyle that referenced this issue Jun 1, 2020
nrmancuso added a commit to nrmancuso/checkstyle that referenced this issue Jun 1, 2020
nrmancuso added a commit to nrmancuso/checkstyle that referenced this issue Jun 1, 2020
nrmancuso added a commit to nrmancuso/checkstyle that referenced this issue Jun 1, 2020
nrmancuso added a commit to nrmancuso/checkstyle that referenced this issue Jun 3, 2020
nrmancuso added a commit to nrmancuso/checkstyle that referenced this issue Jun 3, 2020
nrmancuso added a commit to nrmancuso/checkstyle that referenced this issue Jul 2, 2020
nrmancuso added a commit to nrmancuso/checkstyle that referenced this issue Jul 2, 2020
nrmancuso added a commit to nrmancuso/checkstyle that referenced this issue Jul 4, 2020
nrmancuso added a commit to nrmancuso/checkstyle that referenced this issue Jul 6, 2020
nrmancuso added a commit to nrmancuso/checkstyle that referenced this issue Jul 6, 2020
nrmancuso added a commit to nrmancuso/checkstyle that referenced this issue Jul 6, 2020
nrmancuso added a commit to nrmancuso/checkstyle that referenced this issue Jul 6, 2020
nrmancuso added a commit to nrmancuso/checkstyle that referenced this issue Jul 7, 2020
nrmancuso added a commit to nrmancuso/checkstyle that referenced this issue Jul 7, 2020
nrmancuso added a commit to nrmancuso/checkstyle that referenced this issue Jul 7, 2020
@pbludov
Copy link
Member

pbludov commented Jul 7, 2020

Fix is merged.

@pbludov pbludov closed this as completed Jul 7, 2020
nrmancuso added a commit to nrmancuso/checkstyle that referenced this issue Jul 8, 2020
nrmancuso added a commit to nrmancuso/checkstyle that referenced this issue Jul 8, 2020
nrmancuso added a commit to nrmancuso/checkstyle that referenced this issue Jul 10, 2020
@romani romani added this to the 8.35 milestone Jul 11, 2020
nrmancuso added a commit to nrmancuso/checkstyle that referenced this issue Jul 17, 2020
nrmancuso added a commit to nrmancuso/checkstyle that referenced this issue Jul 18, 2020
nrmancuso added a commit to nrmancuso/checkstyle that referenced this issue Jul 18, 2020
nrmancuso added a commit to nrmancuso/checkstyle that referenced this issue Jul 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants