From 2aea991e20416006f469ca87c488f85ded46fe62 Mon Sep 17 00:00:00 2001 From: Liam Miller-Cushon Date: Mon, 18 Nov 2024 22:02:23 -0800 Subject: [PATCH] Add a check to replace concatenated multiline strings with text blocks PiperOrigin-RevId: 697872521 --- .../google/errorprone/util/ASTHelpers.java | 3 + .../errorprone/util/SourceCodeEscapers.java | 27 +- core/pom.xml | 2 +- .../bugpatterns/StringConcatToTextBlock.java | 284 ++++++++++++++++++ .../scanner/BuiltInCheckerSuppliers.java | 2 + .../StringConcatToTextBlockTest.java | 212 +++++++++++++ test_helpers/pom.xml | 2 +- .../BugCheckerRefactoringTestHelper.java | 4 +- 8 files changed, 530 insertions(+), 6 deletions(-) create mode 100644 core/src/main/java/com/google/errorprone/bugpatterns/StringConcatToTextBlock.java create mode 100644 core/src/test/java/com/google/errorprone/bugpatterns/StringConcatToTextBlockTest.java diff --git a/check_api/src/main/java/com/google/errorprone/util/ASTHelpers.java b/check_api/src/main/java/com/google/errorprone/util/ASTHelpers.java index 57a9b94b5e8..4e06db632d0 100644 --- a/check_api/src/main/java/com/google/errorprone/util/ASTHelpers.java +++ b/check_api/src/main/java/com/google/errorprone/util/ASTHelpers.java @@ -1279,6 +1279,9 @@ public static Nullness getNullnessValue( if (type.hasTag(TypeTag.BOOLEAN) && value instanceof Integer) { return ((Integer) value) == 1; } + if (type.hasTag(TypeTag.CHAR) && value instanceof Integer) { + return (char) (int) value; + } return value; } diff --git a/check_api/src/main/java/com/google/errorprone/util/SourceCodeEscapers.java b/check_api/src/main/java/com/google/errorprone/util/SourceCodeEscapers.java index 9e2e683610d..7115a832dc3 100644 --- a/check_api/src/main/java/com/google/errorprone/util/SourceCodeEscapers.java +++ b/check_api/src/main/java/com/google/errorprone/util/SourceCodeEscapers.java @@ -65,6 +65,27 @@ public static CharEscaper javaCharEscaper() { '\\', "\\\\", '\'', "\\'")); + /** + * Returns an {@link Escaper} instance that escapes special characters in a string so it can + * safely be included in either a Java text block. + * + *

See: The Java Language Specification for more details. + */ + public static CharEscaper getJavaTextBlockEscaper() { + return JAVA_TEXT_BLOCK_ESCAPER; + } + + private static final CharEscaper JAVA_TEXT_BLOCK_ESCAPER = + new JavaCharEscaper( + ImmutableMap.of( + '\b', "\\b", + '\f', "\\f", + '\n', "\\n", + '\r', "\\r", + '\t', "\\t", + '\\', "\\\\")); + // This escaper does not produce octal escape sequences. See: // http://java.sun.com/docs/books/jls/third_edition/html/lexical.html#101089 // "Octal escapes are provided for compatibility with C, but can express @@ -88,11 +109,11 @@ private static char[] asUnicodeHexEscape(char c) { r[0] = '\\'; r[1] = 'u'; r[5] = HEX_DIGITS[c & 0xF]; - c >>>= 4; + c = (char) (c >>> 4); r[4] = HEX_DIGITS[c & 0xF]; - c >>>= 4; + c = (char) (c >>> 4); r[3] = HEX_DIGITS[c & 0xF]; - c >>>= 4; + c = (char) (c >>> 4); r[2] = HEX_DIGITS[c & 0xF]; return r; } diff --git a/core/pom.xml b/core/pom.xml index b425e327b32..fe7614e283e 100644 --- a/core/pom.xml +++ b/core/pom.xml @@ -72,7 +72,7 @@ com.google.googlejavaformat google-java-format - 1.19.1 + 1.24.0 diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/StringConcatToTextBlock.java b/core/src/main/java/com/google/errorprone/bugpatterns/StringConcatToTextBlock.java new file mode 100644 index 00000000000..0bfbef320db --- /dev/null +++ b/core/src/main/java/com/google/errorprone/bugpatterns/StringConcatToTextBlock.java @@ -0,0 +1,284 @@ +/* + * 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.collect.ImmutableList.toImmutableList; +import static com.google.common.collect.Iterables.getLast; +import static com.google.common.collect.Iterables.getOnlyElement; +import static com.google.errorprone.BugPattern.SeverityLevel.WARNING; +import static com.google.errorprone.matchers.Description.NO_MATCH; +import static com.google.errorprone.matchers.method.MethodMatchers.constructor; +import static com.google.errorprone.matchers.method.MethodMatchers.instanceMethod; +import static com.google.errorprone.matchers.method.MethodMatchers.staticMethod; +import static com.google.errorprone.util.ASTHelpers.getReceiver; +import static com.google.errorprone.util.ASTHelpers.getStartPosition; +import static com.google.errorprone.util.ASTHelpers.getType; +import static com.google.errorprone.util.ASTHelpers.isSameType; +import static java.util.stream.Collectors.joining; + +import com.google.common.collect.ImmutableList; +import com.google.common.collect.Streams; +import com.google.errorprone.BugPattern; +import com.google.errorprone.VisitorState; +import com.google.errorprone.bugpatterns.BugChecker.LiteralTreeMatcher; +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.google.errorprone.util.ErrorProneToken; +import com.google.errorprone.util.ErrorProneTokens; +import com.google.errorprone.util.SourceCodeEscapers; +import com.sun.source.tree.BinaryTree; +import com.sun.source.tree.ExpressionTree; +import com.sun.source.tree.LineMap; +import com.sun.source.tree.LiteralTree; +import com.sun.source.tree.MethodInvocationTree; +import com.sun.source.tree.NewClassTree; +import com.sun.source.tree.Tree; +import com.sun.tools.javac.parser.Tokens; +import java.util.ArrayDeque; +import java.util.Deque; +import java.util.List; +import java.util.Optional; +import java.util.stream.Stream; + +/** A {@link BugChecker}; see the associated {@link BugPattern} annotation for details. */ +@BugPattern( + summary = "This string literal can be written more clearly as a text block", + severity = WARNING) +public class StringConcatToTextBlock extends BugChecker + implements LiteralTreeMatcher, MethodInvocationTreeMatcher { + + public static final String DELIMITER = "\"\"\""; + + @Override + public Description matchLiteral(LiteralTree tree, VisitorState state) { + // javac constant folds string concat during parsing, so we don't need to handle binary + // expressions + // see -XDallowStringFolding=false + if (!tree.getKind().equals(Tree.Kind.STRING_LITERAL)) { + return NO_MATCH; + } + if (state.getPath().getParentPath().getLeaf() instanceof BinaryTree parent) { + if (parent.getKind().equals(Tree.Kind.PLUS) + && isSameType(getType(parent), state.getSymtab().stringType, state)) { + // the readability benefit of text blocks is less if they're inside another string + // concat expression + return NO_MATCH; + } + } + List tokens = state.getTokensForNode(tree); + ImmutableList strings = + tokens.stream() + .filter(t -> t.kind().equals(Tokens.TokenKind.STRINGLITERAL)) + .map(t -> t.stringVal()) + .collect(toImmutableList()); + boolean trailingNewline = getLast(strings).endsWith("\n"); + // Only migrate if there are enough lines to make it worthwhile. Escaping the trailing newline + // slightly reduces the readability benefit of migrating, so require an extra line to make it + // worth it. + int thresholdToMigrate = trailingNewline ? 2 : 3; + if (strings.size() < thresholdToMigrate) { + return NO_MATCH; + } + // only migrate if each piece of the string concat ends in an escaped newline. It would be + // possible to migrate more eagerly, and perhaps use escaped newlines to reflow the string, or + // recognize APIs that don't care about extra newlines. + if (!strings.stream().limit(strings.size() - 1).allMatch(s -> s.endsWith("\n"))) { + return NO_MATCH; + } + // shared logic in 'match' assumes the strings are split at newline boundaries, so trim the + // escaped newlines here + strings = + Streams.concat( + strings.stream().limit(strings.size() - 1).map(s -> s.substring(0, s.length() - 1)), + Stream.of(getLast(strings))) + .collect(toImmutableList()); + return match(tree, strings, state); + } + + @Override + public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) { + if (JOINER_JOIN.matches(tree, state)) { + return joiner(tree, state); + } + if (STRING_JOINER_TO_STRING.matches(tree, state)) { + return stringJoiner(tree, state); + } + if (STRING_JOIN.matches(tree, state)) { + return stringJoin(tree, state); + } + if (FOR_SOURCE_LINES.matches(tree, state)) { + return forSourceLines(tree, state); + } + return NO_MATCH; + } + + private Description match(Tree replace, ImmutableList lines, VisitorState state) { + return match(replace, getStartPosition(replace), state.getEndPosition(replace), lines, state); + } + + private Description match( + Tree tree, + int replaceFrom, + int replaceTo, + ImmutableList strings, + VisitorState state) { + if (strings.isEmpty()) { + return NO_MATCH; + } + ImmutableList tokens = + ErrorProneTokens.getTokens( + state.getSourceCode().subSequence(replaceFrom, replaceTo).toString(), state.context); + if (!tokens.stream() + .flatMap(t -> t.comments().stream()) + .map(c -> c.getText()) + .allMatch(x -> x.isEmpty())) { + // For long string literals with interspersed comments, moving all the comments to the + // beginning could regress readability. + // TODO: cushon - handle comments? + return NO_MATCH; + } + // bail out if the contents contain a delimiter that would require escaping + if (strings.stream().anyMatch(s -> s.contains(DELIMITER))) { + return NO_MATCH; + } + boolean trailingNewline = getLast(strings).endsWith("\n"); + strings = + String.join("\n", strings) + .stripIndent() + .lines() + .map(s -> s.stripTrailing()) + .collect(toImmutableList()); + LineMap lineMap = state.getPath().getCompilationUnit().getLineMap(); + String indent = " ".repeat((int) lineMap.getColumnNumber(replaceFrom) - 1); + String suffix = trailingNewline ? "" : "\\"; + String replacement = + strings.stream() + .map(line -> line.isEmpty() ? line : (indent + line)) + .map(SourceCodeEscapers.getJavaTextBlockEscaper()::escape) + .collect(joining("\n", DELIMITER + "\n", suffix + "\n" + indent + DELIMITER)); + SuggestedFix fix = SuggestedFix.replace(replaceFrom, replaceTo, replacement); + return describeMatch(tree, fix); + } + + private static final Matcher JOINER_JOIN = + instanceMethod().onExactClass("com.google.common.base.Joiner").named("join"); + private static final Matcher JOINER_ON = + staticMethod().onClass("com.google.common.base.Joiner").named("on"); + + private static final Matcher STRING_JOINER_TO_STRING = + instanceMethod().onExactClass("java.util.StringJoiner").named("toString"); + private static final Matcher STRING_JOINER_ADD = + instanceMethod().onExactClass("java.util.StringJoiner").named("add"); + private static final Matcher STRING_JOINER_CONSTRUCTOR = + constructor().forClass("java.util.StringJoiner").withParameters("java.lang.CharSequence"); + + private static final Matcher STRING_JOIN = + staticMethod().onClass("java.lang.String").named("join"); + + private static final Matcher FOR_SOURCE_LINES = + staticMethod().onClass("com.google.testing.compile.JavaFileObjects").named("forSourceLines"); + + private Description joiner(MethodInvocationTree tree, VisitorState state) { + ImmutableList strings = stringLiteralArguments(tree.getArguments()); + if (strings.isEmpty()) { + return NO_MATCH; + } + if (!(getReceiver(tree) instanceof MethodInvocationTree receiver)) { + return NO_MATCH; + } + if (!JOINER_ON.matches(receiver, state)) { + return NO_MATCH; + } + if (!newlineLiteral(getOnlyElement(receiver.getArguments()))) { + return NO_MATCH; + } + return match(tree, strings, state); + } + + private Description stringJoiner(MethodInvocationTree tree, VisitorState state) { + Deque strings = new ArrayDeque<>(); + ExpressionTree current = tree; + while (getReceiver(current) instanceof MethodInvocationTree receiver) { + if (!STRING_JOINER_ADD.matches(receiver, state)) { + break; + } + Optional string = stringLiteral(getOnlyElement(receiver.getArguments())); + if (string.isEmpty()) { + return NO_MATCH; + } + strings.addFirst(string.get()); + current = receiver; + } + if (!(getReceiver(current) instanceof NewClassTree constructor + && STRING_JOINER_CONSTRUCTOR.matches(constructor, state))) { + return NO_MATCH; + } + if (!newlineLiteral(getOnlyElement(constructor.getArguments()))) { + return NO_MATCH; + } + return match(tree, ImmutableList.copyOf(strings), state); + } + + private Description stringJoin(MethodInvocationTree tree, VisitorState state) { + ImmutableList strings = + stringLiteralArguments(tree.getArguments().subList(1, tree.getArguments().size())); + if (!newlineLiteral(tree.getArguments().get(0))) { + return NO_MATCH; + } + return match(tree, strings, state); + } + + private Description forSourceLines(MethodInvocationTree tree, VisitorState state) { + ImmutableList strings = + stringLiteralArguments(tree.getArguments().subList(1, tree.getArguments().size())); + return match( + tree.getArguments().get(1), + getStartPosition(tree.getArguments().get(1)), + state.getEndPosition(getLast(tree.getArguments())), + strings, + state); + } + + private static boolean newlineLiteral(ExpressionTree expressionTree) { + Object value = ASTHelpers.constValue(expressionTree); + if (value == null) { + return false; + } + return value.equals("\n") || value.equals('\n'); + } + + static ImmutableList stringLiteralArguments(List arguments) { + ImmutableList strings = + arguments.stream() + .filter(a -> a.getKind().equals(Tree.Kind.STRING_LITERAL)) + .map(x -> (String) ((LiteralTree) x).getValue()) + .collect(toImmutableList()); + if (strings.size() != arguments.size()) { + return ImmutableList.of(); + } + return strings; + } + + static Optional stringLiteral(ExpressionTree tree) { + return tree.getKind().equals(Tree.Kind.STRING_LITERAL) + ? Optional.of((String) ((LiteralTree) tree).getValue()) + : Optional.empty(); + } +} 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 7ddf1061ce9..c4eab2295ac 100644 --- a/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java +++ b/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java @@ -360,6 +360,7 @@ import com.google.errorprone.bugpatterns.StringBuilderInitWithChar; import com.google.errorprone.bugpatterns.StringCaseLocaleUsage; import com.google.errorprone.bugpatterns.StringCharset; +import com.google.errorprone.bugpatterns.StringConcatToTextBlock; import com.google.errorprone.bugpatterns.StringFormatWithLiteral; import com.google.errorprone.bugpatterns.StringSplitter; import com.google.errorprone.bugpatterns.StronglyTypeByteString; @@ -1082,6 +1083,7 @@ public static ScannerSupplier warningChecks() { StreamToIterable.class, StringCaseLocaleUsage.class, StringCharset.class, + StringConcatToTextBlock.class, StringSplitter.class, Suggester.class, SuperCallToObjectMethod.class, diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/StringConcatToTextBlockTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/StringConcatToTextBlockTest.java new file mode 100644 index 00000000000..ab05655ac96 --- /dev/null +++ b/core/src/test/java/com/google/errorprone/bugpatterns/StringConcatToTextBlockTest.java @@ -0,0 +1,212 @@ +/* + * 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.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; + +@RunWith(JUnit4.class) +public class StringConcatToTextBlockTest { + private final CompilationTestHelper testHelper = + CompilationTestHelper.newInstance(StringConcatToTextBlock.class, getClass()); + private final BugCheckerRefactoringTestHelper refactoringHelper = + BugCheckerRefactoringTestHelper.newInstance(StringConcatToTextBlock.class, getClass()); + + @Test + public void negative() { + testHelper + .addSourceLines( + "Test.java", + """ + class Test { + String s = new Object() + "world\\n"; + String oneToken = "hello\\nworld"; + String comment = + "hello\\\\n" + // comment + + "world\\\\n"; + String noNewline = "hello" + "world\\n"; + String extra = "prefix" + s + "hello\\n" + "world\\n"; + String noTrailing = "hello\\n" + "world"; + } + """) + .doTest(); + } + + @Test + public void positive() { + refactoringHelper + .addInputLines( + "Test.java", + """ + class Test { + String s = "hello\\n" + "world\\n"; + } + """) + .addOutputLines( + "Test.java", + """ + class Test { + String s = + \""" + hello + world + \"""; + } + """) + .doTest(TEXT_MATCH); + } + + @Test + public void noTrailing() { + refactoringHelper + .addInputLines( + "Test.java", + """ + class Test { + String s = "hello\\n" + "foo\\n" + "world"; + } + """) + .addOutputLines( + "Test.java", + """ + class Test { + String s = + \""" + hello + foo + world\\ + \"""; + } + """) + .doTest(TEXT_MATCH); + } + + @Test + public void escape() { + refactoringHelper + .addInputLines( + "Test.java", + """ + class Test { + String s = "\\n" + "\\nhello\\n" + "foo\\n\\n" + "world"; + } + """) + .addOutputLines( + "Test.java", + """ + class Test { + String s = + \""" + + + hello + foo + + world\\ + \"""; + } + """) + .doTest(TEXT_MATCH); + } + + @Test + public void guavaJoiner() { + refactoringHelper + .addInputLines( + "Test.java", + """ + import com.google.common.base.Joiner; + + class Test { + String s = Joiner.on('\\n').join("string", "literals"); + } + """) + .addOutputLines( + "Test.java", + """ + import com.google.common.base.Joiner; + class Test { + String s = + \""" + string + literals\\ + \"""; + } + """) + .doTest(TEXT_MATCH); + } + + @Test + public void stringJoiner() { + refactoringHelper + .addInputLines( + "Test.java", + """ + import java.util.StringJoiner; + + class Test { + String s = new StringJoiner("\\n").add("string").add("literals").toString(); + } + """) + .addOutputLines( + "Test.java", + """ + import java.util.StringJoiner; + class Test { + String s = + \""" + string + literals\\ + \"""; + } + """) + .doTest(TEXT_MATCH); + } + + @Test + public void stringJoin() { + refactoringHelper + .addInputLines( + "Test.java", + """ + import java.util.StringJoiner; + + class Test { + String s = String.join("\\n", "string", "literals"); + } + """) + .addOutputLines( + "Test.java", + """ + import java.util.StringJoiner; + class Test { + String s = + \""" + string + literals\\ + \"""; + } + """) + .doTest(TEXT_MATCH); + } +} diff --git a/test_helpers/pom.xml b/test_helpers/pom.xml index 66b12d308c3..d3e33bc9d5d 100644 --- a/test_helpers/pom.xml +++ b/test_helpers/pom.xml @@ -86,7 +86,7 @@ com.google.googlejavaformat google-java-format - 1.19.1 + 1.24.0 diff --git a/test_helpers/src/main/java/com/google/errorprone/BugCheckerRefactoringTestHelper.java b/test_helpers/src/main/java/com/google/errorprone/BugCheckerRefactoringTestHelper.java index 60067294e2d..f984f06660a 100644 --- a/test_helpers/src/main/java/com/google/errorprone/BugCheckerRefactoringTestHelper.java +++ b/test_helpers/src/main/java/com/google/errorprone/BugCheckerRefactoringTestHelper.java @@ -49,6 +49,7 @@ import com.google.errorprone.scanner.ScannerSupplier; import com.google.googlejavaformat.java.Formatter; import com.google.googlejavaformat.java.FormatterException; +import com.google.googlejavaformat.java.StringWrapper; import com.google.testing.compile.JavaFileObjects; import com.sun.source.tree.CompilationUnitTree; import com.sun.source.util.TreePath; @@ -94,7 +95,8 @@ void verifyMatch(JavaFileObject refactoredSource, JavaFileObject expectedSource) private String maybeFormat(String input) { try { - return new Formatter().formatSource(input); + Formatter formatter = new Formatter(); + return StringWrapper.wrap(formatter.formatSource(input), formatter); } catch (FormatterException e) { return input; }