Skip to content

Fix #657 by splitting field initialization region into smaller regions #658

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
lazaroclapp merged 13 commits into from
Nov 8, 2022
Merged

Fix #657 by splitting field initialization region into smaller regions #658

Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
f4dfb7c
add uuid as prefix for all outputs
Jul 27, 2022
cf6a296
fix unit tests
Jul 27, 2022
f99bcb6
remove IDE changes
Jul 28, 2022
1e18d0e
update javadoc
nimakarimipour Jul 28, 2022
295ce8c
resolve issue-657
nimakarimipour Sep 19, 2022
935da67
fix conflicts
nimakarimipour Sep 19, 2022
e4809af
fix rebase issues
nimakarimipour Sep 19, 2022
688074e
fix rebase issues
nimakarimipour Sep 19, 2022
792bfa9
update javadoc, replce method with member
nimakarimipour Sep 19, 2022
d03340c
fix unit test
nimakarimipour Sep 19, 2022
5f797a9
update javadoc
nimakarimipour Sep 19, 2022
c918f21
erge branch 'master' into nimak/issue-657
nimakarimipour Nov 7, 2022
9e6f062
change javadoc to comment
nimakarimipour Nov 8, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
74 changes: 49 additions & 25 deletions ...serialization/out/ClassAndMethodInfo.java → ...serialization/out/ClassAndMemberInfo.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,32 +25,34 @@
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.
*/
Copy link
Collaborator

@lazaroclapp lazaroclapp Nov 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment format (starting with /**) indicates that this is the javadoc documentation for method. I think this is just an unrelated comment about both of the fields below, so I would just have it as:

// Finding values for these properties is costly and are not needed by default, hence, they are
// not final and are only initialized at request.

Copy link
Contributor Author

@nimakarimipour nimakarimipour Nov 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for noticing this :) 9e6f062

@Nullable private MethodTree method;
@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 +62,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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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