Skip to content

Commit

Permalink
Fix Serialization: Split field initialization region into smaller reg…
Browse files Browse the repository at this point in the history 
…ions (#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.
  • Loading branch information
@nimakarimipour
nimakarimipour authored Nov 8, 2022
1 parent 19bbb91 commit eb62d57
Show file tree
Hide file tree
Showing 5 changed files with 177 additions and 56 deletions.
80 changes: 51 additions & 29 deletions ...serialization/out/ClassAndMethodInfo.java → ...serialization/out/ClassAndMemberInfo.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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
Expand Down
18 changes: 9 additions & 9 deletions nullaway/src/main/java/com/uber/nullaway/fixserialization/out/ErrorInfo.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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;
}
Expand Down Expand Up @@ -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();
}

/**
Expand All @@ -125,7 +125,7 @@ public static String header() {
"message_type",
"message",
"enc_class",
"enc_method",
"enc_member",
"target_kind",
"target_class",
"target_method",
Expand Down
16 changes: 8 additions & 8 deletions nullaway/src/main/java/com/uber/nullaway/fixserialization/out/SuggestedNullableFixInfo.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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();
}

/**
Expand Down
99 changes: 99 additions & 0 deletions nullaway/src/test/java/com/uber/nullaway/NullAwaySerializationTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1369,6 +1369,105 @@ public void errorSerializationTestLocalClassField() {
.doTest();
}

@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);
Expand Down
20 changes: 10 additions & 10 deletions nullaway/src/test/java/com/uber/nullaway/tools/ErrorDisplay.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -43,7 +43,7 @@ public ErrorDisplay(
String type,
String message,
String encClass,
String encMethod,
String encMember,
String kind,
String clazz,
String method,
Expand All @@ -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;
Expand All @@ -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
Expand All @@ -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)
Expand All @@ -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
Expand All @@ -105,8 +105,8 @@ public String toString() {
+ "\n\tmessage='"
+ message
+ '\''
+ "\n\tencMethod='"
+ encMethod
+ "\n\tencMember='"
+ encMember
+ '\''
+ "\n\tencClass='"
+ encClass
Expand Down

0 comments on commit eb62d57

Please sign in to comment.