Skip to content

Commit

Permalink
For ReferenceEquality-based checkers, understand that all proto gette…
Browse files Browse the repository at this point in the history
…rs never

return null when making suggestions.

RELNOTES: ReferenceEquality and similar checks understand that proto getters
never return null

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=159963821
  • Loading branch information
nick-someone authored and Eddie Aftandilian committed Jun 28, 2017
1 parent 83c4126 commit 6f98066
Show file tree
Hide file tree
Showing 5 changed files with 88 additions and 12 deletions.
Expand Up @@ -307,7 +307,7 @@ public final TransferResult<Nullness, LocalStore<Nullness>> visitMethodInvocatio
* if I'm careful to give it its correct Nullness instead of hardcoding it to NONNULL as the
* current code does. To avoid problems, we return a RegularTransferResult when possible.
*/
if (tryGetMethodSymbol(node.getTree()).isBoolean) {
if (tryGetMethodSymbol(node.getTree(), null).isBoolean) {
ResultingStore thenStore = updateStore(input.getThenStore(), thenUpdates, bothUpdates);
ResultingStore elseStore = updateStore(input.getElseStore(), elseUpdates, bothUpdates);
return conditionalResult(
Expand Down
Expand Up @@ -29,4 +29,6 @@ public interface MethodInfo {
boolean isStatic();

boolean isPrimitive();

boolean isKnownNonNullReturning();
}
Expand Up @@ -47,8 +47,12 @@
import com.sun.source.util.Trees;
import com.sun.tools.javac.code.Flags;
import com.sun.tools.javac.code.Symbol;
import com.sun.tools.javac.code.Symbol.ClassSymbol;
import com.sun.tools.javac.code.Symbol.MethodSymbol;
import com.sun.tools.javac.code.Symbol.TypeSymbol;
import com.sun.tools.javac.code.Symbol.VarSymbol;
import com.sun.tools.javac.code.Type;
import com.sun.tools.javac.code.Types;
import com.sun.tools.javac.processing.JavacProcessingEnvironment;
import com.sun.tools.javac.tree.JCTree.JCFieldAccess;
import com.sun.tools.javac.tree.JCTree.JCIdent;
Expand Down Expand Up @@ -193,6 +197,9 @@ public boolean apply(MethodInfo methodInfo) {
if (methodInfo.isPrimitive()) {
return true;
}
if (methodInfo.isKnownNonNullReturning()) {
return true;
}
if (CLASSES_WITH_NON_NULLABLE_RETURNS.contains(methodInfo.clazz())) {
return true;
}
Expand Down Expand Up @@ -473,7 +480,7 @@ Nullness visitMethodInvocation(
LocalVariableUpdates thenUpdates,
LocalVariableUpdates elseUpdates,
LocalVariableUpdates bothUpdates) {
ClassAndMethod callee = tryGetMethodSymbol(node.getTree());
ClassAndMethod callee = tryGetMethodSymbol(node.getTree(), Types.instance(context));
setReceiverNonnull(bothUpdates, node.getTarget().getReceiver(), callee);
setUnconditionalArgumentNullness(bothUpdates, node.getArguments(), callee);
setConditionalArgumentNullness(elseUpdates, node.getArguments(), callee);
Expand Down Expand Up @@ -575,10 +582,10 @@ private static ClassAndField tryGetFieldSymbol(Tree tree) {
return null;
}

static ClassAndMethod tryGetMethodSymbol(MethodInvocationTree tree) {
static ClassAndMethod tryGetMethodSymbol(MethodInvocationTree tree, Types types) {
Symbol symbol = tryGetSymbol(tree.getMethodSelect());
if (symbol instanceof MethodSymbol) {
return ClassAndMethod.make((MethodSymbol) symbol);
return ClassAndMethod.make((MethodSymbol) symbol, types);
}
return null;
}
Expand Down Expand Up @@ -791,35 +798,69 @@ static final class ClassAndMethod implements Member, MethodInfo {
final boolean isStatic;
final boolean isPrimitive;
final boolean isBoolean;
final boolean isNonNullReturning;

private ClassAndMethod(
String clazz,
String method,
List<String> annotations,
boolean isStatic,
boolean isPrimitive,
boolean isBoolean) {
boolean isBoolean,
boolean isNonNullReturning) {
this.clazz = clazz;
this.method = method;
this.annotations = ImmutableList.copyOf(annotations);
this.isStatic = isStatic;
this.isPrimitive = isPrimitive;
this.isBoolean = isBoolean;
this.isNonNullReturning = isNonNullReturning;
}

static ClassAndMethod make(MethodSymbol symbol) {
List<? extends AnnotationMirror> annotationMirrors = symbol.getAnnotationMirrors();
static ClassAndMethod make(MethodSymbol methodSymbol, @Nullable Types types) {
List<? extends AnnotationMirror> annotationMirrors = methodSymbol.getAnnotationMirrors();
List<String> annotations = new ArrayList<>(annotationMirrors.size());
for (AnnotationMirror annotationMirror : annotationMirrors) {
annotations.add(annotationMirror.getAnnotationType().toString());
}

ClassSymbol clazzSymbol = (ClassSymbol) methodSymbol.owner;

return new ClassAndMethod(
symbol.owner.getQualifiedName().toString(),
symbol.getSimpleName().toString(),
clazzSymbol.getQualifiedName().toString(),
methodSymbol.getSimpleName().toString(),
annotations,
symbol.isStatic(),
symbol.getReturnType().isPrimitive(),
symbol.getReturnType().getTag() == BOOLEAN);
methodSymbol.isStatic(),
methodSymbol.getReturnType().isPrimitive(),
methodSymbol.getReturnType().getTag() == BOOLEAN,
knownNonNullMethod(methodSymbol, clazzSymbol, types));
}

private static boolean knownNonNullMethod(
MethodSymbol methodSymbol, ClassSymbol clazzSymbol, @Nullable Types types) {
if (types == null) {
return false;
}

// Proto getters are not null
if (methodSymbol.name.toString().startsWith("get")
&& methodSymbol.params().isEmpty()
&& !methodSymbol.isStatic()) {
Type type = clazzSymbol.type;
while (type != null) {
TypeSymbol typeSymbol = type.asElement();
if (typeSymbol == null) {
break;
}
if (typeSymbol
.getQualifiedName()
.contentEquals("com.google.protobuf.AbstractMessageLite")) {
return true;
}
type = types.supertype(type);
}
}
return false;
}

@Override
Expand Down Expand Up @@ -850,6 +891,11 @@ public List<String> annotations() {
public boolean isPrimitive() {
return isPrimitive;
}

@Override
public boolean isKnownNonNullReturning() {
return isNonNullReturning;
}
}

static final class ClassAndField implements Member {
Expand Down
Expand Up @@ -43,6 +43,33 @@ public class ReferenceEqualityTest {
private final BugCheckerRefactoringTestHelper refactoringTestHelper =
BugCheckerRefactoringTestHelper.newInstance(new ReferenceEquality(), getClass());

@Test
public void protoGetter_nonnull() throws Exception {
refactoringTestHelper
.addInput("proto/ProtoTest.java")
.expectUnchanged()
.addInputLines(
"in/Foo.java",
"import com.google.errorprone.bugpatterns.proto.ProtoTest.TestProtoMessage;",
"class Foo {",
" void something(TestProtoMessage f1, TestProtoMessage f2) {",
" boolean b = f1 == f2;",
" b = f1.getMessage() == f2.getMessage();",
" }",
"}")
.addOutputLines(
"out/Foo.java",
"import com.google.errorprone.bugpatterns.proto.ProtoTest.TestProtoMessage;",
"import java.util.Objects;",
"class Foo {",
" void something(TestProtoMessage f1, TestProtoMessage f2) {",
" boolean b = Objects.equals(f1, f2);",
" b = f1.getMessage().equals(f2.getMessage());",
" }",
"}")
.doTest();
}

@Test
public void negative_const() throws Exception {
compilationHelper
Expand Down
Expand Up @@ -19,6 +19,7 @@

package com.google.errorprone.bugpatterns.proto;

@SuppressWarnings("ReferenceEquality")
public final class ProtoTest {
private ProtoTest() {}

Expand Down

0 comments on commit 6f98066

Please sign in to comment.