Introduce `StringCaseLocaleUsage` check #376

mlrprananta · 2022-11-28T09:11:33Z

PR for this Issue.

In short, a default Locale should be specified when using either String#toLowerCase() or String#toUpperCase().

Suggested commit message

Introduce `StringCaseLocaleUsage` check (#376)

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/SpecifyLocale.java

github-actions · 2022-12-01T14:52:52Z

Surviving mutants in this change: 1
Killed mutants in this change: 3

class	surviving	killed
🧟`tech.picnic.errorprone.bugpatterns.SpecifyLocale`	1	3

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

rickie

Hey @mlrprananta thanks for opening the PR, it's a nice one 🚀 !

Made some suggestions and some open questions. Some of them I applied already but they are not final, as in still open for discussion 😄. Don't have time to fully finalize the review but it's in a good state.

The biggest point for me is the name of the check 🤔. Let's see :).

Added a commit.

rickie · 2022-12-02T14:33:16Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/SpecifyLocale.java

+    linkType = CUSTOM,
+    severity = WARNING,
+    tags = LIKELY_ERROR)
+public final class SpecifyLocale extends BugChecker implements MethodInvocationTreeMatcher {


I'm not sure yet about the naming of the check. The alternatives I have are way to long 😅.

Likewise not sure about naming, I'm open for suggestions

Also not sure. Best I got for now is ImplicitLocaleUsage; will push, but also open to suggestions.

I agree with @rickie that we should specify String as well to make it more specific, unless we want to extend this rule to other methods/classes as well later?

Would StringCaseImplicitLocaleUsage be too long? 😅

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/SpecifyLocale.java

rickie · 2022-12-02T14:47:23Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/SpecifyLocale.java

+      String localeToSpecify, MethodInvocationTree tree, VisitorState state) {
+    return SuggestedFix.builder()
+        .replace(
+            tree, SourceCode.treeToString(tree, state).replace("()", "(" + localeToSpecify + ")"))


Another way of doing this could be:

int endPos = state.getEndPosition(tree); SuggestedFix.replace(endPos - 2, endPos, "(Locale.ROOT)")

Not sure if that is better though 🤔.

Hmm I don't have a huge preference, but I find my way a bit more clear in what's happening

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/SpecifyLocale.java

error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/SpecifyLocaleTest.java

rickie · 2022-12-02T14:57:30Z

error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/SpecifyLocaleTest.java

+    compilationTestHelper
+        .addSourceLines(
+            "A.java",
+            "class A {",


We are missing some examples that we wouldn't flag as well.

Now that there are quite some characters of the alphabet I'm not sure if it's still the easiest / best way to use them in that way. Although at first it absolutely made sense :).

Did you have an alternative in mind?

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/SpecifyLocale.java

github-actions · 2022-12-02T15:20:55Z

Looks good. All 4 mutations in this change were killed.

class	surviving	killed
🎉`tech.picnic.errorprone.bugpatterns.SpecifyLocale`	0	4

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

Stephan202

Rebased and added a commit.

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/SpecifyLocale.java

Stephan202 · 2022-12-02T17:35:50Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/SpecifyLocale.java

+  private static Fix suggestLocale(String locale, MethodInvocationTree tree, VisitorState state) {
+    return SuggestedFix.builder()
+        .addImport("java.util.Locale")
+        .replace(tree, SourceCode.treeToString(tree, state).replace("()", "(" + locale + ")"))


This assumes that the code is properly formatted. More robust:

Suggested change

.replace(tree, SourceCode.treeToString(tree, state).replace("()", "(" + locale + ")"))

.replace(tree, SourceCode.treeToString(tree, state).replace("(", '(' + locale ))

Due to auto-formatting of our test code we can't just add an extra space, but this case can be modelled using an extra comment. Will push something.

Edit: even this is too fragile. Consider str.toUpperCase( /* () */ ). So it should be:

Suggested change

.replace(tree, SourceCode.treeToString(tree, state).replace("()", "(" + locale + ")"))

.replace(tree, SourceCode.treeToString(tree, state).replaceFirst("\\(", '(' + locale))

Stephan202 · 2022-12-02T17:39:05Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/SpecifyLocale.java

+  public SpecifyLocale() {}
+
+  @Override
+  public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) {


Next to a MethodInvocationTree we should also handle the associated method reference.

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/SpecifyLocale.java

github-actions · 2022-12-02T18:02:48Z

Looks good. All 4 mutations in this change were killed.

class	surviving	killed
🎉`tech.picnic.errorprone.bugpatterns.SpecifyLocale`	0	4

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

Stephan202

Pushed renaming suggestion. Only open point from my side is the method reference handling. Could also be deferred (I already added an XXX comment), but would like to make a habit of covering this case.

Stephan202 · 2022-12-02T18:04:22Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/SpecifyLocale.java

+    linkType = CUSTOM,
+    severity = WARNING,
+    tags = LIKELY_ERROR)
+public final class SpecifyLocale extends BugChecker implements MethodInvocationTreeMatcher {


Also not sure. Best I got for now is ImplicitLocaleUsage; will push, but also open to suggestions.

github-actions · 2022-12-02T18:07:06Z

Looks good. All 4 mutations in this change were killed.

class	surviving	killed
🎉`tech.picnic.errorprone.bugpatterns.ImplicitLocaleUsage`	0	4

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

Stephan202

Better to defer; expanded the comment. Suggested commit message:

Introduce `ImplicitLocaleUsage` check (#376)

github-actions · 2022-12-02T21:02:37Z

Looks good. All 4 mutations in this change were killed.

class	surviving	killed
🎉`tech.picnic.errorprone.bugpatterns.ImplicitLocaleUsage`	0	4

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

Stephan202 · 2022-12-03T08:27:55Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/ImplicitLocaleUsage.java

+  private static Fix suggestLocale(MethodInvocationTree tree, String locale, VisitorState state) {
+    return SuggestedFix.builder()
+        .addImport("java.util.Locale")
+        .replace(tree, SourceCode.treeToString(tree, state).replaceFirst("\\(", '(' + locale))


Even this isn't correct. Added another comment for further follow-up work.

Just to be sure, should the XXX be tackled in this PR?

Just to be sure, should the XXX be tackled in this PR?

Nope :). W.r.t. filing issues as suggested by @rickie: I'd only do that if we have a serious intention of picking up the work.

github-actions · 2022-12-03T08:30:05Z

Looks good. All 4 mutations in this change were killed.

class	surviving	killed
🎉`tech.picnic.errorprone.bugpatterns.ImplicitLocaleUsage`	0	4

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

rickie

Looked at the changes and they LGTM!

We could also decide to make GH issues out of the XXXes, but not sure if necessary for both though.
I think deferring the XXX'es is a smart thing to do as it indeed probably involves a new utility class which are usually not that easy.

One thing a like about ImplicitLocaleUsage is the word implicit. What I do not really like is that String is "missing". As in, it captures the essense, but the focus is clearly on Strings which is not really clear from the name. Therefore I'm proposing two others:

StringLocaleUsage
StringCaseLocaleUsage

Stephan202

W.r.t. the checker name: StringCaseLocaleUsage work for me. Or perhaps ImplicitStringCaseLocale? 🤔

Stephan202 · 2022-12-05T10:26:03Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/ImplicitLocaleUsage.java

+  private static Fix suggestLocale(MethodInvocationTree tree, String locale, VisitorState state) {
+    return SuggestedFix.builder()
+        .addImport("java.util.Locale")
+        .replace(tree, SourceCode.treeToString(tree, state).replaceFirst("\\(", '(' + locale))


Just to be sure, should the XXX be tackled in this PR?

Nope :). W.r.t. filing issues as suggested by @rickie: I'd only do that if we have a serious intention of picking up the work.

rickie · 2022-12-05T10:29:37Z

W.r.t. the checker name: StringCaseLocaleUsage work for me. Or perhaps ImplicitStringCaseLocale? 🤔

To be honest, I prefer the Usage one as it is more in line with some other names we used till now.

github-actions · 2022-12-05T10:54:22Z

Looks good. All 11 mutations in this change were killed.

class	surviving	killed
🎉`tech.picnic.errorprone.bugpatterns.StringCaseLocaleUsage`	0	11

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

github-actions · 2022-12-05T10:55:36Z

Looks good. All 11 mutations in this change were killed.

class	surviving	killed
🎉`tech.picnic.errorprone.bugpatterns.StringCaseLocaleUsage`	0	11

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

rickie

Nice, thanks for being fast with replying and applying @mlrprananta 😉.

You're probably the first one to receive the new Christmas sticker 🎄 🎅🏻 !

rickie · 2022-12-05T11:57:38Z

@oxkitsune anything from your side :)?

mlrprananta force-pushed the mlrprananta/specify-locale branch from d1a6032 to a817072 Compare November 28, 2022 09:12

mlrprananta requested review from rickie and Stephan202 November 28, 2022 09:13

mlrprananta mentioned this pull request Nov 28, 2022

Specify Locale when calling String#to{Lower,Upper}Case #336

Closed

2 tasks

mlrprananta added the new feature label Nov 28, 2022

mlrprananta commented Nov 28, 2022

View reviewed changes

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/SpecifyLocale.java Outdated Show resolved Hide resolved

oxkitsune reviewed Dec 1, 2022

View reviewed changes

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/SpecifyLocale.java Outdated Show resolved Hide resolved

rickie reviewed Dec 2, 2022

View reviewed changes

Stephan202 force-pushed the mlrprananta/specify-locale branch from da233fe to a43b876 Compare December 2, 2022 18:00

Stephan202 reviewed Dec 2, 2022

View reviewed changes

Stephan202 added this to the 0.6.0 milestone Dec 2, 2022

Stephan202 approved these changes Dec 2, 2022

View reviewed changes

Stephan202 reviewed Dec 3, 2022

View reviewed changes

rickie reviewed Dec 4, 2022

View reviewed changes

Stephan202 reviewed Dec 5, 2022

View reviewed changes

mlrprananta and others added 7 commits December 5, 2022 11:53

Introduce SpecifyLocale check

806334c

Fix test

ff6d3e5

Suggestion

4110426

Suggestions

75b3a92

Suggestions

cdcfe08

Propose rename

1a74b56

Expand details around follow-up work

d3d6c93

Stephan202 and others added 2 commits December 5, 2022 11:53

Another gotcha

f9acde6

Update name to StringCaseLocalUsage

e5b7b25

mlrprananta force-pushed the mlrprananta/specify-locale branch from 5790910 to e5b7b25 Compare December 5, 2022 10:53

mlrprananta requested review from oxkitsune and rickie December 5, 2022 11:00

rickie approved these changes Dec 5, 2022

View reviewed changes

oxkitsune approved these changes Dec 5, 2022

View reviewed changes

rickie changed the title ~~Specify Locale when calling String#to{Lower,Upper}Case~~ Introduce StringCaseLocaleUsage check Dec 5, 2022

rickie merged commit 4f9aba8 into master Dec 5, 2022

rickie deleted the mlrprananta/specify-locale branch December 5, 2022 12:49

rickie linked an issue Dec 6, 2022 that may be closed by this pull request

Specify Locale when calling String#to{Lower,Upper}Case #336

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce `StringCaseLocaleUsage` check #376

Introduce `StringCaseLocaleUsage` check #376

mlrprananta commented Nov 28, 2022 •

edited

Loading

github-actions bot commented Dec 1, 2022

rickie left a comment

rickie Dec 2, 2022

mlrprananta Dec 2, 2022

Stephan202 Dec 2, 2022

mlrprananta Dec 5, 2022

rickie Dec 2, 2022

mlrprananta Dec 2, 2022

rickie Dec 2, 2022

rickie Dec 2, 2022

mlrprananta Dec 2, 2022

github-actions bot commented Dec 2, 2022

Stephan202 left a comment

Stephan202 Dec 2, 2022

Stephan202 Dec 2, 2022

github-actions bot commented Dec 2, 2022

Stephan202 left a comment

Stephan202 Dec 2, 2022

github-actions bot commented Dec 2, 2022

Stephan202 left a comment

github-actions bot commented Dec 2, 2022

Stephan202 Dec 3, 2022

mlrprananta Dec 5, 2022 •

edited

Loading

Stephan202 Dec 5, 2022

github-actions bot commented Dec 3, 2022

rickie left a comment

Stephan202 left a comment

Stephan202 Dec 5, 2022

rickie commented Dec 5, 2022 •

edited

Loading

github-actions bot commented Dec 5, 2022

github-actions bot commented Dec 5, 2022

rickie left a comment

rickie commented Dec 5, 2022

	.replace(tree, SourceCode.treeToString(tree, state).replace("()", "(" + locale + ")"))
	.replace(tree, SourceCode.treeToString(tree, state).replace("(", '(' + locale ))

Introduce StringCaseLocaleUsage check #376

Introduce StringCaseLocaleUsage check #376

Conversation

mlrprananta commented Nov 28, 2022 • edited Loading

Suggested commit message

github-actions bot commented Dec 1, 2022

rickie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Dec 2, 2022

Stephan202 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Dec 2, 2022

Stephan202 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Dec 2, 2022

Stephan202 left a comment

Choose a reason for hiding this comment

github-actions bot commented Dec 2, 2022

Choose a reason for hiding this comment

mlrprananta Dec 5, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Dec 3, 2022

rickie left a comment

Choose a reason for hiding this comment

Stephan202 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rickie commented Dec 5, 2022 • edited Loading

github-actions bot commented Dec 5, 2022

github-actions bot commented Dec 5, 2022

rickie left a comment

Choose a reason for hiding this comment

rickie commented Dec 5, 2022

Introduce `StringCaseLocaleUsage` check #376

Introduce `StringCaseLocaleUsage` check #376

mlrprananta commented Nov 28, 2022 •

edited

Loading

mlrprananta Dec 5, 2022 •

edited

Loading

rickie commented Dec 5, 2022 •

edited

Loading