Skip to content

Commit

Permalink
Move check for the regex "." to a new WARNING-level check
Browse files Browse the repository at this point in the history
PiperOrigin-RevId: 406904784
  • Loading branch information
amalloy authored and Error Prone Team committed Nov 3, 2021
1 parent eb3708a commit a602a69
Show file tree
Hide file tree
Showing 6 changed files with 190 additions and 95 deletions.
Original file line number Diff line number Diff line change
@@ -0,0 +1,78 @@
/*
* Copyright 2021 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;

import static com.google.errorprone.matchers.Description.NO_MATCH;
import static com.google.errorprone.matchers.Matchers.anyOf;
import static com.google.errorprone.matchers.Matchers.instanceMethod;
import static com.google.errorprone.matchers.Matchers.staticMethod;

import com.google.errorprone.VisitorState;
import com.google.errorprone.annotations.ForOverride;
import com.google.errorprone.bugpatterns.BugChecker.MethodInvocationTreeMatcher;
import com.google.errorprone.matchers.Description;
import com.google.errorprone.matchers.Matcher;
import com.google.errorprone.util.ASTHelpers;
import com.sun.source.tree.ExpressionTree;
import com.sun.source.tree.MethodInvocationTree;
import javax.annotation.CheckReturnValue;

/** Finds calls to regex-accepting methods with literal strings. */
@CheckReturnValue
abstract class AbstractPatternSyntaxChecker extends BugChecker
implements MethodInvocationTreeMatcher {

/*
* Match invocations to regex-accepting methods. Subclasses will be consulted to see whether the
* pattern passed to such methods are acceptable.
*
* <p>We deliberately omit Pattern.compile itself, as most of its users appear to be either
* e.g. passing LITERAL flags or deliberately testing the regex compiler.
*/
private static final Matcher<MethodInvocationTree> REGEX_USAGE =
anyOf(
instanceMethod()
.onExactClass("java.lang.String")
.namedAnyOf("matches", "split")
.withParameters("java.lang.String"),
instanceMethod()
.onExactClass("java.lang.String")
.named("split")
.withParameters("java.lang.String", "int"),
instanceMethod()
.onExactClass("java.lang.String")
.namedAnyOf("replaceFirst", "replaceAll")
.withParameters("java.lang.String", "java.lang.String"),
staticMethod().onClass("java.util.regex.Pattern").named("matches"),
staticMethod().onClass("com.google.common.base.Splitter").named("onPattern"));

@Override
public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) {
if (!REGEX_USAGE.matches(tree, state)) {
return NO_MATCH;
}
ExpressionTree arg = tree.getArguments().get(0);
String value = ASTHelpers.constValue(arg, String.class);
if (value == null) {
return NO_MATCH;
}
return matchRegexLiteral(tree, value);
}

