-
Notifications
You must be signed in to change notification settings - Fork 29
Add @IfNullThrows parameter annotation and unify with AssertMethod in CFG #1566
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
base: master
Are you sure you want to change the base?
Changes from 1 commit
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,35 @@ | ||
| package org.checkerframework.dataflow.qual; | ||
|
|
||
| import java.lang.annotation.Documented; | ||
| import java.lang.annotation.ElementType; | ||
| import java.lang.annotation.Retention; | ||
| import java.lang.annotation.RetentionPolicy; | ||
| import java.lang.annotation.Target; | ||
|
|
||
| /** | ||
| * A parameter annotation indicating that the method throws an exception if this parameter is null. | ||
| * | ||
| * <p>When the CFG is built, calls to methods with {@code @IfNullThrows} on a parameter are | ||
| * translated into an explicit branch: if the argument is null, the method throws; otherwise | ||
| * execution continues. This enables flow-sensitive refinement in type checkers (e.g., the Nullness | ||
| * Checker refines the argument to non-null on the continue path). | ||
| * | ||
| * <p><b>Semantic meaning:</b> {@code @IfNullThrows} means: <i>if this parameter is null, then the | ||
| * method throws</i>. Equivalently: when the method returns normally, the parameter was non-null. | ||
| * | ||
| * <p><b>Example:</b> | ||
| * | ||
| * <pre><code> | ||
| * public static <T> T requireNonNull(@IfNullThrows @Nullable T obj) { | ||
| * if (obj == null) throw new NullPointerException(); | ||
| * return obj; | ||
| * } | ||
| * </code></pre> | ||
| * | ||
| * @checker_framework.manual #type-refinement Automatic type refinement (flow-sensitive type | ||
| * qualifier inference) | ||
| */ | ||
| @Documented | ||
| @Retention(RetentionPolicy.RUNTIME) | ||
| @Target(ElementType.PARAMETER) | ||
| public @interface IfNullThrows {} |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,48 @@ | ||
| // Test that @IfNullThrows (parameter-level postcondition: if null then throw) is respected by the | ||
| // Nullness Checker. | ||
|
|
||
| import org.checkerframework.checker.nullness.qual.Nullable; | ||
| import org.checkerframework.dataflow.qual.IfNullThrows; | ||
|
|
||
| public class IfNullThrowsTest { | ||
|
|
||
| // requireNonNull-style: if param is null, throws; so when returns, param is non-null | ||
| public static <T> T requireNonNull(@IfNullThrows @Nullable T obj) { | ||
| if (obj == null) { | ||
| throw new NullPointerException(); | ||
| } | ||
| return obj; | ||
| } | ||
|
|
||
| void useRequireNonNull(@Nullable String s) { | ||
| requireNonNull(s); | ||
| s.toString(); // OK: s refined to non-null after requireNonNull returns | ||
| } | ||
|
|
||
| // With message parameter | ||
| public static <T> T requireNonNull(@IfNullThrows @Nullable T obj, String msg) { | ||
| if (obj == null) { | ||
| throw new NullPointerException(msg); | ||
| } | ||
| return obj; | ||
| } | ||
|
|
||
| void useRequireNonNullWithMsg(@Nullable String s) { | ||
| requireNonNull(s, "s must not be null"); | ||
| s.toString(); // OK | ||
| } | ||
|
|
||
| // Multiple parameters with @IfNullThrows - validates both must be non-null to return | ||
| public static void requireBothNonNull( | ||
| @IfNullThrows @Nullable Object a, @IfNullThrows @Nullable Object b) { | ||
| if (a == null || b == null) { | ||
| throw new NullPointerException(); | ||
| } | ||
| } | ||
|
|
||
| void useRequireBothNonNull(@Nullable Object x, @Nullable Object y) { | ||
| requireBothNonNull(x, y); | ||
| x.toString(); // OK | ||
| y.toString(); // OK | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -58,6 +58,7 @@ | |
| import com.sun.source.util.TreeScanner; | ||
| import com.sun.source.util.Trees; | ||
| import com.sun.tools.javac.code.Type; | ||
| import com.sun.tools.javac.code.TypeTag; | ||
|
|
||
| import org.checkerframework.checker.interning.qual.FindDistinct; | ||
| import org.checkerframework.checker.nullness.qual.Nullable; | ||
|
|
@@ -139,6 +140,7 @@ | |
| import org.checkerframework.dataflow.cfg.node.VariableDeclarationNode; | ||
| import org.checkerframework.dataflow.cfg.node.WideningConversionNode; | ||
| import org.checkerframework.dataflow.qual.AssertMethod; | ||
| import org.checkerframework.dataflow.qual.IfNullThrows; | ||
| import org.checkerframework.dataflow.qual.TerminatesExecution; | ||
| import org.checkerframework.javacutil.AnnotationProvider; | ||
| import org.checkerframework.javacutil.AnnotationUtils; | ||
|
|
@@ -1368,6 +1370,7 @@ protected List<Node> convertCallArguments( | |
|
|
||
| ArrayList<Node> convertedNodes = new ArrayList<>(numFormals); | ||
| AssertMethodTuple assertMethodTuple = getAssertMethodTuple(executable); | ||
| Set<Integer> ifNullThrowsParams = getIfNullThrowsParameterIndices(executable); | ||
|
Member
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. Think whether you can generalize and merge this with AssertMethodTuple. Instead of just a boolean condition, you'll have a parameter that you want to compare and a value you want to compare against. AssertMethod is on the whole method declaration and needs to carry which parameter has the effect. The per-parameter annotations don't need that and get the parameter from where the annotation is. That can be unified into one common datastructure. For AssertMethod, the parameter in the condition is a boolean. For IfNullThrows it's a reference type. Does this difference matter for the CFG you build? Or does it only matter whether you compare against true, false, or null? |
||
|
|
||
| int numActuals = actualExprs.size(); | ||
| if (executable.isVarArgs()) { | ||
|
|
@@ -1386,6 +1389,9 @@ protected List<Node> convertCallArguments( | |
| treatMethodAsAssert( | ||
| (MethodInvocationTree) tree, assertMethodTuple, actualVal); | ||
| } | ||
| if (ifNullThrowsParams.contains(i)) { | ||
| treatMethodAsIfNullThrows((MethodInvocationTree) tree, actualVal); | ||
|
Member
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. First perform the
Member
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. Probably would have made sense for the treatMethodAsAssert already... Think whether this change would make sense for both. |
||
| } | ||
| if (actualVal == null) { | ||
| throw new BugInCF( | ||
| "CFGBuilder: scan returned null for %s [%s]", | ||
|
|
@@ -1421,6 +1427,9 @@ protected List<Node> convertCallArguments( | |
| treatMethodAsAssert( | ||
| (MethodInvocationTree) tree, assertMethodTuple, actualVal); | ||
| } | ||
| if (ifNullThrowsParams.contains(i)) { | ||
| treatMethodAsIfNullThrows((MethodInvocationTree) tree, actualVal); | ||
| } | ||
| convertedNodes.add(methodInvocationConvert(actualVal, formals.get(i))); | ||
| } | ||
|
|
||
|
|
@@ -1453,6 +1462,9 @@ protected List<Node> convertCallArguments( | |
| if (i == assertMethodTuple.booleanParam) { | ||
| treatMethodAsAssert((MethodInvocationTree) tree, assertMethodTuple, actualVal); | ||
| } | ||
| if (ifNullThrowsParams.contains(i)) { | ||
| treatMethodAsIfNullThrows((MethodInvocationTree) tree, actualVal); | ||
| } | ||
| convertedNodes.add(methodInvocationConvert(actualVal, formals.get(i))); | ||
| } | ||
| } | ||
|
|
@@ -1494,6 +1506,67 @@ protected AssertMethodTuple getAssertMethodTuple(ExecutableElement method) { | |
| return new AssertMethodTuple(booleanParam, exceptionType, isAssertFalse); | ||
| } | ||
|
|
||
| /** | ||
| * Returns the 0-based indices of parameters annotated with {@link IfNullThrows}. Such | ||
| * parameters cause the method to throw when null; the CFG is modified to add an explicit | ||
| * branch. | ||
| * | ||
| * @param method the method or constructor | ||
| * @return indices of parameters with {@code @IfNullThrows} | ||
| */ | ||
| protected Set<Integer> getIfNullThrowsParameterIndices(ExecutableElement method) { | ||
| Set<Integer> result = new HashSet<>(); | ||
| List<? extends VariableElement> params = method.getParameters(); | ||
| for (int i = 0; i < params.size(); i++) { | ||
| if (annotationProvider.getDeclAnnotation(params.get(i), IfNullThrows.class) != null) { | ||
| result.add(i); | ||
| } | ||
| } | ||
| return result; | ||
| } | ||
|
|
||
| /** | ||
| * Translates a method call with {@link IfNullThrows} on a parameter into CFG nodes: if the | ||
| * argument is null, the method throws; otherwise execution continues. | ||
| * | ||
| * @param tree the method invocation tree | ||
| * @param argNode the node for the argument (the parameter value) | ||
| */ | ||
| protected void treatMethodAsIfNullThrows(MethodInvocationTree tree, Node argNode) { | ||
| // Create (arg == null) condition | ||
| TypeMirror booleanType = types.getPrimitiveType(TypeKind.BOOLEAN); | ||
| LiteralTree nullTree = TreeUtils.createLiteral(TypeTag.BOT, null, types.getNullType(), env); | ||
| handleArtificialTree(nullTree); | ||
| Node nullNode = new NullLiteralNode(nullTree); | ||
| extendWithNode(nullNode); | ||
|
|
||
| ExpressionTree argTree = (ExpressionTree) argNode.getTree(); | ||
| BinaryTree eqTree = | ||
| treeBuilder.buildBinary(booleanType, Tree.Kind.EQUAL_TO, argTree, nullTree); | ||
| handleArtificialTree(eqTree); | ||
|
|
||
| Node condition = new EqualToNode(eqTree, argNode, nullNode); | ||
| extendWithNode(condition); | ||
|
|
||
| // When (arg == null) is true, throw; else continue | ||
| Label throwLabel = new Label(); | ||
| Label continueLabel = new Label(); | ||
| ConditionalJump cjump = new ConditionalJump(throwLabel, continueLabel); | ||
| extendWithExtendedNode(cjump); | ||
|
|
||
| addLabelForNextNode(throwLabel); | ||
| AssertionErrorNode assertNode = | ||
|
Member
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. Does Assertion error code make sense for our use? |
||
| new AssertionErrorNode(tree, condition, null, nullPointerExceptionType); | ||
| extendWithNode(assertNode); | ||
| NodeWithExceptionsHolder exNode = | ||
| extendWithNodeWithException( | ||
| new ThrowNode(null, assertNode, env.getTypeUtils()), | ||
| nullPointerExceptionType); | ||
| exNode.setTerminatesExecution(true); | ||
|
|
||
| addLabelForNextNode(continueLabel); | ||
| } | ||
|
|
||
| /** Holds the elements of an {@link AssertMethod} annotation. */ | ||
| protected static class AssertMethodTuple { | ||
|
|
||
|
|
||
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.
Also in these tests, ensure that the qualifier is actually enforced - add an incorrect implementation and ensure we raise an error.