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.

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

CA1416 Support custom guard members annotated with guard attributes #5087

Merged
merged 6 commits into from
May 12, 2021

Conversation

buyaa-n
Copy link
Member

@buyaa-n buyaa-n commented May 5, 2021

Fixes #4428 Related to dotnet/runtime#51541 and dotnet/runtime#44922 (comment)

The CA1416 Platform Compatibility analyzer already recognizes platform guards using the methods on OperatingSystem/RuntimeInformation, such as OperatingSystem.IsWindows and OperatingSystem.IsWindowsVersionAtLeast. However, the analyzer does not recognize other guard options like when a field, property, or helper methods cached the platform check result and used as platform guards.

Expanding this support involved adding new attributes SupportedOSPlatformGuard and UnsupportedOSPlatformGuard and annotated the custom guards with those attributes with the corresponding platform and/or version. Now this annotation needs to be recognized and respected in the Platform Compatibility analyzer flow analysis

Usage Examples

Thread.IsThreadStartSupported indicates whether or not the current platform supports calls to Thread.Start(), which is not supported on browser. So in this [UnsupportedOSPlatformGuard] attributes can be used for this field and the analyzer will recognize it as a platform guard.

#if !TARGET_BROWSER
    [UnsupportedOSPlatformGuard("browser")] // The platform guard atribute
    internal const bool IsThreadStartSupported = true;
#endif

protected internal override void QueueTask(Task task)
{
    if (Thread.IsThreadStartSupported) // This will be evaluated same as `!OperatingSystem.IsBrowser()`
    {
        new Thread(s_longRunningThreadWork)
        {
            IsBackground = true,
            Name = ".NET Long Running Task"
        }.UnsafeStart(task);      // Call unsupported on browser APIs safely (without the attribute it would warn here)
    }
}

Example with helper method:

[SupportedOSPlatformGuard("windows10.0")]
public bool  IsWindows10OrAbove()
{ 
     return OperatingSystem.IsWindowsVersionAtLeast(10);
}

[SupportedOSPlatform("windows10.0")]
public bool  Windows10OnlyAPI() { }

void Sample ()
{
    if (IsWindows10OrAbove()) // work same as OperatingSystem.IsWindowsVersionAtLeast(10)
    {
        Windows10OnlyAPI();
    }
}

For guarding multiple platforms need to apply the attribute for each platform

class Test
{
    [SupportedOSPlatformGuard(""linux"")]
    [SupportedOSPlatformGuard(""macOS"")]
    [SupportedOSPlatformGuard(""Windows"")]
    private readonly bool _http3Enabled = OperatingSystem.IsLinux() || OperatingSystem.IsWindows() || OperatingSystem.IsMacOS();

    void M1()
    {
        if (_http3Enabled)
        {
            ApiWorkOnWindowsLinuxMac();  // Not warn
        }
        else
        {
            ApiWorkOnWindowsLinuxMac();  // Warns 
        }
    }

    [SupportedOSPlatform("windows")]
    [SupportedOSPlatform("Linux")]
    [SupportedOSPlatform("macos")]
    void ApiWorkOnWindowsLinuxMac() { }
}

@buyaa-n buyaa-n requested a review from a team as a code owner May 5, 2021 20:34
@codecov
Copy link

codecov bot commented May 5, 2021

Codecov Report

Merging #5087 (9c0a628) into release/6.0.1xx (d515202) will increase coverage by 0.01%.
The diff coverage is 98.89%.

@@                 Coverage Diff                 @@
##           release/6.0.1xx    #5087      +/-   ##
===================================================
+ Coverage            95.55%   95.57%   +0.01%     
===================================================
  Files                 1203     1203              
  Lines               274337   274837     +500     
  Branches             16632    16653      +21     
===================================================
+ Hits                262137   262662     +525     
+ Misses                9988     9970      -18     
+ Partials              2212     2205       -7     

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.

I'm still reviewing, but I wanted to go ahead and post the comments I have so far.

Copy link
Contributor

@mavasani mavasani left a comment

Choose a reason for hiding this comment

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

Please do retarget to 6.0.1xx branch

@buyaa-n buyaa-n changed the base branch from release/6.0.1xx-preview4 to release/6.0.1xx May 11, 2021 18:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants