-
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
Fix Class literals in field access and related type checks #550
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.
Thanks for starting on this!
@@ -351,6 +352,14 @@ public TransferResult<NullnessValue, NullnessStore> visitMethodAccess( | |||
@Override | |||
public TransferResult<NullnessValue, NullnessStore> visitFieldAccess( | |||
FieldAccessNode n, TransferInput<NullnessValue, NullnessStore> p) { | |||
if (n.getFieldName().equals("class")) { |
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 described in #548 we should look into properly representing class literals in the CFG. The description for FieldAccessNode
explicitly states that they are not for class literals. Let's figure out what a good representation for this is.
boolean success = atypeFactory.getTypeHierarchy().isSubtype(widenedValueType, varType); | ||
|
||
boolean success; | ||
if (valueExpTree.toString().endsWith(".class")) { |
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.
This is a really bad idea: it converts every Tree to a String and compares it - this will not be good for performance.
It also doesn't assign a proper type to class literals and instead ignores them in comparisons. We should instead look into determining the proper type for class literals.
@@ -4,6 +4,7 @@ enum Issue3020 { | |||
void retrieveConstant() { | |||
Class<?> theClass = Issue3020.class; | |||
// :: error: (accessing.nullable) | |||
// :: error: (dereference.of.nullable) |
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.
This error comes from incorrectly using @Nullable
as the type of theClass
. That variable should be non-null, as it is assigned a class literal. So again, we should determine proper types for things, not ignore errors.
You need to say something like |
Fixes #548.