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

Add support for opt.map(type::method) pattern. #6370

Merged
merged 18 commits into from
Dec 15, 2023
Merged
Original file line number Diff line number Diff line change
@@ -0,0 +1,122 @@
package org.checkerframework.checker.optional;

import com.sun.source.tree.ExpressionTree;
import com.sun.source.tree.MemberReferenceTree;
import com.sun.source.tree.MethodInvocationTree;
import com.sun.source.tree.Tree;
import com.sun.source.tree.Tree.Kind;
import java.util.Collection;
import java.util.function.Function;
import javax.lang.model.element.AnnotationMirror;
import javax.lang.model.element.ExecutableElement;
import org.checkerframework.checker.optional.qual.Present;
import org.checkerframework.common.basetype.BaseAnnotatedTypeFactory;
import org.checkerframework.common.basetype.BaseTypeChecker;
import org.checkerframework.framework.flow.CFAbstractAnalysis;
import org.checkerframework.framework.flow.CFStore;
import org.checkerframework.framework.flow.CFTransfer;
import org.checkerframework.framework.flow.CFValue;
import org.checkerframework.framework.type.AnnotatedTypeMirror;
import org.checkerframework.javacutil.AnnotationBuilder;
import org.checkerframework.javacutil.TreeUtils;

/** OptionalAnnotatedTypeFactory for the Optional Checker. */
public class OptionalAnnotatedTypeFactory extends BaseAnnotatedTypeFactory {

/** The element for java.util.Optional.map(). */
private final ExecutableElement optionalMap;

/** The @{@link Present} annotation. */
protected final AnnotationMirror PRESENT = AnnotationBuilder.fromClass(elements, Present.class);

/**
* Creates an OptionalAnnotatedTypeFactory.
*
* @param checker the Optional Checker associated with this type factory
*/
public OptionalAnnotatedTypeFactory(BaseTypeChecker checker) {
super(checker);
postInit();
optionalMap = TreeUtils.getMethodOrNull("java.util.Optional", "map", 1, getProcessingEnv());
}

@Override
public AnnotatedTypeMirror getAnnotatedType(Tree tree) {
AnnotatedTypeMirror result = super.getAnnotatedType(tree);
optionalMapNonNull(tree, result);
return result;
}

/**
* If {@code tree} is a call to {@link java.util.Optional#map(Function)} whose argument is a
* method reference, then this method adds {@code @Present} to {@code type} if the following is
* true:
*
* <ul>
* <li>The type of the receiver to {@link java.util.Optional#map(Function)} is {@code @Present},
* and
* <li>{@link #returnsNonNull(MemberReferenceTree)} returns true.
* </ul>
*
* @param tree a tree
* @param type the type of the tree, which may be side-effected by this method
*/
private void optionalMapNonNull(Tree tree, AnnotatedTypeMirror type) {
if (!TreeUtils.isMethodInvocation(tree, optionalMap, processingEnv)) {
return;
}
MethodInvocationTree mapTree = (MethodInvocationTree) tree;
ExpressionTree argTree = mapTree.getArguments().get(0);
if (argTree.getKind() == Kind.MEMBER_REFERENCE) {
MemberReferenceTree memberReferenceTree = (MemberReferenceTree) argTree;
AnnotatedTypeMirror optType = getReceiverType(mapTree);
if (optType == null || !optType.hasEffectiveAnnotation(Present.class)) {
return;
}
if (returnsNonNull(memberReferenceTree)) {
type.replaceAnnotation(PRESENT);
}
}
}

/**
* Returns true if the return type of the function type of {@code memberReferenceTree} is
* non-null.
*
* @param memberReferenceTree a member reference
* @return true if the return type of the function type of {@code memberReferenceTree} is non-null
*/
private boolean returnsNonNull(MemberReferenceTree memberReferenceTree) {
Copy link
Member

Choose a reason for hiding this comment

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

I find returnsNonNull (and the documentation above) a bit misleading. The given function can return null, but only if it is passed null. Another way to say this is that the function never introduces a new null value. Could you please adjust the method name and documentation accordingly? Thank you!

Copy link
Member Author

@smillst smillst Dec 14, 2023

Choose a reason for hiding this comment

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

This method is only checking that the return type does not have an explicit nullable annotation. If the return type is annotated with @PolyNull, then it still may return null even if the argument is non-null. Here's an example method signature with that property.

  @PolyNull Integer convertPoly(List<@PolyNull String> s);

I renamed to the method to make it clear that it is only checking from the lack of a Nullable annotation.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks! That renaming is helpful.

There is an inconsistency between the documentation (which says "true if @Nullable") and the name (which says "true if not @Nullable"). I suspect this is just a typo, Could you make them consistent? My preference would be to avoid negation in method names (& specifications), because negation makes reasoning more difficult.

Regarding the semantics: is there a difference between "is not annotated as @Nullable" and "never introduces a new null value"? I think they are the same, assuming that the code is fully annotated. Noting this equivalence (or explicitly disclaiming it, if I am mistaken) would help to clarify the effect of the code.

Copy link
Member Author

Choose a reason for hiding this comment

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

Regarding the semantics: is there a difference between "is not annotated as @nullable" and "never introduces a new null value"? I think they are the same, assuming that the code is fully annotated. Noting this equivalence (or explicitly disclaiming it, if I am mistaken) would help to clarify the effect of the code.

I don't know what "never introduces a new null value" means. (What's a new null value? How does a method reference introduce a null value?) I think you mean that the function only returns null if its argument is also null. That is not the same a method whose return type is not annotated with a nullable annotation. Here's an example of method that is not annotated with a nullable annotation and returns null when its argument in non-null:

  @PolyNull Integer convertPoly(List<@PolyNull String> s) {
    for (String ins : s) {
      if (ins == null) {
        return null;
      }
    }
    return 0;
  }

Copy link
Member

Choose a reason for hiding this comment

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

Thank you for this example! It is helpful. I indeed meant that the function only returns null if its argument is also null. But, as you point out, @PolyNull might not be written on the formal parameter. I had overlooked that. That would need to be checked.

Could you add a note about this to the documentation? Thanks.

if (TreeUtils.MemberReferenceKind.getMemberReferenceKind(memberReferenceTree)
.isConstructorReference()) {
return true;
}
if (!checker.hasOption("optionalMapAssumeNonNull")) {
return false;
}
ExecutableElement memberReferenceFuncType = TreeUtils.elementFromUse(memberReferenceTree);
return !containsNullable(memberReferenceFuncType.getAnnotationMirrors())
&& !containsNullable(memberReferenceFuncType.getReturnType().getAnnotationMirrors());
}

/**
* Returns true if {@code annos} contains a nullable annotation.
*
* @param annos a collection of annotations
* @return true if {@code annos} contains a nullable annotation
*/
private boolean containsNullable(Collection<? extends AnnotationMirror> annos) {
for (AnnotationMirror anno : annos) {
if (anno.getAnnotationType().asElement().getSimpleName().contentEquals("Nullable")) {
return true;
}
}
return false;
}

