diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/CheckReturnValue.java b/core/src/main/java/com/google/errorprone/bugpatterns/CheckReturnValue.java index 23dd94b0860..c687ab027b3 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/CheckReturnValue.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/CheckReturnValue.java @@ -17,10 +17,15 @@ package com.google.errorprone.bugpatterns; import static com.google.errorprone.BugPattern.SeverityLevel.ERROR; +import static com.google.errorprone.bugpatterns.checkreturnvalue.ExternalCanIgnoreReturnValue.externalIgnoreList; +import static com.google.errorprone.bugpatterns.checkreturnvalue.ProtoRules.protoBuilders; +import static com.google.errorprone.bugpatterns.checkreturnvalue.ResultUsePolicy.EXPECTED; +import static com.google.errorprone.bugpatterns.checkreturnvalue.ResultUsePolicy.OPTIONAL; +import static com.google.errorprone.bugpatterns.checkreturnvalue.Rules.globalDefault; +import static com.google.errorprone.bugpatterns.checkreturnvalue.Rules.mapAnnotationSimpleName; import static com.google.errorprone.util.ASTHelpers.getSymbol; import static com.google.errorprone.util.ASTHelpers.hasDirectAnnotationWithSimpleName; -import com.google.auto.value.AutoValue; import com.google.common.collect.ImmutableMap; import com.google.errorprone.BugPattern; import com.google.errorprone.ErrorProneFlags; @@ -28,7 +33,8 @@ import com.google.errorprone.annotations.CanIgnoreReturnValue; import com.google.errorprone.bugpatterns.BugChecker.ClassTreeMatcher; import com.google.errorprone.bugpatterns.BugChecker.MethodTreeMatcher; -import com.google.errorprone.bugpatterns.checkreturnvalue.ExternalCanIgnoreReturnValue; +import com.google.errorprone.bugpatterns.checkreturnvalue.ResultUsePolicy; +import com.google.errorprone.bugpatterns.checkreturnvalue.ResultUsePolicyEvaluator; import com.google.errorprone.matchers.Description; import com.google.errorprone.matchers.Matcher; import com.google.errorprone.util.ASTHelpers; @@ -39,7 +45,6 @@ import com.sun.source.tree.MethodTree; import com.sun.source.tree.NewClassTree; import com.sun.tools.javac.code.Symbol; -import com.sun.tools.javac.code.Symbol.ClassSymbol; import com.sun.tools.javac.code.Symbol.MethodSymbol; import java.util.Optional; import javax.lang.model.element.ElementKind; @@ -58,64 +63,29 @@ public class CheckReturnValue extends AbstractReturnValueIgnored private static final String CHECK_RETURN_VALUE = "CheckReturnValue"; private static final String CAN_IGNORE_RETURN_VALUE = "CanIgnoreReturnValue"; - private static final ImmutableMap ANNOTATIONS = - ImmutableMap.of( - CHECK_RETURN_VALUE, - CrvOpinion.SHOULD_BE_CRV, - CAN_IGNORE_RETURN_VALUE, - CrvOpinion.SHOULD_BE_CIRV); - - private static Optional controllingAnnotation( - MethodSymbol sym, VisitorState visitorState) { - // In priority order, we want a source-local annotation on the symbol, then the external API - // file, then the enclosing elements of sym. - return findAnnotation(sym) - .or( - () -> - asAnnotationFromConfig(sym, visitorState) - .or(() -> findAnnotationOnEnclosingSymbols(sym.owner))); - } - - private static Optional findAnnotation(Symbol sym) { - return ANNOTATIONS.entrySet().stream() - .filter(annoSpec -> hasDirectAnnotationWithSimpleName(sym, annoSpec.getKey())) - .map(annotation -> FoundAnnotation.create(scope(sym), annotation.getValue())) - .findFirst(); - } - - private static Optional findAnnotationOnEnclosingSymbols(Symbol sym) { - return ASTHelpers.enclosingElements(sym).flatMap(e -> findAnnotation(e).stream()).findFirst(); - } - - private static Optional asAnnotationFromConfig( - MethodSymbol sym, VisitorState visitorState) { - if (ExternalCanIgnoreReturnValue.externallyConfiguredCirvAnnotation(sym, visitorState)) { - return Optional.of( - FoundAnnotation.create(AnnotationScope.METHOD_EXTERNAL_ANNO, CrvOpinion.SHOULD_BE_CIRV)); - } - return Optional.empty(); - } - - private static AnnotationScope scope(Symbol sym) { - if (sym instanceof MethodSymbol) { - return AnnotationScope.METHOD; - } else if (sym instanceof ClassSymbol) { - return AnnotationScope.CLASS; - } else { - return AnnotationScope.PACKAGE; - } - } - static final String CHECK_ALL_CONSTRUCTORS = "CheckReturnValue:CheckAllConstructors"; static final String CHECK_ALL_METHODS = "CheckReturnValue:CheckAllMethods"; - private final boolean checkAllConstructors; - private final boolean checkAllMethods; + private final Optional constructorPolicy; + private final Optional methodPolicy; + private final ResultUsePolicyEvaluator evaluator; public CheckReturnValue(ErrorProneFlags flags) { super(flags); - this.checkAllConstructors = flags.getBoolean(CHECK_ALL_CONSTRUCTORS).orElse(false); - this.checkAllMethods = flags.getBoolean(CHECK_ALL_METHODS).orElse(false); + this.constructorPolicy = defaultPolicy(flags, CHECK_ALL_CONSTRUCTORS); + this.methodPolicy = defaultPolicy(flags, CHECK_ALL_METHODS); + + this.evaluator = + ResultUsePolicyEvaluator.create( + mapAnnotationSimpleName(CHECK_RETURN_VALUE, EXPECTED), + mapAnnotationSimpleName(CAN_IGNORE_RETURN_VALUE, OPTIONAL), + protoBuilders(), + externalIgnoreList(), + globalDefault(methodPolicy, constructorPolicy)); + } + + private static Optional defaultPolicy(ErrorProneFlags flags, String flag) { + return flags.getBoolean(flag).map(check -> check ? EXPECTED : OPTIONAL); } /** @@ -124,49 +94,11 @@ public CheckReturnValue(ErrorProneFlags flags) { */ @Override public Matcher specializedMatcher() { - return (tree, state) -> { - Optional maybeMethod = methodToInspect(tree); - if (maybeMethod.isEmpty()) { - return false; - } - - return crvOpinionForMethod(maybeMethod.get(), tree, state) - .map(CrvOpinion.SHOULD_BE_CRV::equals) - .orElse(false); - }; - } - - private Optional crvOpinionForMethod( - MethodSymbol sym, ExpressionTree tree, VisitorState state) { - Optional annotationForSymbol = controllingAnnotation(sym, state); - if (annotationForSymbol.isPresent()) { - return annotationForSymbol.map(FoundAnnotation::checkReturnValueOpinion); - } - - // In the event there is no opinion from annotations, we use the checker's configuration to - // decide what the "default" for the universe is. - if (checkAllConstructors && sym.isConstructor()) { - return Optional.of(CrvOpinion.SHOULD_BE_CRV); - } - - if (checkAllMethods) { - // There are carveouts for methods we know to be ignorable by default. - if (ModifiedButNotUsed.FLUENT_SETTER.matches(tree, state)) { - return Optional.of(CrvOpinion.SHOULD_BE_CIRV); - } - return Optional.of(CrvOpinion.SHOULD_BE_CRV); - } - // NB: You might consider this SHOULD_BE_CIRV (here, where the default is to not check any - // unannotated method, and no annotation exists) - // However, we also use this judgement in the "should be covered" part of the analysis, so we - // want to distinguish the states of "the world is CRV-by-default, but this method is annotated" - // from "the world is CIRV-by-default, and this method was unannotated". - return Optional.empty(); - } - - enum CrvOpinion { - SHOULD_BE_CRV, - SHOULD_BE_CIRV + return (tree, state) -> + methodToInspect(tree) + .map(method -> evaluator.evaluate(method, state)) + .orElse(OPTIONAL) + .equals(EXPECTED); } private static Optional methodToInspect(ExpressionTree tree) { @@ -205,14 +137,18 @@ private static Optional methodSymbol(ExpressionTree tree) { @Override public boolean isCovered(ExpressionTree tree, VisitorState state) { - return methodSymbol(tree).flatMap(sym -> crvOpinionForMethod(sym, tree, state)).isPresent(); + return methodToInspect(tree).stream() + .flatMap(method -> evaluator.evaluations(method, state)) + .findFirst() + .isPresent(); } @Override public ImmutableMap getMatchMetadata(ExpressionTree tree, VisitorState state) { - return methodSymbol(tree) - .flatMap(sym -> controllingAnnotation(sym, state)) - .map(found -> ImmutableMap.of("annotation_scope", found.scope())) + return methodToInspect(tree).stream() + .flatMap(method -> evaluator.evaluations(method, state)) + .findFirst() + .map(evaluation -> ImmutableMap.of("annotation_scope", evaluation.scope())) .orElse(ImmutableMap.of()); } @@ -234,6 +170,8 @@ public Description matchMethod(MethodTree tree, VisitorState state) { boolean checkReturn = hasDirectAnnotationWithSimpleName(method, CHECK_RETURN_VALUE); boolean canIgnore = hasDirectAnnotationWithSimpleName(method, CAN_IGNORE_RETURN_VALUE); + // TODO(cgdecker): We can check this with evaluator.checkForConflicts now, though I want to + // think more about how we build and format error messages in that. if (checkReturn && canIgnore) { return buildDescription(tree).setMessage(String.format(BOTH_ERROR, "method")).build(); } @@ -274,7 +212,7 @@ && hasDirectAnnotationWithSimpleName(ASTHelpers.getSymbol(tree), CAN_IGNORE_RETU @Override protected String getMessage(Name name) { return String.format( - checkAllMethods + methodPolicy.orElse(OPTIONAL).equals(EXPECTED) ? "Ignored return value of '%s', which wasn't annotated with @CanIgnoreReturnValue" : "Ignored return value of '%s', which is annotated with @CheckReturnValue", name); @@ -282,7 +220,7 @@ protected String getMessage(Name name) { @Override protected Description describeReturnValueIgnored(NewClassTree newClassTree, VisitorState state) { - return checkAllConstructors + return constructorPolicy.orElse(OPTIONAL).equals(EXPECTED) ? buildDescription(newClassTree) .setMessage( String.format( @@ -292,22 +230,4 @@ protected Description describeReturnValueIgnored(NewClassTree newClassTree, Visi .build() : super.describeReturnValueIgnored(newClassTree, state); } - - @AutoValue - abstract static class FoundAnnotation { - static FoundAnnotation create(AnnotationScope scope, CrvOpinion opinion) { - return new AutoValue_CheckReturnValue_FoundAnnotation(scope, opinion); - } - - abstract AnnotationScope scope(); - - abstract CrvOpinion checkReturnValueOpinion(); - } - - enum AnnotationScope { - METHOD, - METHOD_EXTERNAL_ANNO, - CLASS, - PACKAGE - } } diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/checkreturnvalue/ExternalCanIgnoreReturnValue.java b/core/src/main/java/com/google/errorprone/bugpatterns/checkreturnvalue/ExternalCanIgnoreReturnValue.java index ce7bbaea462..72e8ea2bcf9 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/checkreturnvalue/ExternalCanIgnoreReturnValue.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/checkreturnvalue/ExternalCanIgnoreReturnValue.java @@ -23,6 +23,7 @@ import com.google.common.io.LineProcessor; import com.google.common.io.MoreFiles; import com.google.errorprone.VisitorState; +import com.google.errorprone.bugpatterns.checkreturnvalue.ResultUseRule.MethodRule; import com.google.errorprone.suppliers.Supplier; import com.sun.tools.javac.code.Symbol.MethodSymbol; import java.io.IOException; @@ -31,7 +32,13 @@ import java.util.Optional; /** External source of information about @CanIgnoreReturnValue-equivalent API's. */ -public class ExternalCanIgnoreReturnValue { +public final class ExternalCanIgnoreReturnValue extends MethodRule { + + /** Returns a rule using an external list of APIs to ignore. */ + public static ResultUseRule externalIgnoreList() { + return new ExternalCanIgnoreReturnValue(); + } + private ExternalCanIgnoreReturnValue() {} private static final String EXTERNAL_API_EXCLUSION_LIST = "CheckReturnValue:ApiExclusionList"; @@ -45,6 +52,18 @@ private ExternalCanIgnoreReturnValue() {} .get(EXTERNAL_API_EXCLUSION_LIST) .map(ExternalCanIgnoreReturnValue::tryLoadingConfigFile)); + @Override + public String id() { + return "EXTERNAL_API_EXCLUSION_LIST"; + } + + @Override + public Optional evaluateMethod(MethodSymbol method, VisitorState state) { + return externallyConfiguredCirvAnnotation(method, state) + ? Optional.of(ResultUsePolicy.OPTIONAL) + : Optional.empty(); + } + public static boolean externallyConfiguredCirvAnnotation(MethodSymbol m, VisitorState s) { return EXTERNAL_RESOURCE.get(s).map(protoList -> protoList.methodMatches(m, s)).orElse(false); } diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/checkreturnvalue/ProtoRules.java b/core/src/main/java/com/google/errorprone/bugpatterns/checkreturnvalue/ProtoRules.java new file mode 100644 index 00000000000..cbccff5c29d --- /dev/null +++ b/core/src/main/java/com/google/errorprone/bugpatterns/checkreturnvalue/ProtoRules.java @@ -0,0 +1,70 @@ +/* + * Copyright 2022 The Error Prone Authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.google.errorprone.bugpatterns.checkreturnvalue; + +import com.google.errorprone.VisitorState; +import com.google.errorprone.bugpatterns.checkreturnvalue.ResultUseRule.MethodRule; +import com.google.errorprone.suppliers.Supplier; +import com.google.errorprone.util.ASTHelpers; +import com.sun.tools.javac.code.Symbol.MethodSymbol; +import com.sun.tools.javac.code.Type; +import java.util.Optional; +import java.util.regex.Pattern; + +/** Rules for methods on proto messages and builders. */ +public final class ProtoRules { + + private ProtoRules() {} + + /** + * Returns a rule that handles proto builders, making their fluent setter methods' results + * ignorable. + */ + public static ResultUseRule protoBuilders() { + return new ProtoBuilder(); + } + + private static final Supplier MESSAGE_LITE_BUILDER = + supplier("com.google.protobuf.MessageLite.Builder"); + + // TODO(cgdecker): Move proto rules from IgnoredPureGetter and ReturnValueIgnored here + + /** Rules for methods on proto builders. */ + private static final class ProtoBuilder extends MethodRule { + private static final Pattern SETTERS = Pattern.compile("(add|clear|remove|set|put).+"); + + @Override + public String id() { + return "PROTO_BUILDER"; + } + + @Override + public Optional evaluateMethod(MethodSymbol method, VisitorState state) { + Type ownerType = method.owner.type; + if (ASTHelpers.isSubtype(ownerType, MESSAGE_LITE_BUILDER.get(state), state)) { + if (SETTERS.matcher(method.name).matches()) { + return Optional.of(ResultUsePolicy.OPTIONAL); + } + } + return Optional.empty(); + } + } + + private static Supplier supplier(String name) { + return VisitorState.memoize(s -> s.getTypeFromString(name)); + } +} diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/checkreturnvalue/ResultUsePolicy.java b/core/src/main/java/com/google/errorprone/bugpatterns/checkreturnvalue/ResultUsePolicy.java new file mode 100644 index 00000000000..145844b618b --- /dev/null +++ b/core/src/main/java/com/google/errorprone/bugpatterns/checkreturnvalue/ResultUsePolicy.java @@ -0,0 +1,30 @@ +/* + * Copyright 2022 The Error Prone Authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.google.errorprone.bugpatterns.checkreturnvalue; + +/** Policy for use of a method or constructor's result. */ +public enum ResultUsePolicy { + /** + * Use of the result is expected except in certain contexts where the method is being used in a + * way such that not using the result is likely correct. Examples include when the result type at + * the callsite is {@code java.lang.Void} and when the surrounding context seems to be testing + * that the method throws an exception. + */ + EXPECTED, + /** Use of the result is optional. */ + OPTIONAL, +} diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/checkreturnvalue/ResultUsePolicyEvaluator.java b/core/src/main/java/com/google/errorprone/bugpatterns/checkreturnvalue/ResultUsePolicyEvaluator.java new file mode 100644 index 00000000000..bf79646afa7 --- /dev/null +++ b/core/src/main/java/com/google/errorprone/bugpatterns/checkreturnvalue/ResultUsePolicyEvaluator.java @@ -0,0 +1,132 @@ +/* + * Copyright 2022 The Error Prone Authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.google.errorprone.bugpatterns.checkreturnvalue; + +import static com.google.common.collect.ImmutableListMultimap.toImmutableListMultimap; +import static com.google.errorprone.bugpatterns.checkreturnvalue.ResultUsePolicy.OPTIONAL; +import static com.google.errorprone.bugpatterns.checkreturnvalue.ResultUseRule.RuleScope.ENCLOSING_ELEMENTS; +import static com.google.errorprone.bugpatterns.checkreturnvalue.ResultUseRule.RuleScope.GLOBAL; +import static com.google.errorprone.bugpatterns.checkreturnvalue.ResultUseRule.RuleScope.METHOD; +import static java.util.Map.entry; + +import com.google.common.collect.ImmutableListMultimap; +import com.google.errorprone.VisitorState; +import com.google.errorprone.annotations.CanIgnoreReturnValue; +import com.google.errorprone.bugpatterns.checkreturnvalue.ResultUseRule.Evaluation; +import com.google.errorprone.bugpatterns.checkreturnvalue.ResultUseRule.RuleScope; +import com.sun.tools.javac.code.Symbol.MethodSymbol; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.List; +import java.util.Map.Entry; +import java.util.stream.Stream; +import javax.lang.model.element.ElementKind; + +/** + * Evaluates methods and their enclosing classes and packages to determine a {@link ResultUsePolicy} + * for the methods. + */ +public final class ResultUsePolicyEvaluator { + + /** Creates a new {@link ResultUsePolicyEvaluator} using the given {@code rules}. */ + public static ResultUsePolicyEvaluator create(ResultUseRule... rules) { + return create(Arrays.asList(rules)); + } + + /** Creates a new {@link ResultUsePolicyEvaluator} using the given {@code rules}. */ + public static ResultUsePolicyEvaluator create(Iterable rules) { + return builder().addRules(rules).build(); + } + + /** Returns a new {@link Builder} for creating a {@link ResultUsePolicyEvaluator}. */ + public static ResultUsePolicyEvaluator.Builder builder() { + return new Builder(); + } + + /** Map of method symbol kinds to the scopes that should be evaluated for that kind of symbol. */ + private static final ImmutableListMultimap SCOPES = + ImmutableListMultimap.builder() + .putAll(ElementKind.METHOD, METHOD, ENCLOSING_ELEMENTS, GLOBAL) + // TODO(cgdecker): Constructors in particular (though really all methods I think) should + // not be able to get a policy of OPTIONAL from enclosing elements. Only defaults should + // come from enclosing elements, and there should only be one default policy (EXPECTED). + .putAll(ElementKind.CONSTRUCTOR, METHOD, ENCLOSING_ELEMENTS, GLOBAL) + .build(); + + /** All the rules for this evaluator, indexed by the scopes they apply to. */ + private final ImmutableListMultimap rules; + + private ResultUsePolicyEvaluator(Builder builder) { + this.rules = + builder.rules.stream() + .flatMap(rule -> rule.scopes().stream().map(scope -> entry(scope, rule))) + .collect(toImmutableListMultimap(Entry::getKey, Entry::getValue)); + } + + /** + * Evaluates the given {@code method} and returns a single {@link ResultUsePolicy} that should + * apply to it. + */ + public ResultUsePolicy evaluate(MethodSymbol method, VisitorState state) { + return policies(method, state).findFirst().orElse(OPTIONAL); + } + + private Stream policies(MethodSymbol method, VisitorState state) { + return SCOPES.get(method.getKind()).stream() + .flatMap(scope -> scope.policies(method, state, rules)); + } + + /** + * Returns a stream of {@link Evaluation}s made by rules starting from the given {@code method}. + */ + public Stream evaluations(MethodSymbol method, VisitorState state) { + return SCOPES.get(method.getKind()).stream() + .flatMap(scope -> scope.evaluations(method, state, rules)); + } + + /** Builder for {@link ResultUsePolicyEvaluator}. */ + public static final class Builder { + private final List rules = new ArrayList<>(); + + private Builder() {} + + /** Adds the given {@code rule}. */ + @CanIgnoreReturnValue + public Builder addRule(ResultUseRule rule) { + this.rules.add(rule); + return this; + } + + /** Adds all the given {@code rules}. */ + @CanIgnoreReturnValue + public Builder addRules(ResultUseRule... rules) { + return addRules(Arrays.asList(rules)); + } + + /** Adds all the given {@code rules}. */ + @CanIgnoreReturnValue + public Builder addRules(Iterable rules) { + rules.forEach(this::addRule); + return this; + } + + /** Builds a new {@link ResultUsePolicyEvaluator}. */ + public ResultUsePolicyEvaluator build() { + return new ResultUsePolicyEvaluator(this); + } + } +} diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/checkreturnvalue/ResultUseRule.java b/core/src/main/java/com/google/errorprone/bugpatterns/checkreturnvalue/ResultUseRule.java new file mode 100644 index 00000000000..c7aec4f58a3 --- /dev/null +++ b/core/src/main/java/com/google/errorprone/bugpatterns/checkreturnvalue/ResultUseRule.java @@ -0,0 +1,191 @@ +/* + * Copyright 2022 The Error Prone Authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.google.errorprone.bugpatterns.checkreturnvalue; + +import static com.google.errorprone.bugpatterns.checkreturnvalue.ResultUseRule.RuleScope.ENCLOSING_ELEMENTS; +import static com.google.errorprone.bugpatterns.checkreturnvalue.ResultUseRule.RuleScope.GLOBAL; +import static com.google.errorprone.bugpatterns.checkreturnvalue.ResultUseRule.RuleScope.METHOD; +import static com.google.errorprone.util.ASTHelpers.enclosingElements; + +import com.google.auto.value.AutoValue; +import com.google.common.collect.ImmutableSet; +import com.google.common.collect.ListMultimap; +import com.google.errorprone.VisitorState; +import com.sun.tools.javac.code.Symbol; +import com.sun.tools.javac.code.Symbol.ClassSymbol; +import com.sun.tools.javac.code.Symbol.MethodSymbol; +import com.sun.tools.javac.code.Symbol.PackageSymbol; +import java.util.List; +import java.util.Optional; +import java.util.stream.Stream; + +/** A rule for determining {@link ResultUsePolicy} for methods and/or constructors. */ +public abstract class ResultUseRule { + + // TODO(cgdecker): Switching to a model where scopes can only either be in a "marked" or + // "unmarked" state and only methods can have a specific policy will simplify all of this a lot. + + private ResultUseRule() {} // only allow the subclasses below + + /** An ID for uniquely identifying this rule. */ + public abstract String id(); + + /** The scopes this rule applies to. */ + public abstract ImmutableSet scopes(); + + /** Evaluates the given {@code symbol} and optionally returns a {@link ResultUsePolicy} for it. */ + public abstract Optional evaluate(Symbol symbol, VisitorState state); + + /** Evaluates the given symbol and optionally returns an {@link Evaluation} of it. */ + public final Optional evaluate(RuleScope scope, Symbol symbol, VisitorState state) { + return evaluate(symbol, state).map(policy -> Evaluation.create(this, scope, symbol, policy)); + } + + @Override + public final String toString() { + return id(); + } + + /** + * A rule that evaluates methods and constructors to determine a {@link ResultUsePolicy} for them. + */ + public abstract static class MethodRule extends ResultUseRule { + private static final ImmutableSet SCOPES = ImmutableSet.of(METHOD); + + @Override + public final ImmutableSet scopes() { + return SCOPES; + } + + /** + * Evaluates the given {@code method} and optionally returns a {@link ResultUsePolicy} for it. + */ + public abstract Optional evaluateMethod( + MethodSymbol method, VisitorState state); + + @Override + public final Optional evaluate(Symbol symbol, VisitorState state) { + return symbol instanceof MethodSymbol + ? evaluateMethod((MethodSymbol) symbol, state) + : Optional.empty(); + } + } + + /** + * A rule that evaluates symbols of any kind to determine a {@link ResultUsePolicy} to associate + * with them. + */ + public abstract static class SymbolRule extends ResultUseRule { + private static final ImmutableSet SCOPES = + ImmutableSet.of(METHOD, ENCLOSING_ELEMENTS); + + @Override + public final ImmutableSet scopes() { + return SCOPES; + } + } + + /** + * A global rule that is evaluated when none of the more specific rules determine a {@link + * ResultUsePolicy} for a method. + */ + public abstract static class GlobalRule extends ResultUseRule { + private static final ImmutableSet SCOPES = ImmutableSet.of(GLOBAL); + + @Override + public final ImmutableSet scopes() { + return SCOPES; + } + + /** Optionally returns a global policy for methods or constructors. */ + public abstract Optional evaluate(boolean constructor, VisitorState state); + + @Override + public final Optional evaluate(Symbol symbol, VisitorState state) { + return evaluate(symbol.isConstructor(), state); + } + } + + /** Scope to which a rule may apply. */ + public enum RuleScope { + /** The specific method or constructor for which a {@link ResultUsePolicy} is being chosen. */ + METHOD { + @Override + Stream members(MethodSymbol method) { + return Stream.of(method); + } + }, + /** + * Classes and package that enclose a method for which a {@link ResultUsePolicy} is being + * chosen. + */ + ENCLOSING_ELEMENTS { + @Override + Stream members(MethodSymbol method) { + return enclosingElements(method) + .filter(s -> s instanceof ClassSymbol || s instanceof PackageSymbol); + } + }, + /** The global scope. */ + GLOBAL { + @Override + Stream members(MethodSymbol method) { + return Stream.of(method); + } + }; + + /** Returns an ordered stream of elements in this scope relative to the given {@code method}. */ + abstract Stream members(MethodSymbol method); + + /** Returns an ordered stream of policies from rules in this scope. */ + final Stream policies( + MethodSymbol method, VisitorState state, ListMultimap rules) { + List scopeRules = rules.get(this); + return members(method) + .flatMap(symbol -> scopeRules.stream().map(rule -> rule.evaluate(symbol, state))) + .flatMap(Optional::stream); + } + + /** Returns an ordered stream of evaluations in this scope. */ + final Stream evaluations( + MethodSymbol method, VisitorState state, ListMultimap rules) { + List scopeRules = rules.get(this); + return members(method) + .flatMap(symbol -> scopeRules.stream().map(rule -> rule.evaluate(this, symbol, state))) + .flatMap(Optional::stream); + } + } + + /** An evaluation that a rule makes. */ + @AutoValue + public abstract static class Evaluation { + /** Creates a new {@link Evaluation}. */ + public static Evaluation create( + ResultUseRule rule, RuleScope scope, Symbol element, ResultUsePolicy policy) { + return new AutoValue_ResultUseRule_Evaluation(rule, scope, element, policy); + } + + /** The rule that made this evaluation. */ + public abstract ResultUseRule rule(); + /** The scope at which the evaluation was made. */ + public abstract RuleScope scope(); + /** The specific element in the scope for which the evaluation was made. */ + public abstract Symbol element(); + /** The policy the rule selected. */ + public abstract ResultUsePolicy policy(); + } +} diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/checkreturnvalue/Rules.java b/core/src/main/java/com/google/errorprone/bugpatterns/checkreturnvalue/Rules.java new file mode 100644 index 00000000000..8889fc20411 --- /dev/null +++ b/core/src/main/java/com/google/errorprone/bugpatterns/checkreturnvalue/Rules.java @@ -0,0 +1,101 @@ +/* + * Copyright 2022 The Error Prone Authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.google.errorprone.bugpatterns.checkreturnvalue; + +import static com.google.common.base.Preconditions.checkNotNull; +import static com.google.errorprone.util.ASTHelpers.hasDirectAnnotationWithSimpleName; + +import com.google.errorprone.VisitorState; +import com.google.errorprone.bugpatterns.checkreturnvalue.ResultUseRule.GlobalRule; +import com.google.errorprone.bugpatterns.checkreturnvalue.ResultUseRule.SymbolRule; +import com.sun.tools.javac.code.Symbol; +import java.util.Optional; +import java.util.function.BiPredicate; + +/** Factories for common kinds {@link ResultUseRule}s. */ +public final class Rules { + + private Rules() {} + + /** + * Returns a simple global rule that always returns the given defaults for methods and + * constructors. + */ + public static ResultUseRule globalDefault( + Optional methodDefault, Optional constructorDefault) { + return new SimpleGlobalRule("GLOBAL_DEFAULT", methodDefault, constructorDefault); + } + + /** + * Returns a {@link ResultUseRule} that maps annotations with the given {@code simpleName} to the + * given {@code policy}. + */ + public static ResultUseRule mapAnnotationSimpleName(String simpleName, ResultUsePolicy policy) { + return new SimpleRule( + "ANNOTATION @" + simpleName, + (sym, st) -> hasDirectAnnotationWithSimpleName(sym, simpleName), + policy); + } + + private static final class SimpleRule extends SymbolRule { + private final String name; + private final BiPredicate predicate; + private final ResultUsePolicy policy; + + private SimpleRule( + String name, BiPredicate predicate, ResultUsePolicy policy) { + this.name = name; + this.predicate = predicate; + this.policy = policy; + } + + @Override + public String id() { + return name; + } + + @Override + public Optional evaluate(Symbol symbol, VisitorState state) { + return predicate.test(symbol, state) ? Optional.of(policy) : Optional.empty(); + } + } + + private static final class SimpleGlobalRule extends GlobalRule { + private final String id; + private final Optional methodDefault; + private final Optional constructorDefault; + + private SimpleGlobalRule( + String id, + Optional methodDefault, + Optional constructorDefault) { + this.id = checkNotNull(id); + this.methodDefault = methodDefault; + this.constructorDefault = constructorDefault; + } + + @Override + public String id() { + return id; + } + + @Override + public Optional evaluate(boolean constructor, VisitorState state) { + return constructor ? constructorDefault : methodDefault; + } + } +}