Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
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
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,14 @@ public class NullnessNoInitAnnotatedTypeFactory
private final ExecutableElement mapGet =
TreeUtils.getMethod("java.util.Map", "get", 1, processingEnv);

/** The Collection.isEmpty method. */
private final ExecutableElement collectionIsEmpty =
TreeUtils.getMethod("java.util.Collection", "isEmpty", 0, processingEnv);

/** The Queue.poll method. */
private final ExecutableElement queuePoll =
TreeUtils.getMethod("java.util.Queue", "poll", 0, processingEnv);

// List is in alphabetical order. If you update it, also update
// ../../../../../../../../docs/manual/nullness-checker.tex
// and make a pull request for variables NONNULL_ANNOTATIONS and BASE_COPYABLE_ANNOTATIONS in
Expand Down Expand Up @@ -1072,4 +1080,24 @@ private AnnotationMirror ensuresNonNullAnno(String expression) {
public boolean isMapGet(Node node) {
return NodeUtils.isMethodInvocation(node, mapGet, getProcessingEnv());
}

/**
* Returns true if {@code node} is an invocation of Collection.isEmpty.
*
* @param node a CFG node
* @return true if {@code node} is an invocation of Collection.isEmpty
*/
public boolean isCollectionIsEmpty(Node node) {
return NodeUtils.isMethodInvocation(node, collectionIsEmpty, getProcessingEnv());
}

/**
* Returns true if {@code node} is an invocation of Queue.poll.
*
* @param node a CFG node
* @return true if {@code node} is an invocation of Queue.poll
*/
public boolean isQueuePoll(Node node) {
return NodeUtils.isMethodInvocation(node, queuePoll, getProcessingEnv());
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@

import java.util.List;
import java.util.Map;
import java.util.Queue;

import javax.lang.model.element.AnnotationMirror;
import javax.lang.model.element.ExecutableElement;
Expand Down Expand Up @@ -81,6 +82,14 @@ public class NullnessNoInitTransfer
*/
protected final AnnotatedDeclaredType MAP_TYPE;

/**
* Java's Queue interface.
*
* <p>The qualifiers in this type don't matter -- it is not used as a fully-annotated
* AnnotatedDeclaredType, but just passed to asSuper().
*/
protected final AnnotatedDeclaredType QUEUE_TYPE;

/** The type factory for the nullness analysis that was passed to the constructor. */
protected final NullnessNoInitAnnotatedTypeFactory nullnessTypeFactory;

Expand Down Expand Up @@ -130,6 +139,14 @@ public NullnessNoInitTransfer(NullnessNoInitAnalysis analysis) {
nullnessTypeFactory,
false);

QUEUE_TYPE =
(AnnotatedDeclaredType)
AnnotatedTypeMirror.createType(
TypesUtils.typeFromClass(
Queue.class, analysis.getTypes(), elements),
nullnessTypeFactory,
false);

nonNullAssumptionAfterInvocation =
!analysis.getTypeFactory()
.getChecker()
Expand Down Expand Up @@ -464,6 +481,34 @@ public TransferResult<NullnessNoInitValue, NullnessNoInitStore> visitMethodInvoc
}
}

// Handle Collection.isEmpty(), mark receiver as non-empty in the false branch

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// Handle Collection.isEmpty(), mark receiver as non-empty in the false branch
// Handle Collection.isEmpty(): mark receiver as non-empty in the false branch.

The sentence read a bit odd. Documentation comments should still end with a period.

You need to explain somewhere how you mark the receiver as non-empty. The body adds NONNULL, which seems a bit odd - the receiver for any method invocation should already be non-null. So how is this conveying any information you can test for below?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Semantically, marking it NONNULL doesn't map perfectly to collection state, but it acts as a signal to poll() about what we learned from isEmpty, so we can refine the return type. Is a custom annotation for non-empty worth exploring?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add something about poll() to the Store? Check the conditions here and set the return type of poll() to non-null here? Hopefully such a fact would automatically removed on side effects.

if (nullnessTypeFactory.isCollectionIsEmpty(n)) {
JavaExpression receiverExpr = JavaExpression.fromNode(receiver);
if (CFAbstractStore.canInsertJavaExpression(receiverExpr)) {
NullnessNoInitStore thenStore = result.getThenStore();
NullnessNoInitStore elseStore = result.getElseStore();
elseStore.insertValue(receiverExpr, NONNULL);
return new ConditionalTransferResult<>(
result.getResultValue(), thenStore, elseStore);
}
}

// Refine result to @NonNull if n is an invocation of Queue.poll(), the receiver is known to
// be non-empty
// and Queue element type is @NonNull

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// be non-empty
// and Queue element type is @NonNull
// be non-empty, and the Queue element type is @NonNull.

I like the Oxford comma.

if (nullnessTypeFactory.isQueuePoll(n)) {
NullnessNoInitStore store = result.getRegularStore();
JavaExpression receiverExpr = JavaExpression.fromNode(receiver);
NullnessNoInitValue receiverValue = store.getValue(receiverExpr);
if (receiverValue != null && receiverValue.getAnnotations().contains(NONNULL)) {
AnnotatedTypeMirror receiverType = nullnessTypeFactory.getReceiverType(n.getTree());
if (!isElementTypeNullable(receiverType)) {
makeNonNull(result, n);
refineToNonNull(result);
}
}
}

return result;
}

Expand All @@ -485,6 +530,24 @@ private boolean isValueTypeNullable(AnnotatedTypeMirror mapOrSubtype) {
return valueType.hasAnnotation(NULLABLE);
}

/**
* Returns true if queueType's element type (the E type argument to Queue) is @Nullable.
*
* @param queueOrSubtype the Queue type, or a subtype
* @return true if queueType's element type is @Nullable
*/
private boolean isElementTypeNullable(AnnotatedTypeMirror queueOrSubtype) {
AnnotatedDeclaredType queueType =
AnnotatedTypes.asSuper(nullnessTypeFactory, queueOrSubtype, QUEUE_TYPE);
int numTypeArguments = queueType.getTypeArguments().size();
if (numTypeArguments != 1) {
throw new TypeSystemError(
"Wrong number %d of type arguments: %s", numTypeArguments, queueType);
}
AnnotatedTypeMirror elementType = queueType.getTypeArguments().get(0);
return elementType.hasAnnotation(NULLABLE);
}

@Override
public TransferResult<NullnessNoInitValue, NullnessNoInitStore> visitReturn(
ReturnNode n, TransferInput<NullnessNoInitValue, NullnessNoInitStore> in) {
Expand Down
2 changes: 0 additions & 2 deletions checker/tests/nullness/IsEmptyPoll.java
Original file line number Diff line number Diff line change
@@ -1,8 +1,6 @@
// Test case for Issue 399:
// https://github.com/typetools/checker-framework/issues/399

// @skip-test until the issue is fixed

import org.checkerframework.checker.nullness.qual.NonNull;
import org.checkerframework.checker.nullness.qual.Nullable;

Expand Down
Loading