Fix Serialization: Split field initialization region into smaller regions (#658)
This PR resolves #657 by splitting the field initialization region into smaller regions. We currently only consider the value "null", for all `errors/fixes` reported in this regions. In large classes which they have many number of field, this can easily create a lot builds as they all have the value "null" for enclosing method causing conflicts in regions.
This PR generalizes the concept of enclosing method to enclosing member, and breaks a single regions of field initialization regions into a separate region for each field declaration and reduce the conflicts in regions dramatically.
The equivalent for enclosing member is described:
The enclosing member for `fix` or `error` is:
- If it is triggered inside a __method__, the enclosing member will be the __enclosing method__.
- If it is enclosed by a __field declaration statement__, the enclosing member will be the __declared field__.
- If __none__ of the above, the enclosing member will be `"null"` (used for __static initialization region__).
With the current approach the following errors will have the values below:
```java
class C {
Field foo1 = Field(null); // Passing nullable, enclMethod = "null"
Field foo2 = null; // assigning nullable, enclMethod = "null"
static Field foo3 3 = null; // assigning nullable, enclMethod = "null"
{
foo3 = null; // assigning nullable, enclMethod = "null"
}
void bar(){
this.foo1 = null; // assigning nullable, enclMethod = "bar()"
}
}
```
From the point of view of Annotator, that is __4__ conflicts in regions as they all have `enclMethod = "null"`.
This can be greatly improved with the new approach suggested and changed to below:
```java
class C {
Field foo1 = Field(null); // Passing nullable, enclMember = "foo1"
Field foo2 = null; // assigning nullable, enclMember = "foo2"
static Field foo3 = null; // assigning nullable, enclMember = "foo3"
{
Field3 = null; // assigning nullable, enclMember = "null"
}
void bar(){
this.foo1 = null; // assigning nullable, enclMember = "bar()"
}
}
```
None of the error reported above have any conflicts and can be allocated to separate regions.
diff --git a/nullaway/src/main/java/com/uber/nullaway/fixserialization/out/ClassAndMemberInfo.java b/nullaway/src/main/java/com/uber/nullaway/fixserialization/out/ClassAndMemberInfo.java
new file mode 100644
index 0000000..ebbd407
--- /dev/null
+++ b/nullaway/src/main/java/com/uber/nullaway/fixserialization/out/ClassAndMemberInfo.java
@@ -0,0 +1,115 @@
+/*
+ * Copyright (c) 2022 Uber Technologies, Inc.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+package com.uber.nullaway.fixserialization.out;
+
+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 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 final and are only initialized at request.
+ @Nullable private Symbol member;
+
+ @Nullable private ClassTree clazz;
+
+ public ClassAndMemberInfo(TreePath path) {
+ this.path = 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).
+ enclosingMethod =
+ path.getLeaf() instanceof MethodTree
+ ? (MethodTree) path.getLeaf()
+ : ASTHelpers.findEnclosingNode(path, MethodTree.class);
+ // If the error is reported on a class, that class itself is the relevant program point.
+ // Otherwise, use the enclosing class.
+ clazz =
+ path.getLeaf() instanceof ClassTree
+ ? (ClassTree) path.getLeaf()
+ : ASTHelpers.findEnclosingNode(path, ClassTree.class);
+ if (clazz != null) {
+ Symbol.ClassSymbol classSymbol = ASTHelpers.getSymbol(clazz);
+ 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 Symbol getMember() {
+ return member;
+ }
+
+ @Nullable
+ public ClassTree getClazz() {
+ return clazz;
+ }
+}
diff --git a/nullaway/src/main/java/com/uber/nullaway/fixserialization/out/ClassAndMethodInfo.java b/nullaway/src/main/java/com/uber/nullaway/fixserialization/out/ClassAndMethodInfo.java
deleted file mode 100644
index 2f671db..0000000
--- a/nullaway/src/main/java/com/uber/nullaway/fixserialization/out/ClassAndMethodInfo.java
+++ /dev/null
@@ -1,93 +0,0 @@
-/*
- * Copyright (c) 2022 Uber Technologies, Inc.
- *
- * Permission is hereby granted, free of charge, to any person obtaining a copy
- * of this software and associated documentation files (the "Software"), to deal
- * in the Software without restriction, including without limitation the rights
- * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
- * copies of the Software, and to permit persons to whom the Software is
- * furnished to do so, subject to the following conditions:
- *
- * The above copyright notice and this permission notice shall be included in
- * all copies or substantial portions of the Software.
- *
- * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
- * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
- * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
- * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
- * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
- * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
- * THE SOFTWARE.
- */
-
-package com.uber.nullaway.fixserialization.out;
-
-import com.google.errorprone.util.ASTHelpers;
-import com.sun.source.tree.ClassTree;
-import com.sun.source.tree.MethodTree;
-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 */
- 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;
-
- @Nullable private ClassTree clazz;
-
- public ClassAndMethodInfo(TreePath path) {
- this.path = path;
- }
-
- /** Finds the class and method where the error is reported according to {@code path}. */
- public void findValues() {
- // If the error is reported on a method, that method itself is the relevant program point.
- // Otherwise, use the enclosing method (if present).
- method =
- path.getLeaf() instanceof MethodTree
- ? (MethodTree) path.getLeaf()
- : ASTHelpers.findEnclosingNode(path, MethodTree.class);
- // If the error is reported on a class, that class itself is the relevant program point.
- // Otherwise, use the enclosing class.
- clazz =
- 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.
- Symbol.ClassSymbol classSymbol = ASTHelpers.getSymbol(clazz);
- Symbol.MethodSymbol methodSymbol = ASTHelpers.getSymbol(method);
- if (!methodSymbol.isEnclosedBy(classSymbol)) {
- method = null;
- }
- }
- }
-
- @Nullable
- public MethodTree getMethod() {
- return method;
- }
-
- @Nullable
- public ClassTree getClazz() {
- return clazz;
- }
-}
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 07b00e3..7127b82 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 @@
'\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 @@
"\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 @@
"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 c00a0a7..8232ced 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 @@
/** 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 @@
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 019726c..2e49f87 100644
--- a/nullaway/src/test/java/com/uber/nullaway/NullAwaySerializationTest.java
+++ b/nullaway/src/test/java/com/uber/nullaway/NullAwaySerializationTest.java
@@ -1370,6 +1370,105 @@
}
@Test
+ public void errorSerializationTestEnclosedByFieldDeclaration() {
+ SerializationTestHelper<ErrorDisplay> 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<FixDisplay> tester = new SerializationTestHelper<>(root);
tester
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 0531746..1064823 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 @@
String type,
String message,
String encClass,
- String encMethod,
+ String encMember,
String kind,
String clazz,
String method,
@@ -52,7 +52,7 @@
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 @@
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 @@
// 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 @@
@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 @@
+ "\n\tmessage='"
+ message
+ '\''
- + "\n\tencMethod='"
- + encMethod
+ + "\n\tencMember='"
+ + encMember
+ '\''
+ "\n\tencClass='"
+ encClass
