Skip to content

Commit

Permalink
Update region selection for initialization errors. (#713)
Browse files Browse the repository at this point in the history 
This PR updates region selection for initialization errors. Previously, the `null` region was selected for all initialization errors (`METHOD_NO_INIT` and `FIELD_NO_INIT`).  This PR updates the selection logic to choose the actual constructor or the uninitialized field as the region which the error is reported.

Co-authored-by: Manu Sridharan <msridhar@gmail.com>
  • Loading branch information
@nimakarimipour @msridhar
nimakarimipour and msridhar authored Jan 14, 2023
1 parent 2f871a8 commit dcadcfa
Show file tree
Hide file tree
Showing 6 changed files with 70 additions and 30 deletions.
13 changes: 12 additions & 1 deletion nullaway/src/main/java/com/uber/nullaway/ErrorBuilder.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,18 @@ public Description createErrorDescription(
if (nonNullTarget != null) {
SerializationService.serializeFixSuggestion(config, state, nonNullTarget, errorMessage);
}
SerializationService.serializeReportingError(config, state, nonNullTarget, errorMessage);
// For the case of initializer errors, the leaf of state.getPath() may not be the field /
// method on which the error is being reported (since we do a class-wide analysis to find such
// errors). In such cases, the suggestTree is the appropriate field / method tree, so use
// that as the errorTree for serialization.
Tree errorTree =
(suggestTree != null
&& (errorMessage.messageType.equals(FIELD_NO_INIT)
|| errorMessage.messageType.equals(METHOD_NO_INIT)))
? suggestTree
: state.getPath().getLeaf();
SerializationService.serializeReportingError(
config, state, errorTree, nonNullTarget, errorMessage);
}

// #letbuildersbuild
Expand Down
10 changes: 8 additions & 2 deletions nullaway/src/main/java/com/uber/nullaway/fixserialization/SerializationService.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
import com.google.common.base.Preconditions;
import com.google.common.collect.ImmutableMap;
import com.google.errorprone.VisitorState;
import com.sun.source.tree.Tree;
import com.sun.source.util.TreePath;
import com.sun.source.util.Trees;
import com.sun.tools.javac.code.Symbol;
Expand Down Expand Up @@ -114,14 +115,19 @@ public static void serializeFixSuggestion(
*
* @param config NullAway config.
* @param state Visitor state.
* @param errorTree Tree of the element involved in the reporting error.
* @param errorMessage Error caused by the target.
*/
public static void serializeReportingError(
Config config, VisitorState state, @Nullable Symbol target, ErrorMessage errorMessage) {
Config config,
VisitorState state,
Tree errorTree,
@Nullable Symbol target,
ErrorMessage errorMessage) {
Serializer serializer = config.getSerializationConfig().getSerializer();
Preconditions.checkNotNull(
serializer, "Serializer shouldn't be null at this point, error in configuration setting!");
serializer.serializeErrorInfo(new ErrorInfo(state.getPath(), errorMessage, target));
serializer.serializeErrorInfo(new ErrorInfo(state.getPath(), errorTree, errorMessage, target));
}

/**
Expand Down
38 changes: 29 additions & 9 deletions nullaway/src/main/java/com/uber/nullaway/fixserialization/out/ClassAndMemberInfo.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,31 +22,51 @@

package com.uber.nullaway.fixserialization.out;

import com.google.common.base.Preconditions;
import com.google.errorprone.util.ASTHelpers;
import com.sun.source.tree.ClassTree;
import com.sun.source.tree.MethodTree;
import com.sun.source.tree.Tree;
import com.sun.source.tree.VariableTree;
import com.sun.source.util.TreePath;
import com.sun.tools.javac.code.Symbol;
import javax.annotation.Nullable;
import javax.lang.model.element.ElementKind;

/** 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;
@Nullable public 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;
@Nullable private Symbol.ClassSymbol clazz;

public ClassAndMemberInfo(TreePath path) {
Preconditions.checkNotNull(path);
this.path = path;
}

public ClassAndMemberInfo(Tree regionTree) {
// regionTree should either represent a field or a method
if (!(regionTree instanceof MethodTree
|| (regionTree instanceof VariableTree
&& ASTHelpers.getSymbol(regionTree).getKind().equals(ElementKind.FIELD)))) {
throw new RuntimeException(
"not expecting a region tree " + regionTree + " of type " + regionTree.getClass());
}
this.member = ASTHelpers.getSymbol(regionTree);
this.clazz = ASTHelpers.enclosingClass(this.member);
}

/** Finds the class and member where the error / fix is reported according to {@code path}. */
public void findValues() {
if (this.member != null || path == null) {
// Values are already computed.
return;
}
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).
Expand All @@ -56,12 +76,12 @@ public void findValues() {
: 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 =
ClassTree classTree =
path.getLeaf() instanceof ClassTree
? (ClassTree) path.getLeaf()
: ASTHelpers.findEnclosingNode(path, ClassTree.class);
if (clazz != null) {
Symbol.ClassSymbol classSymbol = ASTHelpers.getSymbol(clazz);
if (classTree != null) {
clazz = ASTHelpers.getSymbol(classTree);
if (enclosingMethod != null) {
// It is possible that the computed method is not enclosed by the computed class, e.g., for
// the following case:
Expand All @@ -76,7 +96,7 @@ public void findValues() {
// 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)) {
if (!methodSymbol.isEnclosedBy(clazz)) {
enclosingMethod = null;
}
}
Expand All @@ -86,7 +106,7 @@ public void findValues() {
// 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() && sym.isEnclosedBy(classSymbol)) {
if (sym != null && sym.getKind().isField() && sym.isEnclosedBy(clazz)) {
// Directly on a field declaration.
fieldSymbol = (Symbol.VarSymbol) sym;
} else {
Expand All @@ -96,7 +116,7 @@ public void findValues() {
fieldSymbol = ASTHelpers.getSymbol(fieldDeclTree);
}
}
if (fieldSymbol != null && fieldSymbol.isEnclosedBy(classSymbol)) {
if (fieldSymbol != null && fieldSymbol.isEnclosedBy(clazz)) {
member = fieldSymbol;
}
}
Expand All @@ -109,7 +129,7 @@ public Symbol getMember() {
}

@Nullable
public ClassTree getClazz() {
public Symbol.ClassSymbol getClazz() {
return clazz;
}
}
20 changes: 13 additions & 7 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 @@ -22,7 +22,10 @@

package com.uber.nullaway.fixserialization.out;

import com.google.errorprone.util.ASTHelpers;
import static com.uber.nullaway.ErrorMessage.MessageTypes.FIELD_NO_INIT;
import static com.uber.nullaway.ErrorMessage.MessageTypes.METHOD_NO_INIT;

import com.sun.source.tree.Tree;
import com.sun.source.util.TreePath;
import com.sun.tools.javac.code.Symbol;
import com.sun.tools.javac.util.JCDiagnostic;
Expand Down Expand Up @@ -53,11 +56,16 @@ public class ErrorInfo {
/** Uri to the containing source file where this error is reported. */
private final URI uri;

public ErrorInfo(TreePath path, ErrorMessage errorMessage, @Nullable Symbol nonnullTarget) {
this.classAndMemberInfo = new ClassAndMemberInfo(path);
public ErrorInfo(
TreePath path, Tree errorTree, ErrorMessage errorMessage, @Nullable Symbol nonnullTarget) {
this.classAndMemberInfo =
(errorMessage.getMessageType().equals(FIELD_NO_INIT)
|| errorMessage.getMessageType().equals(METHOD_NO_INIT))
? new ClassAndMemberInfo(errorTree)
: new ClassAndMemberInfo(path);
this.errorMessage = errorMessage;
this.nonnullTarget = nonnullTarget;
JCDiagnostic.DiagnosticPosition treePosition = (JCDiagnostic.DiagnosticPosition) path.getLeaf();
JCDiagnostic.DiagnosticPosition treePosition = (JCDiagnostic.DiagnosticPosition) errorTree;
this.offset = treePosition.getStartPosition();
this.uri = path.getCompilationUnit().getSourceFile().toUri();
}
Expand Down Expand Up @@ -88,9 +96,7 @@ public Symbol getRegionMember() {
*/
@Nullable
public Symbol getRegionClass() {
return classAndMemberInfo.getClazz() == null
? null
: ASTHelpers.getSymbol(classAndMemberInfo.getClazz());
return classAndMemberInfo.getClazz();
}

/**
Expand Down
5 changes: 1 addition & 4 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 @@ -22,7 +22,6 @@

package com.uber.nullaway.fixserialization.out;

import com.google.errorprone.util.ASTHelpers;
import com.sun.source.util.TreePath;
import com.uber.nullaway.ErrorMessage;
import com.uber.nullaway.fixserialization.location.SymbolLocation;
Expand Down Expand Up @@ -76,9 +75,7 @@ public String tabSeparatedToString() {
symbolLocation.tabSeparatedToString(),
errorMessage.getMessageType().toString(),
"nullable",
(classAndMemberInfo.getClazz() == null
? "null"
: ASTHelpers.getSymbol(classAndMemberInfo.getClazz()).flatName()),
(classAndMemberInfo.getClazz() == null ? "null" : classAndMemberInfo.getClazz().flatName()),
(classAndMemberInfo.getMember() == null
? "null"
: classAndMemberInfo.getMember().toString()));
Expand Down
14 changes: 7 additions & 7 deletions nullaway/src/test/java/com/uber/nullaway/NullAwaySerializationTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -678,8 +678,8 @@ public void errorSerializationTest() {
"METHOD_NO_INIT",
"initializer method does not guarantee @NonNull field foo",
"com.uber.Super",
"null",
52,
"Super(boolean)",
180,
"com/uber/Super.java"),
new ErrorDisplay(
"ASSIGN_FIELD_NULLABLE",
Expand Down Expand Up @@ -1370,8 +1370,8 @@ public void errorSerializationTestAnonymousClassField() {
"FIELD_NO_INIT",
"@NonNull field Main$1.bar not initialized",
"com.uber.Main$1",
"null",
119,
"bar",
206,
"com/uber/Main.java",
"FIELD",
"com.uber.Main$1",
Expand Down Expand Up @@ -1412,8 +1412,8 @@ public void errorSerializationTestLocalClassField() {
"FIELD_NO_INIT",
"@NonNull field Main$1Foo.bar not initialized",
"com.uber.Main$1Foo",
"null",
99,
"bar",
199,
"com/uber/Main.java",
"FIELD",
"com.uber.Main$1Foo",
Expand Down Expand Up @@ -1923,7 +1923,7 @@ public void errorSerializationVersion1() {
"METHOD_NO_INIT",
"initializer method does not guarantee @NonNull field foo",
"com.uber.Super",
"null"),
"Super(boolean)"),
new ErrorDisplayV1(
"ASSIGN_FIELD_NULLABLE",
"assigning @Nullable expression to @NonNull field",
Expand Down

0 comments on commit dcadcfa

Please sign in to comment.