Remove unnecessary CA2022 suppressions #47904
Conversation
ghost
added
There was a problem hiding this comment.
Pull Request Overview
This pull request removes the unnecessary CA2022 warning suppressions from test files to clean up the code. The changes update the test files by removing obsolete #pragma warning disable/restore statements.
- Removed CA2022 suppression in GivenDotnetSlnAdd.cs
- Removed CA2022 suppression in GivenDotnetSlnRemove.cs
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| test/dotnet-sln.Tests/GivenDotnetSlnAdd.cs | Removed obsolete CA2022 suppression lines |
| test/dotnet-sln.Tests/GivenDotnetSlnRemove.cs | Removed obsolete CA2022 suppression lines |
|
Those are instances of FileStream, rather than MemoryStream or UnmanagedMemoryStream, so the CA2022 warnings correspond to actual bugs in the tests. Do you intend to fix those, e.g. by making the tests call Stream.ReadExactly? |
|
Yes, I'm trying to fix the CA 2022 warnings by calling Stream.ReadExactly. |
|
Add @dotnet/source-build and @dotnet/product-construction as reviewers. |
src/RazorSdk/Tool/Client.cs
Outdated
| #if NET | ||
| await Stream.ReadExactlyAsync(Array.Empty<byte>(), 0, 0, cancellationToken); | ||
| #else | ||
| await Stream.ReadAsync(Array.Empty<byte>(), 0, 0, cancellationToken); | ||
| #pragma warning restore CA2022 | ||
| #endif |
There was a problem hiding this comment.
Instead of using #if NET to choose between ReadExactlyAsync and ReadAsync, I think it would be better to conditionally define ReadExactlyAsync as an extension method.
src/RazorSdk/Tool/Client.cs
Outdated
| #pragma warning disable CA2022 // Avoid inexact read | ||
| await Stream.ReadAsync(Array.Empty<byte>(), 0, 0, cancellationToken); | ||
| #pragma warning restore CA2022 | ||
| await ReadExactlyAsync(Stream, Array.Empty<byte>(), 0, 0, cancellationToken); |
There was a problem hiding this comment.
On second thought, this is a special case because the number of bytes requested is 0; the code attempts to poke the pipe to update its state, without actually reading any bytes. Because the number of bytes actually read cannot be negative, it is not possible that the call would read fewer bytes than requested. The analyzer should recognise this zero-length read as a special case but I don't know whether it already does. So this should keep using Stream.ReadAsync and suppress the warning if necessary.
There was a problem hiding this comment.
ReadExactlyAsync seems a bad choice here because it is not clear whether that passes the zero-length read down to ReadAsync (as is needed here), or just returns 0 immediately (which would not update the status of the pipe).
There was a problem hiding this comment.
https://github.com/dotnet/runtime/blob/7cc32192cbb4894bfebbb09a903215993207a3c0/src/libraries/System.Private.CoreLib/src/System/IO/Stream.cs#L431
Zero-length is a valid value, because ReadExactlyAsync will check if the count parameter is exactly zero, which is a valid case that should be handled specially (by immediately returning without performing any I/O operations).
I mad a mistake by adding an equal sign here, now I will remove it to maintain consistency with ReadExactlyAsync behavior.
There was a problem hiding this comment.
Yes, a zero-length read is valid. But if ReadExactlyAsync implements it by returning immediately without calling an operating-system API, then it doesn't update the state of the pipe, and .NET SDK won't detect whether the other end has disconnected, according to this comment:
sdk/src/RazorSdk/Tool/Client.cs
Lines 165 to 166 in 4f874f0
There was a problem hiding this comment.
Okay, got it. I will revert the change.
|
Hi @KalleOlaviNiemitalo , I have reverted the changes for |
Related to dotnet/source-build#4322
Remove all the unnecessary CA2022 suppressions
VMR: https://dev.azure.com/dnceng/internal/_build/results?buildId=2673846&view=results (internal Microsoft link)