Add DefaultLocale check to discourage use of the default locale #4487
Conversation
|
|
||
| /** A {@link BugChecker}; see the associated {@link BugPattern} annotation for details. */ | ||
| @BugPattern( | ||
| summary = |
There was a problem hiding this comment.
Would you be willing to also add a brief .md to this directory? https://github.com/google/error-prone/tree/master/docs/bugpattern
I think it'd be helpful to have a few sentences to characterize why this is bad, and some guidance on how to choose between the different possible fixes.
There was a problem hiding this comment.
@tbroyer thoughts on this? This can also happen as a follow up if you don't have time right now.
There was a problem hiding this comment.
Might I let you (or someone else) do it? 😇
I started drafting something but couldn't clearly articulate my thoughts 😞
core/src/main/java/com/google/errorprone/bugpatterns/DefaultLocale.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/google/errorprone/bugpatterns/DefaultLocale.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/google/errorprone/bugpatterns/DefaultLocale.java
Outdated
Show resolved
Hide resolved
|
|
||
| private boolean containsSomeFormattableArgument( | ||
| List<? extends ExpressionTree> arguments, VisitorState state) { | ||
| return arguments.stream().anyMatch(tree -> mightBeFormattable(tree, state)); |
There was a problem hiding this comment.
I noticed this is reporting errors for any String.format arguments that implement an interface (the example I noticed was List<String>), since any interface is castable to Formattable. Was that deliberate?
I think at Google we probably won't apply that approach as-is, because it would result in a large number of findings and false positives. I think there are a few ways to reconcile that. We could make the check less strict, or add configuration to control how strict it is.
There was a problem hiding this comment.
Ah yes, that's going to match quite wide, but it's the "most correct" kind of match, right?
I mean, if you have List<String> it might actually be an instance of class L<T> implements List<T>, Formattable, that could then be locale-dependent.
I suppose there are ways to detect a bunch of cases where this cannot happen (e.g. a local variable List<String> list = new ArrayList<>() is guaranteed to not implement Formattable) but this is beyond my skills I'm afraid.
There was a problem hiding this comment.
Yes, agree it's the most correct option. I see a tradeoff between correctness and false positives here. I think it's reasonable to want the check to be as correct as possible, and e.g. report a match for String.format("%s", someInterface). Partly I wanted to double-check: for your usage, do you want the most correct/sound version of the check, even if that results in false positives?
For Google's codebase the current version reports a lot of findings, and the ratio of String.format to everything else is about 9:1. I'm not sure we'll want to use the most correct version of the check. But that's also something I can look at as a follow-up. If you want the stricter version I'm happy to get this merged and then consider maybe adding flags to make it more lenient or something as a follow-up.
What do you think?
There was a problem hiding this comment.
I have no idea how much Formattable is being used in the wild, and whether I might be affected by this (whether things I format implement Formattable or not, I definitely never implemented Formattable myself ever). Off the cuff I'd say the risk is rather low so I think I'd be OK with a more lenient check; but on the other hand it's not that hard to provide an explicit locale (even if Locale.getDefault()) "just in case".
So maybe make it strict by default (even if "too strict") and have a flag to make it more lenient?
(or if you prefer, start by making it lenient by default, with a flag to make it stricter, then later –possibly only after changes to have fewer false positives– switch the default behavior, with a flag to get back to the more lenient check, and eventually remove the flag)
Fwiw, I'm currently using Forbidden APIs which is even stricter as it flags all String.format(String,Object...) invocations irrespective of the pattern being used, so I'm already providing an explicit locale "just in case" everywhere, and even the strict behavior here would be better than Forbidden APIs.
There was a problem hiding this comment.
I took a quick look at Formattable usage and it seems to be very rare. For Google's internal usage we might be OK not worrying about that case to reduce the number of noise from adding default locales just in case.
Anyway, I'm happy to merge the current version of this. Partly I wanted to confirm how strict a check you wanted for your usage. I'll think more about what I want to enable at Google, and if that ends up being slightly different we can express the difference with flags.
core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java
Outdated
Show resolved
Hide resolved
…ble and only fail when they're known to be Formattables
| return prependLocales(tree, tree.getMethodSelect(), tree.getArguments(), state, localeFixes); | ||
| } | ||
|
|
||
| private Description prependLocales( |
There was a problem hiding this comment.
FYI I think this method is unused
Fixes #632