From 63bcc5f1c9066946bac779baf934e365869e66b2 Mon Sep 17 00:00:00 2001 From: ghm Date: Wed, 25 Sep 2024 06:51:23 -0700 Subject: [PATCH] Add a check to reformat poorly formatted EP tests. PiperOrigin-RevId: 678681902 --- core/pom.xml | 6 + .../bugpatterns/MisformattedTestData.java | 132 ++++++++++++ .../scanner/BuiltInCheckerSuppliers.java | 2 + .../bugpatterns/MisformattedTestDataTest.java | 194 ++++++++++++++++++ 4 files changed, 334 insertions(+) create mode 100644 core/src/main/java/com/google/errorprone/bugpatterns/MisformattedTestData.java create mode 100644 core/src/test/java/com/google/errorprone/bugpatterns/MisformattedTestDataTest.java diff --git a/core/pom.xml b/core/pom.xml index 41f23273342..40dac702e26 100644 --- a/core/pom.xml +++ b/core/pom.xml @@ -68,6 +68,12 @@ ${project.version} test + + + com.google.googlejavaformat + google-java-format + 1.19.1 + org.pcollections diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/MisformattedTestData.java b/core/src/main/java/com/google/errorprone/bugpatterns/MisformattedTestData.java new file mode 100644 index 00000000000..5f47339ff5f --- /dev/null +++ b/core/src/main/java/com/google/errorprone/bugpatterns/MisformattedTestData.java @@ -0,0 +1,132 @@ +/* + * 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.SUGGESTION; +import static com.google.errorprone.matchers.Description.NO_MATCH; +import static com.google.errorprone.matchers.Matchers.anyOf; +import static com.google.errorprone.matchers.method.MethodMatchers.instanceMethod; +import static com.google.errorprone.util.ASTHelpers.getStartPosition; +import static com.google.errorprone.util.ErrorProneTokens.getTokens; +import static com.google.errorprone.util.SourceVersion.supportsTextBlocks; +import static java.util.stream.Collectors.joining; + +import com.google.common.base.Splitter; +import com.google.common.escape.Escaper; +import com.google.common.escape.Escapers; +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.googlejavaformat.java.Formatter; +import com.google.googlejavaformat.java.FormatterException; +import com.sun.source.tree.ExpressionTree; +import com.sun.source.tree.LiteralTree; +import com.sun.source.tree.MethodInvocationTree; +import com.sun.tools.javac.parser.Tokens.TokenKind; + +/** See the summary. */ +@BugPattern( + severity = SUGGESTION, + summary = "This test data will be more readable if correctly formatted.") +public final class MisformattedTestData extends BugChecker implements MethodInvocationTreeMatcher { + @Override + public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) { + // We're not only matching text blocks below, just taking the fact that it's a single literal + // argument containing source code as a sign that it should be formatted in a text block. + if (!supportsTextBlocks(state.context)) { + return NO_MATCH; + } + if (!ADD_SOURCE_CALL.matches(tree, state)) { + return NO_MATCH; + } + if (tree.getArguments().size() != 2) { + return NO_MATCH; + } + var sourceTree = tree.getArguments().get(1); + if (!(sourceTree instanceof LiteralTree)) { + return NO_MATCH; + } + var sourceValue = ((LiteralTree) sourceTree).getValue(); + if (!(sourceValue instanceof String)) { + return NO_MATCH; + } + + Formatter formatter = new Formatter(); + String formattedSource; + try { + formattedSource = formatter.formatSource((String) sourceValue); + } catch (FormatterException exception) { + return NO_MATCH; + } + if (formattedSource.equals(sourceValue)) { + return NO_MATCH; + } + // This is a bit crude: but tokenize between the comma and the 2nd argument in order to work out + // an appropriate indent level for the text block. This is assuming that the source has already + // been formatted so that the arguments are nicely indented. + int startPos = state.getEndPosition(tree.getArguments().get(0)); + int endPos = getStartPosition(tree.getArguments().get(1)); + var tokens = + getTokens( + state.getSourceCode().subSequence(startPos, endPos).toString(), + startPos, + state.context); + var afterCommaPos = + tokens.reverse().stream() + .filter(t -> t.kind().equals(TokenKind.COMMA)) + .findFirst() + .orElseThrow() + .endPos(); + var spaces = + state.getSourceCode().subSequence(afterCommaPos, endPos).toString().replace("\n", ""); + return describeMatch( + sourceTree, + SuggestedFix.replace( + sourceTree, + "\"\"\"\n" + + LINE_SPLITTER + .splitToStream(escape(formattedSource)) + .map(line -> line.isEmpty() ? "" : spaces + line) + .collect(joining("\n")) + + spaces + + "\"\"\"")); + } + + // TODO(ghm): Consider generalising this via an annotation. + private static final Matcher ADD_SOURCE_CALL = + anyOf( + instanceMethod() + .onExactClass("com.google.errorprone.CompilationTestHelper") + .named("addSourceLines"), + instanceMethod() + .onExactClass("com.google.errorprone.BugCheckerRefactoringTestHelper") + .named("addInputLines"), + instanceMethod() + .onExactClass("com.google.errorprone.BugCheckerRefactoringTestHelper") + .named("addOutputLines")); + + private static final Splitter LINE_SPLITTER = Splitter.on('\n'); + + private static String escape(String line) { + return ESCAPER.escape(line).replace("\"\"\"", "\\\"\"\""); + } + + private static final Escaper ESCAPER = Escapers.builder().addEscape('\\', "\\\\").build(); +} 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 7b4c5834598..c373c0575f5 100644 --- a/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java +++ b/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java @@ -235,6 +235,7 @@ import com.google.errorprone.bugpatterns.MemberName; import com.google.errorprone.bugpatterns.MemoizeConstantVisitorStateLookups; import com.google.errorprone.bugpatterns.MethodCanBeStatic; +import com.google.errorprone.bugpatterns.MisformattedTestData; import com.google.errorprone.bugpatterns.MissingBraces; import com.google.errorprone.bugpatterns.MissingCasesInEnumSwitch; import com.google.errorprone.bugpatterns.MissingDefault; @@ -997,6 +998,7 @@ public static ScannerSupplier warningChecks() { MalformedInlineTag.class, MathAbsoluteNegative.class, MemoizeConstantVisitorStateLookups.class, + MisformattedTestData.class, MissingCasesInEnumSwitch.class, MissingFail.class, MissingImplementsComparable.class, diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/MisformattedTestDataTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/MisformattedTestDataTest.java new file mode 100644 index 00000000000..fd0a702173f --- /dev/null +++ b/core/src/test/java/com/google/errorprone/bugpatterns/MisformattedTestDataTest.java @@ -0,0 +1,194 @@ +/* + * 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 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 final class MisformattedTestDataTest { + private final CompilationTestHelper compilationHelper = + CompilationTestHelper.newInstance(MisformattedTestData.class, getClass()); + private final BugCheckerRefactoringTestHelper refactoringHelper = + BugCheckerRefactoringTestHelper.newInstance(MisformattedTestData.class, getClass()); + + @Test + public void alreadyFormatted_noFinding() { + assume().that(Runtime.version().feature()).isAtLeast(14); + + compilationHelper + .addSourceLines( + "Test.java", + """ + import com.google.errorprone.BugCheckerRefactoringTestHelper; + + class Test { + void method(BugCheckerRefactoringTestHelper h) { + h.addInputLines( + "Test.java", + \""" + package foo; + + class Test { + void method() { + int a = 1; + } + } + \"""); + } + } + """) + .doTest(); + } + + @Test + public void misformatted_suggestsFix() { + assume().that(Runtime.version().feature()).isAtLeast(14); + + refactoringHelper + .addInputLines( + "Test.java", + """ + import com.google.errorprone.BugCheckerRefactoringTestHelper; + + class Test { + void method(BugCheckerRefactoringTestHelper h) { + h.addInputLines( + "Test.java", + \""" + package foo; + class Test { + void method() { + int a = + 1; + } + } + \"""); + } + } + """) + .addOutputLines( + "Test.java", + """ + import com.google.errorprone.BugCheckerRefactoringTestHelper; + + class Test { + void method(BugCheckerRefactoringTestHelper h) { + h.addInputLines( + "Test.java", + \""" + package foo; + + class Test { + void method() { + int a = 1; + } + } + \"""); + } + } + """) + .doTest(TEXT_MATCH); + } + + @Test + public void onlyDiffersByIndentation_notReindented() { + assume().that(Runtime.version().feature()).isAtLeast(14); + + refactoringHelper + .addInputLines( + "Test.java", + """ + import com.google.errorprone.BugCheckerRefactoringTestHelper; + + class Test { + void method(BugCheckerRefactoringTestHelper h) { + h.addInputLines( + "Test.java", + \""" + package foo; + + class Test { + void method() { + int a = 1; + } + } + ""\"); + } + } + """) + .expectUnchanged() + .doTest(TEXT_MATCH); + } + + @Test + public void escapesSpecialCharacters() { + assume().that(Runtime.version().feature()).isAtLeast(14); + + refactoringHelper + .addInputLines( + "Test.java", + """ + import com.google.errorprone.BugCheckerRefactoringTestHelper; + + class Test { + void method(BugCheckerRefactoringTestHelper h) { + h.addInputLines( + "Test.java", + \""" + package foo; + + class Test { + void method() { + var foo + = "foo\\\\nbar"; + } + } + \"""); + } + } + """) + .addOutputLines( + "Test.java", + """ + import com.google.errorprone.BugCheckerRefactoringTestHelper; + + class Test { + void method(BugCheckerRefactoringTestHelper h) { + h.addInputLines( + "Test.java", + \""" + package foo; + + class Test { + void method() { + var foo = "foo\\\\nbar"; + } + } + \"""); + } + } + """) + .doTest(TEXT_MATCH); + } +}