From 45a8cb20c9d07df1ac5f7a2a43e80bc446c61bfc Mon Sep 17 00:00:00 2001 From: ghm Date: Wed, 9 Oct 2024 06:11:27 -0700 Subject: [PATCH] Add a check discouraging \s, unless it's a sequence of \s terminating 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: 684010257 --- .../bugpatterns/MisleadingEscapedSpace.java | 76 +++++++++++ .../scanner/BuiltInCheckerSuppliers.java | 2 + .../MisleadingEscapedSpaceTest.java | 129 ++++++++++++++++++ docs/bugpattern/MisleadingEscapedSpace.md | 22 +++ 4 files changed, 229 insertions(+) create mode 100644 core/src/main/java/com/google/errorprone/bugpatterns/MisleadingEscapedSpace.java create mode 100644 core/src/test/java/com/google/errorprone/bugpatterns/MisleadingEscapedSpaceTest.java create mode 100644 docs/bugpattern/MisleadingEscapedSpace.md diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/MisleadingEscapedSpace.java b/core/src/main/java/com/google/errorprone/bugpatterns/MisleadingEscapedSpace.java new file mode 100644 index 000000000000..ff26b540ad29 --- /dev/null +++ b/core/src/main/java/com/google/errorprone/bugpatterns/MisleadingEscapedSpace.java @@ -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; + } +} diff --git a/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java b/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java index c373c0575f51..63501d2466b6 100644 --- a/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java +++ b/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java @@ -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; @@ -767,6 +768,7 @@ public static ScannerSupplier warningChecks() { LossyPrimitiveCompare.class, MathRoundIntLong.class, MislabeledAndroidString.class, + MisleadingEscapedSpace.class, MisplacedScopeAnnotations.class, MissingSuperCall.class, MissingTestCall.class, diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/MisleadingEscapedSpaceTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/MisleadingEscapedSpaceTest.java new file mode 100644 index 000000000000..808edea9e61a --- /dev/null +++ b/core/src/test/java/com/google/errorprone/bugpatterns/MisleadingEscapedSpaceTest.java @@ -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(); + } +} diff --git a/docs/bugpattern/MisleadingEscapedSpace.md b/docs/bugpattern/MisleadingEscapedSpace.md new file mode 100644 index 000000000000..b22382df2448 --- /dev/null +++ b/docs/bugpattern/MisleadingEscapedSpace.md @@ -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 + """; +```