diff --git a/nullaway/src/main/java/com/uber/nullaway/ErrorBuilder.java b/nullaway/src/main/java/com/uber/nullaway/ErrorBuilder.java new file mode 100644 index 0000000000..b305d072b1 --- /dev/null +++ b/nullaway/src/main/java/com/uber/nullaway/ErrorBuilder.java @@ -0,0 +1,314 @@ +/* + * Copyright (c) 2019 Uber Technologies, Inc. + * + * Permission is hereby granted, free of charge, to any person obtaining a copy + * of this software and associated documentation files (the "Software"), to deal + * in the Software without restriction, including without limitation the rights + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell + * copies of the Software, and to permit persons to whom the Software is + * furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in + * all copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN + * THE SOFTWARE. + */ + +package com.uber.nullaway; + +import static com.uber.nullaway.ErrorMessage.MessageTypes.FIELD_NO_INIT; +import static com.uber.nullaway.ErrorMessage.MessageTypes.METHOD_NO_INIT; +import static com.uber.nullaway.NullAway.INITIALIZATION_CHECK_NAME; +import static com.uber.nullaway.NullAway.getTreesInstance; + +import com.google.common.base.Joiner; +import com.google.common.collect.Iterables; +import com.google.common.collect.Lists; +import com.google.errorprone.VisitorState; +import com.google.errorprone.fixes.SuggestedFix; +import com.google.errorprone.matchers.Description; +import com.google.errorprone.util.ASTHelpers; +import com.sun.source.tree.AnnotationTree; +import com.sun.source.tree.ClassTree; +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 java.util.List; +import java.util.Set; +import java.util.stream.StreamSupport; +import javax.annotation.Nullable; +import javax.lang.model.element.Element; + +/** A class to construct error message to be displayed after the analysis finds error. */ +public class ErrorBuilder { + + private final Config config; + + /** Checker name that can be used to suppress the warnings. */ + private final String suppressionName; + + /** Additional identifiers for this check, to be checked for in @SuppressWarnings annotations. */ + private final Set allNames; + + ErrorBuilder(Config config, String suppressionName, Set allNames) { + this.config = config; + this.suppressionName = suppressionName; + this.allNames = allNames; + } + + /** + * create an error description for a nullability warning + * + * @param errorMessage the error message object. + * @param path the TreePath to the error location. Used to compute a suggested fix at the + * enclosing method for the error location + * @param descriptionBuilder the description builder for the error. + * @return the error description + */ + Description createErrorDescription( + ErrorMessage errorMessage, TreePath path, Description.Builder descriptionBuilder) { + Tree enclosingSuppressTree = suppressibleNode(path); + return createErrorDescription(errorMessage, enclosingSuppressTree, descriptionBuilder); + } + + /** + * create an error description for a nullability warning + * + * @param errorMessage the error message object. + * @param suggestTree the location at which a fix suggestion should be made + * @param descriptionBuilder the description builder for the error. + * @return the error description + */ + public Description createErrorDescription( + ErrorMessage errorMessage, + @Nullable Tree suggestTree, + Description.Builder descriptionBuilder) { + Description.Builder builder = descriptionBuilder.setMessage(errorMessage.message); + if (config.suggestSuppressions() && suggestTree != null) { + switch (errorMessage.messageType) { + case DEREFERENCE_NULLABLE: + case RETURN_NULLABLE: + case PASS_NULLABLE: + case ASSIGN_FIELD_NULLABLE: + if (config.getCastToNonNullMethod() != null) { + builder = addCastToNonNullFix(suggestTree, builder); + } else { + builder = addSuppressWarningsFix(suggestTree, builder, suppressionName); + } + break; + case CAST_TO_NONNULL_ARG_NONNULL: + builder = removeCastToNonNullFix(suggestTree, builder); + break; + case WRONG_OVERRIDE_RETURN: + builder = addSuppressWarningsFix(suggestTree, builder, suppressionName); + break; + case WRONG_OVERRIDE_PARAM: + builder = addSuppressWarningsFix(suggestTree, builder, suppressionName); + break; + case METHOD_NO_INIT: + case FIELD_NO_INIT: + builder = addSuppressWarningsFix(suggestTree, builder, INITIALIZATION_CHECK_NAME); + break; + case ANNOTATION_VALUE_INVALID: + break; + default: + builder = addSuppressWarningsFix(suggestTree, builder, suppressionName); + } + } + // #letbuildersbuild + return builder.build(); + } + + /** + * create an error description for a generalized @Nullable value to @NonNull location assignment. + * + *

This includes: field assignments, method arguments and method returns + * + * @param errorMessage the error message object. + * @param suggestTreeIfCastToNonNull the location at which a fix suggestion should be made if a + * castToNonNull method is available (usually the expression to cast) + * @param suggestTreePathIfSuppression the location at which a fix suggestion should be made if a + * castToNonNull method is not available (usually the enclosing method, or any place + * where @SuppressWarnings can be added). + * @param descriptionBuilder the description builder for the error. + * @return the error description. + */ + Description createErrorDescriptionForNullAssignment( + ErrorMessage errorMessage, + @Nullable Tree suggestTreeIfCastToNonNull, + @Nullable TreePath suggestTreePathIfSuppression, + Description.Builder descriptionBuilder) { + final Tree enclosingSuppressTree = suppressibleNode(suggestTreePathIfSuppression); + if (config.getCastToNonNullMethod() != null) { + return createErrorDescription(errorMessage, suggestTreeIfCastToNonNull, descriptionBuilder); + } else { + return createErrorDescription(errorMessage, enclosingSuppressTree, descriptionBuilder); + } + } + + Description.Builder addSuppressWarningsFix( + Tree suggestTree, Description.Builder builder, String suppressionName) { + SuppressWarnings extantSuppressWarnings = + ASTHelpers.getAnnotation(suggestTree, SuppressWarnings.class); + SuggestedFix fix; + if (extantSuppressWarnings == null) { + fix = + SuggestedFix.prefixWith( + suggestTree, + "@SuppressWarnings(\"" + + suppressionName + + "\") " + + config.getAutofixSuppressionComment()); + } else { + // need to update the existing list of warnings + final List suppressions = Lists.newArrayList(extantSuppressWarnings.value()); + suppressions.add(suppressionName); + // find the existing annotation, so we can replace it + final ModifiersTree modifiers = + (suggestTree instanceof MethodTree) + ? ((MethodTree) suggestTree).getModifiers() + : ((VariableTree) suggestTree).getModifiers(); + final List annotations = modifiers.getAnnotations(); + // noinspection ConstantConditions + com.google.common.base.Optional suppressWarningsAnnot = + Iterables.tryFind( + annotations, + annot -> annot.getAnnotationType().toString().endsWith("SuppressWarnings")); + if (!suppressWarningsAnnot.isPresent()) { + throw new AssertionError("something went horribly wrong"); + } + final String replacement = + "@SuppressWarnings({" + + Joiner.on(',').join(Iterables.transform(suppressions, s -> '"' + s + '"')) + + "}) " + + config.getAutofixSuppressionComment(); + fix = SuggestedFix.replace(suppressWarningsAnnot.get(), replacement); + } + return builder.addFix(fix); + } + + /** + * Adapted from {@link com.google.errorprone.fixes.SuggestedFixes}. + * + *

TODO: actually use {@link + * com.google.errorprone.fixes.SuggestedFixes#addSuppressWarnings(VisitorState, String)} instead + */ + @Nullable + private Tree suppressibleNode(@Nullable TreePath path) { + if (path == null) { + return null; + } + return StreamSupport.stream(path.spliterator(), false) + .filter( + tree -> + tree instanceof MethodTree + || (tree instanceof ClassTree + && ((ClassTree) tree).getSimpleName().length() != 0) + || tree instanceof VariableTree) + .findFirst() + .orElse(null); + } + + private Description.Builder addCastToNonNullFix(Tree suggestTree, Description.Builder builder) { + final String fullMethodName = config.getCastToNonNullMethod(); + assert fullMethodName != null; + // Add a call to castToNonNull around suggestTree: + final String[] parts = fullMethodName.split("\\."); + final String shortMethodName = parts[parts.length - 1]; + final String replacement = shortMethodName + "(" + suggestTree.toString() + ")"; + final SuggestedFix fix = + SuggestedFix.builder() + .replace(suggestTree, replacement) + .addStaticImport(fullMethodName) // ensure castToNonNull static import + .build(); + return builder.addFix(fix); + } + + private Description.Builder removeCastToNonNullFix( + Tree suggestTree, Description.Builder builder) { + assert suggestTree.getKind() == Tree.Kind.METHOD_INVOCATION; + final MethodInvocationTree invTree = (MethodInvocationTree) suggestTree; + final Symbol.MethodSymbol methodSymbol = ASTHelpers.getSymbol(invTree); + final String qualifiedName = + ASTHelpers.enclosingClass(methodSymbol) + "." + methodSymbol.getSimpleName().toString(); + if (!qualifiedName.equals(config.getCastToNonNullMethod())) { + throw new RuntimeException("suggestTree should point to the castToNonNull invocation."); + } + // Remove the call to castToNonNull: + final SuggestedFix fix = + SuggestedFix.builder() + .replace(suggestTree, invTree.getArguments().get(0).toString()) + .build(); + return builder.addFix(fix); + } + + void reportInitializerError( + Symbol.MethodSymbol methodSymbol, + String message, + VisitorState state, + Description.Builder descriptionBuilder) { + if (symbolHasSuppressInitializationWarningsAnnotation(methodSymbol)) { + return; + } + Tree methodTree = getTreesInstance(state).getTree(methodSymbol); + state.reportMatch( + createErrorDescription( + new ErrorMessage(METHOD_NO_INIT, message), methodTree, descriptionBuilder)); + } + + boolean symbolHasSuppressInitializationWarningsAnnotation(Symbol symbol) { + SuppressWarnings annotation = symbol.getAnnotation(SuppressWarnings.class); + if (annotation != null) { + for (String s : annotation.value()) { + // we need to check for standard suppression here also since we may report initialization + // errors outside the normal ErrorProne match* methods + if (s.equals(INITIALIZATION_CHECK_NAME) || allNames.stream().anyMatch(s::equals)) { + return true; + } + } + } + return false; + } + + static String errMsgForInitializer(Set uninitFields) { + String message = "initializer method does not guarantee @NonNull "; + if (uninitFields.size() == 1) { + message += "field " + uninitFields.iterator().next().toString() + " is initialized"; + } else { + message += "fields " + Joiner.on(", ").join(uninitFields) + " are initialized"; + } + message += " along all control-flow paths (remember to check for exceptions or early returns)."; + return message; + } + + void reportInitErrorOnField(Symbol symbol, VisitorState state, Description.Builder builder) { + if (symbolHasSuppressInitializationWarningsAnnotation(symbol)) { + return; + } + Tree tree = getTreesInstance(state).getTree(symbol); + if (symbol.isStatic()) { + state.reportMatch( + createErrorDescription( + new ErrorMessage( + FIELD_NO_INIT, "@NonNull static field " + symbol + " not initialized"), + tree, + builder)); + } else { + state.reportMatch( + createErrorDescription( + new ErrorMessage(FIELD_NO_INIT, "@NonNull field " + symbol + " not initialized"), + tree, + builder)); + } + } +} diff --git a/nullaway/src/main/java/com/uber/nullaway/ErrorMessage.java b/nullaway/src/main/java/com/uber/nullaway/ErrorMessage.java index 17e6c0f7d3..15d1910cb6 100644 --- a/nullaway/src/main/java/com/uber/nullaway/ErrorMessage.java +++ b/nullaway/src/main/java/com/uber/nullaway/ErrorMessage.java @@ -1,3 +1,25 @@ +/* + * Copyright (c) 2019 Uber Technologies, Inc. + * + * Permission is hereby granted, free of charge, to any person obtaining a copy + * of this software and associated documentation files (the "Software"), to deal + * in the Software without restriction, including without limitation the rights + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell + * copies of the Software, and to permit persons to whom the Software is + * furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in + * all copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN + * THE SOFTWARE. + */ + package com.uber.nullaway; /** Contains error message string to be displayed and the message type from {@link MessageTypes}. */ @@ -5,7 +27,7 @@ public class ErrorMessage { MessageTypes messageType; String message; - ErrorMessage(MessageTypes messageType, String message) { + public ErrorMessage(MessageTypes messageType, String message) { this.messageType = messageType; this.message = message; } diff --git a/nullaway/src/main/java/com/uber/nullaway/NullAway.java b/nullaway/src/main/java/com/uber/nullaway/NullAway.java index 6019081dd2..e0c357cc87 100644 --- a/nullaway/src/main/java/com/uber/nullaway/NullAway.java +++ b/nullaway/src/main/java/com/uber/nullaway/NullAway.java @@ -28,16 +28,14 @@ import static com.sun.source.tree.Tree.Kind.IDENTIFIER; import static com.sun.source.tree.Tree.Kind.PARENTHESIZED; import static com.sun.source.tree.Tree.Kind.TYPE_CAST; +import static com.uber.nullaway.ErrorBuilder.errMsgForInitializer; import com.google.auto.service.AutoService; import com.google.auto.value.AutoValue; -import com.google.common.base.Joiner; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMultimap; import com.google.common.collect.ImmutableSet; -import com.google.common.collect.Iterables; import com.google.common.collect.LinkedHashMultimap; -import com.google.common.collect.Lists; import com.google.common.collect.Multimap; import com.google.common.collect.SetMultimap; import com.google.common.collect.Sets; @@ -69,7 +67,6 @@ import com.sun.source.tree.MemberSelectTree; import com.sun.source.tree.MethodInvocationTree; import com.sun.source.tree.MethodTree; -import com.sun.source.tree.ModifiersTree; import com.sun.source.tree.NewClassTree; import com.sun.source.tree.ParenthesizedTree; import com.sun.source.tree.ReturnTree; @@ -171,7 +168,7 @@ public class NullAway extends BugChecker BugChecker.MemberReferenceTreeMatcher, BugChecker.CompoundAssignmentTreeMatcher { - private static final String INITIALIZATION_CHECK_NAME = "NullAway.Init"; + static final String INITIALIZATION_CHECK_NAME = "NullAway.Init"; private static final Matcher THIS_MATCHER = (expressionTree, state) -> isThisIdentifier(expressionTree); @@ -183,6 +180,8 @@ public class NullAway extends BugChecker private final Config config; + private final ErrorBuilder errorBuilder; + /** * The handler passed to our analysis (usually a {@code CompositeHandler} including handlers for * various APIs. @@ -231,6 +230,7 @@ public NullAway() { handler = Handlers.buildEmpty(); nonAnnotatedMethod = nonAnnotatedMethodCheck(); customSuppressionAnnotations = ImmutableSet.of(); + errorBuilder = new ErrorBuilder(config, "", ImmutableSet.of()); } public NullAway(ErrorProneFlags flags) { @@ -238,6 +238,7 @@ public NullAway(ErrorProneFlags flags) { handler = Handlers.buildDefault(config); nonAnnotatedMethod = nonAnnotatedMethodCheck(); customSuppressionAnnotations = initCustomSuppressions(); + errorBuilder = new ErrorBuilder(config, canonicalName(), allNames()); // workaround for Checker Framework static state bug; // See https://github.com/typetools/checker-framework/issues/1482 AnnotationUtils.clear(); @@ -410,8 +411,11 @@ public Description matchAssignment(AssignmentTree tree, VisitorState state) { ExpressionTree expression = tree.getExpression(); if (mayBeNullExpr(state, expression)) { String message = "assigning @Nullable expression to @NonNull field"; - return createErrorDescriptionForNullAssignment( - MessageTypes.ASSIGN_FIELD_NULLABLE, tree, message, expression, state.getPath()); + return errorBuilder.createErrorDescriptionForNullAssignment( + new ErrorMessage(MessageTypes.ASSIGN_FIELD_NULLABLE, message), + expression, + state.getPath(), + buildDescription(tree)); } return Description.NO_MATCH; } @@ -525,8 +529,10 @@ private Description checkParamOverriding( + "." + overriddenMethod.toString() + " is @Nullable"; - return createErrorDescription( - MessageTypes.WRONG_OVERRIDE_PARAM, memberReferenceTree, message, state.getPath()); + return errorBuilder.createErrorDescription( + new ErrorMessage(MessageTypes.WRONG_OVERRIDE_PARAM, message), + state.getPath(), + buildDescription(memberReferenceTree)); } } // for unbound member references, we need to adjust parameter indices by 1 when matching with @@ -562,7 +568,7 @@ private Description checkParamOverriding( && NullabilityUtil.lambdaParamIsImplicitlyTyped( lambdaExpressionTree.getParameters().get(methodParamInd)); if (!Nullness.hasNullableAnnotation(paramSymbol) && !implicitlyTypedLambdaParam) { - String message = + final String message = "parameter " + paramSymbol.name.toString() + (memberReferenceTree != null ? " of referenced method" : "") @@ -581,14 +587,16 @@ private Description checkParamOverriding( } else { errorTree = getTreesInstance(state).getTree(paramSymbol); } - return createErrorDescription( - MessageTypes.WRONG_OVERRIDE_PARAM, errorTree, message, state.getPath()); + return errorBuilder.createErrorDescription( + new ErrorMessage(MessageTypes.WRONG_OVERRIDE_PARAM, message), + state.getPath(), + buildDescription(errorTree)); } } return Description.NO_MATCH; } - private static Trees getTreesInstance(VisitorState state) { + static Trees getTreesInstance(VisitorState state) { return Trees.instance(JavacProcessingEnvironment.instance(state.context)); } @@ -608,8 +616,11 @@ private Description checkReturnExpression( } if (mayBeNullExpr(state, retExpr)) { String message = "returning @Nullable expression from method with @NonNull return type"; - return createErrorDescriptionForNullAssignment( - MessageTypes.RETURN_NULLABLE, tree, message, retExpr, state.getPath()); + return errorBuilder.createErrorDescriptionForNullAssignment( + new ErrorMessage(MessageTypes.RETURN_NULLABLE, message), + retExpr, + state.getPath(), + buildDescription(tree)); } return Description.NO_MATCH; } @@ -715,8 +726,10 @@ && getComputedNullness(memberReferenceTree).equals(Nullness.NULLABLE)) { memberReferenceTree != null ? memberReferenceTree : getTreesInstance(state).getTree(overridingMethod); - return createErrorDescription( - MessageTypes.WRONG_OVERRIDE_RETURN, errorTree, message, state.getPath()); + return errorBuilder.createErrorDescription( + new ErrorMessage(MessageTypes.WRONG_OVERRIDE_RETURN, message), + state.getPath(), + buildDescription(errorTree)); } // if any parameter in the super method is annotated @Nullable, // overriding method cannot assume @Nonnull @@ -774,7 +787,7 @@ private Description checkForReadBeforeInit(ExpressionTree tree, VisitorState sta // field is either nullable or initialized at declaration return Description.NO_MATCH; } - if (symbolHasSuppressInitalizationWarningsAnnotation(symbol)) { + if (errorBuilder.symbolHasSuppressInitializationWarningsAnnotation(symbol)) { // also suppress checking read before init, as we may not find explicit initialization return Description.NO_MATCH; } @@ -827,11 +840,11 @@ private Description checkPossibleUninitFieldRead( TreePath enclosingBlockPath) { if (!fieldInitializedByPreviousInitializer(symbol, enclosingBlockPath, state) && !fieldAlwaysInitializedBeforeRead(symbol, path, state, enclosingBlockPath)) { - return createErrorDescription( - MessageTypes.NONNULL_FIELD_READ_BEFORE_INIT, - tree, - "read of @NonNull field " + symbol + " before initialization", - path); + ErrorMessage errorMessage = + new ErrorMessage( + MessageTypes.NONNULL_FIELD_READ_BEFORE_INIT, + "read of @NonNull field " + symbol + " before initialization"); + return errorBuilder.createErrorDescription(errorMessage, path, buildDescription(tree)); } else { return Description.NO_MATCH; } @@ -1050,20 +1063,6 @@ private boolean okToReadBeforeInitialized(TreePath path) { return false; } - private boolean symbolHasSuppressInitalizationWarningsAnnotation(Symbol symbol) { - SuppressWarnings annotation = symbol.getAnnotation(SuppressWarnings.class); - if (annotation != null) { - for (String s : annotation.value()) { - // we need to check for standard suppressions here also since we may report initialization - // errors outside the normal ErrorProne match* methods - if (s.equals(INITIALIZATION_CHECK_NAME) || allNames().stream().anyMatch(s::equals)) { - return true; - } - } - } - return false; - } - @Override public Description matchVariable(VariableTree tree, VisitorState state) { if (!matchWithinClass) { @@ -1080,12 +1079,12 @@ public Description matchVariable(VariableTree tree, VisitorState state) { if (initializer != null) { if (!symbol.type.isPrimitive() && !skipDueToFieldAnnotation(symbol)) { if (mayBeNullExpr(state, initializer)) { - return createErrorDescriptionForNullAssignment( - MessageTypes.ASSIGN_FIELD_NULLABLE, - tree, - "assigning @Nullable expression to @NonNull field", - initializer, - state.getPath()); + final ErrorMessage errorMessage = + new ErrorMessage( + MessageTypes.ASSIGN_FIELD_NULLABLE, + "assigning @Nullable expression to @NonNull field"); + return errorBuilder.createErrorDescriptionForNullAssignment( + errorMessage, initializer, state.getPath(), buildDescription(tree)); } } } @@ -1201,12 +1200,12 @@ public Description matchEnhancedForLoop(EnhancedForLoopTree tree, VisitorState s return Description.NO_MATCH; } ExpressionTree expr = tree.getExpression(); + final ErrorMessage errorMessage = + new ErrorMessage( + MessageTypes.DEREFERENCE_NULLABLE, "enhanced-for expression " + expr + " is @Nullable"); if (mayBeNullExpr(state, expr)) { - return createErrorDescription( - MessageTypes.DEREFERENCE_NULLABLE, - expr, - "enhanced-for expression " + expr + " is @Nullable", - state.getPath()); + return errorBuilder.createErrorDescription( + errorMessage, state.getPath(), buildDescription(expr)); } return Description.NO_MATCH; } @@ -1226,8 +1225,10 @@ private Description doUnboxingCheck(VisitorState state, ExpressionTree... expres } if (!type.isPrimitive()) { if (mayBeNullExpr(state, tree)) { - return createErrorDescription( - MessageTypes.UNBOX_NULLABLE, tree, "unboxing of a @Nullable value", state.getPath()); + final ErrorMessage errorMessage = + new ErrorMessage(MessageTypes.UNBOX_NULLABLE, "unboxing of a @Nullable value"); + return errorBuilder.createErrorDescription( + errorMessage, state.getPath(), buildDescription(tree)); } } } @@ -1290,8 +1291,11 @@ private Description handleInvocation( if (mayBeNullExpr(state, actual)) { String message = "passing @Nullable parameter '" + actual.toString() + "' where @NonNull is required"; - return createErrorDescriptionForNullAssignment( - MessageTypes.PASS_NULLABLE, actual, message, actual, state.getPath()); + return errorBuilder.createErrorDescriptionForNullAssignment( + new ErrorMessage(MessageTypes.PASS_NULLABLE, message), + actual, + state.getPath(), + buildDescription(actual)); } } // Check for @NonNull being passed to castToNonNull (if configured) @@ -1333,8 +1337,10 @@ private Description checkCastToNonNullTakesNullable( + qualifiedName + "). This method should only take arguments that NullAway considers @Nullable " + "at the invocation site, but which are known not to be null at runtime."; - return createErrorDescription( - MessageTypes.CAST_TO_NONNULL_ARG_NONNULL, tree, message, tree); + return errorBuilder.createErrorDescription( + new ErrorMessage(MessageTypes.CAST_TO_NONNULL_ARG_NONNULL, message), + tree, + buildDescription(tree)); } } return Description.NO_MATCH; @@ -1378,7 +1384,8 @@ private void checkFieldInitialization(ClassTree tree, VisitorState state) { // report it on the field, except in the case where the class is externalInit and // we have no initializer methods if (!(isExternalInit(classSymbol) && entities.instanceInitializerMethods().isEmpty())) { - reportInitErrorOnField(uninitField, state); + errorBuilder.reportInitErrorOnField( + uninitField, state, buildDescription(getTreesInstance(state).getTree(uninitField))); } } else { // report it on each constructor that does not initialize it @@ -1391,10 +1398,11 @@ private void checkFieldInitialization(ClassTree tree, VisitorState state) { } } for (Element constructorElement : errorFieldsForInitializer.keySet()) { - reportInitializerError( + errorBuilder.reportInitializerError( (Symbol.MethodSymbol) constructorElement, errMsgForInitializer(errorFieldsForInitializer.get(constructorElement)), - state); + state, + buildDescription(getTreesInstance(state).getTree(constructorElement))); } // For static fields Set notInitializedStaticFields = notInitializedStatic(entities, state); @@ -1402,7 +1410,8 @@ private void checkFieldInitialization(ClassTree tree, VisitorState state) { // Always report it on the field for static fields (can't do @SuppressWarnings on a static // initialization block // anyways). - reportInitErrorOnField(uninitSField, state); + errorBuilder.reportInitErrorOnField( + uninitSField, state, buildDescription(getTreesInstance(state).getTree(uninitSField))); } } @@ -1940,163 +1949,12 @@ private Description matchDereference( handler.onPrepareErrorMessage(baseExpression, state, errorMessage); - return createErrorDescriptionForNullAssignment( - errorMessage.messageType, - derefExpression, - errorMessage.message, - baseExpression, - state.getPath()); + return errorBuilder.createErrorDescriptionForNullAssignment( + errorMessage, baseExpression, state.getPath(), buildDescription(derefExpression)); } return Description.NO_MATCH; } - /** - * create an error description for a nullability warning - * - * @param errorType the type of error encountered. - * @param errorLocTree the location of the error - * @param message the error message - * @param path the TreePath to the error location. Used to compute a suggested fix at the - * enclosing method for the error location - * @return the error description - */ - private Description createErrorDescription( - MessageTypes errorType, Tree errorLocTree, String message, TreePath path) { - Tree enclosingSuppressTree = suppressibleNode(path); - return createErrorDescription(errorType, errorLocTree, message, enclosingSuppressTree); - } - - /** - * create an error description for a nullability warning - * - * @param errorType the type of error encountered. - * @param errorLocTree the location of the error - * @param message the error message - * @param suggestTree the location at which a fix suggestion should be made - * @return the error description - */ - public Description createErrorDescription( - MessageTypes errorType, Tree errorLocTree, String message, @Nullable Tree suggestTree) { - Description.Builder builder = buildDescription(errorLocTree).setMessage(message); - if (config.suggestSuppressions() && suggestTree != null) { - switch (errorType) { - case DEREFERENCE_NULLABLE: - case RETURN_NULLABLE: - case PASS_NULLABLE: - case ASSIGN_FIELD_NULLABLE: - if (config.getCastToNonNullMethod() != null) { - builder = addCastToNonNullFix(suggestTree, builder); - } else { - builder = addSuppressWarningsFix(suggestTree, builder, canonicalName()); - } - break; - case CAST_TO_NONNULL_ARG_NONNULL: - builder = removeCastToNonNullFix(suggestTree, builder); - break; - case WRONG_OVERRIDE_RETURN: - builder = addSuppressWarningsFix(suggestTree, builder, canonicalName()); - break; - case WRONG_OVERRIDE_PARAM: - builder = addSuppressWarningsFix(suggestTree, builder, canonicalName()); - break; - case METHOD_NO_INIT: - case FIELD_NO_INIT: - builder = addSuppressWarningsFix(suggestTree, builder, INITIALIZATION_CHECK_NAME); - break; - case ANNOTATION_VALUE_INVALID: - break; - default: - builder = addSuppressWarningsFix(suggestTree, builder, canonicalName()); - } - } - // #letbuildersbuild - return builder.build(); - } - - /** - * create an error description for a generalized @Nullable value to @NonNull location assignment. - * - *

This includes: field assignments, method arguments and method returns - * - * @param errorType the type of error encountered. - * @param errorLocTree the location of the error - * @param message the error message - * @param suggestTreeIfCastToNonNull the location at which a fix suggestion should be made if a - * castToNonNull method is available (usually the expression to cast) - * @param suggestTreePathIfSuppression the location at which a fix suggestion should be made if a - * castToNonNull method is not available (usually the enclosing method, or any place - * where @SuppressWarnings can be added). - * @return the error description. - */ - private Description createErrorDescriptionForNullAssignment( - MessageTypes errorType, - Tree errorLocTree, - String message, - @Nullable Tree suggestTreeIfCastToNonNull, - @Nullable TreePath suggestTreePathIfSuppression) { - Tree enclosingSuppressTree = suppressibleNode(suggestTreePathIfSuppression); - if (config.getCastToNonNullMethod() != null) { - return createErrorDescription(errorType, errorLocTree, message, suggestTreeIfCastToNonNull); - } else { - return createErrorDescription(errorType, errorLocTree, message, enclosingSuppressTree); - } - } - - /** - * Adapted from {@link com.google.errorprone.fixes.SuggestedFixes}. - * - *

TODO: actually use {@link - * com.google.errorprone.fixes.SuggestedFixes#addSuppressWarnings(VisitorState, String)} instead - */ - @Nullable - private static Tree suppressibleNode(@Nullable TreePath path) { - if (path == null) { - return null; - } - return StreamSupport.stream(path.spliterator(), false) - .filter( - tree -> - tree instanceof MethodTree - || (tree instanceof ClassTree - && ((ClassTree) tree).getSimpleName().length() != 0) - || tree instanceof VariableTree) - .findFirst() - .orElse(null); - } - - private Description.Builder addCastToNonNullFix(Tree suggestTree, Description.Builder builder) { - String fullMethodName = config.getCastToNonNullMethod(); - assert fullMethodName != null; - // Add a call to castToNonNull around suggestTree: - String[] parts = fullMethodName.split("\\."); - String shortMethodName = parts[parts.length - 1]; - String replacement = shortMethodName + "(" + suggestTree.toString() + ")"; - SuggestedFix fix = - SuggestedFix.builder() - .replace(suggestTree, replacement) - .addStaticImport(fullMethodName) // ensure castToNonNull static import - .build(); - return builder.addFix(fix); - } - - private Description.Builder removeCastToNonNullFix( - Tree suggestTree, Description.Builder builder) { - assert suggestTree.getKind() == Tree.Kind.METHOD_INVOCATION; - MethodInvocationTree invTree = (MethodInvocationTree) suggestTree; - final Symbol.MethodSymbol methodSymbol = ASTHelpers.getSymbol(invTree); - String qualifiedName = - ASTHelpers.enclosingClass(methodSymbol) + "." + methodSymbol.getSimpleName().toString(); - if (!qualifiedName.equals(config.getCastToNonNullMethod())) { - throw new RuntimeException("suggestTree should point to the castToNonNull invocation."); - } - // Remove the call to castToNonNull: - SuggestedFix fix = - SuggestedFix.builder() - .replace(suggestTree, invTree.getArguments().get(0).toString()) - .build(); - return builder.addFix(fix); - } - @SuppressWarnings("unused") private Description.Builder changeReturnNullabilityFix( Tree suggestTree, Description.Builder builder) { @@ -2122,47 +1980,6 @@ private Description.Builder changeParamNullabilityFix( return builder.addFix(SuggestedFix.prefixWith(suggestTree, "@Nullable ")); } - private Description.Builder addSuppressWarningsFix( - Tree suggestTree, Description.Builder builder, String checkerName) { - SuppressWarnings extantSuppressWarnings = - ASTHelpers.getAnnotation(suggestTree, SuppressWarnings.class); - SuggestedFix fix; - if (extantSuppressWarnings == null) { - fix = - SuggestedFix.prefixWith( - suggestTree, - "@SuppressWarnings(\"" - + checkerName - + "\") " - + config.getAutofixSuppressionComment()); - } else { - // need to update the existing list of warnings - List suppressions = Lists.newArrayList(extantSuppressWarnings.value()); - suppressions.add(checkerName); - // find the existing annotation, so we can replace it - ModifiersTree modifiers = - (suggestTree instanceof MethodTree) - ? ((MethodTree) suggestTree).getModifiers() - : ((VariableTree) suggestTree).getModifiers(); - List annotations = modifiers.getAnnotations(); - // noinspection ConstantConditions - com.google.common.base.Optional suppressWarningsAnnot = - Iterables.tryFind( - annotations, - annot -> annot.getAnnotationType().toString().endsWith("SuppressWarnings")); - if (!suppressWarningsAnnot.isPresent()) { - throw new AssertionError("something went horribly wrong"); - } - String replacement = - "@SuppressWarnings({" - + Joiner.on(',').join(Iterables.transform(suppressions, s -> '"' + s + '"')) - + "}) " - + config.getAutofixSuppressionComment(); - fix = SuggestedFix.replace(suppressWarningsAnnot.get(), replacement); - } - return builder.addFix(fix); - } - @SuppressWarnings("unused") private int depth(ExpressionTree expression) { switch (expression.getKind()) { @@ -2186,6 +2003,10 @@ private static boolean isThisIdentifier(ExpressionTree expressionTree) { && ((IdentifierTree) expressionTree).getName().toString().equals("this"); } + public ErrorBuilder getErrorBuilder() { + return errorBuilder; + } + /** * strip out enclosing parentheses and type casts. * @@ -2253,49 +2074,6 @@ private Symbol.MethodSymbol getClosestOverriddenMethod(Symbol.MethodSymbol metho return null; } - private void reportInitializerError( - Symbol.MethodSymbol methodSymbol, String message, VisitorState state) { - if (symbolHasSuppressInitalizationWarningsAnnotation(methodSymbol)) { - return; - } - Tree methodTree = getTreesInstance(state).getTree(methodSymbol); - state.reportMatch( - createErrorDescription(MessageTypes.METHOD_NO_INIT, methodTree, message, methodTree)); - } - - private String errMsgForInitializer(Set uninitFields) { - String message = "initializer method does not guarantee @NonNull "; - if (uninitFields.size() == 1) { - message += "field " + uninitFields.iterator().next().toString() + " is initialized"; - } else { - message += "fields " + Joiner.on(", ").join(uninitFields) + " are initialized"; - } - message += " along all control-flow paths (remember to check for exceptions or early returns)."; - return message; - } - - private void reportInitErrorOnField(Symbol symbol, VisitorState state) { - if (symbolHasSuppressInitalizationWarningsAnnotation(symbol)) { - return; - } - Tree tree = getTreesInstance(state).getTree(symbol); - if (symbol.isStatic()) { - state.reportMatch( - createErrorDescription( - MessageTypes.FIELD_NO_INIT, - tree, - "@NonNull static field " + symbol + " not initialized", - tree)); - } else { - state.reportMatch( - createErrorDescription( - MessageTypes.FIELD_NO_INIT, - tree, - "@NonNull field " + symbol + " not initialized", - tree)); - } - } - /** * Returns the computed nullness information from an expression. If none is available, it returns * Nullable. diff --git a/nullaway/src/main/java/com/uber/nullaway/dataflow/AccessPathNullnessAnalysis.java b/nullaway/src/main/java/com/uber/nullaway/dataflow/AccessPathNullnessAnalysis.java index 72d6000160..55b168a7be 100644 --- a/nullaway/src/main/java/com/uber/nullaway/dataflow/AccessPathNullnessAnalysis.java +++ b/nullaway/src/main/java/com/uber/nullaway/dataflow/AccessPathNullnessAnalysis.java @@ -48,8 +48,6 @@ public final class AccessPathNullnessAnalysis { private final DataFlow dataFlow; - private static String OPTIONAL_PATH = "java.util.Optional"; - // Use #instance to instantiate private AccessPathNullnessAnalysis( Predicate methodReturnsNonNull, diff --git a/nullaway/src/main/java/com/uber/nullaway/handlers/ContractHandler.java b/nullaway/src/main/java/com/uber/nullaway/handlers/ContractHandler.java index 4a1c4aefcb..e32bf9fa62 100644 --- a/nullaway/src/main/java/com/uber/nullaway/handlers/ContractHandler.java +++ b/nullaway/src/main/java/com/uber/nullaway/handlers/ContractHandler.java @@ -22,6 +22,8 @@ package com.uber.nullaway.handlers; +import static com.google.errorprone.BugCheckerInfo.buildDescriptionFromChecker; + import com.google.common.base.Preconditions; import com.google.errorprone.VisitorState; import com.google.errorprone.util.ASTHelpers; @@ -221,11 +223,12 @@ private void reportMatch(Tree errorLocTree, String message) { assert this.analysis != null && this.state != null; if (this.analysis != null && this.state != null) { this.state.reportMatch( - this.analysis.createErrorDescription( - ErrorMessage.MessageTypes.ANNOTATION_VALUE_INVALID, - errorLocTree, - message, - errorLocTree)); + analysis + .getErrorBuilder() + .createErrorDescription( + new ErrorMessage(ErrorMessage.MessageTypes.ANNOTATION_VALUE_INVALID, message), + errorLocTree, + buildDescriptionFromChecker(errorLocTree, analysis))); } }