Skip to content

Commit

Permalink
Add a check discouraging \s, unless it's a sequence of \s terminating…
Browse files Browse the repository at this point in the history
… a line.

What's your view on this being an error? On one hand, it seems sensible to make things errors now, given there's zero adoption, and relax later as needed.

PiperOrigin-RevId: 684828665
  • Loading branch information
graememorgan authored and Error Prone Team committed Oct 11, 2024
1 parent 1dbcb6e commit 78729ff
Show file tree
Hide file tree
Showing 4 changed files with 229 additions and 0 deletions.
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
/*
* Copyright 2024 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.ERROR;
import static com.google.errorprone.matchers.Description.NO_MATCH;
import static com.google.errorprone.util.SourceVersion.supportsTextBlocks;

import com.google.errorprone.BugPattern;
import com.google.errorprone.VisitorState;
import com.google.errorprone.bugpatterns.BugChecker.LiteralTreeMatcher;
import com.google.errorprone.matchers.Description;
import com.sun.source.tree.LiteralTree;

/** See the summary. */
@BugPattern(
summary =
"Using \\s anywhere except at the end of a line in a text block is potentially misleading.",
severity = ERROR)
public final class MisleadingEscapedSpace extends BugChecker implements LiteralTreeMatcher {
@Override
public Description matchLiteral(LiteralTree tree, VisitorState state) {
if (!supportsTextBlocks(state.context)) {
return NO_MATCH;
}
if (tree.getValue() instanceof Character) {
if (tree.getValue().equals(' ') && state.getSourceForNode(tree).equals("'\\s'")) {
return describeMatch(tree);
}
}
if (tree.getValue() instanceof String) {
// Fast path out and avoid scanning through source code if there are simply no spaces in the
// literal.
String value = (String) tree.getValue();
if (!value.contains(" ")) {
return NO_MATCH;
}
String source = state.getSourceForNode(tree);
boolean seenEscape = false;
for (int i = 0; i < source.length(); ++i) {
switch (source.charAt(i)) {
case '\n':
seenEscape = false;
break;
case '\\':
i++;
if (source.charAt(i) == 's') {
seenEscape = true;
break;
}
// fall through
default:
if (seenEscape) {
return describeMatch(tree);
}
break;
}
}
}
return NO_MATCH;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -236,6 +236,7 @@
import com.google.errorprone.bugpatterns.MemoizeConstantVisitorStateLookups;
import com.google.errorprone.bugpatterns.MethodCanBeStatic;
import com.google.errorprone.bugpatterns.MisformattedTestData;
import com.google.errorprone.bugpatterns.MisleadingEscapedSpace;
import com.google.errorprone.bugpatterns.MissingBraces;
import com.google.errorprone.bugpatterns.MissingCasesInEnumSwitch;
import com.google.errorprone.bugpatterns.MissingDefault;
Expand Down Expand Up @@ -767,6 +768,7 @@ public static ScannerSupplier warningChecks() {
LossyPrimitiveCompare.class,
MathRoundIntLong.class,
MislabeledAndroidString.class,
MisleadingEscapedSpace.class,
MisplacedScopeAnnotations.class,
MissingSuperCall.class,
MissingTestCall.class,
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,129 @@
/*
* Copyright 2024 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.common.truth.TruthJUnit.assume;

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

@RunWith(JUnit4.class)
public final class MisleadingEscapedSpaceTest {
private final CompilationTestHelper testHelper =
CompilationTestHelper.newInstance(MisleadingEscapedSpace.class, getClass());

@Test
public void misleadingEscape() {
assume().that(Runtime.version().feature()).isAtLeast(14);

testHelper
.addSourceLines(
"Test.class",
"""
class Test {
// BUG: Diagnostic contains:
private static final String FOO = " \\s ";
}""")
.doTest();
}

@Test
public void literalBackslashS() {
assume().that(Runtime.version().feature()).isAtLeast(14);

testHelper
.addSourceLines(
"Test.class",
"""
class Test {
private static final String FOO = " \\\\s ";
}""")
.doTest();
}

@Test
public void asSingleCharacter_misleading() {
assume().that(Runtime.version().feature()).isAtLeast(14);

testHelper
.addSourceLines(
"Test.class",
"""
class Test {
// BUG: Diagnostic contains:
private static final char x = '\\s';
}""")
.doTest();
}

@Test
public void withinTextBlock_notAtEndOfLine_misleading() {
assume().that(Runtime.version().feature()).isAtLeast(14);

testHelper
.addSourceLines(
"Test.class",
"""
class Test {
// BUG: Diagnostic contains:
private static final String FOO = \"""
foo \\s bar
\""";
// BUG: Diagnostic contains:
private static final String BAZ = \"""
foo \\s
bar \\s baz
\""";
}""")
.doTest();
}

@Test
public void atEndOfLine_notMisleading() {
assume().that(Runtime.version().feature()).isAtLeast(14);

testHelper
.addSourceLines(
"Test.class",
"""
class Test {
private static final String FOO = \"""
foo \\s
bar \\s
\""";
}""")
.doTest();
}

@Test
public void multipleAtEndOfLine_notMisleading() {
assume().that(Runtime.version().feature()).isAtLeast(14);

testHelper
.addSourceLines(
"Test.class",
"""
class Test {
private static final String FOO = \"""
foo \\s\\s\\s\\s
\""";
}""")
.doTest();
}
}
22 changes: 22 additions & 0 deletions docs/bugpattern/MisleadingEscapedSpace.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
When Java introduced text blocks as a feature, it also introduced a new string
escape sequence `\s`. This escape sequence is another way to write a normal
space, but it has the advantage that it can be used at the end of a line in a
text block, where a normal space would be stripped.

This new escape sequence can easily be confused with the regex `\s`, which is a
metacharacter that matches any kind of whitespace character. To write that
metacharacter in a Java string, you must still write `\\s`: an escaped backslash
followed by an `s`.

There is little reason to ever write the Java escape `\s` except at the end of a
line. Either use a normal space, or switch to `\\s` if you are trying to write
the regex metacharacter.

```java
// Each line here is five characters long.
String colors = """
one \s
two \s
three
""";
```

0 comments on commit 78729ff

Please sign in to comment.