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

[DO NOT LAND] @EnsuresNonNullIf benchmarking only #1046

Closed
wants to merge 53 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
53 commits
Select commit Hold shift + click to select a range
60c09cd
First draft of @EnsuresNonNullIf
mauricioaniche Sep 24, 2024
dfcfb04
Merge branch 'master' into ensures-non-null-if
mauricioaniche Sep 24, 2024
c674bf9
Improve validation of the semantics of the EnsuresNonNullIfHandler
mauricioaniche Sep 25, 2024
c511a79
More tests
mauricioaniche Sep 25, 2024
9eac416
Improve docs
mauricioaniche Sep 25, 2024
38d6d88
Validate post-conditions in child classes
mauricioaniche Sep 25, 2024
c2a5dc2
A few more tests
mauricioaniche Sep 25, 2024
17b9173
Tests cleanup
mauricioaniche Sep 25, 2024
e4ab9ba
Fix error message in FieldContractUtils
mauricioaniche Sep 25, 2024
9cbcf4e
Fix error message in FieldContractUtils again
mauricioaniche Sep 25, 2024
d044fae
Add trueIfNonNull to invert the result of the EnsuresNonNullIf method
mauricioaniche Sep 25, 2024
d700ee3
Throw exception instead of returning default value in exceptional case
mauricioaniche Sep 25, 2024
6842120
Rename variable
mauricioaniche Sep 25, 2024
df1293f
Fix nullability issues in my code
mauricioaniche Sep 25, 2024
1bf794c
One more test
mauricioaniche Sep 25, 2024
c471082
Move ignored tests to the bottom
mauricioaniche Sep 25, 2024
6ebac11
add TODO
msridhar Sep 26, 2024
3b27dae
Make use of internal state to get the enclosing method of a return tree
mauricioaniche Sep 27, 2024
4aa9ed0
Improvements here and there
mauricioaniche Sep 27, 2024
c0a70ac
New approach
mauricioaniche Sep 28, 2024
5f2ad31
Code review feedback
mauricioaniche Sep 28, 2024
3ad3944
Nit on javadoc
mauricioaniche Sep 28, 2024
3afb836
Nit in comments
mauricioaniche Sep 28, 2024
de81671
Put class back to original state to reduce changes in PR
mauricioaniche Sep 28, 2024
92b3a77
Got it working
mauricioaniche Sep 29, 2024
04a7d42
Cleaning up the doc and more docs
mauricioaniche Sep 29, 2024
ee32c48
Fix clean up
mauricioaniche Sep 29, 2024
81b1edc
More fixes
mauricioaniche Sep 29, 2024
fbd05a7
Rename trueIfNonNull to result and make it compulsory, to match Check…
mauricioaniche Sep 29, 2024
b1faf00
More cleanups
mauricioaniche Sep 29, 2024
0f71fde
Handle missing case where allFieldsInPathAreVerified is false and eva…
mauricioaniche Sep 29, 2024
880ae42
nit comment
mauricioaniche Sep 29, 2024
f478ac4
one more test
mauricioaniche Sep 29, 2024
d53958b
one more test
mauricioaniche Sep 29, 2024
13b0cca
FIX typo name of the test
mauricioaniche Sep 29, 2024
5d8157d
FIX typo name of the test
mauricioaniche Sep 29, 2024
2623bfa
one more test
mauricioaniche Sep 29, 2024
1b1967c
Fix nullaway issues in nullaway
mauricioaniche Sep 29, 2024
80ef2ca
JSpecify annotation
mauricioaniche Sep 30, 2024
5428602
small refactoring; fix compilation
mauricioaniche Sep 30, 2024
ca7a54a
Use elseStore when result is set to false
mauricioaniche Sep 30, 2024
bb139df
Set return default value to true again
mauricioaniche Sep 30, 2024
f23a8d4
Improve tests
mauricioaniche Sep 30, 2024
45907e1
test for error message when result=false
mauricioaniche Sep 30, 2024
7a3d075
Return true in case body is null just like existing EnsureNonNull han…
mauricioaniche Sep 30, 2024
3c7295e
simplify error message
mauricioaniche Sep 30, 2024
2bbbac2
Broken test
mauricioaniche Sep 30, 2024
ea10969
Fix issue when return=false and method returns boolean
mauricioaniche Sep 30, 2024
3cb7d51
Let's not use an Optional here. Since we check the code with NullAwa…
msridhar Sep 30, 2024
56a0984
No need to report when all paths are verified but return is wrong
mauricioaniche Sep 30, 2024
f98fb3c
minor comment tweak
msridhar Sep 30, 2024
fa7728e
simplify dataflow update logic
msridhar Sep 30, 2024
1c5610c
comments
msridhar Sep 30, 2024
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
@@ -0,0 +1,54 @@
package com.uber.nullaway.annotations;

import java.lang.annotation.ElementType;
import java.lang.annotation.Retention;
import java.lang.annotation.RetentionPolicy;
import java.lang.annotation.Target;

/**
* An annotation describing a nullability post-condition for an instance method. Each parameter to
* the annotation should be a field of the enclosing class. The method must ensure that the method
* returns true in case the fields are non-null. The method can also return false in case the fields
* are non-null (inverse logic), and in such case, you must set {@code result} to false. NullAway
* verifies that the property holds.
*
* <p>Here is an example:
*
* <pre>
* class Foo {
* {@literal @}Nullable Object theField;
*
* {@literal @}EnsuresNonNullIf("theField", result=true) // "this.theField" is also valid
* boolean hasTheField() {
* return theField != null;
* }
*
* void bar() {
* if(!hasTheField()) {
* return;
* }
*
* // No error, NullAway knows theField is non-null after call to hasTheField()
* theField.toString();
* }
* }
* </pre>
*/
@Retention(RetentionPolicy.CLASS)
@Target({ElementType.METHOD})
public @interface EnsuresNonNullIf {
/**
* The list of fields that are non-null after the method returns the given result.
*
* @return The list of field names
*/
String[] value();

/**
* The return value of the method under which the postcondition holds. The default is set to true,
* which means the method should return true in case fields are non-null.
*
* @return true or false
*/
boolean result() default true;
}
36 changes: 28 additions & 8 deletions nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java
Original file line number Diff line number Diff line change
Expand Up @@ -230,14 +230,7 @@ public static Stream<? extends AnnotationMirror> getAllAnnotations(Symbol symbol
*/
public static @Nullable Set<String> getAnnotationValueArray(
Symbol.MethodSymbol methodSymbol, String annotName, boolean exactMatch) {
AnnotationMirror annot = null;
for (AnnotationMirror annotationMirror : methodSymbol.getAnnotationMirrors()) {
String name = AnnotationUtils.annotationName(annotationMirror);
if ((exactMatch && name.equals(annotName)) || (!exactMatch && name.endsWith(annotName))) {
annot = annotationMirror;
break;
}
}
AnnotationMirror annot = findAnnotation(methodSymbol, annotName, exactMatch);
if (annot == null) {
return null;
}
Expand All @@ -255,6 +248,33 @@ public static Stream<? extends AnnotationMirror> getAllAnnotations(Symbol symbol
return null;
}

/**
* Retrieve the specific annotation of a method.
*
* @param methodSymbol A method to check for the annotation.
* @param annotName The qualified name or simple name of the annotation depending on the value of
* {@code exactMatch}.
* @param exactMatch If true, the annotation name must match the full qualified name given in
* {@code annotName}, otherwise, simple names will be checked.
* @return an {@code AnnotationMirror} representing that annotation, or null in case the
* annotation with a given name {@code annotName} doesn't exist in {@code methodSymbol}.
*/
public static @Nullable AnnotationMirror findAnnotation(
Symbol.MethodSymbol methodSymbol, String annotName, boolean exactMatch) {
AnnotationMirror annot = null;
for (AnnotationMirror annotationMirror : methodSymbol.getAnnotationMirrors()) {
String name = AnnotationUtils.annotationName(annotationMirror);
if ((exactMatch && name.equals(annotName)) || (!exactMatch && name.endsWith(annotName))) {
annot = annotationMirror;
break;
}
}
if (annot == null) {
return null;
}
return annot;
}

/**
* Works for method parameters defined either in source or in class files
*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,8 @@
import static com.uber.nullaway.NullabilityUtil.castToNonNull;

import com.google.common.base.Preconditions;
import com.google.common.collect.ImmutableList;
import com.google.errorprone.VisitorState;
import com.google.errorprone.dataflow.nullnesspropagation.NullnessAnalysis;
import com.sun.source.tree.Tree;
import com.sun.source.util.TreePath;
import com.sun.tools.javac.util.Context;
Expand Down Expand Up @@ -157,25 +157,7 @@ public Set<Element> getNonnullFieldsOfReceiverAtExit(TreePath path, Context cont
// be conservative and say nothing is initialized
return Collections.emptySet();
}
return getNonnullReceiverFields(nullnessResult);
}

private Set<Element> getNonnullReceiverFields(NullnessStore nullnessResult) {
Set<AccessPath> nonnullAccessPaths = nullnessResult.getAccessPathsWithValue(Nullness.NONNULL);
Set<Element> result = new LinkedHashSet<>();
for (AccessPath ap : nonnullAccessPaths) {
// A null root represents the receiver
if (ap.getRoot() == null) {
ImmutableList<AccessPathElement> elements = ap.getElements();
if (elements.size() == 1) {
Element elem = elements.get(0).getJavaElement();
if (elem.getKind().equals(ElementKind.FIELD)) {
result.add(elem);
}
}
}
}
return result;
return nullnessResult.getNonNullReceiverFields();
}

/**
Expand All @@ -190,7 +172,7 @@ public Set<Element> getNonnullFieldsOfReceiverBefore(TreePath path, Context cont
if (store == null) {
return Collections.emptySet();
}
return getNonnullReceiverFields(store);
return store.getNonNullReceiverFields();
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -852,7 +852,8 @@ public TransferResult<Nullness, NullnessStore> visitSuper(
@Override
public TransferResult<Nullness, NullnessStore> visitReturn(
ReturnNode returnNode, TransferInput<Nullness, NullnessStore> input) {
handler.onDataflowVisitReturn(returnNode.getTree(), input.getThenStore(), input.getElseStore());
handler.onDataflowVisitReturn(
returnNode.getTree(), state, input.getThenStore(), input.getElseStore());
return noStoreChanges(NULLABLE, input);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
import static com.google.common.base.Preconditions.checkNotNull;
import static com.google.common.collect.Sets.intersection;

import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.errorprone.VisitorState;
import com.uber.nullaway.Nullness;
Expand All @@ -30,6 +31,7 @@
import java.util.function.Predicate;
import java.util.stream.Collectors;
import javax.lang.model.element.Element;
import javax.lang.model.element.ElementKind;
import org.checkerframework.nullaway.dataflow.analysis.Store;
import org.checkerframework.nullaway.dataflow.cfg.node.FieldAccessNode;
import org.checkerframework.nullaway.dataflow.cfg.node.LocalVariableNode;
Expand Down Expand Up @@ -261,6 +263,39 @@ public NullnessStore filterAccessPaths(Predicate<AccessPath> pred) {
.collect(Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue)));
}

/**
* Return all the fields in the store that are Non-Null.
*
* @return Set of fields (represented as {@code Element}s) that are non-null
*/
public Set<Element> getNonNullReceiverFields() {
return getReceiverFields(Nullness.NONNULL);
}

/**
* Return all the fields in the store that hold the {@code nullness} state.
*
* @param nullness The {@code Nullness} state
* @return Set of fields (represented as {@code Element}s) with the given {@code nullness}.
*/
public Set<Element> getReceiverFields(Nullness nullness) {
Set<AccessPath> nonnullAccessPaths = this.getAccessPathsWithValue(nullness);
Set<Element> result = new LinkedHashSet<>();
for (AccessPath ap : nonnullAccessPaths) {
// A null root represents the receiver
if (ap.getRoot() == null) {
ImmutableList<AccessPathElement> elements = ap.getElements();
if (elements.size() == 1) {
Element elem = elements.get(0).getJavaElement();
if (elem.getKind().equals(ElementKind.FIELD)) {
result.add(elem);
}
}
}
}
return result;
}

/** class for building up instances of the store. */
public static final class Builder {
private final Map<AccessPath, Nullness> contents;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -174,7 +174,7 @@ public NullnessHint onDataflowVisitFieldAccess(

@Override
public void onDataflowVisitReturn(
ReturnTree tree, NullnessStore thenStore, NullnessStore elseStore) {
ReturnTree tree, VisitorState state, NullnessStore thenStore, NullnessStore elseStore) {
// NoOp
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -220,9 +220,9 @@ public NullnessHint onDataflowVisitFieldAccess(

@Override
public void onDataflowVisitReturn(
ReturnTree tree, NullnessStore thenStore, NullnessStore elseStore) {
ReturnTree tree, VisitorState state, NullnessStore thenStore, NullnessStore elseStore) {
for (Handler h : handlers) {
h.onDataflowVisitReturn(tree, thenStore, elseStore);
h.onDataflowVisitReturn(tree, state, thenStore, elseStore);
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -269,12 +269,14 @@ NullnessHint onDataflowVisitFieldAccess(
* Called when the Dataflow analysis visits a return statement.
*
* @param tree The AST node for the return statement being matched.
* @param state The current visitor state
* @param thenStore The NullnessStore for the true case of the expression inside the return
* statement.
* @param elseStore The NullnessStore for the false case of the expression inside the return
* statement.
*/
void onDataflowVisitReturn(ReturnTree tree, NullnessStore thenStore, NullnessStore elseStore);
void onDataflowVisitReturn(
ReturnTree tree, VisitorState state, NullnessStore thenStore, NullnessStore elseStore);

/**
* Called when the Dataflow analysis visits the result expression inside the body of lambda.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
import com.uber.nullaway.handlers.contract.ContractCheckHandler;
import com.uber.nullaway.handlers.contract.ContractHandler;
import com.uber.nullaway.handlers.contract.fieldcontract.EnsuresNonNullHandler;
import com.uber.nullaway.handlers.contract.fieldcontract.EnsuresNonNullIfHandler;
import com.uber.nullaway.handlers.contract.fieldcontract.RequiresNonNullHandler;
import com.uber.nullaway.handlers.temporary.FluentFutureHandler;

Expand Down Expand Up @@ -69,6 +70,7 @@ public static Handler buildDefault(Config config) {
handlerListBuilder.add(new GrpcHandler());
handlerListBuilder.add(new RequiresNonNullHandler());
handlerListBuilder.add(new EnsuresNonNullHandler());
handlerListBuilder.add(new EnsuresNonNullIfHandler());
handlerListBuilder.add(new SynchronousCallbackHandler());
if (config.serializationIsActive() && config.getSerializationConfig().fieldInitInfoEnabled) {
handlerListBuilder.add(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -595,7 +595,7 @@ public NullnessStore.Builder onDataflowInitialStore(

@Override
public void onDataflowVisitReturn(
ReturnTree tree, NullnessStore thenStore, NullnessStore elseStore) {
ReturnTree tree, VisitorState state, NullnessStore thenStore, NullnessStore elseStore) {
Tree filterTree = returnToEnclosingMethodOrLambda.get(tree);
if (filterTree != null) {
assert (filterTree instanceof MethodTree || filterTree instanceof LambdaExpressionTree);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,6 @@
import com.uber.nullaway.handlers.MethodAnalysisContext;
import com.uber.nullaway.handlers.contract.ContractUtils;
import java.util.Collections;
import java.util.Iterator;
import java.util.Set;
import java.util.stream.Collectors;
import javax.lang.model.element.VariableElement;
Expand Down Expand Up @@ -124,46 +123,8 @@ protected void validateOverridingRules(
VisitorState state,
MethodTree tree,
Symbol.MethodSymbol overriddenMethod) {
Set<String> overriddenFieldNames = getAnnotationValueArray(overriddenMethod, annotName, false);
if (overriddenFieldNames == null) {
return;
}
if (overridingFieldNames == null) {
overridingFieldNames = Collections.emptySet();
}
if (overridingFieldNames.containsAll(overriddenFieldNames)) {
return;
}
overriddenFieldNames.removeAll(overridingFieldNames);

StringBuilder errorMessage = new StringBuilder();
errorMessage
.append(
"postcondition inheritance is violated, this method must guarantee that all fields written in the @EnsuresNonNull annotation of overridden method ")
.append(castToNonNull(ASTHelpers.enclosingClass(overriddenMethod)).getSimpleName())
.append(".")
.append(overriddenMethod.getSimpleName())
.append(" are @NonNull at exit point as well. Fields [");
Iterator<String> iterator = overriddenFieldNames.iterator();
while (iterator.hasNext()) {
errorMessage.append(iterator.next());
if (iterator.hasNext()) {
errorMessage.append(", ");
}
}
errorMessage.append(
"] must explicitly appear as parameters at this method @EnsuresNonNull annotation");
state.reportMatch(
analysis
.getErrorBuilder()
.createErrorDescription(
new ErrorMessage(
ErrorMessage.MessageTypes.WRONG_OVERRIDE_POSTCONDITION,
errorMessage.toString()),
tree,
analysis.buildDescription(tree),
state,
null));
FieldContractUtils.ensureStrictPostConditionInheritance(
annotName, overridingFieldNames, analysis, state, tree, overriddenMethod);
}

/**
Expand Down
Loading
Loading