@ForOverride
protected abstract Description matchRegexLiteral(MethodInvocationTree tree, String pattern);
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
/*
* Copyright 2021 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;

import static com.google.errorprone.BugPattern.SeverityLevel.WARNING;
import static com.google.errorprone.matchers.Description.NO_MATCH;

import com.google.errorprone.BugPattern;
import com.google.errorprone.bugpatterns.BugChecker.MethodInvocationTreeMatcher;
import com.google.errorprone.fixes.SuggestedFix;
import com.google.errorprone.matchers.Description;
import com.sun.source.tree.MethodInvocationTree;

/** A BugChecker; see the associated BugPattern for details. */
@BugPattern(
name = "BareDotMetacharacter",
summary =
"\".\" is rarely useful as a regex, as it matches any character. To match a literal '.'"
+ " character, instead write \"\\.\".",
severity = WARNING,
// So that suppressions added before this check was split into two apply to both halves.
altNames = {"InvalidPatternSyntax"})
public class BareDotMetacharacter extends AbstractPatternSyntaxChecker
implements MethodInvocationTreeMatcher {

@Override
protected final Description matchRegexLiteral(MethodInvocationTree tree, String regex) {
if (regex.equals(".")) {
return describeMatch(tree, SuggestedFix.replace(tree.getArguments().get(0), "\"\\\\.\""));
} else {
return NO_MATCH;
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -17,20 +17,10 @@
package com.google.errorprone.bugpatterns;

import static com.google.errorprone.BugPattern.SeverityLevel.ERROR;
import static com.google.errorprone.matchers.Matchers.allOf;
import static com.google.errorprone.matchers.Matchers.anyOf;
import static com.google.errorprone.matchers.Matchers.argument;
import static com.google.errorprone.matchers.method.MethodMatchers.instanceMethod;
import static com.google.errorprone.matchers.method.MethodMatchers.staticMethod;
import static com.google.errorprone.matchers.Description.NO_MATCH;

import com.google.errorprone.BugPattern;
import com.google.errorprone.VisitorState;
import com.google.errorprone.bugpatterns.BugChecker.MethodInvocationTreeMatcher;
import com.google.errorprone.fixes.SuggestedFix;
import com.google.errorprone.matchers.Description;
import com.google.errorprone.matchers.Matcher;
import com.google.errorprone.util.ASTHelpers;
import com.sun.source.tree.ExpressionTree;
import com.sun.source.tree.MethodInvocationTree;
import java.util.regex.Pattern;
import java.util.regex.PatternSyntaxException;
Expand All @@ -40,84 +30,17 @@
name = "InvalidPatternSyntax",
summary = "Invalid syntax used for a regular expression",
severity = ERROR)
public class InvalidPatternSyntax extends BugChecker implements MethodInvocationTreeMatcher {
public class InvalidPatternSyntax extends AbstractPatternSyntaxChecker {

private static final String MESSAGE_BASE = "Invalid syntax used for a regular expression: ";

/* Match string literals that are not valid syntax for regular expressions. */
private static final Matcher<ExpressionTree> BAD_REGEX_LITERAL =
new Matcher<ExpressionTree>() {
@Override
public boolean matches(ExpressionTree tree, VisitorState state) {
String value = ASTHelpers.constValue(tree, String.class);
return value != null && !isValidSyntax(value);
}

private boolean isValidSyntax(String regex) {
// Actually valid, but useless.
if (".".equals(regex)) {
return false;
}
try {
Pattern.compile(regex);
return true;
} catch (PatternSyntaxException e) {
return false;
}
}
};

/*
* Match invocations to regex-accepting methods with bad string literals.
*
* <p>We deliberately omit Pattern.compile itself, as most of its users appear to be either
* passing e.g. LITERAL flags, deliberately testing the regex compiler, or deliberately
* using "." as the "vacuously true regex."
*/
private static final Matcher<MethodInvocationTree> BAD_REGEX_USAGE =
allOf(
anyOf(
instanceMethod()
.onExactClass("java.lang.String")
.namedAnyOf("matches", "split")
.withParameters("java.lang.String"),
instanceMethod()
.onExactClass("java.lang.String")
.named("split")
.withParameters("java.lang.String", "int"),
instanceMethod()
.onExactClass("java.lang.String")
.namedAnyOf("replaceFirst", "replaceAll")
.withParameters("java.lang.String", "java.lang.String"),
staticMethod().onClass("java.util.regex.Pattern").named("matches"),
staticMethod().onClass("com.google.common.base.Splitter").named("onPattern")),
argument(0, BAD_REGEX_LITERAL));

@Override
public Description matchMethodInvocation(
MethodInvocationTree methodInvocationTree, VisitorState state) {
if (!BAD_REGEX_USAGE.matches(methodInvocationTree, state)) {
return Description.NO_MATCH;
protected final Description matchRegexLiteral(MethodInvocationTree tree, String regex) {
try {
Pattern.compile(regex);
return NO_MATCH;
} catch (PatternSyntaxException e) {
return buildDescription(tree).setMessage(MESSAGE_BASE + e.getMessage()).build();
}

// TODO: Suggest fixes for more situations.
Description.Builder descriptionBuilder = buildDescription(methodInvocationTree);
ExpressionTree arg = methodInvocationTree.getArguments().get(0);
String value = ASTHelpers.constValue(arg, String.class);
String reasonInvalid = "";

if (".".equals(value)) {
descriptionBuilder.addFix(SuggestedFix.replace(arg, "\"\\\\.\""));
reasonInvalid = "\".\" is a valid but useless regex";
} else {
try {
Pattern.compile(value);
} catch (PatternSyntaxException e) {
reasonInvalid = e.getMessage();
}
}

descriptionBuilder.setMessage(MESSAGE_BASE + reasonInvalid);
return descriptionBuilder.build();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@
import com.google.errorprone.bugpatterns.BadInstanceof;
import com.google.errorprone.bugpatterns.BadShiftAmount;
import com.google.errorprone.bugpatterns.BanSerializableRead;
import com.google.errorprone.bugpatterns.BareDotMetacharacter;
import com.google.errorprone.bugpatterns.BigDecimalEquals;
import com.google.errorprone.bugpatterns.BigDecimalLiteralDouble;
import com.google.errorprone.bugpatterns.BooleanParameter;
Expand Down Expand Up @@ -764,6 +765,7 @@ public static ScannerSupplier errorChecks() {
BadComparable.class,
BadImport.class,
BadInstanceof.class,
BareDotMetacharacter.class,
BigDecimalEquals.class,
BigDecimalLiteralDouble.class,
BoxedPrimitiveConstructor.class,
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
/*
* Copyright 2012 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;

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

/** Tests for {@link BareDotMetacharacter}. */
@RunWith(JUnit4.class)
public class BareDotMetacharacterTest {

private final BugCheckerRefactoringTestHelper refactoringHelper =
BugCheckerRefactoringTestHelper.newInstance(BareDotMetacharacter.class, getClass());

@Test
public void testPositiveCase() {
refactoringHelper
.addInputLines(
"Test.java",
"import com.google.common.base.Splitter;",
"class Test {",
" private static final String DOT = \".\";",
" Object x = \"foo.bar\".split(\".\");",
" Object y = \"foo.bonk\".split(DOT);",
" Object z = Splitter.onPattern(\".\");",
"}")
.addOutputLines(
"Test.java",
"import com.google.common.base.Splitter;",
"class Test {",
" private static final String DOT = \".\";",
" Object x = \"foo.bar\".split(\"\\\\.\");",
" Object y = \"foo.bonk\".split(\"\\\\.\");",
" Object z = Splitter.onPattern(\"\\\\.\");",
"}")
.doTest();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -16,13 +16,11 @@

package com.google.errorprone.bugpatterns.testdata;

import com.google.common.base.Splitter;
import java.util.regex.Pattern;

/** @author mdempsky@google.com (Matthew Dempsky) */
public class InvalidPatternSyntaxPositiveCases {
public static final String INVALID = "*";
public static final String DOT = ".";

{
// BUG: Diagnostic contains: Unclosed character class
Expand All @@ -44,13 +42,5 @@ public class InvalidPatternSyntaxPositiveCases {
"".split(INVALID);
// BUG: Diagnostic contains:
"".split(INVALID, 0);

// BUG: Diagnostic contains: "foo.bar".split("\\.")
"foo.bar".split(".");
// BUG: Diagnostic contains:
"foo.bonk".split(DOT);

// BUG: Diagnostic contains: Splitter.onPattern("\\.")
Splitter.onPattern(".");
}
}

0 comments on commit a602a69

Please sign in to comment.