@Override
public CFTransfer createFlowTransferFunction(
CFAbstractAnalysis<CFValue, CFStore, CFTransfer> analysis) {
return new OptionalTransfer(analysis);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
import org.checkerframework.common.basetype.BaseTypeChecker;
import org.checkerframework.framework.qual.RelevantJavaTypes;
import org.checkerframework.framework.qual.StubFiles;
import org.checkerframework.framework.source.SupportedOptions;

/**
* A type-checker that prevents misuse of the {@link java.util.Optional} class.
Expand All @@ -14,6 +15,7 @@
// @NonNull, make the return type have type @Present.
@RelevantJavaTypes(Optional.class)
@StubFiles({"javaparser.astub"})
@SupportedOptions("optionalMapAssumeNonNull")
Copy link
Member

Choose a reason for hiding this comment

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

I think a better name would be "assumeNullAnnotated" or the like. This isn't specific to Optional.map, and it's about the assumption that nullness annotations are available for method references.

Copy link
Member Author

Choose a reason for hiding this comment

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

The code as implemented is specific to Optional.map. (And is specific to method references.)

public class OptionalChecker extends BaseTypeChecker {
/** Create an OptionalChecker. */
public OptionalChecker() {}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,10 @@
import org.checkerframework.dataflow.cfg.UnderlyingAST.CFGLambda;
import org.checkerframework.dataflow.cfg.node.LocalVariableNode;
import org.checkerframework.dataflow.expression.JavaExpression;
import org.checkerframework.framework.flow.CFAnalysis;
import org.checkerframework.framework.flow.CFAbstractAnalysis;
import org.checkerframework.framework.flow.CFStore;
import org.checkerframework.framework.flow.CFTransfer;
import org.checkerframework.framework.flow.CFValue;
import org.checkerframework.framework.type.AnnotatedTypeFactory;
import org.checkerframework.javacutil.AnnotationBuilder;
import org.checkerframework.javacutil.TreeUtils;
Expand All @@ -45,7 +46,7 @@ public class OptionalTransfer extends CFTransfer {
*
* @param analysis the Optional Checker instance
*/
public OptionalTransfer(CFAnalysis analysis) {
public OptionalTransfer(CFAbstractAnalysis<CFValue, CFStore, CFTransfer> analysis) {
super(analysis);
atypeFactory = analysis.getTypeFactory();
Elements elements = atypeFactory.getElementUtils();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,11 @@ public class OptionalTest extends CheckerFrameworkPerDirectoryTest {
* @param testFiles the files containing test code, which will be type-checked
*/
public OptionalTest(List<File> testFiles) {
super(testFiles, org.checkerframework.checker.optional.OptionalChecker.class, "optional");
super(
testFiles,
org.checkerframework.checker.optional.OptionalChecker.class,
"optional",
"-AoptionalMapAssumeNonNull");
}

@Parameters
Expand Down
16 changes: 16 additions & 0 deletions checker/tests/optional/MapNoNewNull.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
import java.math.BigInteger;
import java.util.Optional;

class MapNoNewNull {

@SuppressWarnings("optional.parameter")
void m(Optional<Digits> digitsAnnotation) {
if (digitsAnnotation.isPresent()) {
BigInteger maxValue = digitsAnnotation.map(Digits::integer).map(BigInteger::valueOf).get();
}
}
}

@interface Digits {
public int integer();
}
34 changes: 34 additions & 0 deletions checker/tests/optional/OptionalMapMethodReference.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
import java.util.Optional;
import org.checkerframework.checker.nullness.qual.Nullable;
import org.checkerframework.checker.nullness.qual.PolyNull;
import org.checkerframework.checker.optional.qual.Present;

public class OptionalMapMethodReference {
Optional<String> getString() {
return Optional.of("");
}

@Present Optional<Integer> method() {
Optional<String> o = getString();
@Present Optional<Integer> oInt;
if (o.isPresent()) {
// :: error: (assignment)
oInt = o.map(this::convertNull);
oInt = o.map(this::convertPoly);
return o.map(this::convert);
}
return Optional.of(0);
}

@Nullable Integer convertNull(String s) {
smillst marked this conversation as resolved.
Show resolved Hide resolved
return null;
}

@PolyNull Integer convertPoly(@PolyNull String s) {
return null;
}

Integer convert(String s) {
return 0;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -1931,6 +1931,15 @@ public boolean isUnbound() {
return unbound;
}

/**
* Returns whether this kind is a constructor reference.
*
* @return whether this kind is a constructor reference
*/
public boolean isConstructorReference() {
return mode == ReferenceMode.NEW;
}

/**
* Returns the kind of member reference {@code tree} is.
*
Expand Down
Loading