Skip to content
Open
156 changes: 141 additions & 15 deletions nullaway/src/main/java/com/uber/nullaway/ErrorBuilder.java
Original file line number Diff line number Diff line change
Expand Up @@ -45,19 +45,24 @@
import com.google.errorprone.util.ASTHelpers;
import com.sun.source.tree.AnnotationTree;
import com.sun.source.tree.ClassTree;
import com.sun.source.tree.ExpressionTree;
import com.sun.source.tree.MethodInvocationTree;
import com.sun.source.tree.MethodTree;
import com.sun.source.tree.ModifiersTree;
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 com.sun.tools.javac.tree.JCTree;
import com.sun.tools.javac.tree.JCTree.JCCompilationUnit;
import com.sun.tools.javac.util.DiagnosticSource;
import com.sun.tools.javac.util.JCDiagnostic.DiagnosticPosition;
import com.uber.nullaway.fixserialization.SerializationService;
import com.uber.nullaway.fixserialization.scanners.OriginScanner;
import com.uber.nullaway.fixserialization.scanners.OriginTrace;
import java.util.Iterator;
import java.util.List;
import java.util.Map;
import java.util.Objects;
import java.util.Set;
import java.util.stream.StreamSupport;
Expand Down Expand Up @@ -97,10 +102,49 @@ public Description createErrorDescription(
ErrorMessage errorMessage,
Description.Builder descriptionBuilder,
VisitorState state,
@Nullable Symbol nonNullTarget) {
NullAway.MayBeNullableInquiry inquiry,
Comment thread
nimakarimipour marked this conversation as resolved.
@Nullable Symbol nonNullTarget,
@Nullable ExpressionTree nullableExpression) {
Tree enclosingSuppressTree = suppressibleNode(state.getPath());
return createErrorDescription(
errorMessage, enclosingSuppressTree, descriptionBuilder, state, nonNullTarget);
return createErrorDescriptionWithInfo(
errorMessage,
enclosingSuppressTree,
descriptionBuilder,
state,
inquiry,
nonNullTarget,
nullableExpression,
Map.of());
}

/**
* create an error description for a nullability warning
*
* @param errorMessage the error message object.
* @param descriptionBuilder the description builder for the error.
* @param state the visitor state (used for e.g. suppression finding).
* @param nonNullTarget if non-null, this error involved a pseudo-assignment of a @Nullable
* expression into a @NonNull target, and this parameter is the Symbol for that target.
* @return the error description
*/
public Description createErrorDescriptionWithInfo(
ErrorMessage errorMessage,
Description.Builder descriptionBuilder,
VisitorState state,
NullAway.MayBeNullableInquiry inquiry,
@Nullable Symbol nonNullTarget,
@Nullable ExpressionTree nullableExpression,
Map<String, String> args) {
Tree enclosingSuppressTree = suppressibleNode(state.getPath());
return createErrorDescriptionWithInfo(
errorMessage,
enclosingSuppressTree,
descriptionBuilder,
state,
inquiry,
nonNullTarget,
nullableExpression,
args);
}

