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

Fix Class literals in field access and related type checks #550

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

Conversation

Ao-senXiong
Copy link
Member

@Ao-senXiong Ao-senXiong commented Aug 4, 2023

Fixes #548.

@Ao-senXiong Ao-senXiong marked this pull request as draft August 4, 2023 01:10
Copy link
Member

@wmdietl wmdietl left a 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")) {
Copy link
Member

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")) {
Copy link
Member

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)
Copy link
Member

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.

@wmdietl
Copy link
Member

wmdietl commented Aug 8, 2023

#548

You need to say something like Fixes #548 for GitHub to make the connection to the issue.

@Ao-senXiong Ao-senXiong mentioned this pull request Aug 9, 2023
@Ao-senXiong Ao-senXiong marked this pull request as ready for review October 16, 2023 05:21
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.

Class literals in CFGs and their type with conservative nullness
2 participants