Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Enabling CA1416: Validate platform compatibility #41760

Merged
merged 6 commits into from
Sep 10, 2020
Merged

Enabling CA1416: Validate platform compatibility #41760

merged 6 commits into from
Sep 10, 2020

Conversation

tannergooding
Copy link
Member

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.cs file 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 disable due to: dotnet/roslyn-analyzers#4090
There are likewise a couple places that were bypassed as they only build netstandard versions of the respective binaries and so OperatingSystem.IsWindows was not available.

@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Sep 2, 2020
jeffhandley
jeffhandley reviewed Sep 2, 2020
View reviewed changes
src/libraries/Microsoft.Extensions.Logging.EventLog/src/EventLogLogger.cs Outdated
@@ -164,6 +164,8 @@ private void WriteMessage(string message, EventLogEntryType eventLogEntryType, i
}
}

#pragma warning disable CA1416 // Debug.Assert(OperatingSystem.IsWindows()) is not available
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@buyaa-n is there a bug we can reference in this comment?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is because the assembly targets netstandard, so the relevant APIs aren't available

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, thanks! I think Debug.Assert(RuntimeInformation.IsOSPlatform(OSPlatform.Windows)) will work in this case then.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes that should work

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
@jeffhandley
Copy link
Member

Seeing that this did not result in any new [SupportedOSPlatform] or [UnsupportedOSPlatform] attributes, I'm unsure if we would even need or want to port this into 5.0.0. @stephentoub / @terrajobst, do you see any reason for us to do that, or could this all just be in master, enabling the analyzer there, and then submitting a follow-up issue to do "The Right Thing™️" with the build system in a follow-up issue?

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.

@stephentoub
Copy link
Member

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.

@jeffhandley jeffhandley mentioned this pull request Sep 4, 2020
Closed
jeffhandley
jeffhandley reviewed Sep 4, 2020
View reviewed changes
src/libraries/System.Net.Sockets/src/System/Net/Sockets/SocketAsyncEventArgs.Windows.cs Outdated
@@ -1219,7 +1223,11 @@ private void FinishOperationSendPackets()

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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 jeffhandley mentioned this pull request Sep 8, 2020
Closed
jeffhandley
jeffhandley approved these changes Sep 9, 2020
View reviewed changes
Copy link
Member

@jeffhandley jeffhandley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. And we have #41901 filed to try to find a way to annotate platform-specific assemblies with assembly-level attributes.

@jeffhandley
Copy link
Member

@steveisok @MaximLipnin @akoeplinger FYI--this will enable the Platform Compatibility Analyzer enabled in the runtime repo's master branch. I wonder if we could/should start adding <SupportedPlatform Include="browser" /> to the libraries (especially the ones where we've applied [UnsupportedOSPlatform("browser")]) and ensuring all of that plumbing works properly in our repo. I did a quick spike of that and it didn't seem to Just Work™, but I'm not sure why.

@tannergooding
Copy link
Member Author

I believe this is ready to merge if there is no additional feedback.

jeffhandley
jeffhandley approved these changes Sep 10, 2020
View reviewed changes
Copy link
Member

@jeffhandley jeffhandley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good; merge away!

@tannergooding tannergooding merged commit 23919a4 into dotnet:master Sep 10, 2020
@jeffhandley jeffhandley mentioned this pull request Sep 16, 2020
Closed
3 tasks
@ghost ghost locked as resolved and limited conversation to collaborators Dec 7, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@buyaa-n buyaa-n buyaa-n left review comments

@terrajobst terrajobst terrajobst left review comments

@stephentoub stephentoub stephentoub left review comments

@jeffhandley jeffhandley jeffhandley approved these changes

Assignees
No one assigned
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@tannergooding @jeffhandley @stephentoub @terrajobst @buyaa-n @Dotnet-GitSync-Bot