Enabling CA1416: Validate platform compatibility #41760
Conversation
Dotnet-GitSync-Bot
added
the
area-CodeGen-coreclr
src/libraries/System.Net.Sockets/src/System/Net/Sockets/BaseOverlappedAsyncResult.Windows.cs
Show resolved
Hide resolved
| } | ||
| } | ||
|
|
||
| #pragma warning disable CA1416 // Debug.Assert(OperatingSystem.IsWindows()) is not available |
There was a problem hiding this comment.
@buyaa-n is there a bug we can reference in this comment?
There was a problem hiding this comment.
This is because the assembly targets netstandard, so the relevant APIs aren't available
There was a problem hiding this comment.
Ah, thanks! I think Debug.Assert(RuntimeInformation.IsOSPlatform(OSPlatform.Windows)) will work in this case then.
src/libraries/Microsoft.Extensions.Logging.EventLog/src/WindowsEventLog.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Sockets/src/System/Net/Sockets/BaseOverlappedAsyncResult.Windows.cs
Show resolved
Hide resolved
|
Seeing that this did not result in any new I wanted to make sure we worked through this before RC2 closed down in case we saw any need to augment our annotations or anything in public APIs. |
|
With no changes to the refs and no functional fixes in src, this doesn't need to be in 5.0. The only reason to consider porting it is if it were extensive enough that we'd want to minimize diffs in future ports back to 5.0, but that doesn't appear to be the case. |
| private static readonly unsafe IOCompletionCallback s_completionPortCallback = delegate (uint errorCode, uint numBytes, NativeOverlapped* nativeOverlapped) | ||
| { | ||
| var saeaBox = (StrongBox<SocketAsyncEventArgs>)ThreadPoolBoundHandle.GetNativeOverlappedState(nativeOverlapped)!; | ||
| #pragma warning disable CA1416 // https://github.com/dotnet/roslyn-analyzers/issues/4090 |
There was a problem hiding this comment.
dotnet/roslyn-analyzers#4090 was resolved. Is the #pragma warning disable still required? If we don't yet have a build of the analyzer with this, we might want to just wait on this PR until we do.
There was a problem hiding this comment.
I believe it is built to https://dev.azure.com/dnceng/public/_packaging?_a=package&feed=dotnet5&view=versions&package=Microsoft.CodeAnalysis.NetAnalyzers&version=5.0.0-rc2.20453.2&protocolType=NuGet, we need to update the dogfood version https://github.com/dotnet/runtime/blob/master/eng/Analyzers.props#L9
jeffhandley
left a comment
jeffhandley
left a comment
There was a problem hiding this comment.
Looks good to me. And we have #41901 filed to try to find a way to annotate platform-specific assemblies with assembly-level attributes.
|
@steveisok @MaximLipnin @akoeplinger FYI--this will enable the Platform Compatibility Analyzer enabled in the |
|
I believe this is ready to merge if there is no additional feedback. |
ghost
locked as resolved and limited conversation to collaborators
6 participants
This enables CA1416: Validate platform compatibility. Most scenarios are handled by adding
Debug.Assert(OperatingSystem.IsWindows());. This was done because most code paths also have a Unix version in a separate*.Unix.csfile and the files are already limited to running on Windows only via the build system.There is one place that is bypassed via
#pragma warning disabledue to: dotnet/roslyn-analyzers#4090There are likewise a couple places that were bypassed as they only build netstandard versions of the respective binaries and so
OperatingSystem.IsWindowswas not available.