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

Support @EnsuresNonNullIf #1044

Merged
merged 53 commits into from
Sep 30, 2024
Merged
Show file tree
Hide file tree
Changes from 16 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,40 @@
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 true in case the fields
* are null (inverse logic), and in such case, you must set trueIfNonNull to false. NullAway
* verifies that the property holds.
*
* <p>Here is an example:
*
* <pre>
* class Foo {
* {@literal @}Nullable Object theField;
* {@literal @}EnsuresNonNullIf("theField") // @EnsuresNonNullIf("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 {
String[] value();
mauricioaniche marked this conversation as resolved.
Show resolved Hide resolved

boolean trueIfNonNull() default true;
mauricioaniche marked this conversation as resolved.
Show resolved Hide resolved
}
25 changes: 17 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,22 @@ public static Stream<? extends AnnotationMirror> getAllAnnotations(Symbol symbol
return null;
}

public static @Nullable AnnotationMirror findAnnotation(
msridhar marked this conversation as resolved.
Show resolved Hide resolved
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 @@ -53,6 +53,11 @@ public abstract class AbstractFieldContractHandler extends BaseNoOpHandler {
/** Simple name of the annotation in {@code String} */
protected final String annotName;

// Set to true in case we are visiting a method with the annotation under analysis
protected boolean visitingAnnotatedMethod;
// Points to the method symbol that's currently being visited
protected Symbol.MethodSymbol visitingMethodSymbol;

mauricioaniche marked this conversation as resolved.
Show resolved Hide resolved
protected AbstractFieldContractHandler(String annotName) {
this.annotName = annotName;
}
Expand All @@ -65,12 +70,13 @@ protected AbstractFieldContractHandler(String annotName) {
*/
@Override
public void onMatchMethod(MethodTree tree, MethodAnalysisContext methodAnalysisContext) {

Symbol.MethodSymbol methodSymbol = methodAnalysisContext.methodSymbol();
VisitorState state = methodAnalysisContext.state();
Set<String> annotationContent =
NullabilityUtil.getAnnotationValueArray(methodSymbol, annotName, false);
boolean isAnnotated = annotationContent != null;
this.visitingAnnotatedMethod = isAnnotated;
this.visitingMethodSymbol = methodSymbol;
boolean isValid =
isAnnotated
&& validateAnnotationSyntax(
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 @@ -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.validateOverridingRules(
annotName, overridingFieldNames, analysis, state, tree, overriddenMethod);
}

/**
Expand Down
Loading
Loading