Warn about use of String charset methods

[gaul]

Using typed Charset methods avoids unneeded
UnsupportedEncodingException handling.

[gaul]

CI fails due to #942.

[madrob]

For a while using the typed charset was about 15% slower than using the string charset, has this gone away? I know people were explicitly using string methods because of this before.

[gaul]

@madrob Can you provide a reference? String.getBytes(String) basically calls String.getBytes(Charset) after consulting a custom two-element cache. Some more background:

https://blog.codecentric.de/en/2014/04/faster-cleaner-code-since-java-7/

[madrob]

Sure, here's experimental results from several communities:

https://issues.apache.org/jira/browse/LOG4J2-935 <-- description claims Java 6, 7, and 8 affected, benchmark shows only 6?
https://bugs.openjdk.java.net/browse/JDK-6764325 <-- Looks like JDK6 only maybe, but issue is still open.

Unsure what to make of these results, I figured to run my own jmh tests.

https://gist.github.com/madrob/f0688f7148aa66a5406ce1b0fc1964c9

The charset usage is slower but within the error bounds for single encoding and much worse for various encodings (an idea borrowed from the blog post you link).

Edit: And much faster in Java 9!

[cushon]

(Sorry for the extremely slow response to this PR.)

Warning on these APIs SGTM overall.

I think it would be nice to have a suggested fix. If the charset is a string literal, we could recommend the corresponding StandardCharsets constant. And if it isn't a literal, I wonder if it's still worth emitting the diagnostic, if the fix is to add an explicit call to Charset.forName() is that still worthwhile?

Also, this might be worth splitting out into a separate check from JdkObsolete. These APIs are arguably less obsolete than the other APIs here, and JdkObsolete has been around for a while and having the new diagnostics in a separate check makes it easier to roll out.

[cushon]

(Sorry for the extremely slow response to this PR.)

Warning on these APIs SGTM overall.

I think it would be nice to have a suggested fix. If the charset is a string literal, we could recommend the corresponding StandardCharsets constant. And if it isn't a literal, I wonder if it's still worth emitting the diagnostic, if the fix is to add an explicit call to Charset.forName() is that still worthwhile?

Also, this might be worth splitting out into a separate check from JdkObsolete. These APIs are arguably less obsolete than the other APIs here, and JdkObsolete has been around for a while and having the new diagnostics in a separate check makes it easier to roll out.

I added a separate check with a suggested fix in 1147184

[Marcono1234]

@cushon, this new pattern only covers String(byte[], String) and String.getBytes(String), right?

Would it make sense to cover some other common methods and constructors which take a string charset name as well, at least Charset.forName? (incidentally one of the test templates has an example for this)

Here is SonarSource's corresponding implementation, though I guess covering all of these cases is out of scope for Error Prone; especially since some of those methods and constructors might be used quite rarely.

[cushon]

@Marcono1234 yep, it's just those two APIs for now. I think covering more APIs definitely makes sense, I'd be happy to review PRs for that.