-
Notifications
You must be signed in to change notification settings - Fork 18
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
Don't erase the type argument of receiver #793
base: master
Are you sure you want to change the base?
Conversation
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.
As discussed, please add a jtreg test.
Maybe this would fix #104? Can you try with tests from that issue? |
Just checked, this did not fix it. |
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.
Thanks for starting to look into this. We should look at this properly and fix #104 while we touch this code.
@@ -3772,7 +3772,7 @@ protected void checkMethodInvocability( | |||
return; | |||
} | |||
|
|||
AnnotatedTypeMirror methodReceiver = method.getReceiverType().getErased(); | |||
AnnotatedTypeMirror methodReceiver = method.getReceiverType(); | |||
AnnotatedTypeMirror treeReceiver = methodReceiver.shallowCopy(false); | |||
AnnotatedTypeMirror rcv = atypeFactory.getReceiverType(tree); |
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.
The subtype test below is typeHierarchy.isSubtype(treeReceiver, methodReceiver);
treeReceiver
is a shallow copy of methodReceiver
, with just the main modifier copied from the actual receiver rcv
of the method invocation.
So to fix #104 we should really understand better what this code is supposed to do.
What goes from if we use typeHierarchy.isSubtype(rcv, methodReceiver);
? Why this strange copying of annotations?
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 will look into #104 after this PR.
* | ||
* @compile/fail/ref=TypeArgument.out -XDrawDiagnostics -processor org.checkerframework.checker.nullness.NullnessChecker TypeArgument.java | ||
*/ | ||
public class TypeArgument<T> { |
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.
TypeArgument
is not a very descriptive name for this test.
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.
Renamed to NonRawTypeArgumentTest
@@ -0,0 +1,14 @@ | |||
/* | |||
* @test | |||
* @summary Test case for type argument in method invocation. |
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 find this summary confusing and don't see how this matches the code - there is no type argument there.
You need to say something about what type argument you mean.
Same with the test name 'NonRawTypeArgument` isn't very helpful - all type arguments are non-raw, otherwise there wouldn't be type arguments...
This is a partial fix, look into the error message in subclass and #104 |
#104 seems like really different issue after I debugging the code. Maybe we should merge this independently. |
for this code
error message before the change:
error message after the change: