Skip to content

Commit

Permalink
Add a test for PreconditionsExpensiveString
Browse files Browse the repository at this point in the history 
and update the check to include a suggested fix.

MOE_MIGRATED_REVID=127252263
  • Loading branch information
@cushon
cushon committed Jul 13, 2016
1 parent 865a6b8 commit b41d114
Show file tree
Hide file tree
Showing 2 changed files with 129 additions and 56 deletions.
116 changes: 60 additions & 56 deletions core/src/main/java/com/google/errorprone/bugpatterns/PreconditionsExpensiveString.java
View file
Expand Up @@ -24,98 +24,102 @@
import static com.google.errorprone.matchers.Matchers.argument; import static com.google.errorprone.matchers.Matchers.argument;
import static com.google.errorprone.matchers.Matchers.staticMethod; import static com.google.errorprone.matchers.Matchers.staticMethod;


import com.google.common.collect.Iterables;
import com.google.errorprone.BugPattern; import com.google.errorprone.BugPattern;
import com.google.errorprone.VisitorState; import com.google.errorprone.VisitorState;
import com.google.errorprone.bugpatterns.BugChecker.MethodInvocationTreeMatcher; import com.google.errorprone.bugpatterns.BugChecker.MethodInvocationTreeMatcher;
import com.google.errorprone.fixes.SuggestedFix;
import com.google.errorprone.matchers.Description; import com.google.errorprone.matchers.Description;
import com.google.errorprone.matchers.Matcher; import com.google.errorprone.matchers.Matcher;
import com.google.errorprone.matchers.Matchers; import com.google.errorprone.matchers.Matchers;

import com.google.errorprone.util.ASTHelpers;
import com.sun.source.tree.ExpressionTree; import com.sun.source.tree.ExpressionTree;
import com.sun.source.tree.LiteralTree;
import com.sun.source.tree.MemberSelectTree;
import com.sun.source.tree.MethodInvocationTree; import com.sun.source.tree.MethodInvocationTree;
import com.sun.source.tree.Tree.Kind; import com.sun.source.tree.Tree.Kind;

import com.sun.tools.javac.tree.JCTree;
import java.util.List;
import java.util.regex.Pattern; import java.util.regex.Pattern;


/** /**
* Error checker for calls to the Preconditions class in Guava which use * Error checker for calls to the Preconditions class in Guava which use 'expensive' methods of
* 'expensive' methods of producing the error string. In most cases, users are * producing the error string. In most cases, users are better off using the equivalent methods
* better off using the equivalent methods which defer the computation of the * which defer the computation of the string until the test actually fails.
* string until the test actually fails.
* *
* @author sjnickerson@google.com (Simon Nickerson) * @author sjnickerson@google.com (Simon Nickerson)
*/ */
@BugPattern(name = "PreconditionsErrorMessageEagerEvaluation", @BugPattern(
summary = "Second argument to Preconditions.* is a call to String.format(), which " + name = "PreconditionsErrorMessageEagerEvaluation",
"can be unwrapped", summary =
explanation = "Second argument to Preconditions.* is a call to String.format(), which "
"Preconditions checks take an error message to display if the check fails. " + + "can be unwrapped",
"The error message is rarely needed, so it should either be cheap to construct " + explanation =
"or constructed only when needed. This check ensures that these error messages " + "Preconditions checks take an error message to display if the check fails. "
"are not constructed using expensive methods that are evaluated eagerly.", + "The error message is rarely needed, so it should either be cheap to construct "
category = GUAVA, severity = WARNING, maturity = EXPERIMENTAL) + "or constructed only when needed. This check ensures that these error messages "
public class PreconditionsExpensiveString + "are not constructed using expensive methods that are evaluated eagerly.",
extends BugChecker implements MethodInvocationTreeMatcher { category = GUAVA,
severity = WARNING,
maturity = EXPERIMENTAL
)
public class PreconditionsExpensiveString extends BugChecker
implements MethodInvocationTreeMatcher {


@SuppressWarnings({"vararg", "unchecked"}) @SuppressWarnings({"vararg", "unchecked"})
private static final Matcher<MethodInvocationTree> matcher = allOf( private static final Matcher<MethodInvocationTree> matcher =
anyOf( allOf(
staticMethod().onClass("com.google.common.base.Preconditions").named("checkNotNull"), anyOf(
staticMethod().onClass("com.google.common.base.Preconditions").named("checkState"), staticMethod().onClass("com.google.common.base.Preconditions").named("checkNotNull"),
staticMethod().onClass("com.google.common.base.Preconditions").named("checkArgument")), staticMethod().onClass("com.google.common.base.Preconditions").named("checkState"),
argument(1, Matchers.<ExpressionTree>allOf( staticMethod()
Matchers.<ExpressionTree>kindIs(Kind.METHOD_INVOCATION), .onClass("com.google.common.base.Preconditions")
staticMethod().onClass("java.lang.String").named("format"), .named("checkArgument")),
new StringFormatCallContainsNoSpecialFormattingMatcher(Pattern.compile("%[^%s]")) argument(
)) 1,
); Matchers.allOf(
Matchers.<ExpressionTree>kindIs(Kind.METHOD_INVOCATION),
staticMethod().onClass("java.lang.String").named("format"),
new StringFormatCallContainsNoSpecialFormattingMatcher(
Pattern.compile("%[^%s]")))));


@Override @Override
public Description matchMethodInvocation(MethodInvocationTree methodInvocationTree, VisitorState state) { public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) {
if (!matcher.matches(methodInvocationTree, state)) { if (!matcher.matches(tree, state)) {
return Description.NO_MATCH; return Description.NO_MATCH;
} }

MethodInvocationTree stringFormat = (MethodInvocationTree) tree.getArguments().get(1);
MemberSelectTree method = return describeMatch(
(MemberSelectTree) methodInvocationTree.getMethodSelect(); stringFormat,

SuggestedFix.builder()
List<? extends ExpressionTree> arguments = .replace(
methodInvocationTree.getArguments(); ((JCTree) stringFormat).getStartPosition(),
MethodInvocationTree stringFormat = (MethodInvocationTree) arguments.get(1); ((JCTree) stringFormat.getArguments().get(0)).getStartPosition(),

"")
// TODO(sjnickerson): Figure out how to get a suggested fix. Basically we .replace(
// remove the String.format() wrapper, but I don't know how to express state.getEndPosition((JCTree) Iterables.getLast(stringFormat.getArguments())),
// this. state.getEndPosition((JCTree) stringFormat),
return describeMatch(arguments.get(1)); "")
.build());
} }


private static class StringFormatCallContainsNoSpecialFormattingMatcher private static class StringFormatCallContainsNoSpecialFormattingMatcher
implements Matcher<ExpressionTree> { implements Matcher<ExpressionTree> {


private final Pattern invalidFormatCharacters; private final Pattern invalidFormatCharacters;


StringFormatCallContainsNoSpecialFormattingMatcher( StringFormatCallContainsNoSpecialFormattingMatcher(Pattern invalidFormatCharacters) {
Pattern invalidFormatCharacters) {
this.invalidFormatCharacters = invalidFormatCharacters; this.invalidFormatCharacters = invalidFormatCharacters;
} }


@Override @Override
public boolean matches(ExpressionTree t, VisitorState state) { public boolean matches(ExpressionTree tree, VisitorState state) {
if (!(t instanceof MethodInvocationTree)) { if (!(tree instanceof MethodInvocationTree)) {
return false; return false;
} }
MethodInvocationTree stringFormatInvocation = (MethodInvocationTree) t; String formatString =
if (!(stringFormatInvocation.getArguments().get(0) ASTHelpers.constValue(((MethodInvocationTree) tree).getArguments().get(0), String.class);
instanceof LiteralTree)) { if (formatString == null) {
return false; return false;
} }
LiteralTree firstArg = (LiteralTree) return !invalidFormatCharacters.matcher(formatString).find();
stringFormatInvocation.getArguments().get(0);
String literal = firstArg.getValue().toString();
return !invalidFormatCharacters.matcher(literal).find();
} }
} }
} }
69 changes: 69 additions & 0 deletions core/src/test/java/com/google/errorprone/bugpatterns/PreconditionsExpensiveStringTest.java
View file
@@ -0,0 +1,69 @@
/*
* Copyright 2016 Google 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.google.errorprone.bugpatterns;

import static com.google.errorprone.BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH;

import com.google.errorprone.BugCheckerRefactoringTestHelper;
import com.google.errorprone.CompilationTestHelper;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.JUnit4;

/** {@link PreconditionsExpensiveString}Test */
@RunWith(JUnit4.class)
public class PreconditionsExpensiveStringTest {

@Test
public void positive() throws Exception {
BugCheckerRefactoringTestHelper.newInstance(new PreconditionsExpensiveString(), getClass())
.addInputLines(
"in/Test.java",
"import com.google.common.base.Preconditions;",
"class Test {",
" void f() {",
" Preconditions.checkNotNull(this, String.format(\"%s\", \"hello\"));",
" Preconditions.checkNotNull(this, String.format(\"hello\"));",
" }",
"}")
.addOutputLines(
"out/Test.java",
"import com.google.common.base.Preconditions;",
"class Test {",
" void f() {",
" Preconditions.checkNotNull(this, \"%s\", \"hello\");",
" Preconditions.checkNotNull(this, \"hello\");",
" }",
"}")
.doTest(TEXT_MATCH);
}

@Test
public void negative() throws Exception {
CompilationTestHelper.newInstance(PreconditionsExpensiveString.class, getClass())
.addSourceLines(
"Test.java",
"import com.google.common.base.Preconditions;",
"class Test {",
" void f() {",
" Preconditions.checkNotNull(this, String.format(\"%d\", 42));",
" Preconditions.checkNotNull(this, \"%s\", \"hello\");",
" }",
"}")
.doTest();
}
}

0 comments on commit b41d114

Please sign in to comment.