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

Model Lombok-generated equals methods as having a @Nullable parameter #874

Merged
merged 4 commits into from
Dec 6, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
7 changes: 2 additions & 5 deletions nullaway/src/main/java/com/uber/nullaway/NullAway.java
Original file line number Diff line number Diff line change
Expand Up @@ -760,10 +760,7 @@ private Description checkParamOverriding(
// Check handlers for any further/overriding nullness information
overriddenMethodArgNullnessMap =
handler.onOverrideMethodInvocationParametersNullability(
state.context,
overriddenMethod,
isOverriddenMethodAnnotated,
overriddenMethodArgNullnessMap);
state, overriddenMethod, isOverriddenMethodAnnotated, overriddenMethodArgNullnessMap);

// If we have an unbound method reference, the first parameter of the overridden method must be
// @NonNull, as this parameter will be used as a method receiver inside the generated lambda.
Expand Down Expand Up @@ -1714,7 +1711,7 @@ private Description handleInvocation(
// Allow handlers to override the list of non-null argument positions
argumentPositionNullness =
handler.onOverrideMethodInvocationParametersNullability(
state.context, methodSymbol, isMethodAnnotated, argumentPositionNullness);
state, methodSymbol, isMethodAnnotated, argumentPositionNullness);

// now actually check the arguments
// NOTE: the case of an invocation on a possibly-null reference
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -206,7 +206,7 @@ private static Node unwrapAssignExpr(Node node) {
public NullnessStore initialStore(
UnderlyingAST underlyingAST, List<LocalVariableNode> parameters) {
return nullnessStoreInitializer.getInitialStore(
underlyingAST, parameters, handler, state.context, state.getTypes(), config);
underlyingAST, parameters, handler, state, config);
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
import static com.uber.nullaway.Nullness.NONNULL;
import static com.uber.nullaway.Nullness.NULLABLE;

import com.google.errorprone.VisitorState;
import com.google.errorprone.util.ASTHelpers;
import com.sun.source.tree.ClassTree;
import com.sun.source.tree.LambdaExpressionTree;
Expand Down Expand Up @@ -30,9 +31,9 @@ public NullnessStore getInitialStore(
UnderlyingAST underlyingAST,
List<LocalVariableNode> parameters,
Handler handler,
Context context,
Types types,
VisitorState state,
Config config) {
Context context = state.context;
if (underlyingAST.getKind().equals(UnderlyingAST.Kind.ARBITRARY_CODE)) {
// not a method or a lambda; an initializer expression or block
UnderlyingAST.CFGStatement ast = (UnderlyingAST.CFGStatement) underlyingAST;
Expand All @@ -44,8 +45,7 @@ public NullnessStore getInitialStore(
(UnderlyingAST.CFGLambda) underlyingAST,
parameters,
handler,
context,
types,
state,
config,
getCodeAnnotationInfo(context));
} else {
Expand Down Expand Up @@ -77,20 +77,20 @@ private static NullnessStore lambdaInitialStore(
UnderlyingAST.CFGLambda underlyingAST,
List<LocalVariableNode> parameters,
Handler handler,
Context context,
Types types,
VisitorState state,
Config config,
CodeAnnotationInfo codeAnnotationInfo) {
// include nullness info for locals from enclosing environment
EnclosingEnvironmentNullness environmentNullness =
EnclosingEnvironmentNullness.instance(context);
EnclosingEnvironmentNullness.instance(state.context);
NullnessStore environmentMapping =
Objects.requireNonNull(
environmentNullness.getEnvironmentMapping(underlyingAST.getLambdaTree()),
"no environment stored for lambda");
NullnessStore.Builder result = environmentMapping.toBuilder();
LambdaExpressionTree code = underlyingAST.getLambdaTree();
// need to check annotation for i'th parameter of functional interface declaration
Types types = state.getTypes();
Symbol.MethodSymbol fiMethodSymbol = NullabilityUtil.getFunctionalInterfaceMethod(code, types);
com.sun.tools.javac.util.List<Symbol.VarSymbol> fiMethodParameters =
fiMethodSymbol.getParameters();
Expand Down Expand Up @@ -119,7 +119,7 @@ private static NullnessStore lambdaInitialStore(
}
fiArgumentPositionNullness =
handler.onOverrideMethodInvocationParametersNullability(
context, fiMethodSymbol, isFIAnnotated, fiArgumentPositionNullness);
state, fiMethodSymbol, isFIAnnotated, fiArgumentPositionNullness);

for (int i = 0; i < parameters.size(); i++) {
LocalVariableNode param = parameters.get(i);
Expand Down
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
package com.uber.nullaway.dataflow;

import com.google.errorprone.VisitorState;
import com.google.errorprone.util.ASTHelpers;
import com.sun.source.tree.ClassTree;
import com.sun.source.util.Trees;
import com.sun.tools.javac.code.Symbol;
import com.sun.tools.javac.code.Types;
import com.sun.tools.javac.processing.JavacProcessingEnvironment;
import com.sun.tools.javac.util.Context;
import com.uber.nullaway.Config;
Expand All @@ -30,17 +30,15 @@ public abstract class NullnessStoreInitializer {
* @param underlyingAST The AST node being matched.
* @param parameters list of local variable nodes.
* @param handler reference to the handler invoked.
* @param context context.
* @param types types.
* @param state the visitor state.
* @param config config for analysis.
* @return Initial Nullness store.
*/
public abstract NullnessStore getInitialStore(
UnderlyingAST underlyingAST,
List<LocalVariableNode> parameters,
Handler handler,
Context context,
Types types,
VisitorState state,
Config config);

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,7 @@ public Nullness onOverrideMethodReturnNullability(

@Override
public Nullness[] onOverrideMethodInvocationParametersNullability(
Context context,
VisitorState state,
Symbol.MethodSymbol methodSymbol,
boolean isAnnotated,
Nullness[] argumentPositionNullness) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -137,14 +137,14 @@ public Nullness onOverrideMethodReturnNullability(

@Override
public Nullness[] onOverrideMethodInvocationParametersNullability(
Context context,
VisitorState state,
Symbol.MethodSymbol methodSymbol,
boolean isAnnotated,
Nullness[] argumentPositionNullness) {
for (Handler h : handlers) {
argumentPositionNullness =
h.onOverrideMethodInvocationParametersNullability(
context, methodSymbol, isAnnotated, argumentPositionNullness);
state, methodSymbol, isAnnotated, argumentPositionNullness);
}
return argumentPositionNullness;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -182,7 +182,7 @@ Nullness onOverrideMethodReturnNullability(
* considered isAnnotated or not. We use a mutable map for performance, but it should not outlive
* the chain of handler invocations.
*
* @param context The current context.
* @param state The current visitor state.
* @param methodSymbol The method symbol for the method in question.
* @param isAnnotated A boolean flag indicating whether the called method is considered to be
* within isAnnotated or unannotated code, used to avoid querying for this information
Expand All @@ -195,7 +195,7 @@ Nullness onOverrideMethodReturnNullability(
* handler.
*/
Nullness[] onOverrideMethodInvocationParametersNullability(
Context context,
VisitorState state,
Symbol.MethodSymbol methodSymbol,
boolean isAnnotated,
Nullness[] argumentPositionNullness);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@
import com.sun.source.tree.Tree;
import com.sun.tools.javac.code.Symbol;
import com.sun.tools.javac.code.Type;
import com.sun.tools.javac.util.Context;
import com.uber.nullaway.Config;
import com.uber.nullaway.NullAway;
import com.uber.nullaway.Nullness;
Expand Down Expand Up @@ -122,7 +121,7 @@ private void loadStubxFiles() {

@Override
public Nullness[] onOverrideMethodInvocationParametersNullability(
Context context,
VisitorState state,
Symbol.MethodSymbol methodSymbol,
boolean isAnnotated,
Nullness[] argumentPositionNullness) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -82,11 +82,11 @@ public LibraryModelsHandler(Config config) {

@Override
public Nullness[] onOverrideMethodInvocationParametersNullability(
Context context,
VisitorState state,
Symbol.MethodSymbol methodSymbol,
boolean isAnnotated,
Nullness[] argumentPositionNullness) {
OptimizedLibraryModels optimizedLibraryModels = getOptLibraryModels(context);
OptimizedLibraryModels optimizedLibraryModels = getOptLibraryModels(state.context);
ImmutableSet<Integer> nullableParamsFromModel =
optimizedLibraryModels.explicitlyNullableParameters(methodSymbol);
ImmutableSet<Integer> nonNullParamsFromModel =
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -86,4 +86,26 @@
}
return returnNullness;
}

/**
* Mark the first argument of Lombok-generated {@code equals} methods as {@code @Nullable}, since
* Lombok does not generate the annotation.
*/
@Override
public Nullness[] onOverrideMethodInvocationParametersNullability(
VisitorState state,
Symbol.MethodSymbol methodSymbol,
boolean isAnnotated,
Nullness[] argumentPositionNullness) {
if (ASTHelpers.hasAnnotation(methodSymbol, LOMBOK_GENERATED_ANNOTATION_NAME, state)) {
// We assume that Lombok-generated equals methods with a single argument override
// Object.equals and are not an overload.
if (methodSymbol.getSimpleName().contentEquals("equals")
&& methodSymbol.params().size() == 1) {
// The parameter is not annotated with @Nullable, but it should be.
argumentPositionNullness[0] = Nullness.NULLABLE;

Check warning on line 106 in nullaway/src/main/java/com/uber/nullaway/handlers/LombokHandler.java

View check run for this annotation

Codecov / codecov/patch

nullaway/src/main/java/com/uber/nullaway/handlers/LombokHandler.java#L106

Added line #L106 was not covered by tests
}
}
return argumentPositionNullness;
}
Comment on lines +89 to +110
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the only real logic change

}
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ private CodeAnnotationInfo getCodeAnnotationInfo(Context context) {

@Override
public Nullness[] onOverrideMethodInvocationParametersNullability(
Context context,
VisitorState state,
Symbol.MethodSymbol methodSymbol,
boolean isAnnotated,
Nullness[] argumentPositionNullness) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,11 @@
import static com.uber.nullaway.Nullness.NONNULL;
import static com.uber.nullaway.Nullness.NULLABLE;

import com.google.errorprone.VisitorState;
import com.google.errorprone.util.ASTHelpers;
import com.sun.source.tree.ClassTree;
import com.sun.source.tree.MethodTree;
import com.sun.tools.javac.code.Symbol;
import com.sun.tools.javac.code.Types;
import com.sun.tools.javac.util.Context;
import com.uber.nullaway.Config;
import com.uber.nullaway.Nullness;
import com.uber.nullaway.dataflow.AccessPath;
Expand All @@ -31,8 +30,7 @@ public NullnessStore getInitialStore(
UnderlyingAST underlyingAST,
List<LocalVariableNode> parameters,
Handler handler,
Context context,
Types types,
VisitorState state,
Config config) {
assert underlyingAST.getKind() == UnderlyingAST.Kind.METHOD;

Expand All @@ -49,7 +47,7 @@ public NullnessStore getInitialStore(
String[] parts = clauses[0].split("->");
String[] antecedent = parts[0].split(",");

NullnessStore envStore = getEnvNullnessStoreForClass(classTree, context);
NullnessStore envStore = getEnvNullnessStoreForClass(classTree, state.context);
NullnessStore.Builder result = envStore.toBuilder();

for (int i = 0; i < antecedent.length; ++i) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,4 +38,9 @@ public static String foo(LombokDTO ldto) {
s += (ldto.getNullableField() == null ? "" : ldto.getNullableField().toString());
return s;
}

public static boolean callEquals(LombokDTO ldto, @Nullable Object o) {
// No error should be reported since equals() parameter should be treated as @Nullable
return ldto.equals(o);
}
}