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

Class or Interface type AST has incorrect position and confusing structure #10287

Closed
nrmancuso opened this issue Jul 9, 2021 · 2 comments
Closed

Comments

@nrmancuso
Copy link
Member

nrmancuso commented Jul 9, 2021

A class or interface type used in a variable definition creates an AST with incorrect column and/or line numbers and strange structure:

➜  src cat Test.java
import java.lang.annotation.ElementType;
import java.lang.annotation.Retention;
import java.lang.annotation.RetentionPolicy;
import java.lang.annotation.Target;
import java.util.Map;

public class Test {
    public static void main() {
        TypeToken<Class3<String>>.TypeSet types = new TypeToken<Class3<String>>() {}.getTypes();
        Map.@MyAnnotation Entry e;
    }
    static class TypeToken<C>{
        public TypeSet getTypes() {
            return null;
        }

        public class TypeSet {
        }
    }
    class Class3<E>{}
}

@Retention(RetentionPolicy.CLASS)
@Target({ ElementType.TYPE_USE })
@interface MyAnnotation {
} 
                                                                                                                                

Output(extraneous output omitted):

➜  src java -jar checkstyle-8.44-all.jar -t Test.java
...
    |       |--VARIABLE_DEF -> VARIABLE_DEF [9:8]
    |       |   |--MODIFIERS -> MODIFIERS [9:8]
    |       |   |--TYPE -> TYPE [9:8]
    |       |   |   |--IDENT -> TypeToken [9:8]
    |       |   |   `--TYPE_ARGUMENTS -> TYPE_ARGUMENTS [9:17]
    |       |   |       |--GENERIC_START -> < [9:17]
    |       |   |       |--TYPE_ARGUMENT -> TYPE_ARGUMENT [9:33] // type argument actually starts on column 17
    |       |   |       |   `--DOT -> . [9:33]  // should not be nested in type arguments AST, should not be root of Class3 AST
    |       |   |       |       |--IDENT -> Class3 [9:18]
    |       |   |       |       |--TYPE_ARGUMENTS -> TYPE_ARGUMENTS [9:24]
    |       |   |       |       |   |--GENERIC_START -> < [9:24]
    |       |   |       |       |   |--TYPE_ARGUMENT -> TYPE_ARGUMENT [9:25]
    |       |   |       |       |   |   `--IDENT -> String [9:25]
    |       |   |       |       |   `--GENERIC_END -> > [9:31]
    |       |   |       |       `--IDENT -> TypeSet [9:34]  // should not be nested in type arguments AST
    |       |   |       `--GENERIC_END -> > [9:32]
    |       |   |--IDENT -> types [9:42]
    |       |   `--ASSIGN -> = [9:48]
...
    |       |--VARIABLE_DEF -> VARIABLE_DEF [10:11]
    |       |   |--MODIFIERS -> MODIFIERS [10:11]
    |       |   |--TYPE -> TYPE [10:11]   // type(Map) actually begins on column 8
    |       |   |   `--DOT -> . [10:11]
    |       |   |       |--IDENT -> Map [10:8]
    |       |   |       |--ANNOTATIONS -> ANNOTATIONS [10:12]
    |       |   |       |   `--ANNOTATION -> ANNOTATION [10:12]
    |       |   |       |       |--AT -> @ [10:12]
    |       |   |       |       `--IDENT -> MyAnnotation [10:13]
    |       |   |       `--IDENT -> Entry [10:26]
    |       |   `--IDENT -> e [10:32]
    |       |--SEMI -> ; [10:33]
  ...

Full AST can be found here:
https://gist.github.com/nmancus1/73b4219c86eed420f18fd185e6893384

I would expect the AST to look like this:

...
    |       |--VARIABLE_DEF -> VARIABLE_DEF [9:8]
    |       |   |--MODIFIERS -> MODIFIERS [9:8]
    |       |   |--TYPE -> TYPE [9:8]
    |       |   |   |--IDENT -> TypeToken [9:8]
    |       |   |   `--TYPE_ARGUMENTS -> TYPE_ARGUMENTS [9:17] // correct position
    |       |   |       |--GENERIC_START -> < [9:17]
    |       |   |       |--TYPE_ARGUMENT -> TYPE_ARGUMENT [9:18]
    |       |   |       |   |--IDENT -> Class3 [9:18]
    |       |   |       |   |--TYPE_ARGUMENTS -> TYPE_ARGUMENTS [9:24]
    |       |   |       |   |   |--GENERIC_START -> < [9:24]
    |       |   |       |   |   |--TYPE_ARGUMENT -> TYPE_ARGUMENT [9:25]
    |       |   |       |   |   |   `--IDENT -> String [9:25]
    |       |   |       |   |   `--GENERIC_END -> > [9:31]
    |       |   |       |   |--DOT -> . [9:33]
    |       |   |       |   `--IDENT -> TypeSet [9:34]
    |       |   |       `--GENERIC_END -> > [9:32] // can't be fixed until ANTLR4 update, but should be above the DOT at 9:33
    |       |   |--IDENT -> types [9:42]
    |       |   `--ASSIGN -> = [9:48]
...
    |       |--VARIABLE_DEF -> VARIABLE_DEF [10:8]
    |       |   |--MODIFIERS -> MODIFIERS [10:8]
    |       |   |--TYPE -> TYPE [10:8]
    |       |   |   |--IDENT -> Map [10:8]
    |       |   |   |--DOT -> . [10:11]
    |       |   |   |--ANNOTATIONS -> ANNOTATIONS [10:12]
    |       |   |   |   `--ANNOTATION -> ANNOTATION [10:12]
    |       |   |   |       |--AT -> @ [10:12]
    |       |   |   |       `--IDENT -> MyAnnotation [10:13]
    |       |   |   `--IDENT -> Entry [10:26]
    |       |   `--IDENT -> e [10:32]
    |       |--SEMI -> ; [10:33]
...

Link to diff: https://www.diffchecker.com/U0zE8Dno

This can be fixed by removing the ^ ANTLR operator from the DOT token in the classOrInterfaceType production rule. While it is true that the DOT token is an operator in Java (and would arguably be the "root" of this AST), I think in the classOrInterfaceType production rule, where we have have three distinct elements (annotations, type parameters, and identifiers) it makes sense to have the DOT as a parallel element in respect to the IDENTs and ANNOTATIONS.

@nrmancuso nrmancuso changed the title Class or Interface type AST incorrect position and confusing structure Class or Interface type AST has incorrect position and confusing structure Jul 9, 2021
@nrmancuso
Copy link
Member Author

nrmancuso commented Jul 9, 2021

After discussion with @romani we will keep this structure as is, to preserve the DOT operator as root. But, the structure should be more consistent.

@nrmancuso nrmancuso reopened this Jul 9, 2021
@nrmancuso
Copy link
Member Author

Closed via #10280 :

 ➜  src cat Test.java
import java.lang.annotation.ElementType;
import java.lang.annotation.Retention;
import java.lang.annotation.RetentionPolicy;
import java.lang.annotation.Target;
import java.util.Map;

public class Test {
    public static void main() {
        TypeToken<Class3<String>>.TypeSet types = new TypeToken<Class3<String>>() {}.getTypes();
        Map.@MyAnnotation Entry e;
    }
    static class TypeToken<C>{
        public TypeSet getTypes() {
            return null;
        }

        public class TypeSet {
        }
    }
    class Class3<E>{}
}

@Retention(RetentionPolicy.CLASS)
@Target({ ElementType.TYPE_USE })
@interface MyAnnotation {
} 

 ➜  src java -jar checkstyle-9.0-SNAPSHOT-all.jar -t Test.java
...
    |       |--VARIABLE_DEF -> VARIABLE_DEF [9:33]
    |       |   |--MODIFIERS -> MODIFIERS [9:33]
    |       |   |--TYPE -> TYPE [9:33]
    |       |   |   `--DOT -> . [9:33]
    |       |   |       |--IDENT -> TypeToken [9:8]
    |       |   |       |--TYPE_ARGUMENTS -> TYPE_ARGUMENTS [9:17]
    |       |   |       |   |--GENERIC_START -> < [9:17]
    |       |   |       |   |--TYPE_ARGUMENT -> TYPE_ARGUMENT [9:18]
    |       |   |       |   |   |--IDENT -> Class3 [9:18]
    |       |   |       |   |   `--TYPE_ARGUMENTS -> TYPE_ARGUMENTS [9:24]
    |       |   |       |   |       |--GENERIC_START -> < [9:24]
    |       |   |       |   |       |--TYPE_ARGUMENT -> TYPE_ARGUMENT [9:25]
    |       |   |       |   |       |   `--IDENT -> String [9:25]
    |       |   |       |   |       `--GENERIC_END -> > [9:31]
    |       |   |       |   `--GENERIC_END -> > [9:32]
    |       |   |       `--IDENT -> TypeSet [9:34]
    |       |   |--IDENT -> types [9:42]
    |       |   `--ASSIGN -> = [9:48]
  ...
    |       |--VARIABLE_DEF -> VARIABLE_DEF [10:11]
    |       |   |--MODIFIERS -> MODIFIERS [10:11]
    |       |   |--TYPE -> TYPE [10:11]
    |       |   |   `--DOT -> . [10:11]
    |       |   |       |--IDENT -> Map [10:8]
    |       |   |       |--ANNOTATIONS -> ANNOTATIONS [10:12]
    |       |   |       |   `--ANNOTATION -> ANNOTATION [10:12]
    |       |   |       |       |--AT -> @ [10:12]
    |       |   |       |       `--IDENT -> MyAnnotation [10:13]
    |       |   |       `--IDENT -> Entry [10:26]
    |       |   `--IDENT -> e [10:32]
    |       |--SEMI -> ; [10:33]
    |       `--RCURLY -> } [11:4]
 ...

DOT operator as root is now consistent.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant