Skip to content

Commit

Permalink
Fix error-prone mathcing literal null as a subtype. (#1020)
Browse files Browse the repository at this point in the history
Fix error-prone mathcing literal null as a subtype.
The most common issue this fixes is failures on `SafeArg.of("name", null)`
assuming that the null literal value parameter may be a throwable.
  • Loading branch information
carterkozak authored and bulldozer-bot[bot] committed Nov 4, 2019
1 parent e208633 commit e877003
Show file tree
Hide file tree
Showing 13 changed files with 111 additions and 49 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ public final class CatchBlockLogException extends BugChecker implements BugCheck
Matchers.toType(ExpressionTree.class, logMethod));

private static final Matcher<ExpressionTree> logException = Matchers.methodInvocation(
logMethod, ChildMultiMatcher.MatchType.LAST, Matchers.isSubtypeOf(Throwable.class));
logMethod, ChildMultiMatcher.MatchType.LAST, MoreMatchers.isSubtypeOf(Throwable.class));

private static final Matcher<Tree> containslogException = Matchers.contains(Matchers.toType(
ExpressionTree.class, logException));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@
import com.google.errorprone.bugpatterns.BugChecker;
import com.google.errorprone.matchers.Description;
import com.google.errorprone.matchers.Matcher;
import com.google.errorprone.matchers.Matchers;
import com.google.errorprone.matchers.method.MethodMatchers;
import com.sun.source.tree.ExpressionTree;
import com.sun.source.tree.MethodInvocationTree;
Expand All @@ -51,7 +50,7 @@ public final class DangerousThrowableMessageSafeArg extends BugChecker
.onDescendantOf(Throwable.class.getName())
.named("getMessage");

private static final Matcher<ExpressionTree> THROWABLE_MATCHER = Matchers.isSubtypeOf(Throwable.class);
private static final Matcher<ExpressionTree> THROWABLE_MATCHER = MoreMatchers.isSubtypeOf(Throwable.class);

@Override
public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@
public final class GradleCacheableTaskAction extends BugChecker implements LambdaExpressionTreeMatcher {

private static final long serialVersionUID = 1L;
private static final Matcher<ExpressionTree> IS_ACTION = Matchers.isSubtypeOf("org.gradle.api.Action");
private static final Matcher<ExpressionTree> IS_ACTION = MoreMatchers.isSubtypeOf("org.gradle.api.Action");

private static final Matcher<ExpressionTree> TASK_ACTION = MethodMatchers.instanceMethod()
.onDescendantOf("org.gradle.api.Task")
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
/*
* (c) Copyright 2018 Palantir Technologies Inc. All rights reserved.
*
* 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.palantir.baseline.errorprone;

import com.google.errorprone.matchers.Matcher;
import com.google.errorprone.matchers.Matchers;
import com.sun.source.tree.Tree;

/** Additional {@link Matcher} factory methods shared by baseline checks. */
final class MoreMatchers {

/**
* Delegates to {@link Matchers#isSubtypeOf(Class)}, but adds a defensive check against null literals
* to work around error-prone#1397.
*
* @see Matchers#isSubtypeOf(Class)
* @see <a href="https://github.com/google/error-prone/issues/1397">error-prone#1397</a>
*/
static <T extends Tree> Matcher<T> isSubtypeOf(Class<?> baseType) {
return Matchers.allOf(
Matchers.isSubtypeOf(baseType),
Matchers.not(Matchers.kindIs(Tree.Kind.NULL_LITERAL)));
}

/**
* Delegates to {@link Matchers#isSubtypeOf(String)}, but adds a defensive check against null literals
* to work around error-prone#1397.
*
* @see Matchers#isSubtypeOf(String)
* @see <a href="https://github.com/google/error-prone/issues/1397">error-prone#1397</a>
*/
static <T extends Tree> Matcher<T> isSubtypeOf(String baseTypeString) {
return Matchers.allOf(
Matchers.isSubtypeOf(baseTypeString),
Matchers.not(Matchers.kindIs(Tree.Kind.NULL_LITERAL)));
}

private MoreMatchers() {}
}
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@
import com.google.errorprone.fixes.SuggestedFixes;
import com.google.errorprone.matchers.Description;
import com.google.errorprone.matchers.Matcher;
import com.google.errorprone.matchers.Matchers;
import com.google.errorprone.matchers.method.MethodMatchers;
import com.sun.source.tree.ExpressionTree;
import com.sun.source.tree.MethodInvocationTree;
Expand All @@ -52,8 +51,8 @@ public final class PreferCollectionTransform extends BugChecker
.onClass("com.google.common.collect.Iterables")
.named("transform");

private static final Matcher<Tree> LIST_MATCHER = Matchers.isSubtypeOf("java.util.List");
private static final Matcher<Tree> COLLECTION_MATCHER = Matchers.isSubtypeOf("java.util.Collection");
private static final Matcher<Tree> LIST_MATCHER = MoreMatchers.isSubtypeOf("java.util.List");
private static final Matcher<Tree> COLLECTION_MATCHER = MoreMatchers.isSubtypeOf("java.util.Collection");

@Override
public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@
import com.google.errorprone.fixes.SuggestedFixes;
import com.google.errorprone.matchers.Description;
import com.google.errorprone.matchers.Matcher;
import com.google.errorprone.matchers.Matchers;
import com.google.errorprone.matchers.method.MethodMatchers;
import com.sun.source.tree.ExpressionTree;
import com.sun.source.tree.MethodInvocationTree;
Expand Down Expand Up @@ -55,7 +54,7 @@ public final class PreferListsPartition extends BugChecker
.named("partition")
.withParameters("java.lang.Iterable", "int");

private static final Matcher<Tree> LIST_MATCHER = Matchers.isSubtypeOf("java.util.List");
private static final Matcher<Tree> LIST_MATCHER = MoreMatchers.isSubtypeOf("java.util.List");

@Override
public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@
import com.google.errorprone.matchers.method.MethodMatchers;
import com.sun.source.tree.ExpressionTree;
import com.sun.source.tree.MethodInvocationTree;
import com.sun.source.tree.Tree;

@AutoService(BugChecker.class)
@BugPattern(
Expand All @@ -46,13 +45,9 @@ public final class PreventTokenLogging extends BugChecker implements BugChecker.
.onClassAny("com.palantir.logsafe.SafeArg", "com.palantir.logsafe.UnsafeArg")
.named("of"));

private static final Matcher<ExpressionTree> AUTH_MATCHER =
Matchers.allOf(
Matchers.anyOf(
Matchers.isSubtypeOf("com.palantir.tokens.auth.AuthHeader"),
Matchers.isSubtypeOf("com.palantir.tokens.auth.BearerToken")),
Matchers.not(
Matchers.kindIs(Tree.Kind.NULL_LITERAL)));
private static final Matcher<ExpressionTree> AUTH_MATCHER = Matchers.anyOf(
MoreMatchers.isSubtypeOf("com.palantir.tokens.auth.AuthHeader"),
MoreMatchers.isSubtypeOf("com.palantir.tokens.auth.BearerToken"));

@Override
public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@
import com.google.errorprone.matchers.Description;
import com.google.errorprone.matchers.Matcher;
import com.google.errorprone.matchers.method.MethodMatchers;
import com.google.errorprone.util.ASTHelpers;
import com.sun.source.tree.ExpressionTree;
import com.sun.source.tree.MethodInvocationTree;
import java.util.List;
Expand All @@ -47,6 +46,8 @@ public final class Slf4jConstantLogMessage extends BugChecker implements MethodI
.onDescendantOf("org.slf4j.Logger")
.withNameMatching(Pattern.compile("trace|debug|info|warn|error"));

private static final Matcher<ExpressionTree> MARKER = MoreMatchers.isSubtypeOf("org.slf4j.Marker");

private final Matcher<ExpressionTree> compileTimeConstExpressionMatcher =
new CompileTimeConstantExpressionMatcher();

Expand All @@ -57,10 +58,7 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState
}

List<? extends ExpressionTree> args = tree.getArguments();
ExpressionTree messageArg = ASTHelpers.isCastable(
ASTHelpers.getType(tree.getArguments().get(0)),
state.getTypeFromString("org.slf4j.Marker"),
state)
ExpressionTree messageArg = MARKER.matches(tree.getArguments().get(0), state)
? args.get(1)
: args.get(0);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,11 +29,9 @@
import com.google.errorprone.matchers.Matcher;
import com.google.errorprone.matchers.Matchers;
import com.google.errorprone.matchers.method.MethodMatchers;
import com.google.errorprone.util.ASTHelpers;
import com.sun.source.tree.ExpressionTree;
import com.sun.source.tree.MethodInvocationTree;
import com.sun.source.util.SimpleTreeVisitor;
import com.sun.tools.javac.code.Type;
import java.util.List;
import java.util.Optional;
import java.util.regex.Pattern;
Expand All @@ -54,7 +52,9 @@ public final class Slf4jLogsafeArgs extends BugChecker implements MethodInvocati
.onDescendantOf("org.slf4j.Logger")
.withNameMatching(Pattern.compile("trace|debug|info|warn|error"));

private static final Matcher<ExpressionTree> THROWABLE = Matchers.isSubtypeOf(Throwable.class);
private static final Matcher<ExpressionTree> THROWABLE = MoreMatchers.isSubtypeOf(Throwable.class);
private static final Matcher<ExpressionTree> ARG = MoreMatchers.isSubtypeOf("com.palantir.logsafe.Arg");
private static final Matcher<ExpressionTree> MARKER = MoreMatchers.isSubtypeOf("org.slf4j.Marker");

@Override
public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) {
Expand All @@ -69,18 +69,14 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState

List<? extends ExpressionTree> allArgs = tree.getArguments();
int lastIndex = allArgs.size() - 1;
int startArg = ASTHelpers.isCastable(
ASTHelpers.getType(allArgs.get(0)),
state.getTypeFromString("org.slf4j.Marker"),
state) ? 2 : 1;
int startArg = MARKER.matches(allArgs.get(0), state) ? 2 : 1;
ExpressionTree lastArg = allArgs.get(lastIndex);
boolean lastArgIsThrowable = THROWABLE.matches(lastArg, state);
int endArg = lastArgIsThrowable ? lastIndex - 1 : lastIndex;

ImmutableList.Builder<Integer> badArgsBuilder = ImmutableList.builder();
Type argType = state.getTypeFromString("com.palantir.logsafe.Arg");
for (int i = startArg; i <= endArg; i++) {
if (!ASTHelpers.isCastable(ASTHelpers.getType(allArgs.get(i)), argType, state)) {
if (!ARG.matches(allArgs.get(i), state)) {
badArgsBuilder.add(i);
}
}
Expand Down Expand Up @@ -111,13 +107,13 @@ private Optional<Description> checkThrowableArgumentNotWrapped(MethodInvocationT
private static final class ThrowableArgVisitor extends SimpleTreeVisitor<Optional<ExpressionTree>, VisitorState> {
private static final ThrowableArgVisitor INSTANCE = new ThrowableArgVisitor();

private static final Matcher<ExpressionTree> ARG = Matchers.staticMethod()
private static final Matcher<ExpressionTree> ARG_FACTORY = Matchers.staticMethod()
.onClassAny("com.palantir.logsafe.SafeArg", "com.palantir.logsafe.UnsafeArg")
.named("of")
.withParameters(String.class.getName(), Object.class.getName());

private static final Matcher<ExpressionTree> THROWABLE_ARG = Matchers.methodInvocation(
ARG, ChildMultiMatcher.MatchType.LAST, THROWABLE);
ARG_FACTORY, ChildMultiMatcher.MatchType.LAST, THROWABLE);

private ThrowableArgVisitor() {
super(Optional.empty());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ public final class ThrowError extends BugChecker implements BugChecker.ThrowTree

private static final Matcher<ExpressionTree> compileTimeConstExpressionMatcher =
new CompileTimeConstantExpressionMatcher();
private static final String ERROR_NAME = Error.class.getName();
private static final Matcher<ExpressionTree> ERROR = MoreMatchers.isSubtypeOf(Error.class);

@Override
public Description matchThrow(ThrowTree tree, VisitorState state) {
Expand All @@ -62,11 +62,7 @@ public Description matchThrow(ThrowTree tree, VisitorState state) {
return Description.NO_MATCH;
}
NewClassTree newClassTree = (NewClassTree) expression;
Type throwableType = ASTHelpers.getType(newClassTree.getIdentifier());
if (!ASTHelpers.isCastable(
throwableType,
state.getTypeFromString(ERROR_NAME),
state)
if (!ERROR.matches(newClassTree.getIdentifier(), state)
// Don't discourage developers from testing edge cases involving Errors.
// It's also fine for tests throw AssertionError internally in test objects.
|| TestCheckUtils.isTestCode(state)) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -76,4 +76,28 @@ public void unsafe_safearg_throwable() {
" }",
"}").doTest();
}

@Test
public void safe_null_allowed() {
compilationHelper.addSourceLines(
"Bean.java",
"import " + SafeArg.class.getName() + ';',
"class Bean {",
" public SafeArg<?> foo() {",
" return SafeArg.of(\"foo\", null);",
" }",
"}").doTest();
}

@Test
public void safe_object_allowed() {
compilationHelper.addSourceLines(
"Bean.java",
"import " + SafeArg.class.getName() + ';',
"class Bean {",
" public SafeArg<?> foo(Object object) {",
" return SafeArg.of(\"foo\", object);",
" }",
"}").doTest();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@
import com.google.common.collect.ImmutableList;
import com.google.errorprone.BugCheckerRefactoringTestHelper;
import com.google.errorprone.CompilationTestHelper;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.parallel.Execution;
import org.junit.jupiter.api.parallel.ExecutionMode;
Expand All @@ -29,15 +28,8 @@ public final class Slf4jLogsafeArgsTest {

private static final ImmutableList<String> LOG_LEVELS = ImmutableList.of("trace", "debug", "info", "warn", "error");

private CompilationTestHelper compilationHelper;

@BeforeEach
public void before() {
compilationHelper = CompilationTestHelper.newInstance(Slf4jLogsafeArgs.class, getClass());
}

private void test(String logArgs, String failingArgs) throws Exception {
LOG_LEVELS.forEach(logLevel -> compilationHelper
LOG_LEVELS.forEach(logLevel -> CompilationTestHelper.newInstance(Slf4jLogsafeArgs.class, getClass())
.addSourceLines(
"Test.java",
"import com.palantir.logsafe.SafeArg;",
Expand Down Expand Up @@ -88,11 +80,14 @@ public void testFailingLogsafeArgs() throws Exception {

// log.<>(String, Object, Arg<T>, Throwable)"
test("\"constant {} {}\", \"string\", SafeArg.of(\"name\", \"string\"), new Throwable()", "[1]");

// log.<>(String, Arg<T>, null literal)
test("\"constant {}\", SafeArg.of(\"name\", \"string\"), null", "[2]");
}

@Test
public void testPassingLogsafeArgs() throws Exception {
LOG_LEVELS.forEach(logLevel -> compilationHelper
LOG_LEVELS.forEach(logLevel -> CompilationTestHelper.newInstance(Slf4jLogsafeArgs.class, getClass())
.addSourceLines(
"Test.java",
"import com.palantir.logsafe.SafeArg;",
Expand Down
8 changes: 8 additions & 0 deletions changelog/@unreleased/pr-1020.v2.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
type: fix
fix:
description: |-
Fix error-prone mathcing literal null as a subtype.
The most common issue this fixes is failures on `SafeArg.of("name", null)`
assuming that the null literal value parameter may be a throwable.
links:
- https://github.com/palantir/gradle-baseline/pull/1020

0 comments on commit e877003

Please sign in to comment.