[Analyzer Proposal]: Analyzer/fixer for converting Stream.Read calls to ReadAtLeast and ReadExactly

[eerhardt]

With the addition of the new Stream ReadAtLeast and ReadExactly methods, there is a pattern of mistakes we can catch with an analyzer and suggest the caller use the new APIs. For example:

stream.Read(buffer, 0, buffer.Length);

where the return value isn't checked, and recommend it be changed to a call to ReadExactly.

Or cases where the return value is checked but required to be the same as what was requested:

if (stream.Read(buffer, 0, buffer.Length) != buffer.Length) throw ...;

and similarly suggest it use ReadExactly.

If the caller is using a length that is different than the length of the buffer being passed in, and not checking the return value, we can suggest to use ReadAtLeast:

stream.Read(buffer, 0, count);

would become:

stream.ReadAtLeast(buffer, count);

However, note in the ReadAtLeast case, the caller should still be checking the return value because the ReadAtLeast call could be returning more than count bytes, and this data would be missed if the caller only consumed count bytes from buffer.

One problem with this analyzer might be when we implement #34098. If the caller saw the "Do not discard results of methods marked as DoNotIgnoreReturnValue" warning first and addressed it, they would lose out on this fixer.

cc @stephentoub

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

Issue Details

---

With the addition of the new Stream ReadAtLeast and ReadExactly methods, there is a pattern of mistakes we can catch with an analyzer and suggest the caller use the new APIs. For example:

stream.Read(buffer, 0, buffer.Length);

where the return value isn't checked, and recommend it be changed to a call to ReadExactly.

Or cases where the return value is checked but required to be the same as what was requested:

if (stream.Read(buffer, 0, buffer.Length) != buffer.Length) throw ...;

and similarly suggest it use ReadExactly.

If the caller is using a length that is different than the length of the buffer being passed in, and not checking the return value, we can suggest to use ReadAtLeast:

stream.Read(buffer, 0, count);

would become:

stream.ReadAtLeast(buffer, count);

However, note in the ReadAtLeast case, the caller should still be checking the return value because the ReadAtLeast call could be returning more than count bytes, and this data would be missed if the caller only consumed count bytes from buffer.

One problem with this analyzer might be when we implement #34098. If the caller saw the "Do not discard results of methods marked as DoNotIgnoreReturnValue" warning first and addressed it, they would lose out on this fixer.

cc @stephentoub

Author:	eerhardt
Assignees:	-
Labels:	api-suggestion, area-System.IO, code-analyzer, code-fixer
Milestone:	-

[weltkante]

One problem with this analyzer might be when we implement #34098. If the caller saw the "Do not discard results of methods marked as DoNotIgnoreReturnValue" warning first and addressed it, they would lose out on this fixer.

I don't think these conflict, as the methods aren't pure (changing state of the stream) and won't be marked as such?

[edit] I was pointed at the fact that the referenced issue is no longer about purity alone

[danmoseley]

@jeffhandley @stephentoub with 3.1 EOL in 3 months, and based on some of the issues posted here recently, we can expect a number of folks migrating before support expires. Some of them will hit this and get a hang in their app. It might take a while to connect to the breaking change notice because there isn't an obvious "smell" (the doc doesn't contain the word "hang" for example). Once they've made that connection, they may have many places in their codebase that need auditing for the bug and fixing.

An analyzer/fixer could help with both those hurdles. I wonder whether we should start now to front load getting one into the SDK ..

[stephentoub]

The intent had been that this be addressed in .NET 7. It unfortunately got pushed out due to lack of time. We should tackle it soon.

[eerhardt]

Note that another analyzer that would help in this case is #34098. Code that doesn't capture the result of Stream.Read or BinaryReader.Read would get flagged by that analyzer as well (along with other APIs that shouldn't have their results ignored).

However, the downfall of #34098 is that the necessary attribute didn't get added to net7.0, so we can't mark the APIs as "do not discard" yet.