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

[java] AST inconsistency between primitive and reference type arrays #910

Open
oowekyala opened this Issue Feb 13, 2018 · 5 comments

Comments

Projects
None yet
3 participants
@oowekyala
Copy link
Member

oowekyala commented Feb 13, 2018

Array types are represented by a ReferenceType with attributes IsArray and ArrayDepth set accordingly. ReferenceType nodes then either have a PrimitiveType or a ClassOrInterfaceType child, depending on whether the component type of the array is primitive or not. Both these node types have the attributes IsArray and ArrayDepth too.

The problem is that, with arrays of primitive types, these attributes are set only on the parent ReferenceType, and not on the PrimitiveType node. For arrays of reference type, the attributes are correctly set.

This causes ClassTypeResolver to resolve the type of the formal parameter keys as int and not int[] in the following snippet, since the type of the ReferenceType node is rolled up from its child. (try with the designer)

public abstract class AbstractClass {
    public void arrayParams(int[] keys, String[] labels) {
	int[] foo = new int[5];
   }
}

I think the root problem is that only the parent ReferenceType should know about array dimensions, if the child node is to represent the component type. If removing those attributes is too inconvenient, we could also just patch the parser so that the attributes are correctly set on PrimitiveType. Either way, there's an inconsistency to rule out.

@oowekyala oowekyala added this to the 6.1.0 milestone Feb 13, 2018

@oowekyala oowekyala changed the title [java] Type inconsistency between primitive and reference type arrays [java] AST inconsistency between primitive and reference type arrays Feb 13, 2018

oowekyala added a commit to oowekyala/pmd that referenced this issue Feb 13, 2018

Add MissingOverrideRule and some tests
Doesn't support method overriden from generic supertype.
Known limitations that are supposed to be fixed shortly
are pmd#910 and anonymous enum constants (the fix for that
is somewhere in my branches, after pmd#895)
@jsotuyod

This comment has been minimized.

Copy link
Member

jsotuyod commented Feb 14, 2018

@oowekyala interesting...

I did fix a bunch of issues on type resolution of arrays back for 6.0.0 on 05927af but it seems I only fixed VariableDeclaratorId nodes....

My two cents as to the correct behavior:

  1. PrimitiveType refers exclusively to the int keyword, I'm ok with it having type int.class
  2. ReferenceType refers to int[] and properly indicates it's an array and has depth 1, but it's type is set to int.class but it shouldn't, it should be an int array.
  3. Type is rolling up the type from ReferenceType, it should be an int array once again.

So, I'd simply change the implementation of ClassTypeResolver.visit(ASTReferenceType node, Object data) to consider array information on the roll up process.

@oowekyala

This comment has been minimized.

Copy link
Member

oowekyala commented Feb 14, 2018

Ok, but then, for arrays of reference type, typeres would resolve both the ReferenceType and the ClassOrInterfaceType to the array type, whereas for arrays of primitive type, the PrimitiveType would not have the type of the array, but the component type (int.class, not int[].class).

I think the ClassOrInterfaceType should have the component type, eg String, and the ReferenceType should have type String[] to make it consistent with primitive type arrays. In this scenario, the IsArray and ArrayDepth attributes of ClassOrInterfaceType and PrimitiveType would make no sense, since only ReferenceType can represent an array type, which is consistent with the jls

@jsotuyod

This comment has been minimized.

Copy link
Member

jsotuyod commented Feb 15, 2018

@oowekyala you are absolutely right. ClassOrInterfaceType shouldn't be an array type / know of array dimens.

I'd mark those as deprecated now, make sure we don't use them internally, and get them removed from the grammar on 7.0.0

oowekyala added a commit to oowekyala/pmd that referenced this issue Feb 15, 2018

Deprecate IsArray and ArrayDepth on ClassOrInterfaceType and Primitiv…
…eType

Only ReferenceType knows about array dimensions, the former nodes
represent the component type of the array when a child of an array
ReferenceType.

Fixes pmd#910

Remember to remove those for release 7.0.0
@oowekyala

This comment has been minimized.

Copy link
Member

oowekyala commented Feb 16, 2018

Looking at this more in-depth, FieldDeclaration and LocalVariableDeclaration should not either have these attributes. Look at the following code:

class Foo {
  int a[], b;
  {
    int a[], b;
  }
}

In both cases, a should be an int[] and b should be an int

FieldDeclaration and LocalVariableDeclaration have inconsistent and completely arbitrary implementation for IsArray and ArrayDepth. The following XPath expressions match the code:

//FieldDeclaration[@IsArray=false()]
//LocalVariableDeclaration[@IsArray=true()]

This is because the FieldDeclaration IsArray is true if the last declared variable is an array, while the LocalVariableDeclaration IsArray is true if the first declared variable is an array.

About type resolution:

For the field declaration, typeof(a) == typeof(b) == int[].class (a and b are the VariableDeclaratorIds) while the FieldDeclaration node itself has type int.class.

On the other hand for the local var declaration, typeof(a) == typeof(b) == int.class. LocalVariableDeclaration isn't a type node.

Talk about inconsistency lol

...

Anyway here's my take on making the behaviour consistent:

  • FieldDeclaration and LocalVariableDeclaration should neither be TypeNodes, nor be Dimensionable (have IsArray and ArrayDepth). This is because the JLS allows variable declarators to declare additional pairs of brackets, so the variables declared in a multi declaration can have different types... ew
  • ClassTypeResolver should be aware of that, and resolve each variable declarator's type independently. For now it relies on Symbol Table to resolve the type of declarator ids, I'd much rather have it use its own implementation. You just need the Type node, and add the array dimensions of the declarators
  • Symbol table probably needs to be updated too, since it caused typeres to fail
  • We could add a convenience method to FieldDeclaration and LocalVariableDeclaration to get the declarator ids declared in one method call.

Maybe we should have a rule to detect this type of array declaration gore. I mean, I hope it never reports anything anywhere, but it looks like that section of the JLS was written so that static analysers can write inspections to forbid it :)

@jsotuyod

This comment has been minimized.

Copy link
Member

jsotuyod commented Feb 16, 2018

@oowekyala very good analysis! We have plenty of work to do!

I'd personally split this among several PRs to make this easier to audit / test.

  1. A PR fixing ASTReferenceType to properly know when things are arrays
  2. A PR deprecating Dimensionable methods from ASTClassOrInterfaceType
  3. A PR fixing ASTFieldDeclaration

@adangel adangel modified the milestones: 6.7.0, 6.8.0 Sep 1, 2018

@adangel adangel removed this from the 6.8.0 milestone Sep 29, 2018

@oowekyala oowekyala added this to the 7.0.0 milestone Nov 6, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment