From e877003c927a5788002f390d2f070d8b3d9732d9 Mon Sep 17 00:00:00 2001 From: Carter Kozak Date: Mon, 4 Nov 2019 11:21:33 -0500 Subject: [PATCH] Fix error-prone mathcing literal null as a subtype. (#1020) 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. --- .../errorprone/CatchBlockLogException.java | 2 +- .../DangerousThrowableMessageSafeArg.java | 3 +- .../errorprone/GradleCacheableTaskAction.java | 2 +- .../baseline/errorprone/MoreMatchers.java | 53 +++++++++++++++++++ .../errorprone/PreferCollectionTransform.java | 5 +- .../errorprone/PreferListsPartition.java | 3 +- .../errorprone/PreventTokenLogging.java | 11 ++-- .../errorprone/Slf4jConstantLogMessage.java | 8 ++- .../baseline/errorprone/Slf4jLogsafeArgs.java | 18 +++---- .../baseline/errorprone/ThrowError.java | 8 +-- .../DangerousThrowableMessageSafeArgTest.java | 24 +++++++++ .../errorprone/Slf4jLogsafeArgsTest.java | 15 ++---- changelog/@unreleased/pr-1020.v2.yml | 8 +++ 13 files changed, 111 insertions(+), 49 deletions(-) create mode 100644 baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/MoreMatchers.java create mode 100644 changelog/@unreleased/pr-1020.v2.yml diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/CatchBlockLogException.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/CatchBlockLogException.java index d2e294de3..74199b7d5 100755 --- a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/CatchBlockLogException.java +++ b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/CatchBlockLogException.java @@ -61,7 +61,7 @@ public final class CatchBlockLogException extends BugChecker implements BugCheck Matchers.toType(ExpressionTree.class, logMethod)); private static final Matcher logException = Matchers.methodInvocation( - logMethod, ChildMultiMatcher.MatchType.LAST, Matchers.isSubtypeOf(Throwable.class)); + logMethod, ChildMultiMatcher.MatchType.LAST, MoreMatchers.isSubtypeOf(Throwable.class)); private static final Matcher containslogException = Matchers.contains(Matchers.toType( ExpressionTree.class, logException)); diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/DangerousThrowableMessageSafeArg.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/DangerousThrowableMessageSafeArg.java index 8ad02eb0f..6bfc472df 100755 --- a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/DangerousThrowableMessageSafeArg.java +++ b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/DangerousThrowableMessageSafeArg.java @@ -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; @@ -51,7 +50,7 @@ public final class DangerousThrowableMessageSafeArg extends BugChecker .onDescendantOf(Throwable.class.getName()) .named("getMessage"); - private static final Matcher THROWABLE_MATCHER = Matchers.isSubtypeOf(Throwable.class); + private static final Matcher THROWABLE_MATCHER = MoreMatchers.isSubtypeOf(Throwable.class); @Override public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) { diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/GradleCacheableTaskAction.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/GradleCacheableTaskAction.java index 6c5807a6e..fb96ca1d7 100644 --- a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/GradleCacheableTaskAction.java +++ b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/GradleCacheableTaskAction.java @@ -40,7 +40,7 @@ public final class GradleCacheableTaskAction extends BugChecker implements LambdaExpressionTreeMatcher { private static final long serialVersionUID = 1L; - private static final Matcher IS_ACTION = Matchers.isSubtypeOf("org.gradle.api.Action"); + private static final Matcher IS_ACTION = MoreMatchers.isSubtypeOf("org.gradle.api.Action"); private static final Matcher TASK_ACTION = MethodMatchers.instanceMethod() .onDescendantOf("org.gradle.api.Task") diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/MoreMatchers.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/MoreMatchers.java new file mode 100644 index 000000000..5815fc5de --- /dev/null +++ b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/MoreMatchers.java @@ -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 error-prone#1397 + */ + static Matcher 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 error-prone#1397 + */ + static Matcher isSubtypeOf(String baseTypeString) { + return Matchers.allOf( + Matchers.isSubtypeOf(baseTypeString), + Matchers.not(Matchers.kindIs(Tree.Kind.NULL_LITERAL))); + } + + private MoreMatchers() {} +} diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/PreferCollectionTransform.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/PreferCollectionTransform.java index 47e5e1475..abc5293e1 100644 --- a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/PreferCollectionTransform.java +++ b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/PreferCollectionTransform.java @@ -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; @@ -52,8 +51,8 @@ public final class PreferCollectionTransform extends BugChecker .onClass("com.google.common.collect.Iterables") .named("transform"); - private static final Matcher LIST_MATCHER = Matchers.isSubtypeOf("java.util.List"); - private static final Matcher COLLECTION_MATCHER = Matchers.isSubtypeOf("java.util.Collection"); + private static final Matcher LIST_MATCHER = MoreMatchers.isSubtypeOf("java.util.List"); + private static final Matcher COLLECTION_MATCHER = MoreMatchers.isSubtypeOf("java.util.Collection"); @Override public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) { diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/PreferListsPartition.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/PreferListsPartition.java index ff8fecfb8..712ab4f44 100644 --- a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/PreferListsPartition.java +++ b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/PreferListsPartition.java @@ -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; @@ -55,7 +54,7 @@ public final class PreferListsPartition extends BugChecker .named("partition") .withParameters("java.lang.Iterable", "int"); - private static final Matcher LIST_MATCHER = Matchers.isSubtypeOf("java.util.List"); + private static final Matcher LIST_MATCHER = MoreMatchers.isSubtypeOf("java.util.List"); @Override public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) { diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/PreventTokenLogging.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/PreventTokenLogging.java index 0804c68dd..3db7f56d3 100644 --- a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/PreventTokenLogging.java +++ b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/PreventTokenLogging.java @@ -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( @@ -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 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 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) { diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/Slf4jConstantLogMessage.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/Slf4jConstantLogMessage.java index 9e1a06de7..9b2dd20f6 100644 --- a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/Slf4jConstantLogMessage.java +++ b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/Slf4jConstantLogMessage.java @@ -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; @@ -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 MARKER = MoreMatchers.isSubtypeOf("org.slf4j.Marker"); + private final Matcher compileTimeConstExpressionMatcher = new CompileTimeConstantExpressionMatcher(); @@ -57,10 +58,7 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState } List 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); diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/Slf4jLogsafeArgs.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/Slf4jLogsafeArgs.java index ba3a3491c..565cc571e 100644 --- a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/Slf4jLogsafeArgs.java +++ b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/Slf4jLogsafeArgs.java @@ -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; @@ -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 THROWABLE = Matchers.isSubtypeOf(Throwable.class); + private static final Matcher THROWABLE = MoreMatchers.isSubtypeOf(Throwable.class); + private static final Matcher ARG = MoreMatchers.isSubtypeOf("com.palantir.logsafe.Arg"); + private static final Matcher MARKER = MoreMatchers.isSubtypeOf("org.slf4j.Marker"); @Override public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) { @@ -69,18 +69,14 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState List 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 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); } } @@ -111,13 +107,13 @@ private Optional checkThrowableArgumentNotWrapped(MethodInvocationT private static final class ThrowableArgVisitor extends SimpleTreeVisitor, VisitorState> { private static final ThrowableArgVisitor INSTANCE = new ThrowableArgVisitor(); - private static final Matcher ARG = Matchers.staticMethod() + private static final Matcher 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 THROWABLE_ARG = Matchers.methodInvocation( - ARG, ChildMultiMatcher.MatchType.LAST, THROWABLE); + ARG_FACTORY, ChildMultiMatcher.MatchType.LAST, THROWABLE); private ThrowableArgVisitor() { super(Optional.empty()); diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/ThrowError.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/ThrowError.java index b876e5200..edfb0b0c1 100644 --- a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/ThrowError.java +++ b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/ThrowError.java @@ -53,7 +53,7 @@ public final class ThrowError extends BugChecker implements BugChecker.ThrowTree private static final Matcher compileTimeConstExpressionMatcher = new CompileTimeConstantExpressionMatcher(); - private static final String ERROR_NAME = Error.class.getName(); + private static final Matcher ERROR = MoreMatchers.isSubtypeOf(Error.class); @Override public Description matchThrow(ThrowTree tree, VisitorState state) { @@ -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)) { diff --git a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/DangerousThrowableMessageSafeArgTest.java b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/DangerousThrowableMessageSafeArgTest.java index 117b3cceb..6d03d9dc4 100644 --- a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/DangerousThrowableMessageSafeArgTest.java +++ b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/DangerousThrowableMessageSafeArgTest.java @@ -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(); + } } diff --git a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/Slf4jLogsafeArgsTest.java b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/Slf4jLogsafeArgsTest.java index b91962741..1d17b3d17 100644 --- a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/Slf4jLogsafeArgsTest.java +++ b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/Slf4jLogsafeArgsTest.java @@ -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; @@ -29,15 +28,8 @@ public final class Slf4jLogsafeArgsTest { private static final ImmutableList 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;", @@ -88,11 +80,14 @@ public void testFailingLogsafeArgs() throws Exception { // log.<>(String, Object, Arg, Throwable)" test("\"constant {} {}\", \"string\", SafeArg.of(\"name\", \"string\"), new Throwable()", "[1]"); + + // log.<>(String, Arg, 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;", diff --git a/changelog/@unreleased/pr-1020.v2.yml b/changelog/@unreleased/pr-1020.v2.yml new file mode 100644 index 000000000..ee614dd76 --- /dev/null +++ b/changelog/@unreleased/pr-1020.v2.yml @@ -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