-
Notifications
You must be signed in to change notification settings - Fork 13
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
Get default type for new class tree #414
base: master
Are you sure you want to change the base?
Conversation
@@ -190,7 +191,11 @@ public Void visitAnnotatedType(AnnotatedTypeTree tree, Void unused) { | |||
@Override | |||
public Void visitNewClass(NewClassTree tree, Void unused) { | |||
AnnotatedTypeMirror defaultType = getDefaultTypeFor(tree); | |||
defaultTypes.put(tree.getIdentifier(), defaultType); | |||
ExpressionTree type = tree.getIdentifier(); | |||
if ((type instanceof ParameterizedTypeTree) && !((ParameterizedTypeTree) type).getTypeArguments().isEmpty()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you need to loop through the type arguments and put the corresponding types into defaultTypes
. You can take visitParameterizedType
as a reference.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a small example like new ArrayList<String>()
, and I find super.VisitNewClass()
already recursively call the visitParameterizedType() method for tree ArrayList<String>
. So, I don't think I need to call that method again.
Besides, what are the type arguments of a new class tree?
As said in the javadoc in NewClassTree.java
A tree node to declare a new instance of a class. For example:
new typeArguments identifier ( arguments )
classBody
The typearguments is in front of the identifier. And when I played around with this example new ArrayList<String>()
, using tree.getTypeArguments()
returns an empty list.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, I forgot the typeArguments
are not arguments to the class' type parameters. Those are arguments to type parameters on the constructor.
if ((type instanceof ParameterizedTypeTree) && !((ParameterizedTypeTree) type).getTypeArguments().isEmpty()) | ||
return super.visitNewClass(tree, unused); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We probably don't need the logic here. We should always save defaultType
to the cache before visiting child nodes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Without the if
branch, CI failed on the test case AnonymousProblem.java
. It raised the assertion error from the visitParameterizedType
The reason is because: (take new ArrayList<String>() as an example
). the NewClassTree does not have String as the type argument, which has 0 type argument, but the ParameterizedTypeTree has 1 type argument, and thus cause the assertion failure.
Previously, for new class tree like
new @Slot1 ArrayList<String>();
the default type for slot1 was derived by visiting the identifier tree, not the new class tree. By overriding this method, the default type for slot1 now is derived by visiting the new class tree, which is a more precise context for annotating.And I don't think we have to do anything else, like getting the default type for the type argument
String
in the above example, as the new method will recursively callvisitParameterizedType
and do corresponding things there.