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

Add SupportedOSPlatformGuard and UnsupportedOSPlatformGuard Platform-Guard attributes #51541

Closed
buyaa-n opened this issue Apr 20, 2021 · 7 comments · Fixed by #52028
Closed
Labels
api-approved API was approved in API review, it can be implemented area-Meta
Milestone

Comments

@buyaa-n
Copy link
Member

buyaa-n commented Apr 20, 2021

Related to #44922

Background and Motivation

The CA1416 Platform Compatibility analyzer already recognizes platform guards using the methods on OperatingSystem, such as OperatingSystem.IsWindows and OperatingSystem.IsWindowsVersionAtLeast. However, the analyzer does not recognize other guard methods like a field, property or helper methods that assert platform guards. Expanding this support involves creating new attributes that indicate that an API asserts platform checks the same way the APIs on OperatingSystem do.

Proposed API

namespace System.Runtime.Versioning
{
    // Existing base type for all platform-specific attributes.
    public abstract class OSPlatformAttribute : Attribute
    {
        private protected OSPlatformAttribute(string platformName);
        public string PlatformName { get; }
    }

+   [AttributeUsage(AttributeTargets.Method | AttributeTargets.Property | AttributeTargets.Field | AttributeTargets.Enum, AllowMultiple = true, Inherited = false)]
+   public sealed class SupportedOSPlatformGuardAttribute : OSPlatformAttribute
+   {
+       public SupportedOSPlatformGuardAttribute(string platformName) ;
+   }

+   [AttributeUsage(AttributeTargets.Method | AttributeTargets.Property | AttributeTargets.Field | AttributeTargets.Enum, AllowMultiple = true, Inherited = false)]
+   public sealed class UnsupportedOSPlatformGuardAttribute : OSPlatformAttribute
+   {
+       public UnsupportedOSPlatformGuardAttribute(string platformName) ;
+   }
}

Usage Examples

One example is in Thread, there is an internal field for IsThreadStartSupported that 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

// Usage example
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)
    }
}

Additionally, it's expected that projects will commonly create helper methods that wrap around the platform-specific API calls. The platform being guarded using these attributes can be versioned.

[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() { }
}

cc @terrajobst @jeffhandley

@buyaa-n buyaa-n added area-Meta api-ready-for-review API is ready for review, it is NOT ready for implementation labels Apr 20, 2021
@buyaa-n buyaa-n added this to the 6.0.0 milestone Apr 20, 2021
@ghost ghost added this to Needs triage in Triage POD for Meta, Reflection, etc Apr 20, 2021
@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged New issue has not been triaged by the area owner label Apr 20, 2021
@buyaa-n buyaa-n removed the untriaged New issue has not been triaged by the area owner label Apr 20, 2021
@buyaa-n buyaa-n moved this from Needs triage to v-Next in Triage POD for Meta, Reflection, etc Apr 20, 2021
@buyaa-n buyaa-n added the blocking Marks issues that we want to fast track in order to unblock other important work label Apr 23, 2021
@bartonjs
Copy link
Member

bartonjs commented Apr 27, 2021

Video

We did a long discussion as to whether we should accept a boolean in the manner of [MaybeNullWhen], but felt like it probably doesn't come up much, and we could add it later.

We did remove AttributeTargets.Enum from the attribute decl, though.

namespace System.Runtime.Versioning
{
    [AttributeUsage(AttributeTargets.Method | AttributeTargets.Property | AttributeTargets.Field, AllowMultiple = true, Inherited = false)]
    public sealed class SupportedOSPlatformGuardAttribute : OSPlatformAttribute
    {
        public SupportedOSPlatformGuardAttribute(string platformName) ;
    }

    [AttributeUsage(AttributeTargets.Method | AttributeTargets.Property | AttributeTargets.Field, AllowMultiple = true, Inherited = false)]
    public sealed class UnsupportedOSPlatformGuardAttribute : OSPlatformAttribute
    {
        public UnsupportedOSPlatformGuardAttribute(string platformName) ;
    }
}

@bartonjs bartonjs added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for review, it is NOT ready for implementation blocking Marks issues that we want to fast track in order to unblock other important work labels Apr 27, 2021
@marek-safar
Copy link
Contributor

What is the intention with AttributeTargets.Property target?

@buyaa-n
Copy link
Member Author

buyaa-n commented Apr 28, 2021

What is the intention with AttributeTargets.Property target?

Same as AttributeTargets.Field a boolean property can be used as a guard when the attribute is applied.

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Apr 29, 2021
@marek-safar
Copy link
Contributor

Same as AttributeTargets.Field a boolean property can be used as a guard when the attribute is applied.

Sure but what is the intended behaviour in this case

	[SupportedOSPlatformGuardAttribute("windows")]
	public string TestProperty { set { /* does nothing */ } }

@bartonjs
Copy link
Member

Sure but what is the intended behaviour in th[e case of a set-only public property]

The Keeper of the Guidelines quotes things at you:

DO NOT provide set-only properties or properties with the setter having broader accessibility than the getter.

But in terms of the actual question, I'm sure the attribute applies only to the get. And maybe the analyzer will complain if it's anything other than a get-only property.

@jeffhandley
Copy link
Member

There's probably a whole set of checks we could do for the guard annotations being misused; perhaps they could be added to #44914 ("Developers get warnings when they misuse [SupportedOSPlatform] or [UnsupportedOSPlatform] attributes").

@buyaa-n
Copy link
Member Author

buyaa-n commented Apr 29, 2021

Right, the attributes will only be accounted for a member with boolen type and have a getter, for now, other misuses will be ignored, and yes we could add them into the "Developers get warnings when they misuse [SupportedOSPlatform] or [UnsupportedOSPlatform] attributes" analyzer and warn such cases, I will see where to add it.

Triage POD for Meta, Reflection, etc automation moved this from v-Next to Done May 3, 2021
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label May 3, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Jun 2, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-Meta
Projects
No open projects
Development

Successfully merging a pull request may close this issue.

4 participants