Add @IfNullThrows parameter annotation and unify with AssertMethod in CFG#1566
Add @IfNullThrows parameter annotation and unify with AssertMethod in CFG#1566HenryXi1 wants to merge 3 commits intoeisop:masterfrom
Conversation
| @Documented | ||
| @Retention(RetentionPolicy.RUNTIME) | ||
| @Target(ElementType.PARAMETER) | ||
| public @interface IfNullThrows {} |
There was a problem hiding this comment.
This qualifier should be in checker/.../nullness, not in the framework.
|
|
||
| // 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) { |
There was a problem hiding this comment.
Also in these tests, ensure that the qualifier is actually enforced - add an incorrect implementation and ensure we raise an error.
| (MethodInvocationTree) tree, assertMethodTuple, actualVal); | ||
| } | ||
| if (ifNullThrowsParams.contains(i)) { | ||
| treatMethodAsIfNullThrows((MethodInvocationTree) tree, actualVal); |
There was a problem hiding this comment.
First perform the actualVal == null check from the line below, before using the value here.
There was a problem hiding this comment.
Probably would have made sense for the treatMethodAsAssert already... Think whether this change would make sense for both.
| extendWithExtendedNode(cjump); | ||
|
|
||
| addLabelForNextNode(throwLabel); | ||
| AssertionErrorNode assertNode = |
There was a problem hiding this comment.
Does Assertion error code make sense for our use?
At the beginning, can you write down what the transformation is? Are you adding an if/else or are you adding an "assert"?
|
|
||
| ArrayList<Node> convertedNodes = new ArrayList<>(numFormals); | ||
| AssertMethodTuple assertMethodTuple = getAssertMethodTuple(executable); | ||
| Set<Integer> ifNullThrowsParams = getIfNullThrowsParameterIndices(executable); |
There was a problem hiding this comment.
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?
…ness, Ensure qualifer is enforced
edfd922 to
1c4a676
Compare
|
Azure Pipelines: 1 pipeline(s) were filtered out due to trigger conditions. |
Goal
Support expressing "if this parameter is null, the method throws" by annotating the parameter with
@IfNullThrows(e.g.requireNonNull(@IfNullThrows @Nullable T obj)), so the Nullness Checker can refine the argument to non-null on the normal-return path. This branch also unifies CFG handling of parameter-triggered throws with@AssertMethodvia a single spec and shared emission logic.Changes
checker-qual
@IfNullThrowsfromorg.checkerframework.dataflow.qualtoorg.checkerframework.checker.nullness.qual.dataflow/qual/IfNullThrows.java; addedchecker/nullness/qual/IfNullThrows.java(same semantics and Javadoc).dataflow – CFG translation
CompareValueenum (TRUE, FALSE, NULL) andParameterConditionalThrowSpec(parameterIndex, compareValue, exceptionType). One spec list for both@AssertMethodand@IfNullThrows.getParameterConditionalThrowSpecs(executable): One spec from@AssertMethod(if present) plus one per parameter with@IfNullThrows; returns a single list.buildConditionNodeForParameterThrow(NULL →(arg == null), TRUE/FALSE → argument node) thenemitConditionalThrow(branch + AssertionErrorNode + ThrowNode + continue label).AssertMethodTuple,getAssertMethodTuple,getIfNullThrowsParameterIndices,treatMethodAsAssert,treatMethodAsIfNullThrows.actualVal == nullcheck runs before any conditional-throw logic.checker – Nullness
checkIfNullThrowsEnforced(tree)runs fromprocessMethodTree. If the method has any@IfNullThrowsparameter and a body, the body must contain athrow; otherwise reportif.null.throws.must.throw.if.null.throws.must.throwin nullnessmessages.properties.org.checkerframework.checker.nullness.qual.IfNullThrows.Caching hook discovery
Tests
badNoThrowandbadJustReturn(nothrowin body) with expected// :: error: (if.null.throws.must.throw). Existing tests still check refinement afterrequireNonNull/requireBothNonNull.