Skip to content

Commit 0f34024

Browse files
amalloyError Prone Team
amalloy
authored and
Error Prone Team
committed
Move check for the regex "." to a new WARNING-level check
PiperOrigin-RevId: 407388923
1 parent eb3708a commit 0f34024

File tree

6 files changed

+190
-95
lines changed

6 files changed

+190
-95
lines changed

‎core/src/main/java/com/google/errorprone/bugpatterns/AbstractPatternSyntaxChecker.java

+78
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,78 @@
1+
/*
2+
* Copyright 2021 The Error Prone Authors.
3+
*
4+
* Licensed under the Apache License, Version 2.0 (the "License");
5+
* you may not use this file except in compliance with the License.
6+
* You may obtain a copy of the License at
7+
*
8+
* http://www.apache.org/licenses/LICENSE-2.0
9+
*
10+
* Unless required by applicable law or agreed to in writing, software
11+
* distributed under the License is distributed on an "AS IS" BASIS,
12+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
* See the License for the specific language governing permissions and
14+
* limitations under the License.
15+
*/
16+
17+
package com.google.errorprone.bugpatterns;
18+
19+
import static com.google.errorprone.matchers.Description.NO_MATCH;
20+
import static com.google.errorprone.matchers.Matchers.anyOf;
21+
import static com.google.errorprone.matchers.Matchers.instanceMethod;
22+
import static com.google.errorprone.matchers.Matchers.staticMethod;
23+
24+
import com.google.errorprone.VisitorState;
25+
import com.google.errorprone.annotations.ForOverride;
26+
import com.google.errorprone.bugpatterns.BugChecker.MethodInvocationTreeMatcher;
27+
import com.google.errorprone.matchers.Description;
28+
import com.google.errorprone.matchers.Matcher;
29+
import com.google.errorprone.util.ASTHelpers;
30+
import com.sun.source.tree.ExpressionTree;
31+
import com.sun.source.tree.MethodInvocationTree;
32+
import javax.annotation.CheckReturnValue;
33+
34+
/** Finds calls to regex-accepting methods with literal strings. */
35+
@CheckReturnValue
36+
abstract class AbstractPatternSyntaxChecker extends BugChecker
37+
implements MethodInvocationTreeMatcher {
38+
39+
/*
40+
* Match invocations to regex-accepting methods. Subclasses will be consulted to see whether the
41+
* pattern passed to such methods are acceptable.
42+
*
43+
* <p>We deliberately omit Pattern.compile itself, as most of its users appear to be either
44+
* e.g. passing LITERAL flags or deliberately testing the regex compiler.
45+
*/
46+
private static final Matcher<MethodInvocationTree> REGEX_USAGE =
47+
anyOf(
48+
instanceMethod()
49+
.onExactClass("java.lang.String")
50+
.namedAnyOf("matches", "split")
51+
.withParameters("java.lang.String"),
52+
instanceMethod()
53+
.onExactClass("java.lang.String")
54+
.named("split")
55+
.withParameters("java.lang.String", "int"),
56+
instanceMethod()
57+
.onExactClass("java.lang.String")
58+
.namedAnyOf("replaceFirst", "replaceAll")
59+
.withParameters("java.lang.String", "java.lang.String"),
60+
staticMethod().onClass("java.util.regex.Pattern").named("matches"),
61+
staticMethod().onClass("com.google.common.base.Splitter").named("onPattern"));
62+
63+
@Override
64+
public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) {
65+
if (!REGEX_USAGE.matches(tree, state)) {
66+
return NO_MATCH;
67+
}
68+
ExpressionTree arg = tree.getArguments().get(0);
69+
String value = ASTHelpers.constValue(arg, String.class);
70+
if (value == null) {
71+
return NO_MATCH;
72+
}
73+
return matchRegexLiteral(tree, value);
74+
}
75+
76+
@ForOverride
77+
protected abstract Description matchRegexLiteral(MethodInvocationTree tree, String pattern);
78+
}

‎core/src/main/java/com/google/errorprone/bugpatterns/BareDotMetacharacter.java

+48
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,48 @@
1+
/*
2+
* Copyright 2021 The Error Prone Authors.
3+
*
4+
* Licensed under the Apache License, Version 2.0 (the "License");
5+
* you may not use this file except in compliance with the License.
6+
* You may obtain a copy of the License at
7+
*
8+
* http://www.apache.org/licenses/LICENSE-2.0
9+
*
10+
* Unless required by applicable law or agreed to in writing, software
11+
* distributed under the License is distributed on an "AS IS" BASIS,
12+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
* See the License for the specific language governing permissions and
14+
* limitations under the License.
15+
*/
16+
17+
package com.google.errorprone.bugpatterns;
18+
19+
import static com.google.errorprone.BugPattern.SeverityLevel.WARNING;
20+
import static com.google.errorprone.matchers.Description.NO_MATCH;
21+
22+
import com.google.errorprone.BugPattern;
23+
import com.google.errorprone.bugpatterns.BugChecker.MethodInvocationTreeMatcher;
24+
import com.google.errorprone.fixes.SuggestedFix;
25+
import com.google.errorprone.matchers.Description;
26+
import com.sun.source.tree.MethodInvocationTree;
27+
28+
/** A BugChecker; see the associated BugPattern for details. */
29+
@BugPattern(
30+
name = "BareDotMetacharacter",
31+
summary =
32+
"\".\" is rarely useful as a regex, as it matches any character. To match a literal '.'"
33+
+ " character, instead write \"\\\\.\".",
34+
severity = WARNING,
35+
// So that suppressions added before this check was split into two apply to both halves.
36+
altNames = {"InvalidPatternSyntax"})
37+
public class BareDotMetacharacter extends AbstractPatternSyntaxChecker
38+
implements MethodInvocationTreeMatcher {
39+
40+
@Override
41+
protected final Description matchRegexLiteral(MethodInvocationTree tree, String regex) {
42+
if (regex.equals(".")) {
43+
return describeMatch(tree, SuggestedFix.replace(tree.getArguments().get(0), "\"\\\\.\""));
44+
} else {
45+
return NO_MATCH;
46+
}
47+
}
48+
}

‎core/src/main/java/com/google/errorprone/bugpatterns/InvalidPatternSyntax.java

+8-85
Original file line numberDiff line numberDiff line change
@@ -17,20 +17,10 @@
1717
package com.google.errorprone.bugpatterns;
1818

1919
import static com.google.errorprone.BugPattern.SeverityLevel.ERROR;
20-
import static com.google.errorprone.matchers.Matchers.allOf;
21-
import static com.google.errorprone.matchers.Matchers.anyOf;
22-
import static com.google.errorprone.matchers.Matchers.argument;
23-
import static com.google.errorprone.matchers.method.MethodMatchers.instanceMethod;
24-
import static com.google.errorprone.matchers.method.MethodMatchers.staticMethod;
20+
import static com.google.errorprone.matchers.Description.NO_MATCH;
2521

2622
import com.google.errorprone.BugPattern;
27-
import com.google.errorprone.VisitorState;
28-
import com.google.errorprone.bugpatterns.BugChecker.MethodInvocationTreeMatcher;
29-
import com.google.errorprone.fixes.SuggestedFix;
3023
import com.google.errorprone.matchers.Description;
31-
import com.google.errorprone.matchers.Matcher;
32-
import com.google.errorprone.util.ASTHelpers;
33-
import com.sun.source.tree.ExpressionTree;
3424
import com.sun.source.tree.MethodInvocationTree;
3525
import java.util.regex.Pattern;
3626
import java.util.regex.PatternSyntaxException;
@@ -40,84 +30,17 @@
4030
name = "InvalidPatternSyntax",
4131
summary = "Invalid syntax used for a regular expression",
4232
severity = ERROR)
43-
public class InvalidPatternSyntax extends BugChecker implements MethodInvocationTreeMatcher {
33+
public class InvalidPatternSyntax extends AbstractPatternSyntaxChecker {
4434

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

47-
/* Match string literals that are not valid syntax for regular expressions. */
48-
private static final Matcher<ExpressionTree> BAD_REGEX_LITERAL =
49-
new Matcher<ExpressionTree>() {
50-
@Override
51-
public boolean matches(ExpressionTree tree, VisitorState state) {
52-
String value = ASTHelpers.constValue(tree, String.class);
53-
return value != null && !isValidSyntax(value);
54-
}
55-
56-
private boolean isValidSyntax(String regex) {
57-
// Actually valid, but useless.
58-
if (".".equals(regex)) {
59-
return false;
60-
}
61-
try {
62-
Pattern.compile(regex);
63-
return true;
64-
} catch (PatternSyntaxException e) {
65-
return false;
66-
}
67-
}
68-
};
69-
70-
/*
71-
* Match invocations to regex-accepting methods with bad string literals.
72-
*
73-
* <p>We deliberately omit Pattern.compile itself, as most of its users appear to be either
74-
* passing e.g. LITERAL flags, deliberately testing the regex compiler, or deliberately
75-
* using "." as the "vacuously true regex."
76-
*/
77-
private static final Matcher<MethodInvocationTree> BAD_REGEX_USAGE =
78-
allOf(
79-
anyOf(
80-
instanceMethod()
81-
.onExactClass("java.lang.String")
82-
.namedAnyOf("matches", "split")
83-
.withParameters("java.lang.String"),
84-
instanceMethod()
85-
.onExactClass("java.lang.String")
86-
.named("split")
87-
.withParameters("java.lang.String", "int"),
88-
instanceMethod()
89-
.onExactClass("java.lang.String")
90-
.namedAnyOf("replaceFirst", "replaceAll")
91-
.withParameters("java.lang.String", "java.lang.String"),
92-
staticMethod().onClass("java.util.regex.Pattern").named("matches"),
93-
staticMethod().onClass("com.google.common.base.Splitter").named("onPattern")),
94-
argument(0, BAD_REGEX_LITERAL));
95-
9637
@Override
97-
public Description matchMethodInvocation(
98-
MethodInvocationTree methodInvocationTree, VisitorState state) {
99-
if (!BAD_REGEX_USAGE.matches(methodInvocationTree, state)) {
100-
return Description.NO_MATCH;
38+
protected final Description matchRegexLiteral(MethodInvocationTree tree, String regex) {
39+
try {
40+
Pattern.compile(regex);
41+
return NO_MATCH;
42+
} catch (PatternSyntaxException e) {
43+
return buildDescription(tree).setMessage(MESSAGE_BASE + e.getMessage()).build();
10144
}
102-
103-
// TODO: Suggest fixes for more situations.
104-
Description.Builder descriptionBuilder = buildDescription(methodInvocationTree);
105-
ExpressionTree arg = methodInvocationTree.getArguments().get(0);
106-
String value = ASTHelpers.constValue(arg, String.class);
107-
String reasonInvalid = "";
108-
109-
if (".".equals(value)) {
110-
descriptionBuilder.addFix(SuggestedFix.replace(arg, "\"\\\\.\""));
111-
reasonInvalid = "\".\" is a valid but useless regex";
112-
} else {
113-
try {
114-
Pattern.compile(value);
115-
} catch (PatternSyntaxException e) {
116-
reasonInvalid = e.getMessage();
117-
}
118-
}
119-
120-
descriptionBuilder.setMessage(MESSAGE_BASE + reasonInvalid);
121-
return descriptionBuilder.build();
12245
}
12346
}

‎core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java

+2
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,7 @@
4747
import com.google.errorprone.bugpatterns.BadInstanceof;
4848
import com.google.errorprone.bugpatterns.BadShiftAmount;
4949
import com.google.errorprone.bugpatterns.BanSerializableRead;
50+
import com.google.errorprone.bugpatterns.BareDotMetacharacter;
5051
import com.google.errorprone.bugpatterns.BigDecimalEquals;
5152
import com.google.errorprone.bugpatterns.BigDecimalLiteralDouble;
5253
import com.google.errorprone.bugpatterns.BooleanParameter;
@@ -764,6 +765,7 @@ public static ScannerSupplier errorChecks() {
764765
BadComparable.class,
765766
BadImport.class,
766767
BadInstanceof.class,
768+
BareDotMetacharacter.class,
767769
BigDecimalEquals.class,
768770
BigDecimalLiteralDouble.class,
769771
BoxedPrimitiveConstructor.class,

‎core/src/test/java/com/google/errorprone/bugpatterns/BareDotMetacharacterTest.java

+54
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,54 @@
1+
/*
2+
* Copyright 2012 The Error Prone Authors.
3+
*
4+
* Licensed under the Apache License, Version 2.0 (the "License");
5+
* you may not use this file except in compliance with the License.
6+
* You may obtain a copy of the License at
7+
*
8+
* http://www.apache.org/licenses/LICENSE-2.0
9+
*
10+
* Unless required by applicable law or agreed to in writing, software
11+
* distributed under the License is distributed on an "AS IS" BASIS,
12+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
* See the License for the specific language governing permissions and
14+
* limitations under the License.
15+
*/
16+
17+
package com.google.errorprone.bugpatterns;
18+
19+
import com.google.errorprone.BugCheckerRefactoringTestHelper;
20+
import org.junit.Test;
21+
import org.junit.runner.RunWith;
22+
import org.junit.runners.JUnit4;
23+
24+
/** Tests for {@link BareDotMetacharacter}. */
25+
@RunWith(JUnit4.class)
26+
public class BareDotMetacharacterTest {
27+
28+
private final BugCheckerRefactoringTestHelper refactoringHelper =
29+
BugCheckerRefactoringTestHelper.newInstance(BareDotMetacharacter.class, getClass());
30+
31+
@Test
32+
public void testPositiveCase() {
33+
refactoringHelper
34+
.addInputLines(
35+
"Test.java",
36+
"import com.google.common.base.Splitter;",
37+
"class Test {",
38+
" private static final String DOT = \".\";",
39+
" Object x = \"foo.bar\".split(\".\");",
40+
" Object y = \"foo.bonk\".split(DOT);",
41+
" Object z = Splitter.onPattern(\".\");",
42+
"}")
43+
.addOutputLines(
44+
"Test.java",
45+
"import com.google.common.base.Splitter;",
46+
"class Test {",
47+
" private static final String DOT = \".\";",
48+
" Object x = \"foo.bar\".split(\"\\\\.\");",
49+
" Object y = \"foo.bonk\".split(\"\\\\.\");",
50+
" Object z = Splitter.onPattern(\"\\\\.\");",
51+
"}")
52+
.doTest();
53+
}
54+
}

‎core/src/test/java/com/google/errorprone/bugpatterns/testdata/InvalidPatternSyntaxPositiveCases.java

-10
Original file line numberDiff line numberDiff line change
@@ -16,13 +16,11 @@
1616

1717
package com.google.errorprone.bugpatterns.testdata;
1818

19-
import com.google.common.base.Splitter;
2019
import java.util.regex.Pattern;
2120

2221
/** @author [email protected] (Matthew Dempsky) */
2322
public class InvalidPatternSyntaxPositiveCases {
2423
public static final String INVALID = "*";
25-
public static final String DOT = ".";
2624

2725
{
2826
// BUG: Diagnostic contains: Unclosed character class
@@ -44,13 +42,5 @@ public class InvalidPatternSyntaxPositiveCases {
4442
"".split(INVALID);
4543
// BUG: Diagnostic contains:
4644
"".split(INVALID, 0);
47-
48-
// BUG: Diagnostic contains: "foo.bar".split("\\.")
49-
"foo.bar".split(".");
50-
// BUG: Diagnostic contains:
51-
"foo.bonk".split(DOT);
52-
53-
// BUG: Diagnostic contains: Splitter.onPattern("\\.")
54-
Splitter.onPattern(".");
5545
}
5646
}

0 commit comments

Comments
 (0)