BugPattern for using the default system Locale

[leventov]

Similar to http://errorprone.info/bugpattern/DefaultCharset, it seems reasonable to have a warning of using the default system Locale, e. g. in String.format().

[Stephan202]

A list of candidate methods can be extracted from the forbidden-apis project. I'd like to see "smart" support for String.format, though, for reasons outlined in policeman-tools/forbidden-apis#119. (The good thing is that Error Prone is sufficiently expressive to pull this off.)

[tbroyer]

Fwiw, looking at which checks it provides, this might be the only reason for using forbidden-apis (unless you also want the commons.io checks), all others are either @Deprecated that can be handled by -Xlint and -Werror, or are covered by DefaultCharset and JavaTimeDefaultTimeZone (only covers java.time APIs but should be enough nowadays)

Of course, forbidden-apis works at the bytecode level so can also check bytecode generated by Groovy, Kotlin, or Scala, so can still be useful.

[cushon]

We have https://errorprone.info/bugpattern/StringCaseLocaleUsage specifically for this issue in toLowerCase/toUpperCase, I'd be interesting in a contribution for a more general check.

A list of candidate methods can be extracted from the forbidden-apis project.

It looks like their list is here: https://github.com/policeman-tools/forbidden-apis/blob/ce478c1ad357535518c39c5311d46ec7c0311777/src/main/resources/de/thetaphi/forbiddenapis/signatures/jdk-unsafe-1.7.txt#L52-L108

I'd like to see "smart" support for String.format, though, for reasons outlined in policeman-tools/forbidden-apis#119. (The good thing is that Error Prone is sufficiently expressive to pull this off.)

That makes sense to me, I think for something that was enabled by default in Error Prone we'd probably want it to have a lower false positive rate, and perhaps only report diagnostics if the format string is a compile-time constant that contains format specifiers that are known to be bad.

[tbroyer]

Oh, hadn't even seen that StringCaseLocaleUsage check! (or had forgotten about it)

Anyway, I'm working on that DefaultLocale check, trying to categorize the various constructors and methods into buckets (between those that could use Locale.getDefault(FORMAT) vs Locale.getDefault(DISPLAY) vs just Locale.getDefault() when it's not perfectly clear which category to use; those that have more overloads to handle, like java.text.DateFormat.getXxxInstance; and those with no fix to suggest such as java.text.MessageFormat.format(String, Object...))
Not sure I'll be able to do that smart format/printf check though, so maybe do it in 2 steps: first add the check without it and not enabled by default, then make it smarter and enable the check by default.

Also fwiw, there are more than those listed in the jdk-unsafe-1.7 bundled signatures file: https://github.com/search?q=repo%3Apoliceman-tools%2Fforbidden-apis%20%22uses%20default%20locale%22&type=code

[tbroyer]

That finally wasn't that hard 😅

PR #4487 is obviously opinionated: I finally didn't include many "display" methods (only Currency.getSymbol), I didn't include Console and explicitly excluded System.out/System.err as they're directed towards "users" and should probably use the default locale in most cases, the "smart" String.format (and similar format methods) should (hopefully) not have too much false positives (and no false negative 🤞), and I didn't suggest fixes for everything (some Formatter constructors that also use the default charset and would imply permutation "explosion" of fixes), and similarly didn't include the "Uses default locale or time zone" methods