Avoid StringBuilder parameters for P/Invokes

[elinor-fung]

Marshalling for StringBuilder always creates a native buffer copy and can thus be very inefficient. Consider using a char buffer from ArrayPool instead.

// Flag StringBuilder parameter
[DllImport("MyLibrary")]
private static extern void Foo(StringBuilder buffer);

Category: Performance
Default: Disabled

Since each instance of this violation would require user intervention to determine if it makes sense to address and how to address it, this would be off-by-default and up to the user to enable for code where perf is a concern.

cc @terrajobst @stephentoub @AaronRobinsonMSFT @jkoritzinsky

[stephentoub]

I'm no fan of using StringBuilder in P/Invokes, and previously did a sweep through dotnet/runtime to remove almost all of it, but we do still have a few cases left, in particular where StringBuilders are passed [In] and with CharSet.Unicode; in such situations, if you already have a StringBuilder, it's a reasonable thing to use in a P/Invoke. As such I'm a little concerned this could be fairly noisy, though I agree it's a valuable thing to warn about in general in perf-sensitive code bases (maybe an IdeSuggestion, maybe off by default and folks can turn it in if desired). For code that doesn't care as much about absolute throughput, this could just be annoying.

[GrabYourPitchforks]

I get the spirit of the rule, but I'm concerned that adherence to this rule significantly increases the concept count over what the original code looked like. If this rule triggers over your code and you try to address it, then in addition to understanding how StringBuilder behaves across a marshaling boundary, you also need to understand:

1. How the array pool works, including best practices for using it
2. How to move data between StringBuilder and a rented array
3. Null terminator behavior when marshaling strings

And you'll need to account for these effects in your code manually.

[elinor-fung]

Yeah, I think it would fall under the IdeSuggestion level.

There would definitely be some additional concepts tied to addressing a violation. I think it still makes sense to provide a suggestion about things like that, since it gives an immediate practical application for those concepts. Do you think documentation for a new rule pointing out and explaining (or linking to other resources) all those would not be enough help?

[stephentoub]

I think part of the problem here is we can't reasonably provide an auto-fix. So we'd be saying "Consider alternatives to StringBuilder", but without a) knowing whether that actually makes sense for how they're using it, and b) with potentially non-trivial re-work required for them to address it.

[GrabYourPitchforks]

Do you think documentation for a new rule pointing out and explaining (or linking to other resources) all those would not be enough help?

There is something already along these lines at https://docs.microsoft.com/en-us/dotnet/standard/native-interop/best-practices#string-parameters. It mentions the downsides of using StringBuilder but doesn't demonstrate how to work around them in your own code. If there were samples there showing best practice, we could link to it.

[elinor-fung]

So we'd be saying "Consider alternatives to StringBuilder", but without a) knowing whether that actually makes sense for how they're using it, and b) with potentially non-trivial re-work required for them to address it.

Perhaps mitigated if it were off by default, so people would have had to choose to be warned about this specific rule (or performance rules)?

[stephentoub]

Yup.

[terrajobst]

Video

Shipping this as a warning and off by default makes sense. Setting the severity as suggestion would likely mean that nobody will ever see this.