Warn about ArrayPool.Return without clearArray specified when T is not a value type or has references

[YohDeadfall]

Describe the problem you are trying to solve

A new rule checking that clearArray is specified on ArrayPool.Return would solve the problem with unintentional return of an array which contains references and prevent these object being garbage collected. Currently there's a problem with that method the mentioned parameter is optional and can be easily be forgotten especially by new developers, and that clearArray is false by default just for performance optimization purposes not taking into account that there are reference types. The best solution would be to have an enum for that parameter telling the pool that it should decide the right logic depending on T, but the current method is written in stone and cannot be changed. Therefore, the best possible solution is to add a rule.

Describe suggestions on how to achieve the rule

The rule should warn the developer when clearArray is omitted for types which are not value types or has references inside. Explicit setting the parameter to any value resolves the issue.

[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.

[ghost]

Tagging subscribers to this area: @dotnet/area-system-runtime
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Describe the problem you are trying to solve

A new rule checking that clearArray is specified on ArrayPool.Return would solve the problem with unintentional return of an array which contains references and prevent these object being garbage collected. Currently there's a problem with that method the mentioned parameter is optional and can be easily be forgotten especially by new developers, and that clearArray is false by default just for performance optimization purposes not taking into account that there are reference types. The best solution would be to have an enum for that parameter telling the pool that it should decide the right logic depending on T, but the current method is written in stone and cannot be changed. Therefore, the best possible solution is to add a rule.

Describe suggestions on how to achieve the rule

The rule should warn the developer when clearArray is omitted for types which are not value types or has references inside. Explicit setting the parameter to any value resolves the issue.

Author:	YohDeadfall
Assignees:	-
Labels:	area-System.Runtime, untriaged, code-analyzer
Milestone:	-

[stephentoub]

One of the things that makes this rule less of a slam dunk is that code will often handle clearing on its own prior to returning an array to the pool. That's in part because the pool will necessarily clear the whole array, whereas if the code knows how much of the array it used, it can clear only the portions it wrote to and spend less time doing so. To avoid false positives, the analyzer would need to somehow validate that the array being returned wasn't cleared in this manner, and that would be much more challenging to get right.

[colejohnson66]

@stephentoub If the user is indeed clearing the arrays themselves, couldn't they just disable the analyzer for that ArrayPool.Return call?

[stephentoub]

If the user is indeed clearing the arrays themselves, couldn't they just disable the analyzer for that ArrayPool.Return call?

Yes, it could be suppressed, e.g.

#pragma warning disable CAWHATEVERIDHERE
...
#pragma warning restore CAWHATEVERIDHERE

at which point the question becomes how much value does the analyzer provide for finding real issues vs how much noise and maintenance burden does it cost. At some point if you have to suppress it too much, the rule switches from being useful to a drag on productivity.

[danmoseley]

Related #7532

(Although until we have a way to have a toggle able debug mode with zero cost when it's off, any debug checks of course would only help find bugs in our own libraries)