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

Get default type for new class tree #414

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

AndrewShf
Copy link
Member

@AndrewShf AndrewShf commented Feb 28, 2023

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 call visitParameterizedType and do corresponding things there.

@AndrewShf AndrewShf requested a review from zcai1 February 28, 2023 15:28
@@ -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())
Copy link

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.

Copy link
Member Author

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.

Copy link

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.

@AndrewShf AndrewShf requested a review from wmdietl February 28, 2023 19:52
Comment on lines 195 to 196
if ((type instanceof ParameterizedTypeTree) && !((ParameterizedTypeTree) type).getTypeArguments().isEmpty())
return super.visitNewClass(tree, unused);
Copy link

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.

Copy link
Member Author

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.

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

Successfully merging this pull request may close these issues.

2 participants