diff --git a/AUTHORS b/AUTHORS index 9fe9c2929ea..76a89c0e264 100644 --- a/AUTHORS +++ b/AUTHORS @@ -1,7 +1,8 @@ # This is the list of Error Prone authors for copyright purposes. # # This does not necessarily list everyone who has contributed code, since in -# some cases, their employer may be the copyright holder. To see the full list +# some cases, their employer may be the copyright holder. To see the full list # of contributors, see the revision history in source control. Google Inc. Two Sigma Open Source +Picnic Technologies diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/StringCaseLocaleUsage.java b/core/src/main/java/com/google/errorprone/bugpatterns/StringCaseLocaleUsage.java new file mode 100644 index 00000000000..933bd3cab8e --- /dev/null +++ b/core/src/main/java/com/google/errorprone/bugpatterns/StringCaseLocaleUsage.java @@ -0,0 +1,90 @@ +/* + * Copyright 2023 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.WARNING; +import static com.google.errorprone.BugPattern.StandardTags.FRAGILE_CODE; +import static com.google.errorprone.matchers.method.MethodMatchers.instanceMethod; +import static com.sun.tools.javac.parser.Tokens.TokenKind.RPAREN; + +import com.google.common.collect.Streams; +import com.google.errorprone.BugPattern; +import com.google.errorprone.VisitorState; +import com.google.errorprone.bugpatterns.BugChecker.MethodInvocationTreeMatcher; +import com.google.errorprone.fixes.Fix; +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.ErrorProneTokens; +import com.sun.source.tree.ExpressionTree; +import com.sun.source.tree.MethodInvocationTree; +import com.sun.tools.javac.util.Position; + +/** + * A {@link BugChecker} that flags calls to {@link String#toLowerCase()} and {@link + * String#toUpperCase()}, as these methods implicitly rely on the environment's default locale. + */ +@BugPattern( + summary = "Specify a `Locale` when calling `String#to{Lower,Upper}Case`", + severity = WARNING, + tags = FRAGILE_CODE) +public final class StringCaseLocaleUsage extends BugChecker implements MethodInvocationTreeMatcher { + private static final Matcher DEFAULT_LOCALE_CASE_CONVERSION = + instanceMethod() + .onExactClass(String.class.getName()) + .namedAnyOf("toLowerCase", "toUpperCase") + .withNoParameters(); + + @Override + public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) { + if (!DEFAULT_LOCALE_CASE_CONVERSION.matches(tree, state)) { + return Description.NO_MATCH; + } + + int closingParenPosition = getClosingParenPosition(tree, state); + if (closingParenPosition == Position.NOPOS) { + return describeMatch(tree); + } + + return buildDescription(tree) + .addFix(suggestLocale(closingParenPosition, "Locale.ROOT")) + .addFix(suggestLocale(closingParenPosition, "Locale.getDefault()")) + .build(); + } + + private static Fix suggestLocale(int insertPosition, String locale) { + return SuggestedFix.builder() + .addImport("java.util.Locale") + .replace(insertPosition, insertPosition, locale) + .build(); + } + + // TODO: Consider making this a helper method in `SuggestedFixes`. + private static int getClosingParenPosition(MethodInvocationTree tree, VisitorState state) { + int startPosition = ASTHelpers.getStartPosition(tree); + if (startPosition == Position.NOPOS) { + return Position.NOPOS; + } + + return Streams.findLast( + ErrorProneTokens.getTokens(state.getSourceForNode(tree), state.context).stream() + .filter(t -> t.kind() == RPAREN)) + .map(token -> startPosition + token.pos()) + .orElse(Position.NOPOS); + } +} 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 6102472ab5f..ef87f32b72e 100644 --- a/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java +++ b/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java @@ -327,6 +327,7 @@ import com.google.errorprone.bugpatterns.StreamToIterable; import com.google.errorprone.bugpatterns.StreamToString; import com.google.errorprone.bugpatterns.StringBuilderInitWithChar; +import com.google.errorprone.bugpatterns.StringCaseLocaleUsage; import com.google.errorprone.bugpatterns.StringSplitter; import com.google.errorprone.bugpatterns.StronglyTypeByteString; import com.google.errorprone.bugpatterns.SubstringOfZero; @@ -987,6 +988,7 @@ public static ScannerSupplier errorChecks() { StaticMockMember.class, StreamResourceLeak.class, StreamToIterable.class, + StringCaseLocaleUsage.class, StringSplitter.class, Suggester.class, SwigMemoryLeak.class, diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/StringCaseLocaleUsageTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/StringCaseLocaleUsageTest.java new file mode 100644 index 00000000000..fde7da00950 --- /dev/null +++ b/core/src/test/java/com/google/errorprone/bugpatterns/StringCaseLocaleUsageTest.java @@ -0,0 +1,149 @@ +/* + * Copyright 2023 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 com.google.errorprone.BugCheckerRefactoringTestHelper; +import com.google.errorprone.BugCheckerRefactoringTestHelper.FixChoosers; +import com.google.errorprone.BugCheckerRefactoringTestHelper.TestMode; +import com.google.errorprone.CompilationTestHelper; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; + +/** Tests for {@link StringCaseLocaleUsage}. */ +@RunWith(JUnit4.class) +public final class StringCaseLocaleUsageTest { + CompilationTestHelper compilationTestHelper = + CompilationTestHelper.newInstance(StringCaseLocaleUsage.class, getClass()); + BugCheckerRefactoringTestHelper refactoringTestHelper = + BugCheckerRefactoringTestHelper.newInstance(StringCaseLocaleUsage.class, getClass()); + + @Test + public void identification() { + compilationTestHelper + .addSourceLines( + "A.java", + "import static java.util.Locale.ROOT;", + "", + "import java.util.Locale;", + "", + "class A {", + " void m() {", + " \"a\".toLowerCase(Locale.ROOT);", + " \"a\".toUpperCase(Locale.ROOT);", + " \"b\".toLowerCase(ROOT);", + " \"b\".toUpperCase(ROOT);", + " \"c\".toLowerCase(Locale.getDefault());", + " \"c\".toUpperCase(Locale.getDefault());", + " \"d\".toLowerCase(Locale.ENGLISH);", + " \"d\".toUpperCase(Locale.ENGLISH);", + " \"e\".toLowerCase(new Locale(\"foo\"));", + " \"e\".toUpperCase(new Locale(\"foo\"));", + "", + " // BUG: Diagnostic contains:", + " \"f\".toLowerCase();", + " // BUG: Diagnostic contains:", + " \"g\".toUpperCase();", + "", + " String h = \"h\";", + " // BUG: Diagnostic contains:", + " h.toLowerCase();", + " String i = \"i\";", + " // BUG: Diagnostic contains:", + " i.toUpperCase();", + " }", + "}") + .doTest(); + } + + @Test + public void replacementFirstSuggestedFix() { + refactoringTestHelper + .addInputLines( + "A.java", + "class A {", + " void m() {", + " \"a\".toLowerCase(/* Comment with parens: (). */ );", + " \"b\".toUpperCase();", + " \"c\".toLowerCase().toString();", + "", + " toString().toLowerCase();", + " toString().toUpperCase /* Comment with parens: (). */();", + "", + " this.toString().toLowerCase() /* Comment with parens: (). */;", + " this.toString().toUpperCase();", + " }", + "}") + .addOutputLines( + "A.java", + "import java.util.Locale;", + "", + "class A {", + " void m() {", + " \"a\".toLowerCase(/* Comment with parens: (). */ Locale.ROOT);", + " \"b\".toUpperCase(Locale.ROOT);", + " \"c\".toLowerCase(Locale.ROOT).toString();", + "", + " toString().toLowerCase(Locale.ROOT);", + " toString().toUpperCase /* Comment with parens: (). */(Locale.ROOT);", + "", + " this.toString().toLowerCase(Locale.ROOT) /* Comment with parens: (). */;", + " this.toString().toUpperCase(Locale.ROOT);", + " }", + "}") + .doTest(TestMode.TEXT_MATCH); + } + + @Test + public void replacementSecondSuggestedFix() { + BugCheckerRefactoringTestHelper.newInstance(StringCaseLocaleUsage.class, getClass()) + .setFixChooser(FixChoosers.SECOND) + .addInputLines( + "A.java", + "class A {", + " void m() {", + " \"a\".toLowerCase();", + " \"b\".toUpperCase(/* Comment with parens: (). */ );", + " \"c\".toLowerCase().toString();", + "", + " toString().toLowerCase();", + " toString().toUpperCase /* Comment with parens: (). */();", + "", + " this.toString().toLowerCase() /* Comment with parens: (). */;", + " this.toString().toUpperCase();", + " }", + "}") + .addOutputLines( + "A.java", + "import java.util.Locale;", + "", + "class A {", + " void m() {", + " \"a\".toLowerCase(Locale.getDefault());", + " \"b\".toUpperCase(/* Comment with parens: (). */ Locale.getDefault());", + " \"c\".toLowerCase(Locale.getDefault()).toString();", + "", + " toString().toLowerCase(Locale.getDefault());", + " toString().toUpperCase /* Comment with parens: (). */(Locale.getDefault());", + "", + " this.toString().toLowerCase(Locale.getDefault()) /* Comment with parens: (). */;", + " this.toString().toUpperCase(Locale.getDefault());", + " }", + "}") + .doTest(TestMode.TEXT_MATCH); + } +}