Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Introduce StringCaseLocaleUsage check #3813

Closed
wants to merge 4 commits into from
Closed

Introduce StringCaseLocaleUsage check #3813

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
358fa8d
Introduce `StringCaseLocaleUsage` check
rickie Mar 9, 2023
0145d59
Add `Picnic Technologies` to `AUTHORS` and add copyright headers
rickie Mar 10, 2023
9df685e
Add `TODO`
rickie Mar 10, 2023
59abfd4
Reword...
rickie Mar 10, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
73 changes: 73 additions & 0 deletions core/src/main/java/com/google/errorprone/bugpatterns/StringCaseLocaleUsage.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
package com.google.errorprone.bugpatterns;
Copy link
Collaborator

@cushon cushon Mar 9, 2023 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'll need the copyright headers, I can also add them during the import if that's easier.

/*
 * 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.
 */

('The Error Prone Authors' refers to https://github.com/google/error-prone/blob/master/AUTHORS, you're welcome to add yourself there also. There's some more detail about that in https://opensource.google/documentation/reference/releasing/authors)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will push a commit with the copyright headers.

you're welcome to add yourself there also.

That'd be cool! Added that in the commit.

I can't reach the website with more details though, it asked me to do a sign in with a "@google.com" email, so it's probably an internal thing?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, there's an external mirror with the same content, I just pasted the wrong link. That should have been https://opensource.google/documentation/reference/releasing/authors


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<ExpressionTree> 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();
}

private static int getClosingParenPosition(MethodInvocationTree tree, VisitorState state) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just curious, did you see examples like .toUpperCase /* Comment with parens: (). */() in practice?

It's nice to handle this correctly, but I would normally have just done something like replace(getEndPosition(tree.getMethodSelect()), getEndPosition(tree), "(Locale.ROOT)")

Copy link
Contributor Author

@rickie rickie Mar 10, 2023 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, no we didn't encounter a lot of problems, we created this after our initial implementation (that was based on a String#replaceFirst) which wasn't correct enough.

It's indeed a bit more complex than what you are proposing. We can do 3 things:
(1) Extract this functionality to a utility class and re-use this setup, which might make it worth that it is slightly more complex.
(2) Keep it as-is.
(3) Use the method you are proposing.

Let me know what you prefer 😄.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd suggest creating a helper in SuggestedFixes, but I'm not sure how general it is without a way to choose where the argument is inserted, and I don't have a great ideas for the API. Maybe something like addArgumentToMethod with a parameter for the insertion index?

How about (2) for now, and if you feel like it you can add a TODO to consider promoting it to SuggestedFixes later :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deal, let's go for (2), I'll add a TODO.

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);
}
}
2 changes: 2 additions & 0 deletions core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -987,6 +988,7 @@ public static ScannerSupplier errorChecks() {
StaticMockMember.class,
StreamResourceLeak.class,
StreamToIterable.class,
StringCaseLocaleUsage.class,
StringSplitter.class,
Suggester.class,
SwigMemoryLeak.class,
Expand Down
133 changes: 133 additions & 0 deletions core/src/test/java/com/google/errorprone/bugpatterns/StringCaseLocaleUsageTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,133 @@
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);
}
}