Skip to content

Commit

Permalink
Revert "Fix Serialization: Split field initialization region into sma…
Browse files Browse the repository at this point in the history 
…ller regions (uber#658)"

This reverts commit eb62d57.
  • Loading branch information
@msridhar
msridhar committed Jul 18, 2023
1 parent 6ee02e6 commit 8037fea
Show file tree
Hide file tree
Showing 5 changed files with 56 additions and 177 deletions.
80 changes: 29 additions & 51 deletions ...serialization/out/ClassAndMemberInfo.java → ...serialization/out/ClassAndMethodInfo.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 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 */
/** 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 final and are only initialized at request.
@Nullable private Symbol member;
/**
* 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 ClassAndMemberInfo(TreePath path) {
public ClassAndMethodInfo(TreePath path) {
this.path = path;
}

/** Finds the class and member where the error / fix is reported according to {@code path}. */
/** Finds the class and method where the error 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 =
method =
path.getLeaf() instanceof MethodTree
? (MethodTree) path.getLeaf()
: ASTHelpers.findEnclosingNode(path, MethodTree.class);
Expand All @@ -60,52 +60,30 @@ public void findValues() {
path.getLeaf() instanceof ClassTree
? (ClassTree) path.getLeaf()
: ASTHelpers.findEnclosingNode(path, ClassTree.class);
if (clazz != null) {
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);
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;
}
Symbol.MethodSymbol methodSymbol = ASTHelpers.getSymbol(method);
if (!methodSymbol.isEnclosedBy(classSymbol)) {
method = null;
}
}
}

@Nullable
public Symbol getMember() {
return member;
public MethodTree getMethod() {
return method;
}

@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 ClassAndMemberInfo classAndMemberInfo;
private final ClassAndMethodInfo classAndMethodInfo;

/**
* 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.classAndMemberInfo = new ClassAndMemberInfo(path);
this.classAndMethodInfo = new ClassAndMethodInfo(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()),
(classAndMemberInfo.getClazz() != null
? ASTHelpers.getSymbol(classAndMemberInfo.getClazz()).flatName()
(classAndMethodInfo.getClazz() != null
? ASTHelpers.getSymbol(classAndMethodInfo.getClazz()).flatName()
: "null"),
(classAndMemberInfo.getMember() != null
? classAndMemberInfo.getMember().toString()
(classAndMethodInfo.getMethod() != null
? ASTHelpers.getSymbol(classAndMethodInfo.getMethod()).toString()
: "null"),
(nonnullTarget != null
? SymbolLocation.createLocationFromSymbol(nonnullTarget).tabSeparatedToString()
: EMPTY_NONNULL_TARGET_LOCATION_STRING));
}

/** Finds the class and member of program point where the error is reported. */
/** Finds the class and method of program point where the error is reported. */
public void initEnclosing() {
classAndMemberInfo.findValues();
classAndMethodInfo.findValues();
}

/**
Expand All @@ -125,7 +125,7 @@ public static String header() {
"message_type",
"message",
"enc_class",
"enc_member",
"enc_method",
"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 ClassAndMemberInfo classAndMemberInfo;
private final ClassAndMethodInfo classAndMethodInfo;

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",
(classAndMemberInfo.getClazz() == null
(classAndMethodInfo.getClazz() == null
? "null"
: ASTHelpers.getSymbol(classAndMemberInfo.getClazz()).flatName()),
(classAndMemberInfo.getMember() == null
: ASTHelpers.getSymbol(classAndMethodInfo.getClazz()).flatName()),
(classAndMethodInfo.getMethod() == null
? "null"
: classAndMemberInfo.getMember().toString()));
: ASTHelpers.getSymbol(classAndMethodInfo.getMethod()).toString()));
}

/** Finds the class and member of program point where triggered this type change. */
/** Finds the class and method of program point where triggered this type change. */
public void initEnclosing() {
classAndMemberInfo.findValues();
classAndMethodInfo.findValues();
}

/**
Expand Down
99 changes: 0 additions & 99 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,105 +1369,6 @@ 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 encMember;
public final String encMethod;
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 encMember,
String encMethod,
String kind,
String clazz,
String method,
Expand All @@ -52,7 +52,7 @@ public ErrorDisplay(
String uri) {
this.type = type;
this.message = message;
this.encMember = encMember;
this.encMethod = encMethod;
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 encMember) {
this(type, message, encClass, encMember, "null", "null", "null", "null", "null", "null");
public ErrorDisplay(String type, String message, String encClass, String encMethod) {
this(type, message, encClass, encMethod, "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))
&& encMember.equals(that.encMember)
&& clazz.equals(that.clazz)
&& encMethod.equals(that.encMethod)
&& 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, encMember, encClass, kind, clazz, method, variable, index, uri);
type, message, encMethod, encClass, kind, clazz, method, variable, index, uri);
}

@Override
Expand All @@ -105,8 +105,8 @@ public String toString() {
+ "\n\tmessage='"
+ message
+ '\''
+ "\n\tencMember='"
+ encMember
+ "\n\tencMethod='"
+ encMethod
+ '\''
+ "\n\tencClass='"
+ encClass
Expand Down

0 comments on commit 8037fea

Please sign in to comment.