diff --git a/nullaway/src/main/java/com/uber/nullaway/fixserialization/out/ClassAndMethodInfo.java b/nullaway/src/main/java/com/uber/nullaway/fixserialization/out/ClassAndMemberInfo.java similarity index 50% rename from nullaway/src/main/java/com/uber/nullaway/fixserialization/out/ClassAndMethodInfo.java rename to nullaway/src/main/java/com/uber/nullaway/fixserialization/out/ClassAndMemberInfo.java index 2f671db65c..ebbd407c5d 100644 --- a/nullaway/src/main/java/com/uber/nullaway/fixserialization/out/ClassAndMethodInfo.java +++ b/nullaway/src/main/java/com/uber/nullaway/fixserialization/out/ClassAndMemberInfo.java @@ -25,32 +25,32 @@ import com.google.errorprone.util.ASTHelpers; import com.sun.source.tree.ClassTree; import com.sun.source.tree.MethodTree; +import com.sun.source.tree.VariableTree; import com.sun.source.util.TreePath; import com.sun.tools.javac.code.Symbol; import javax.annotation.Nullable; -/** Class and method corresponding to a program point at which an error was reported. */ -public class ClassAndMethodInfo { - /** Path to the program point of the reported error */ +/** Class and member corresponding to a program point at which an error / fix was reported. */ +public class ClassAndMemberInfo { + /** Path to the program point of the reported error / fix */ public final TreePath path; - /** - * Finding values for these properties is costly and are not needed by default, hence, they are - * not {@code final} and are only initialized at request. - */ - @Nullable private MethodTree method; + // Finding values for these properties is costly and are not needed by default, hence, they are + // not final and are only initialized at request. + @Nullable private Symbol member; @Nullable private ClassTree clazz; - public ClassAndMethodInfo(TreePath path) { + public ClassAndMemberInfo(TreePath path) { this.path = path; } - /** Finds the class and method where the error is reported according to {@code path}. */ + /** Finds the class and member where the error / fix is reported according to {@code path}. */ public void findValues() { + MethodTree enclosingMethod; // If the error is reported on a method, that method itself is the relevant program point. // Otherwise, use the enclosing method (if present). - method = + enclosingMethod = path.getLeaf() instanceof MethodTree ? (MethodTree) path.getLeaf() : ASTHelpers.findEnclosingNode(path, MethodTree.class); @@ -60,30 +60,52 @@ public void findValues() { path.getLeaf() instanceof ClassTree ? (ClassTree) path.getLeaf() : ASTHelpers.findEnclosingNode(path, ClassTree.class); - if (clazz != null && method != null) { - // It is possible that the computed method is not enclosed by the computed class, e.g., for - // the following case: - // class C { - // void foo() { - // class Local { - // Object f = null; // error - // } - // } - // } - // Here the above code will compute clazz to be Local and method as foo(). In such cases, set - // method to null, we always want the corresponding method to be nested in the corresponding - // class. + if (clazz != null) { Symbol.ClassSymbol classSymbol = ASTHelpers.getSymbol(clazz); - Symbol.MethodSymbol methodSymbol = ASTHelpers.getSymbol(method); - if (!methodSymbol.isEnclosedBy(classSymbol)) { - method = null; + if (enclosingMethod != null) { + // It is possible that the computed method is not enclosed by the computed class, e.g., for + // the following case: + // class C { + // void foo() { + // class Local { + // Object f = null; // error + // } + // } + // } + // Here the above code will compute clazz to be Local and method as foo(). In such cases, + // set method to null, we always want the corresponding method to be nested in the + // corresponding class. + Symbol.MethodSymbol methodSymbol = ASTHelpers.getSymbol(enclosingMethod); + if (!methodSymbol.isEnclosedBy(classSymbol)) { + enclosingMethod = null; + } + } + if (enclosingMethod != null) { + member = ASTHelpers.getSymbol(enclosingMethod); + } else { + // Node is not enclosed by any method, can be a field declaration or enclosed by it. + Symbol sym = ASTHelpers.getSymbol(path.getLeaf()); + Symbol.VarSymbol fieldSymbol = null; + if (sym != null && sym.getKind().isField()) { + // Directly on a field declaration. + fieldSymbol = (Symbol.VarSymbol) sym; + } else { + // Can be enclosed by a field declaration tree. + VariableTree fieldDeclTree = ASTHelpers.findEnclosingNode(path, VariableTree.class); + if (fieldDeclTree != null) { + fieldSymbol = ASTHelpers.getSymbol(fieldDeclTree); + } + } + if (fieldSymbol != null && fieldSymbol.isEnclosedBy(classSymbol)) { + member = fieldSymbol; + } } } } @Nullable - public MethodTree getMethod() { - return method; + public Symbol getMember() { + return member; } @Nullable diff --git a/nullaway/src/main/java/com/uber/nullaway/fixserialization/out/ErrorInfo.java b/nullaway/src/main/java/com/uber/nullaway/fixserialization/out/ErrorInfo.java index 07b00e38c4..7127b82834 100644 --- a/nullaway/src/main/java/com/uber/nullaway/fixserialization/out/ErrorInfo.java +++ b/nullaway/src/main/java/com/uber/nullaway/fixserialization/out/ErrorInfo.java @@ -36,7 +36,7 @@ public class ErrorInfo { private final ErrorMessage errorMessage; - private final ClassAndMethodInfo classAndMethodInfo; + private final ClassAndMemberInfo classAndMemberInfo; /** * if non-null, this error involved a pseudo-assignment of a @Nullable expression into a @NonNull @@ -60,7 +60,7 @@ public class ErrorInfo { '\r', 'r'); public ErrorInfo(TreePath path, ErrorMessage errorMessage, @Nullable Symbol nonnullTarget) { - this.classAndMethodInfo = new ClassAndMethodInfo(path); + this.classAndMemberInfo = new ClassAndMemberInfo(path); this.errorMessage = errorMessage; this.nonnullTarget = nonnullTarget; } @@ -97,20 +97,20 @@ public String tabSeparatedToString() { "\t", errorMessage.getMessageType().toString(), escapeSpecialCharacters(errorMessage.getMessage()), - (classAndMethodInfo.getClazz() != null - ? ASTHelpers.getSymbol(classAndMethodInfo.getClazz()).flatName() + (classAndMemberInfo.getClazz() != null + ? ASTHelpers.getSymbol(classAndMemberInfo.getClazz()).flatName() : "null"), - (classAndMethodInfo.getMethod() != null - ? ASTHelpers.getSymbol(classAndMethodInfo.getMethod()).toString() + (classAndMemberInfo.getMember() != null + ? classAndMemberInfo.getMember().toString() : "null"), (nonnullTarget != null ? SymbolLocation.createLocationFromSymbol(nonnullTarget).tabSeparatedToString() : EMPTY_NONNULL_TARGET_LOCATION_STRING)); } - /** Finds the class and method of program point where the error is reported. */ + /** Finds the class and member of program point where the error is reported. */ public void initEnclosing() { - classAndMethodInfo.findValues(); + classAndMemberInfo.findValues(); } /** @@ -125,7 +125,7 @@ public static String header() { "message_type", "message", "enc_class", - "enc_method", + "enc_member", "target_kind", "target_class", "target_method", diff --git a/nullaway/src/main/java/com/uber/nullaway/fixserialization/out/SuggestedNullableFixInfo.java b/nullaway/src/main/java/com/uber/nullaway/fixserialization/out/SuggestedNullableFixInfo.java index c00a0a751d..8232ced29b 100644 --- a/nullaway/src/main/java/com/uber/nullaway/fixserialization/out/SuggestedNullableFixInfo.java +++ b/nullaway/src/main/java/com/uber/nullaway/fixserialization/out/SuggestedNullableFixInfo.java @@ -36,13 +36,13 @@ public class SuggestedNullableFixInfo { /** Error which will be resolved by this type change. */ private final ErrorMessage errorMessage; - private final ClassAndMethodInfo classAndMethodInfo; + private final ClassAndMemberInfo classAndMemberInfo; public SuggestedNullableFixInfo( TreePath path, SymbolLocation symbolLocation, ErrorMessage errorMessage) { - this.classAndMethodInfo = new ClassAndMethodInfo(path); this.symbolLocation = symbolLocation; this.errorMessage = errorMessage; + this.classAndMemberInfo = new ClassAndMemberInfo(path); } @Override @@ -76,17 +76,17 @@ public String tabSeparatedToString() { symbolLocation.tabSeparatedToString(), errorMessage.getMessageType().toString(), "nullable", - (classAndMethodInfo.getClazz() == null + (classAndMemberInfo.getClazz() == null ? "null" - : ASTHelpers.getSymbol(classAndMethodInfo.getClazz()).flatName()), - (classAndMethodInfo.getMethod() == null + : ASTHelpers.getSymbol(classAndMemberInfo.getClazz()).flatName()), + (classAndMemberInfo.getMember() == null ? "null" - : ASTHelpers.getSymbol(classAndMethodInfo.getMethod()).toString())); + : classAndMemberInfo.getMember().toString())); } - /** Finds the class and method of program point where triggered this type change. */ + /** Finds the class and member of program point where triggered this type change. */ public void initEnclosing() { - classAndMethodInfo.findValues(); + classAndMemberInfo.findValues(); } /** diff --git a/nullaway/src/test/java/com/uber/nullaway/NullAwaySerializationTest.java b/nullaway/src/test/java/com/uber/nullaway/NullAwaySerializationTest.java index 019726c100..2e49f878d9 100644 --- a/nullaway/src/test/java/com/uber/nullaway/NullAwaySerializationTest.java +++ b/nullaway/src/test/java/com/uber/nullaway/NullAwaySerializationTest.java @@ -1369,6 +1369,105 @@ public void errorSerializationTestLocalClassField() { .doTest(); } + @Test + public void errorSerializationTestEnclosedByFieldDeclaration() { + SerializationTestHelper tester = new SerializationTestHelper<>(root); + tester + .setArgs( + Arrays.asList( + "-d", + temporaryFolder.getRoot().getAbsolutePath(), + "-XepOpt:NullAway:AnnotatedPackages=com.uber", + "-XepOpt:NullAway:SerializeFixMetadata=true", + "-XepOpt:NullAway:FixSerializationConfigPath=" + configPath)) + .addSourceLines( + "com/uber/Main.java", + "package com.uber;", + "import javax.annotation.Nullable;", + "public class Main {", + " // BUG: Diagnostic contains: passing @Nullable parameter", + " Foo f = new Foo(null);", // Member should be "f" + " // BUG: Diagnostic contains: assigning @Nullable expression", + " Foo f1 = null;", // Member should be "f1" + " // BUG: Diagnostic contains: assigning @Nullable expression", + " static Foo f2 = null;", // Member should be "f2" + " static {", + " // BUG: Diagnostic contains: assigning @Nullable expression", + " f2 = null;", // Member should be "null" (not field or method) + " }", + " class Inner {", + " // BUG: Diagnostic contains: passing @Nullable parameter", + " Foo f = new Foo(null);", // Member should be "f" but class is Main$Inner + " }", + "}") + .addSourceLines( + "com/uber/Foo.java", + "package com.uber;", + "public class Foo {", + " public Foo(Object foo){", + " }", + "}") + .setExpectedOutputs( + new ErrorDisplay( + "PASS_NULLABLE", + "passing @Nullable parameter", + "com.uber.Main", + "f", + "PARAMETER", + "com.uber.Foo", + "Foo(java.lang.Object)", + "foo", + "0", + "com/uber/Foo.java"), + new ErrorDisplay( + "ASSIGN_FIELD_NULLABLE", + "assigning @Nullable expression to @NonNull field", + "com.uber.Main", + "f1", + "FIELD", + "com.uber.Main", + "null", + "f1", + "null", + "com/uber/Main.java"), + new ErrorDisplay( + "ASSIGN_FIELD_NULLABLE", + "assigning @Nullable expression to @NonNull field", + "com.uber.Main", + "f2", + "FIELD", + "com.uber.Main", + "null", + "f2", + "null", + "com/uber/Main.java"), + new ErrorDisplay( + "ASSIGN_FIELD_NULLABLE", + "assigning @Nullable expression to @NonNull field", + "com.uber.Main", + "null", + "FIELD", + "com.uber.Main", + "null", + "f2", + "null", + "com/uber/Main.java"), + new ErrorDisplay( + "PASS_NULLABLE", + "passing @Nullable parameter", + "com.uber.Main$Inner", + "f", + "PARAMETER", + "com.uber.Foo", + "Foo(java.lang.Object)", + "foo", + "0", + "com/uber/Foo.java")) + .setFactory(errorDisplayFactory) + .setOutputFileNameAndHeader(ERROR_FILE_NAME, ERROR_FILE_HEADER) + .doTest(); + } + @Test public void suggestNullableArgumentOnBytecode() { SerializationTestHelper tester = new SerializationTestHelper<>(root); diff --git a/nullaway/src/test/java/com/uber/nullaway/tools/ErrorDisplay.java b/nullaway/src/test/java/com/uber/nullaway/tools/ErrorDisplay.java index 05317462e8..106482336b 100644 --- a/nullaway/src/test/java/com/uber/nullaway/tools/ErrorDisplay.java +++ b/nullaway/src/test/java/com/uber/nullaway/tools/ErrorDisplay.java @@ -30,7 +30,7 @@ public class ErrorDisplay implements Display { public final String type; public final String message; - public final String encMethod; + public final String encMember; public final String encClass; public final String kind; public final String clazz; @@ -43,7 +43,7 @@ public ErrorDisplay( String type, String message, String encClass, - String encMethod, + String encMember, String kind, String clazz, String method, @@ -52,7 +52,7 @@ public ErrorDisplay( String uri) { this.type = type; this.message = message; - this.encMethod = encMethod; + this.encMember = encMember; this.encClass = encClass; this.kind = kind; this.clazz = clazz; @@ -63,8 +63,8 @@ public ErrorDisplay( this.uri = uri.contains("com/uber/") ? uri.substring(uri.indexOf("com/uber/")) : uri; } - public ErrorDisplay(String type, String message, String encClass, String encMethod) { - this(type, message, encClass, encMethod, "null", "null", "null", "null", "null", "null"); + public ErrorDisplay(String type, String message, String encClass, String encMember) { + this(type, message, encClass, encMember, "null", "null", "null", "null", "null", "null"); } @Override @@ -80,10 +80,10 @@ public boolean equals(Object o) { // To increase readability, a shorter version of the actual message might be present in the // expected output of tests. && (message.contains(that.message) || that.message.contains(message)) - && encMethod.equals(that.encMethod) + && encMember.equals(that.encMember) + && clazz.equals(that.clazz) && encClass.equals(that.encClass) && kind.equals(that.kind) - && clazz.equals(that.clazz) && method.equals(that.method) && variable.equals(that.variable) && index.equals(that.index) @@ -93,7 +93,7 @@ public boolean equals(Object o) { @Override public int hashCode() { return Objects.hash( - type, message, encMethod, encClass, kind, clazz, method, variable, index, uri); + type, message, encMember, encClass, kind, clazz, method, variable, index, uri); } @Override @@ -105,8 +105,8 @@ public String toString() { + "\n\tmessage='" + message + '\'' - + "\n\tencMethod='" - + encMethod + + "\n\tencMember='" + + encMember + '\'' + "\n\tencClass='" + encClass