-
Notifications
You must be signed in to change notification settings - Fork 356
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
Changes from 11 commits
893d7e4
3cf9b7e
5494cc8
04e4b3e
6a5e789
f812779
43d4094
1a2733b
cb42e22
8789e98
137fb52
8e5b989
d39a021
b9385f4
8904ce1
55afb39
122dda1
f82405c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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) { | ||
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 |
---|---|---|
|
@@ -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. | ||
|
@@ -14,6 +15,7 @@ | |
// @NonNull, make the return type have type @Present. | ||
@RelevantJavaTypes(Optional.class) | ||
@StubFiles({"javaparser.astub"}) | ||
@SupportedOptions("optionalMapAssumeNonNull") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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() {} | ||
|
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(); | ||
} |
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; | ||
} | ||
} |
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.
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!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 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.I renamed to the method to make it clear that it is only checking from the lack of a Nullable annotation.
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! 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.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.
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:
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.
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.