/**
Expand All @@ -119,7 +163,29 @@ public Description createErrorDescription(
@Nullable Tree suggestTree,
Description.Builder descriptionBuilder,
VisitorState state,
@Nullable Symbol nonNullTarget) {
NullAway.MayBeNullableInquiry inquiry,
@Nullable Symbol nonNullTarget,
@Nullable ExpressionTree nullableExpression) {
return createErrorDescriptionWithInfo(
errorMessage,
suggestTree,
descriptionBuilder,
state,
inquiry,
nonNullTarget,
nullableExpression,
Map.of());
}

public Description createErrorDescriptionWithInfo(
ErrorMessage errorMessage,
@Nullable Tree suggestTree,
Description.Builder descriptionBuilder,
VisitorState state,
NullAway.MayBeNullableInquiry inquiry,
@Nullable Symbol nonNullTarget,
@Nullable ExpressionTree nullableExpression,
Map<String, String> args) {
Description.Builder builder = descriptionBuilder.setMessage(errorMessage.message);
String checkName = CORE_CHECK_NAME;
if (errorMessage.messageType.equals(GET_ON_EMPTY_OPTIONAL)) {
Expand All @@ -140,7 +206,25 @@ public Description createErrorDescription(
builder = addSuggestedSuppression(errorMessage, suggestTree, builder, state);
}

Set<OriginTrace> origins = Set.of();
if (config.serializationIsActive()) {
if (nullableExpression != null) {
Symbol nullableExpressionSymbol = ASTHelpers.getSymbol(nullableExpression);
if (nullableExpressionSymbol != null
&& nullableExpressionSymbol.getKind() == ElementKind.LOCAL_VARIABLE) {
// locate assignments to this local variable. Use the nullable expression's start
// position as the bound: the enclosing leaf can start earlier (e.g. an outer
// MethodInvocationTree or enhanced-for), which would cause OriginScanner to drop
// assignments that sit between the leaf start and the actual use.
int diagPos = ((JCTree) nullableExpression).getStartPosition();
if (diagPos < 0) {
diagPos = ((JCTree) state.getPath().getLeaf()).getStartPosition();
}
origins =
new OriginScanner(inquiry, state, diagPos)
.retrieveOrigins(state.findEnclosing(MethodTree.class), nullableExpressionSymbol);
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Comment on lines +219 to +225
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Use OriginScanner.NO_BOUND if both source positions are unavailable.

If both nullableExpression and the current leaf return a negative start position, diagPos stays negative and OriginScanner will treat every assignment as “after” the diagnostic, producing an empty origin set. Fall back to OriginScanner.NO_BOUND instead of leaving the bound invalid.

🐛 Proposed fix
           int diagPos = ((JCTree) nullableExpression).getStartPosition();
           if (diagPos < 0) {
             diagPos = ((JCTree) state.getPath().getLeaf()).getStartPosition();
           }
+          if (diagPos < 0) {
+            diagPos = OriginScanner.NO_BOUND;
+          }
           origins =
               new OriginScanner(inquiry, state, diagPos)
                   .retrieveOrigins(state.findEnclosing(MethodTree.class), nullableExpressionSymbol);
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
int diagPos = ((JCTree) nullableExpression).getStartPosition();
if (diagPos < 0) {
diagPos = ((JCTree) state.getPath().getLeaf()).getStartPosition();
}
origins =
new OriginScanner(inquiry, state, diagPos)
.retrieveOrigins(state.findEnclosing(MethodTree.class), nullableExpressionSymbol);
int diagPos = ((JCTree) nullableExpression).getStartPosition();
if (diagPos < 0) {
diagPos = ((JCTree) state.getPath().getLeaf()).getStartPosition();
}
if (diagPos < 0) {
diagPos = OriginScanner.NO_BOUND;
}
origins =
new OriginScanner(inquiry, state, diagPos)
.retrieveOrigins(state.findEnclosing(MethodTree.class), nullableExpressionSymbol);
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@nullaway/src/main/java/com/uber/nullaway/ErrorBuilder.java` around lines 219
- 225, The diagPos computed for OriginScanner can remain negative if both
((JCTree) nullableExpression).getStartPosition() and ((JCTree)
state.getPath().getLeaf()).getStartPosition() return < 0, causing OriginScanner
to treat every assignment as after the diagnostic and yield empty origins;
change the fallback so that after checking nullableExpression and the current
leaf you set diagPos to OriginScanner.NO_BOUND when both positions are negative
before calling new OriginScanner(inquiry, state, diagPos).retrieveOrigins(...),
ensuring OriginScanner receives a valid non-negative bound constant instead of a
negative position.

}
}
Comment thread
nimakarimipour marked this conversation as resolved.
// 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
Expand All @@ -152,7 +236,7 @@ public Description createErrorDescription(
? suggestTree
: state.getPath().getLeaf();
SerializationService.serializeReportingError(
config, state, errorTree, nonNullTarget, errorMessage);
config, state, errorTree, nonNullTarget, errorMessage, origins, args);
}

// #letbuildersbuild
Expand Down Expand Up @@ -240,25 +324,57 @@ private Description.Builder addSuggestedSuppression(
* expression into a @NonNull target, and this parameter is the Symbol for that target.
* @return the error description.
*/
Description createErrorDescriptionForNullAssignment(
Description createErrorDescriptionForNullAssignmentWithInfo(
ErrorMessage errorMessage,
@Nullable Tree suggestTreeIfCastToNonNull,
Description.Builder descriptionBuilder,
VisitorState state,
@Nullable Symbol nonNullTarget) {
NullAway.MayBeNullableInquiry inquiry,
@Nullable Symbol nonNullTarget,
@Nullable ExpressionTree nullableExpression,
Map<String, String> args) {
if (config.getCastToNonNullMethod() != null) {
return createErrorDescription(
errorMessage, suggestTreeIfCastToNonNull, descriptionBuilder, state, nonNullTarget);
return createErrorDescriptionWithInfo(
errorMessage,
suggestTreeIfCastToNonNull,
descriptionBuilder,
state,
inquiry,
nonNullTarget,
nullableExpression,
args);
} else {
return createErrorDescription(
return createErrorDescriptionWithInfo(
errorMessage,
suppressibleNode(state.getPath()),
descriptionBuilder,
state,
nonNullTarget);
inquiry,
nonNullTarget,
nullableExpression,
args);
}
}

Description createErrorDescriptionForNullAssignment(
ErrorMessage errorMessage,
@Nullable Tree suggestTreeIfCastToNonNull,
Description.Builder descriptionBuilder,
VisitorState state,
NullAway.MayBeNullableInquiry inquiry,
@Nullable Symbol nonNullTarget,
@Nullable ExpressionTree nullableExpression) {
return createErrorDescriptionForNullAssignmentWithInfo(
errorMessage,
suggestTreeIfCastToNonNull,
descriptionBuilder,
state,
inquiry,
nonNullTarget,
nullableExpression,
Map.of());
}

Description.Builder addSuppressWarningsFix(
Tree suggestTree, Description.Builder builder, String suppressionName) {
SuppressWarnings extantSuppressWarnings = null;
Expand Down Expand Up @@ -403,6 +519,7 @@ void reportInitializerError(
Symbol.MethodSymbol methodSymbol,
String message,
VisitorState state,
NullAway.MayBeNullableInquiry inquiry,
Description.Builder descriptionBuilder) {
// Check needed here, despite check in hasPathSuppression because initialization
// checking happens at the class-level (meaning state.getPath() might not include the
Expand All @@ -415,7 +532,8 @@ void reportInitializerError(
Tree methodTree = getTreesInstance(state).getTree(methodSymbol);
ErrorMessage errorMessage = new ErrorMessage(METHOD_NO_INIT, message);
state.reportMatch(
createErrorDescription(errorMessage, methodTree, descriptionBuilder, state, null));
createErrorDescription(
errorMessage, methodTree, descriptionBuilder, state, inquiry, null, null));
}

boolean symbolHasSuppressWarningsAnnotation(Symbol symbol, String suppression) {
Expand Down Expand Up @@ -493,7 +611,11 @@ static String errMsgForInitializer(Set<Element> uninitFields, VisitorState state
return message.toString();
}

void reportInitErrorOnField(Symbol symbol, VisitorState state, Description.Builder builder) {
void reportInitErrorOnField(
Symbol symbol,
VisitorState state,
NullAway.MayBeNullableInquiry inquiry,
Description.Builder builder) {
// Check needed here, despite check in hasPathSuppression because initialization
// checking happens at the class-level (meaning state.getPath() might not include the
// field itself).
Expand All @@ -518,15 +640,19 @@ void reportInitErrorOnField(Symbol symbol, VisitorState state, Description.Build
tree,
builder,
state,
symbol));
inquiry,
symbol,
null));
} else {
state.reportMatch(
createErrorDescription(
new ErrorMessage(FIELD_NO_INIT, "@NonNull field " + fieldName + " not initialized"),
tree,
builder,
state,
symbol));
inquiry,
symbol,
null));
}
}
}
Loading
Loading