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 19 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 10 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions checker/jtreg/nullness/EISOPissue548/ConservativeClassLiteral.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
/*
* @test
* @summary Test class literals in CFGs and their type with conservative nullness.
*
* @compile MyEnum.java
* @compile -processor org.checkerframework.checker.nullness.NullnessChecker -AuseConservativeDefaultsForUncheckedCode=bytecode,-source ConservativeClassLiteral.java
*/

import java.util.EnumSet;

class ConservativeClassLiteral {
EnumSet<MyEnum> none() {
return EnumSet.noneOf(MyEnum.class);
}
}
3 changes: 3 additions & 0 deletions checker/jtreg/nullness/EISOPissue548/MyEnum.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
enum MyEnum {
VALUE;
}
Original file line number Diff line number Diff line change
Expand Up @@ -168,7 +168,7 @@ public TransferResult<V, S> visitAssignment(AssignmentNode n, TransferInput<V, S
}

/**
* If an invariant field is initialized and has the invariant annotation, than it has at least
* If an invariant field is initialized and has the invariant annotation, then it has at least
* the invariant annotation. Note that only fields of the 'this' receiver are tracked for
* initialization.
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
import org.checkerframework.checker.nullness.qual.PolyNull;
import org.checkerframework.common.basetype.BaseTypeChecker;
import org.checkerframework.dataflow.analysis.ConditionalTransferResult;
import org.checkerframework.dataflow.analysis.RegularTransferResult;
import org.checkerframework.dataflow.analysis.TransferInput;
import org.checkerframework.dataflow.analysis.TransferResult;
import org.checkerframework.dataflow.cfg.node.ArrayAccessNode;
Expand Down Expand Up @@ -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.

NullnessStore store = p.getRegularStore();
NullnessValue storeValue = store.getValue(n);
TransferResult<NullnessValue, NullnessStore> result =
new RegularTransferResult<NullnessValue, NullnessStore>(storeValue, store);
makeNonNull(result, n.getReceiver());
return result;
}
TransferResult<NullnessValue, NullnessStore> result = super.visitFieldAccess(n, p);
makeNonNull(result, n.getReceiver());
return result;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
*
* @checker_framework.manual #optional-checker Optional Checker
*/
// TODO: For a call to ofNullable, if the argument has type @NonNull, make the return type have type
// @Present. Make Optional Checker a subchecker of the Nullness Checker.
// TODO: For a call to of Nullable, if the argument has type @NonNull, make the return type have
// type @Present. Make Optional Checker a subchecker of the Nullness Checker.
@RelevantJavaTypes(Optional.class)
public class OptionalChecker extends BaseTypeChecker {}
1 change: 1 addition & 0 deletions checker/tests/nullness/Issue3020.java
Original file line number Diff line number Diff line change
Expand Up @@ -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.

Object unused = passThrough(theClass.getEnumConstants())[0];
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ public class FieldAccessNode extends Node {
* Creates a new FieldAccessNode.
*
* @param tree the tree from which to create a FieldAccessNode
* @param receiver the receiver for the resuling FieldAccessNode
* @param receiver the receiver for the resulting FieldAccessNode
*/
public FieldAccessNode(Tree tree, Node receiver) {
super(TreeUtils.typeOf(tree));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3103,12 +3103,14 @@ protected void commonAssignmentCheck(
Tree valueExpTree,
@CompilerMessageKey String errorKey,
Object... extraArgs) {

commonAssignmentCheckStartDiagnostic(varType, valueType, valueExpTree);

AnnotatedTypeMirror widenedValueType = atypeFactory.getWidenedType(valueType, varType);
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.

success = true;
} else {
commonAssignmentCheckStartDiagnostic(varType, valueType, valueExpTree);
AnnotatedTypeMirror widenedValueType = atypeFactory.getWidenedType(valueType, varType);
success = atypeFactory.getTypeHierarchy().isSubtype(widenedValueType, varType);
}
// TODO: integrate with subtype test.
if (success) {
for (Class<? extends Annotation> mono :
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -589,7 +589,7 @@ protected TransferResult<V, S> createTransferResult(@Nullable V value, TransferI
*
* @param value the value; possibly null
* @param in the TransferResult to copy
* @return the input informatio
* @return the input information
*/
@SideEffectFree
protected TransferResult<V, S> recreateTransferResult(
Expand Down
Loading