Add analyzer: Prevent wrong usage of string Split

[Mrxx99]

Describe the problem you are trying to solve

Consider the following usage of Split on a string where you want to split on space and tab:
var split = s.Split(' ', '\t'); (which works fine)
If you now get to the conclusion to remove empty entries, it is tempting to do this:
var split = s.Split(' ', '\t', StringSplitOptions.RemoveEmptyEntries);
But this won't give the expected result and will result in a bug in the application, because the second char is converted to an int and used as the count argument for this overload:
public string[] Split(char separator, int count, StringSplitOptions options = StringSplitOptions.None);

Describe suggestions on how to achieve the rule

To prevent this, an analyzer should detect that a char is passed to the count parameter of this specific overload and should warn that it won't split on that char.
A codefix can provide a conversion to the following:
var split = s.Split(new[] { ' ', '\t' }, StringSplitOptions.RemoveEmptyEntries);

(btw. it would be good to also have a template for analyzers in this repository, not only in the roslyn-analyzers repo)

[dotnet-issue-labeler]

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

[GrabYourPitchforks]

A more generalized form of this analyzer would be "You used a char, but it got widened to an int when you passed it to this method."

Not sure how many false positives such an analyzer would produce. It's common to perform integer arithmetic on chars; e.g., (char)('a' + offset) to generate an output in the range a..z when offset is known to be 0..25. But I wouldn't imagine it's all that common to pass literal chars (or standalone char locals) to APIs that expect a wider data type.

See also #46103.

[joperezr]

cc: @carlossanlop FYI

[carlossanlop]

Since it's possible to pass a char where a method takes an int, and it gets casted silently, it makes sense that we warn the user to make sure they intended to pass a char array instead.

Analyzer: small
Fixer: small
Suggested category: Reliability
Suggested severity: Warning - The user should manually suppress it in the rare case where they actually intended to get a char casted as int, as described by @GrabYourPitchforks above.
Suggested milestone: Future

Example:

string txt = "bla bla";
string[] splitted1 = txt.Split(' ', '\t'); // OK because argument is "params char[] separators"

// BEFORE
// Second argument is passed as int because "params" can only be used for the last argument
string[] splitted2 = txt.Split(' ', '\t', StringSplitOptions.RemoveEmptyEntries);

// AFTER
// Suggested fix: Wrap chars in array of chars
string[] splitted2 = txt.Split(new char[] { ' ', '\t' }, StringSplitOptions.RemoveEmptyEntries);

// DO NOT FLAG
// Make sure to detect the data type of the second argument
char c = ' ';
int count = 5;
string[] splitted3 = txt.Split(c, count, StringSplitOptions.RemoveEmptyEntries);

[bartonjs]

Video

Using a messaging along the lines of "suspicious cast", having the rule identify calls to string.Split or StringBuilder..ctor that involves an implicit conversion from char to int seems like goodness (and we may add more specific API over time). All of the methods/constructors that we group together in this rule will use the same diagnostic ID.

There are some generalizations (any implicit char->int in an argument; or that but only when there's an overload that takes char/string) which would be good to follow up on, but they're out of scope for this issue.

• Category: Correctness
• Severity: Warning

[NewellClark]

I would like to be assigned to this, please.