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

Revert "Model Lombok-generated equals methods as having a @Nullable p… #886

Merged
merged 1 commit into from
Dec 27, 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: 5 additions & 2 deletions nullaway/src/main/java/com/uber/nullaway/NullAway.java
Original file line number Diff line number Diff line change
Expand Up @@ -761,7 +761,10 @@ private Description checkParamOverriding(
// Check handlers for any further/overriding nullness information
overriddenMethodArgNullnessMap =
handler.onOverrideMethodInvocationParametersNullability(
state, overriddenMethod, isOverriddenMethodAnnotated, overriddenMethodArgNullnessMap);
state.context,
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 @@ -1712,7 +1715,7 @@ private Description handleInvocation(
// Allow handlers to override the list of non-null argument positions
argumentPositionNullness =
handler.onOverrideMethodInvocationParametersNullability(
state, methodSymbol, isMethodAnnotated, argumentPositionNullness);
state.context, 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, config);
underlyingAST, parameters, handler, state.context, state.getTypes(), config);
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
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 @@ -31,9 +30,9 @@ public NullnessStore getInitialStore(
UnderlyingAST underlyingAST,
List<LocalVariableNode> parameters,
Handler handler,
VisitorState state,
Context context,
Types types,
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 @@ -45,7 +44,8 @@ public NullnessStore getInitialStore(
(UnderlyingAST.CFGLambda) underlyingAST,
parameters,
handler,
state,
context,
types,
config,
getCodeAnnotationInfo(context));
} else {
Expand Down Expand Up @@ -77,20 +77,20 @@ private static NullnessStore lambdaInitialStore(
UnderlyingAST.CFGLambda underlyingAST,
List<LocalVariableNode> parameters,
Handler handler,
VisitorState state,
Context context,
Types types,
Config config,
CodeAnnotationInfo codeAnnotationInfo) {
// include nullness info for locals from enclosing environment
EnclosingEnvironmentNullness environmentNullness =
EnclosingEnvironmentNullness.instance(state.context);
EnclosingEnvironmentNullness.instance(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(
state, fiMethodSymbol, isFIAnnotated, fiArgumentPositionNullness);
context, 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,15 +30,17 @@ 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 state the visitor state.
* @param context context.
* @param types types.
* @param config config for analysis.
* @return Initial Nullness store.
*/
public abstract NullnessStore getInitialStore(
UnderlyingAST underlyingAST,
List<LocalVariableNode> parameters,
Handler handler,
VisitorState state,
Context context,
Types types,
Config config);

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,7 @@ public boolean onOverrideFieldNullability(Symbol field) {

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

@Override
public Nullness[] onOverrideMethodInvocationParametersNullability(
VisitorState state,
Context context,
Symbol.MethodSymbol methodSymbol,
boolean isAnnotated,
Nullness[] argumentPositionNullness) {
for (Handler h : handlers) {
argumentPositionNullness =
h.onOverrideMethodInvocationParametersNullability(
state, methodSymbol, isAnnotated, argumentPositionNullness);
context, methodSymbol, isAnnotated, argumentPositionNullness);
}
return argumentPositionNullness;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -192,7 +192,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 state The current visitor state.
* @param context The current context.
* @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 @@ -205,7 +205,7 @@ Nullness onOverrideMethodReturnNullability(
* handler.
*/
Nullness[] onOverrideMethodInvocationParametersNullability(
VisitorState state,
Context context,
Symbol.MethodSymbol methodSymbol,
boolean isAnnotated,
Nullness[] argumentPositionNullness);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
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 @@ -121,7 +122,7 @@ private void loadStubxFiles() {

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

@Override
public Nullness[] onOverrideMethodInvocationParametersNullability(
VisitorState state,
Context context,
Symbol.MethodSymbol methodSymbol,
boolean isAnnotated,
Nullness[] argumentPositionNullness) {
OptimizedLibraryModels optimizedLibraryModels = getOptLibraryModels(state.context);
OptimizedLibraryModels optimizedLibraryModels = getOptLibraryModels(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,26 +86,4 @@ public Nullness onOverrideMethodReturnNullability(
}
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;
}
}
return argumentPositionNullness;
}
}
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(
VisitorState state,
Context context,
Symbol.MethodSymbol methodSymbol,
boolean isAnnotated,
Nullness[] argumentPositionNullness) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,12 @@
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 @@ -30,7 +31,8 @@ public NullnessStore getInitialStore(
UnderlyingAST underlyingAST,
List<LocalVariableNode> parameters,
Handler handler,
VisitorState state,
Context context,
Types types,
Config config) {
assert underlyingAST.getKind() == UnderlyingAST.Kind.METHOD;

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

NullnessStore envStore = getEnvNullnessStoreForClass(classTree, state.context);
NullnessStore envStore = getEnvNullnessStoreForClass(classTree, 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,9 +38,4 @@ 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);
}
}