-
Notifications
You must be signed in to change notification settings - Fork 468
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.
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
Platform compatibility analyzer #3917
Platform compatibility analyzer #3917
Conversation
overload of guard method
...tAnalyzers/Core/Microsoft.NetCore.Analyzers/InteropServices/PlatformCompatabilityAnalyzer.cs
Outdated
Show resolved
Hide resolved
...yzers/Core/Microsoft.NetCore.Analyzers/InteropServices/PlatformCompatabilityAnalyzer.Data.cs
Outdated
Show resolved
Hide resolved
...yzers/Core/Microsoft.NetCore.Analyzers/InteropServices/PlatformCompatabilityAnalyzer.Data.cs
Outdated
Show resolved
Hide resolved
...yzers/Core/Microsoft.NetCore.Analyzers/InteropServices/PlatformCompatabilityAnalyzer.Data.cs
Outdated
Show resolved
Hide resolved
...yzers/Core/Microsoft.NetCore.Analyzers/InteropServices/PlatformCompatabilityAnalyzer.Data.cs
Outdated
Show resolved
Hide resolved
...yzers/Core/Microsoft.NetCore.Analyzers/InteropServices/PlatformCompatabilityAnalyzer.Data.cs
Outdated
Show resolved
Hide resolved
@buyaa-n I think your PR needs to target release/rc-1 branch, master is targeting .NET5 RC2. Last set of merges indicate you have pulled in some changes in your branch that are only in master branch. You may have to revert that merge commit and retarget your PR to the correct branch. |
Good catch, @mavasani! |
1d4733c
to
d5e1e74
Compare
...tAnalyzers/Core/Microsoft.NetCore.Analyzers/InteropServices/PlatformCompatabilityAnalyzer.cs
Outdated
Show resolved
Hide resolved
...tAnalyzers/Core/Microsoft.NetCore.Analyzers/InteropServices/PlatformCompatabilityAnalyzer.cs
Outdated
Show resolved
Hide resolved
...tAnalyzers/Core/Microsoft.NetCore.Analyzers/InteropServices/PlatformCompatabilityAnalyzer.cs
Outdated
Show resolved
Hide resolved
...tAnalyzers/Core/Microsoft.NetCore.Analyzers/InteropServices/PlatformCompatabilityAnalyzer.cs
Outdated
Show resolved
Hide resolved
...tAnalyzers/Core/Microsoft.NetCore.Analyzers/InteropServices/PlatformCompatabilityAnalyzer.cs
Show resolved
Hide resolved
src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/MicrosoftNetCoreAnalyzersResources.resx
Outdated
Show resolved
Hide resolved
Please note that we need to address the build warnings that this analyzer will introduce in the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks great to me! I haven't reviewed the tests thoroughly, but the given the amount of dogfooding that was done for this analyzer, along with follow-up bug fixes with regression tests, I have no concerns in that space.
Awesome work @buyaa-n!
Customer Impact
Customers could get a warning if they referenced platform-specific APIs
Testing
Unit tests, manual tests, and dogfooding within the
dotnet/runtime
repo on local builds.Risk
Medium
Description:
We have added new attributes to annotate APIs being platform-specific;
SupportedOSPlatform
andUnsupportedOSPlatform
.Based on them we want a Roslyn analyzer that informs developers when they use platform-specific APIs from call sites where the API might not be available.
The semantics of these attributes are updated to become as follows:
[SupportedOSPlatform
or[UnsupportedOSPlatform]
attributes are present, we group all attributes by OS platform identifier:[SupportedOSPlatform]
attribute, the API is considered to only be supported by the listed platforms and unsupported by all other platforms.[UnsupportedOSPlatform]
attribute, then the API is considered to only be unsupported by the listed platforms and supported by all other platforms.[SupportedOSPlatform]
while for others it is[UnsupportedOSPlatform]
Sample 1: Deny list.
The API is unsupported only on Windows, then added support from version 10.0.1903, then removed from version 10.0.2004:
Sample 2: Allow list:
Api only allowed on ios12.0 - 14.0 and on ipados from version 13.0
Related design doc https://github.com/dotnet/designs/blob/master/accepted/2020/platform-checks/platform-checks.md,
https://github.com/dotnet/designs/blob/master/accepted/2020/platform-exclusion/platform-exclusion.md
Runtime issue: dotnet/runtime#37359
Finding Diagnostics
The analyzer could start analyzing diagnostics for any method | event | property | enum invocation, check if they have annotated with any OSPlatformAttribute.
OSPlatformAttribute
that attribute applies to all members defined within the assembly/class. So a member will have a combination of its immediate attributes and derived attributes from containing type or assembly.In general, the analyzer can produce the following diagnostics:
SupportedOSPlatform
which haven't suppressed => could warn with '{API}' is supported on {OS} {OS version} and later.* (Fixer could suggest for addingSupportedOSPlatform
or platform guardOperatingSystem.IsOSPlatformVersionAtLeast(…)
)UnsupportedOSPlatform
which haven't supppressed => could warn with '{API}' is unsupported on {OS} {OS version} and later.Automatic suppression via Platform context or platform guard methods
Diagnostics can be suppressed by call site annotated with the same attribute with the correct version or a call to guard methods:
OSPlatformAttribute
to the containing member, type, module, or assembly related to the current diagnostic.if (1) call-site's containing method, type, module, or assembly's
OSPlatformAttribute
type andosPlatform
string matches check the versions to determine if context suppresses the diagnostic.If (1) not suppressing the diagnostic, determine if the call-site is guarded by a platform check using flow analysis.
Sample: diagnostic suppressed by (2) a call to OperatingSystem.IsOSPlatformVersionAtLeast(). This should work for simple cases where the call is contained inside an if check but also when the check causes control flow to never reach the call:
Diagnostic properties