StringSplitter provides no actionable fix

[oprypin]

What version of Error Prone are you using?

https://chromium.googlesource.com/chromium/third_party/errorprone/+/ecc57c2b00627667874744b9ad8efe10734d97a8

Does this issue reproduce with the latest release?

Don't know

What did you do?

https://cs.chromium.org/chromium/src/third_party/webrtc/sdk/android/api/org/webrtc/FileVideoCapturer.java?rcl=3e189a6dc342dcf115c910783444ba7ef9a354af&l=59

What did you expect to see?

Information on how to fix the "StringSplitter" warning using Java standard library.

What did you see instead?

../../sdk/android/api/org/webrtc/FileVideoCapturer.java:59: warning: [StringSplitter] Prefer Splitter to String.split
      String[] headerTokens = header.split("[ ]");
                                          ^
    (see http://errorprone.info/bugpattern/StringSplitter)
  Did you mean 'Iterable<String> headerTokens = Splitter.onPattern("[ ]").split(header);'?

This replacement uses code that is not part of standard library and I have no interest in adding a dependency.

If there is no direct replacement, maybe this isn't a good warning.

[epmjohnston]

If you don't want to use Guava Splitter, you can disable the StringSplitter check with -Xep:StringSplitter:OFF and ensure StringSplit, which does provide a direct replacement within the Java standard library, is turned on.

[arakelian]

I'm grateful for the opportunity to use errorprone, and appreciate your suggestipn to use Xep:StringSpitter:OFF, but I have to say that this warning feels entirely self-serving.

Reading the explanation (http://errorprone.info/bugpattern/StringSplitter), the bit about "more predictable behaviour" is just wrong. The behavior of String.split is well documented in the Javadoc: "If the expression does not match any part of the input then the resulting array has just one element, namely this string". The errorprone documentation goes on to say, "[Guava's Splitter] provides explicit control over the handling of empty strings and the trimming of whitespace", which reads like an advertisement.

Finally, the recommended solution, e.g. use Guava, is far more problematic. I have used Guava for many years both commercially and for government work, and Guava very well known for a rapid update cycle of binary incompatible updates. (Google: "Guava Stopwatch Hbase" just for starter).

Thank you for an excellent tool in errorprone. Keep up the good work.

[jbduncan]

I personally agree with @arakelian's observations, specifically about how (1) checker http://errorprone.info/bugpattern/StringSplitter feels self-serving (even though I'm sure that wasn't the author's intention!), and (2) how it recommends a solution in the form of Splitter from Guava, which as much as I personally love it, is unfortunately not a universal solution outside of Google due to Guava's tendency to break binary backwards-compatibility often (which I understand happens because Google-internal projects "live at HEAD").

I wonder if the best course of action would be to merge this checker with http://errorprone.info/bugpattern/StringSplit, doing so in such a way that the resulting checker would recommend aString.split(regex, -1) by default, but Guava's Splitter would be mentioned as an alternative solution in the docs which the checker could accept as a positive fix.

WDYT @cushon @epmjohnston?

[cpovirk]

To understand the behavior of String.split, you have to read at least 2 methods' Javadoc: split(String), to understand that the behavior matches split(s, 0), and split(String, int), to understand the full behavior of split(s, 0). That behavior is to omit trailing empty strings but (as you note) not if that would result in an empty array. I think it's fair to call that behavior unpredictable (and also not as well documented as it could be, though that part could be fixed).

That said, I don't suppose that Error Prone can suggest the Guava Splitter only if it's on the classpath, so that people don't need to set Xep:StringSpitter:OFF manually?

[arakelian]

Respectfully, I'll suggest one last time that String.split's behavior is entirely predictable, even if you have to read 2 Javadocs to understand it.

To this reader, unpredictable (http://www.thesaurus.com/browse/unpredictable), is synonymous with uncertain, unreliable, fluctuating, unforeseeable and chancy. That doesn't feel appropriate to me when the method's inputs and outputs are documented.

With respect to the idea that Error Prone would only suggest Guava Splitter if it is on the class path, I reviewed the API changes (https://github.com/google/guava/wiki/ReleaseHistory) for every revision of Guava Splitter since the initial release, and it appears to be very stable, only adding a method here or there (in version 8, and version 14).

[JakeWharton]

It's predictable but error prone.

[cpovirk]

Ah, I see -- not "unpredictable" in the sense of "undefined behavior" or "depends on thread scheduling." Just "different than some users would predict." Perhaps that word was chosen to sound less judgmental than "unnatural." But "error-prone" or "surprising" sounds about right :)

[cpovirk]

(Oh, and Splitter isn't annotated as @Beta. Guava recently stopped removing non-@Beta APIs, so Splitter, at least, should be fairly safe. Of course I understand that many people still have reasons to avoid Guava entirely.)

[jbduncan]

@cpovirk @cushon Am I right to think that this issue is resolved now? Or does more need to be done before it can be closed? :)

[oprypin]

It's been closed since the first day.

[jbduncan]

D'oh! Thanks @oprypin. :P

[eaftan]

To be clear, we took the feedback from this issue and removed the Guava-only suggested fix for the StringSplitter fix. The check now only suggests Guava's Splittter if Guava is already on the classpath. Otherwise we suggest rewriting to String#split(str, -1), which has more obvious behavior.