search

dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
15.24k stars 4.73k forks source link

Guarding calls to platform-specific APIs #33331

Closed terrajobst closed 4 years ago

terrajobst commented 4 years ago

For iOS and Android in particular we want the developer to be able to do runtime checks for the OS version in order to guard method calls. We've steered people away from Environment.OSVersion in favor of RuntimeInformation.IsOSPlatform(). However, we (deliberately) omitted a way to detect version numbers because the guidance has been to move to feature detection instead. However, we've learned that version checks are a practical necessity. Also, they are the status quo on iOS and Android.

We plan on combining these guards with a set of custom attributes that are used by an analyzer to flag code that isn't properly guarded. For more details, see this spec.

API Proposal

namespace System.Runtime.InteropServices
{
    public partial struct OSPlatform
    {
        // Existing properties
        // public static OSPlatform FreeBSD { get; }
        // public static OSPlatform Linux { get; }
        // public static OSPlatform OSX { get; }
        // public static OSPlatform Windows { get; }
        public static OSPlatform Android { get; }
        public static OSPlatform iOS { get; }
        // public static OSPlatform macOS { get; } /* We already have OSX */
        public static OSPlatform tvOS { get; }
        public static OSPlatform watchOS { get; }
    }

    public partial static class RuntimeInformation
    {
        // Existing API
        // public static bool IsOSPlatform(OSPlatform osPlatform);

        // Check for the OS with a >= version comparison
        // Used to guard APIs that were added in the given OS release.
        public static bool IsOSPlatformOrLater(OSPlatform osPlatform, int major);
        public static bool IsOSPlatformOrLater(OSPlatform osPlatform, int major, int minor);
        public static bool IsOSPlatformOrLater(OSPlatform osPlatform, int major, int minor, int build);
        public static bool IsOSPlatformOrLater(OSPlatform osPlatform, int major, int minor, int build, int revision);

        // Allows checking for the OS with a < version comparison
        // Used to guard APIs that were obsoleted or removed in the given OS release. The comparison
        // is less than (rather than less than or equal) so that people can pass in the version where
        // API became obsoleted/removed.
        public static bool IsOSPlatformEarlierThan(OSPlatform osPlatform, int major);
        public static bool IsOSPlatformEarlierThan(OSPlatform osPlatform, int major, int minor);
        public static bool IsOSPlatformEarlierThan(OSPlatform osPlatform, int major, int minor, int build);
        public static bool IsOSPlatformEarlierThan(OSPlatform osPlatform, int major, int minor, int build, int revision);
    }
}

namespace System.Runtime.Versioning
{
    // Base type for all platform-specific attributes. Primarily used to allow grouping
    // in documentation.
    public abstract class PlatformAttribute : Attribute
    {
        protected PlatformAttribute (string platformName);
        public string PlatformName { get; }
    }

    // Records the platform that the project targeted.
    [AttributeUsage(AttributeTargets.Assembly,
                    AllowMultiple=false, Inherited=false)]
    public sealed class TargetPlatformAttribute : PlatformAttribute
    {
        public TargetPlatformAttribute(string platformName);
        public string PlatformName { get; }
    }

    // Records the minimum platform that is required in order to the marked thing.
    //
    // * When applied to an assembly, it means the entire assembly cannot be called
    //   into on earlier versions. It records the TargetPlatformMinVersion property.
    //
    // * When applied to an API, it means the API cannot be called from an earlier
    //   version.
    //
    // In either case, the caller can either mark itself with MinimumPlatformAttribute
    // or guard the call with a platform check.
    //
    // The attribute can be applied multiple times for different operating systems.
    // That means the API is supported on multiple operating systems.
    //
    // A given platform should only be specified once.

    [AttributeUsage(AttributeTargets.Assembly |
                    AttributeTargets.Class |
                    AttributeTargets.Constructor |
                    AttributeTargets.Event |
                    AttributeTargets.Method |
                    AttributeTargets.Module |
                    AttributeTargets.Property |
                    AttributeTargets.Struct,
                    AllowMultiple=true, Inherited=false)]
    public sealed class MinimumPlatformAttribute : PlatformAttribute
    {
        public MinimumPlatformAttribute(string platformName);
    }

    // Marks APIs that were removed in a given operating system version.
    //
    // Primarily used by OS bindings to indicate APIs that are only available in
    // earlier versions.
    [AttributeUsage(AttributeTargets.Assembly |
                    AttributeTargets.Class |
                    AttributeTargets.Constructor |
                    AttributeTargets.Event |
                    AttributeTargets.Method |
                    AttributeTargets.Module |
                    AttributeTargets.Property |
                    AttributeTargets.Struct,
                    AllowMultiple=true, Inherited=false)]
    public sealed class RemovedInPlatformAttribute : PlatformAttribute
    {
        public RemovedInPlatformAttribute(string platformName);
    }

    // Marks APIs that were obsoleted in a given operating system version.
    //
    // Primarily used by OS bindings to indicate APIs that should only be used in
    // earlier versions.
    [AttributeUsage(AttributeTargets.Assembly |
                    AttributeTargets.Class |
                    AttributeTargets.Constructor |
                    AttributeTargets.Event |
                    AttributeTargets.Method |
                    AttributeTargets.Module |
                    AttributeTargets.Property |
                    AttributeTargets.Struct,
                    AllowMultiple=true, Inherited=false)]
    public sealed class ObsoletedInPlatformAttribute : PlatformAttribute
    {
        public ObsoletedInPlatformAttribute(string platformName);
        public string Url { get; set; }
    }
}

This design allows us to encapsulate the version comparison, i.e. the "and later" part.

Usage: Recording Project Properties

<Project>
    <Properties>
        <TargetFramework>net5.0-ios12.0</TargetFramework>
        <TargetPlatformMinVersion>10.0</TargetPlatformMinVersion>
    </Properties>
    ...
</Project>

The SDK already generates a file called AssemblyInfo.cs which includes the TFM. We'll extend on this to also record the target platform and minium version (which can be omitted in the project file which means it's the same as the target platform):

[assembly: TargetFramework(".NETCoreApp, Version=5.0")] // Already exists today
[assembly: TargetPlatform("ios12.0")]  // new
[assembly: MinimumPlatform("ios10.0")] // new

Usage: Guarding Platform-Specific APIs

NSFizzBuzz is an iOS API that was introduced in iOS 14. Since I only want to call the API when I'm running on a version of the OS that supports it I'd guard the call using IsOSPlatformOrLater:

static void ProvideExtraPop()
{
    if (!RuntimeInformation.IsOSPlatformOrLater(OSPlatform.iOS, 14))
        return;

    NSFizzBuzz();
}

Usage: Declaring Platform-Specific APIs

The RemovedInPlatformAttribute and ObsoletedInPlatformAttribute will primarily be used by the OS bindings to indicate whether a given API shouldn't be used any more.

The MinimumPlatformAttribute will be for two things:

  1. Indicate which OS a given assembly can run on (mostly used by user code)
  2. Indicate which OS a given API is supported on (mostly used by OS bindings)

Both scenarios have effectively the same meaning for our analyzer: calls into the assembly/API are only legal if the call site is from the given operating system, in the exact or later version.

The second scenario can also be used by user code to forward the requirement. For example, imagine the NSFizzBuzz API to be complex. User code might want to encapsulate it's usage in a helper type:

[MinimumPlatform("ios14.0")]
internal class NSFizzBuzzHelper
{
    public void Fizz() { ... }
    public void Buzz() { ... }
}

As far as the analyzer is concerned, NSFizzBuzzHelper can only be used on iOS 14, which means that its members can call iOS 14 APIs without any warnings. The requirement to check for iOS is effectively forwarded to code that calls any members on NSFizzBuzzHelper.

@dotnet/fxdc @mhutch

bartonjs commented 4 years ago

I feel like the method name needs something about the versioning. Like IsOSPlatformVersionOrNewer; or IsMinimumOSPlatformVersion. Because, as written, I could see people writing it thinking it's exact, and where they should have lightup they have fallback.

GrabYourPitchforks commented 4 years ago

Piggybacking off Jeremy's comment, I recommend IsOSVersionAtLeast (or similar). This somewhat matches the APIs that Windows and iOS expose. (Not sure about Android.)

It also reads fairly well IMO.

if (RuntimeInformation.IsOSVersionAtLeast(OSPlatform.iOS, 12, 0)
{
    /* do something */
}
nil4 commented 4 years ago

Could the two overloads be replaced by a single method taking a System.Version argument? e.g.

public static bool IsOSVersionAtLeast(OSPlatform osPlatform, Version minimumVersion);
marek-safar commented 4 years ago

It'd be great if we could design this together with API for minimal version annotations. If we had both standardized someone could, for example, write missing version check analyzer which would work everywhere.

The expanded version of the sample

public void OnClick(object sender, EventArgs e)
{
    if (RuntimeInformation.IsOSPlatform(OSPlatform.iOS, 12, 0))
    {
        NSFizBuzz();
    }
}

[IntroducedOn(OSPlatform.iOS, 12, 0)]
static void NSFizBuzz()
{
}

I think the same can apply to libraries targetting Linux or Windows API which could be annotated when API minimal OS version is higher than the lowest version.

Xamarin version can be explored at https://github.com/xamarin/xamarin-macios/blob/master/src/ObjCRuntime/PlatformAvailability2.cs

/cc @chamons @jonpryor

jkotas commented 4 years ago

What is the version that this will use on Linux?

Is Android treated as a Linux flavor in these APIs?

the guidance has been to move to feature detection instead. However, it seems this has never taken on in iOS & Android land

The feature detection is a fine theory, but it is not an option in number of situations on Windows either. https://github.com/dotnet/runtime/issues/32575 has recent example.

scalablecory commented 4 years ago

Will this API work correctly in Windows too?

davidsh commented 4 years ago

Will this API work correctly in Windows too?

The current OS version APIs in Windows (for which .NET uses) lie about the real Windows OS version due to how Windows OS uses EXE manifests to decide if reporting the actual version number would break an app or not.

However, there are native Win32 APIs on Windows that will always return the true Windows OS version (including build number). Will this new .NET API use those Windows APIs?

jkotas commented 4 years ago

Why not have an overload that takes major version only? I would expect that people will check for major version only most of the time.

svick commented 4 years ago

Why use two separate overloads instead of one with optional parameter? I.e.:

public static bool IsOSPlatform(
    OSPlatform osPlatform, int majorVersion, int minorVersion, int revision = 0);

(I'm assuming that revision of 0 is the same as not specifying revision in the current proposal. If that's not the case, the default value could be null or -1 instead.)

@nil4 Using Version would make sense to me if there were common cases where you specify the version at a different point in your code than where you call IsOSPlatform. Can you think of any?

If the only common way of calling this method is with hard-coded version, then using Version only makes calling the method more verbose for no benefit. (Though C# 9.0 target-typed new will decrease that verbosity somewhat.)

nil4 commented 4 years ago

@svick you were probably thinking of inline Version arguments. The downside is that this usage would incur allocations on every call.

I was thinking along the lines of storing the Version instance in a readonly static field, which can be reused, e.g. something like:

class App {
  static readonly Version ios12 = new Version(12, 0);

  static void Main() {
    if (RuntimeInformation.IsOSVersionAtLeast(OSPlatform.iOS, ios12))
        NSFizBuzz();

    // .. more stuff ..

    if (RuntimeInformation.IsOSVersionAtLeast(OSPlatform.iOS, ios12))
        NSFizBuzzML();
  }
}

Take the code given in https://github.com/dotnet/runtime/issues/32575 as an example:

https://github.com/dotnet/runtime/blob/3be5238a376cd4bca32db1acb732dc7c15091cdf/src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/Internal/MsQuicApi.cs#L144-L146

That could end up looking like this:

static readonly Version quicMinVersion = new Version(10, 0, 19041, 0);

static MsQuicApi() {
  IsQuicSupported = RuntimeInformation.IsOSVersionAtLeast(OSPlatform.Windows, quicMinVersion);
}

The benefits I see are that Version comparisons are well-defined and understood, and there is no need to expose multiple overloads.

rolfbjarne commented 4 years ago

Why not have an overload that takes major version only? I would expect that people will check for major version only most of the time.

macOS 10 is 19 years old and still going strong.

jkotas commented 4 years ago

Right, this overload would not apply to OSes that have decided to be stuck on the same major version.

Both iOS and Android that are motivating this API have a more sane versioning story that increments the major version number.

tannergooding commented 4 years ago

Why not just public static bool IsOSVersionAtLeast(OSPlatform osPlatform, int majorVersion, int minorVersion = 0, int revision = 0);? It should still be constant foldable in the same way and makes the latter two parts optional.

GrabYourPitchforks commented 4 years ago

I'd be absolutely shocked if this API is in such a hot path that any allocations would be noticeable.

tannergooding commented 4 years ago

I'd be absolutely shocked if this API is in such a hot path that any allocations would be noticeable.

I think that would depend on what kind of feature you are querying for. If this was something like a low level timer or a graphics/multimedia API, then it could be used in a hot path and constant folding/allocation free would basically be a must.

GrabYourPitchforks commented 4 years ago

I think that would depend on what kind of feature you are querying for. If this was something like a low level timer or a graphics/multimedia API, then it could be used in a hot path and constant folding/allocation free would basically be a must.

If it's really that hot, then wouldn't the application itself want to cache the result? In your scenario I imagine they'd want to query the feature once upfront and then create a static function pointer to MethodA or MethodB, depending on detected OS.

rolfbjarne commented 4 years ago

If this was something like a low level timer or a graphics/multimedia API, then it could be used in a hot path and constant folding/allocation free would basically be a must.

The API doesn't have to be fast, people can cache the value:

class App {
  static readonly bool ios12 = RuntimeInformation.IsOSVersionAtLeast(OSPlatform.iOS, new Version(12, 0));

  static void Main() {
    if (ios12)
        NSFizBuzz();

    // .. more stuff ..

    if (ios12)
        NSFizBuzzML();
  }
}

Although I agree that the faster the better of course.

Personally I'm in favor of giving people the API that best suit their needs, and since it seems in this case we don't really know how people are going to use the API, we should provide all:

public static bool IsOSVersionAtLeast(OSPlatform osPlatform, int majorVersion);
public static bool IsOSVersionAtLeast(OSPlatform osPlatform, int majorVersion, int minorVersion);
public static bool IsOSVersionAtLeast(OSPlatform osPlatform, int majorVersion, int minorVersion, int revision);
public static bool IsOSVersionAtLeast(OSPlatform osPlatform, Version version);

A point to have in mind is that these two are not identical:

var a = new Version (1, 0);
var b = new Version (1, 0, 0);
Console.WriteLine (a == b);

this prints false, and which is why I'm using overloads instead of default values.

tannergooding commented 4 years ago

If it's really that hot, then wouldn't the application itself want to cache the result? In your scenario I imagine they'd want to query the feature once upfront and then create a static function pointer to MethodA or MethodB, depending on detected OS.

I think there are two scenarios to consider (much as we are considering for the hardware intrinsic Isa.IsSupported checks).

The first is you are JIT compiled, in which case you know the exact machine you are targeting and the check can become a constant. This isn't readily applicable to iOS/Android, but it is to Desktop and some other scenarios. The second is that you are AOT compiled, in which case you have a baseline that can be used to remove some checks and other checks can become cached lookups that result in a branch or a decision to use some other kind of dynamic dispatch.

I imagine the default scenario for most people is to use cached lookups, rather than dynamic dispatch, but we should likely support users deciding to do both (and having a cached lookup doesn't preclude doing dynamic dispatch).

I think in both cases, however, not using Version makes the check easier for the compiler (whether JIT or AOT) to understand. I imagine it will also be more readable than IsOSVersionAtLeast(OSPlatform.Windows, new Version(10, 0, 18362))

spouliot commented 4 years ago

The API doesn't have to be fast, people can cache the value:

No, but that makes code analysis much harder. I guess we can't have it all ;-)

terrajobst commented 4 years ago

This needs more thought

terrajobst commented 4 years ago

Video

Some notes:

Joe4evr commented 4 years ago

During the review, it was said that by making the version parameters in IsOSPlatformOrLater()/IsOSPlatformEarlierThan() normal ints, it's more likely that people will pass them in as literals, which makes things easier for the associated analyzer. Would this qualify those parameters to be marked with [ConstantExpected] (#33771)?

wli3 commented 4 years ago

Could we move OSPlatformVersionAttribute up in schedule? Seems there is not debate over the definition of this attribute. I hope to get it in sooner so we can unblock https://github.com/dotnet/sdk/issues/11239

wli3 commented 4 years ago

Actually, there is something to discuss in OSPlatformVersionAttribute too.

MSBuild could only generate assembly attributes with strings as parameter.

So int cannot be generated by MSBuild

OSPlatformVersionAttribute (string osPlatform,
                                              int major,
                                              int minor,
                                              int build,
                                              int revision);

now the question is. Should MSBuild add this support or we change the signature to take in all strings? I can see why it is hard for MSBuild considering the input of _Parameter1 is metadata already there is no place to specify the type. At the same time older attribute like AssemblyVersionAttribute just take one "string" in the constructor. We could add an overload to take a string and parse it in the constructor

wli3 commented 4 years ago

Also considering the name "AddedInOSPlatformVersionAttribute" it makes more sense when the attribute is on a framework type. However, since we want to create this attribute automatically for the user. It is kind of odd when a user's type is "added in iOS12"

terrajobst commented 4 years ago

Video

namespace System.Runtime.InteropServices
{
    public partial struct OSPlatform
    {
        // Existing properties
        // public static OSPlatform FreeBSD { get; }
        // public static OSPlatform Linux { get; }
        [EditorBrowsable(EditorBrowsableState.Never)]
        public static OSPlatform OSX { get; }
        // public static OSPlatform Windows { get; }
        public static OSPlatform Android { get; }
        public static OSPlatform iOS { get; }
        public static OSPlatform macOS { get; }
        public static OSPlatform tvOS { get; }
        public static OSPlatform watchOS { get; }
    }

    public partial static class RuntimeInformation
    {
        // Existing API
        // public static bool IsOSPlatform(OSPlatform osPlatform);

        // Check for the OS with a >= version comparison
        // Used to guard APIs that were added in the given OS release.
        public static bool IsOSPlatformOrLater(string platformName);
        public static bool IsOSPlatformOrLater(OSPlatform osPlatform, int major);
        public static bool IsOSPlatformOrLater(OSPlatform osPlatform, int major, int minor);
        public static bool IsOSPlatformOrLater(OSPlatform osPlatform, int major, int minor, int build);
        public static bool IsOSPlatformOrLater(OSPlatform osPlatform, int major, int minor, int build, int revision);

        // Allows checking for the OS with a < version comparison
        // Used to guard APIs that were obsoleted or removed in the given OS release. The comparison
        // is less than (rather than less than or equal) so that people can pass in the version where
        // API became obsoleted/removed.
        public static bool IsOSPlatformEarlierThan(string platformName);
        public static bool IsOSPlatformEarlierThan(OSPlatform osPlatform, int major);
        public static bool IsOSPlatformEarlierThan(OSPlatform osPlatform, int major, int minor);
        public static bool IsOSPlatformEarlierThan(OSPlatform osPlatform, int major, int minor, int build);
        public static bool IsOSPlatformEarlierThan(OSPlatform osPlatform, int major, int minor, int build, int revision);
    }
}

namespace System.Runtime.Versioning
{
    // Base type for all platform-specific attributes. Primarily used to allow grouping
    // in documentation.
    public abstract class OSPlatformAttribute : Attribute
    {
        private protected OSPlatformAttribute(string platformName);
        public string PlatformName { get; }
    }

    // Records the platform that the project targeted.
    [AttributeUsage(AttributeTargets.Assembly,
                    AllowMultiple=false, Inherited=false)]
    public sealed class TargetPlatformAttribute : OSPlatformAttribute
    {
        public TargetPlatformAttribute(string platformName);
        public string PlatformName { get; }
    }

    // Records the minimum platform that is required in order to the marked thing.
    //
    // * When applied to an assembly, it means the entire assembly cannot be called
    //   into on earlier versions. It records the TargetPlatformMinVersion property.
    //
    // * When applied to an API, it means the API cannot be called from an earlier
    //   version.
    //
    // In either case, the caller can either mark itself with MinimumPlatformAttribute
    // or guard the call with a platform check.
    //
    // The attribute can be applied multiple times for different operating systems.
    // That means the API is supported on multiple operating systems.
    //
    // A given platform should only be specified once.

    [AttributeUsage(AttributeTargets.Assembly |
                    AttributeTargets.Class |
                    AttributeTargets.Constructor |
                    AttributeTargets.Event |
                    AttributeTargets.Method |
                    AttributeTargets.Module |
                    AttributeTargets.Property |
                    AttributeTargets.Struct,
                    AllowMultiple=true, Inherited=false)]
    public sealed class MinimumOSPlatformAttribute : OSPlatformAttribute
    {
        public MinimumOSPlatformAttribute(string platformName);
    }

    // Marks APIs that were removed in a given operating system version.
    //
    // Primarily used by OS bindings to indicate APIs that are only available in
    // earlier versions.
    [AttributeUsage(AttributeTargets.Assembly |
                    AttributeTargets.Class |
                    AttributeTargets.Constructor |
                    AttributeTargets.Event |
                    AttributeTargets.Method |
                    AttributeTargets.Module |
                    AttributeTargets.Property |
                    AttributeTargets.Struct,
                    AllowMultiple=true, Inherited=false)]
    public sealed class RemovedInOSPlatformAttribute : OSPlatformAttribute
    {
        public RemovedInPlatformAttribute(string platformName);
    }

    // Marks APIs that were obsoleted in a given operating system version.
    //
    // Primarily used by OS bindings to indicate APIs that should only be used in
    // earlier versions.
    [AttributeUsage(AttributeTargets.Assembly |
                    AttributeTargets.Class |
                    AttributeTargets.Constructor |
                    AttributeTargets.Event |
                    AttributeTargets.Method |
                    AttributeTargets.Module |
                    AttributeTargets.Property |
                    AttributeTargets.Struct,
                    AllowMultiple=true, Inherited=false)]
    public sealed class ObsoletedInOSPlatformAttribute : OSPlatformAttribute
    {
        public ObsoletedInPlatformAttribute(string platformName);
        public ObsoletedInPlatformAttribute(string platformName, string message);
        public string Message { get; }
        public string Url { get; set; }
    }
}
juliusfriedman commented 4 years ago

Saw the review on this today and wanted to add my 0.02.

Say I want to support iOS or Android versions specific e.g.:

iOS ==10 && >= 12 (Not iOS 11 for some reason)

Android >=6 && false == Android === 8 && Android >= 10 (Android 6,7, 10 etc, not 8 or 9 for some reason)

Perhaps Windows 7 or 10 but not 8 or 8.1 ...

I reason for the above that a TargetOSPlatformNotSupported Attribute should also be added to indicate which versions of that platform are NOT supported.

I also think we can combine some of the attribute such as Minimum/Target with Attributes which can take an array where the members are the specific versions, or possibly even multiple Range's / Versions etc.

I think perhaps also the attribute could have the IsSupported property with the version so then you end up with something like [OSPlatformSupportAttribute(new MinSupportedPlatformAttribute(){ Platform, Version, IsSupported = true /*default?*/ }, new NotSupportedPlatform(){ Platform, Version}), new MaxSupportedPlatformAttribute(){ Platform, Version, IsSupported = true /*default?*/})]

wli3 commented 4 years ago

@danmosemsft we passed the API review with some edit. What is the timeline for getting it checked in?

danmoseley commented 4 years ago

@wli3 if it’s as straightforward as you say I suggest you go ahead and do it. Let us know if you need any pointers.

jeffhandley commented 4 years ago

@terrajobst @bartonjs -- Should the PlatformName property be renamed to OSPlatformName, or is it OK as-is?

marek-safar commented 4 years ago

@terrajobst is the intention to use RemovedInOSPlatformAttribute also for APIs which were never supported on the OS platform or is the plan to have another concept/attribute for APIs which always throw PlatformNotSupportedException ?

terrajobst commented 4 years ago

@jeffhandley

@terrajobst @bartonjs -- Should the PlatformName property be renamed to OSPlatformName, or is it OK as-is?

Parameters are often prefixes/suffixes of types names. I'd leave it as platformName. /cc @fxdc

terrajobst commented 4 years ago

@marek-safar

@terrajobst is the intention to use RemovedInOSPlatformAttribute also for APIs which were never supported on the OS platform or is the plan to have another concept/attribute for APIs which always throw PlatformNotSupportedException ?

If an API is marked as MinimumOSPlatform then it's assumed that only the marked OS are supported. An API that has no MinimumOSPlatform attribute is assumed to be supported everywhere. So no, we wouldn't use RemovedInOSPlatformAttribute for that purpose. Rather, we'd omit the MinimumOSPlatform attribution.

bartonjs commented 4 years ago

I agree that neither the property or parameter need the "OS" prefix on it. The type of platform is inferred from the type name. (The bare word "Name" wouldn't be good since it could be mistaken for a label for the attribute instance, vs the platform identifier.)

If we wanted to change anything about it, I'd change the parameter and property to PlatformId, since "name" (vs "id") might confuse someone as to think it's the versionless part. ("Id" doesn't do a whole lot better, so it's not a significant improvement but might remove a brief hesitation of "but where does the version go?". "[Pp]latformNameAndVersion" is quite clear, but seems unnecessarily verbose.) But, I don't think there's significant confusion with "[Pp]latformName", so I'm not advocating for a change, just rambling.

jeffhandley commented 4 years ago

So no, we wouldn't use RemovedInOSPlatformAttribute for that purpose. Rather, we'd omit the MinimumOSPlatform attribution.

@terrajobst The attributes as we have them allow us to create a list of platforms with included support. Was there consideration for an attribute to indicate excluded support for scenarios like this one? On the surface, it seems like an OSPlatformNotSupportedAttribute could help here, but I’ve not thought it through too deeply.

jeffhandley commented 4 years ago

@adamsitnik @buyaa-n During the Design Review meeting today, we talked about some details that affect the open PRs related to this work.

  1. For the OSPlatform.OSX and OSPlatform.macOS equivalence:
    • We should leave the OSPlatform type itself ignorant of the equivalence between the two platforms
    • .OSX would therefore still be backed by the "OSX" string, and .macOS would be backed by the "macOS" string
    • RuntimeInformation.IsOSPlatform is where equivalence logic would exist
    • In IsOSPlatform, asking for either OSPlatform.OSX or OSPlatform.macOS would have the same result
  2. The Platform Compat Analyzer will need to duplicate the macOS/OSX equivalence check
    • That code should be factored such that we can extend equivalence checks later on if needed
jkotas commented 4 years ago

RuntimeInformation.IsOSPlatform is where equivalence logic would exist

This makes this API even more expensive than it is today and increases chances that people will need to cache the result to avoid performance bottlenecks.

Is the compat analyzer going to do the required data flow analysis to see through cached checks for this?

jeffhandley commented 4 years ago

Is the compat analyzer going to do the required data flow analysis to see through cached checks for this?

No, we determined we couldn't feasibly handle platform checks being in helper/utility methods and therefore only direct calls to IsOSPlatformOrLater and IsOSPlatformEarlierThan would be supported.

Let's recap the options we have for where this logic could exist:

  1. In RuntimeInformation.IsOSPlatform
    • Introduces a potential perf bottleneck that users would not be able to wrap/cache
  2. By having OSPlatform.OSX return a "MACOS"-backed value
    • Perceived as a breaking change for user code that was applying OSPlatform.ToString() == "OSX"
  3. By having OSPlatform.Equals() return true for the macOS/OSX pair
    • Leaves the ToString() comparisons inconsistent with Equals()
  4. By having OSPlatform.macOS return an "OSX"-backed value
    • Opens up the possibility of a user expecting to evaluate the backing string against "MACOS" and not getting expected behavior

Is that accurate? Are there any other options?

jkotas commented 4 years ago

Cache the result of the "is current platform check?" in the OSPlatform constructor in a bool? The IsOSPlatform checks that we care about can then just check this bool without doing any expensive string comparisons, etc.

jeffhandley commented 4 years ago

Interesting... Something like this?

RuntimeInformation.Unix.cs (platform-specific, to be defined on Windows and Browser as well)

public static partial class RuntimeInformation
{
    internal static IsOSPlatformOrEquivalent(string osPlatform)
    {
        string name = s_osPlatformName ??= Interop.Sys.GetUnixName();

        if (osPlatform.Equals(name)) return true;
        if (name == "OSX") return osPlatform.Equals("MACOS");

        return false;
    }
}

OSPlatform.cs

public readonly struct OSPlatform : IEquatable<OSPlatform>
{
    internal bool IsCurrentOSPlatform { get; set; }

    private OSPlatform(string osPlatform)
    {
        if (osPlatform == null) throw new ArgumentNullException(nameof(osPlatform));
        if (osPlatform.Length == 0) throw new ArgumentException(SR.Argument_EmptyValue, nameof(osPlatform));

        _osPlatform = osPlatform;
        IsCurrentOSPlatform = RuntimeInformation.IsOSPlatformOrEquivalent(_osPlatform);
    }
}

RuntimeInformation.cs (cross-platform)

public static partial class RuntimeInformation
{
    public static bool IsOSPlatform(OSPlatform osPlatform)
    {
        return osPlatform.IsCurrentOSPlatform;
    }
}
buyaa-n commented 4 years ago

Is the compat analyzer going to do the required data flow analysis to see through cached checks for this?

No, we determined we couldn't feasibly handle platform checks being in helper/utility methods and therefore only direct calls to IsOSPlatformOrLater and IsOSPlatformEarlierThan would be supported.

@jeffhandley GlobalFlowStateAnalysis API provided by roslyn-analyzers (Manish recently added) supports cached checks, so we have it for free (just by using GlobalFlowStateAnalysis)

https://github.com/buyaa-n/roslyn-analyzers/blob/c7a4bc1b05781834f5dcf758fefb6a5690110dd4/src/NetAnalyzers/UnitTests/Microsoft.NetCore.Analyzers/InteropServices/PlatformCompatabilityAnalyzerTests.cs#L918-L954

jeffhandley commented 4 years ago

Thanks, @buyaa-n; I obviously didn't realize that made it in. Would that also cover caching within utility/helper methods or properties though, or is it limited to caching within the scope of the method?

Would the following work?

private _canUseIOS14 = RuntimeInformation.IsOSPlatformOrLater(OSPlatform.iOS, 14);

private void M1()
{
    if (_canUseIOS14)
    {
        M2();
    }
}

[MinimumOSPlatform("ios14.0")]
private void M2()
{
}
juliusfriedman commented 4 years ago

I really think a set of PlatformSupportAttributes dervived from PlatformSupportAttribute is better than a single attribute here,

That array given to a single PlatformSupportAttribute as I indicated above and it solves more problems than this does today.

cc @terrajobst @stephentoub @jkotas

buyaa-n commented 4 years ago

Would that also cover caching within utility/helper methods or properties though, or is it limited to caching within the scope of the method?

I think it is limited within the scope of the method as the action registered to OperationBlockStartAction, I am not sure if we can support the example you wrote, @mavasani might answer that

mavasani commented 4 years ago

Currently it doesn’t support reading values in fields and properties, but we can add support. It would be much cheaper if we only cared about read only fields and properties. Otherwise, we would need to enable PointsTo/Alias analysis, which is implemented in the repo, but would obviously make the analysis more expensive.

Analysis supports interprocedural analysis, but it is off by default. We can consider enabling it by default with a max level 1 of call chain (single utility/helper). Call chain analysis length is configurable for all DFA in the repo, but obviously makes it more expensive with longer call chain threshold.

In short, all analyses requested here is/can be supported without too much implementation cost. We only need to be cautious about how much we want to enable by default considering the standard performance vs precision trade off for any DFA.

mavasani commented 4 years ago

Note that the analysis understands Debug asserts. I would personally recommend we take route similar to dataflow analysis for C# nullable reference types:

  1. Default analysis to scope of the current method. No interprocedural analysis by default.
  2. Define well known attributes, say EnsuresOSPlatformXXX(name, version), that can be applied to methods, fields and properties to aid analysis without requiring interprocedural analysis. The attributes will be conditional for debug builds only.
  3. Allow users to add debug asserts to aid analysis and avoid false positives for complex control flow or dataflow cases.

Users can choose any of the above two modes:

  1. Enable interprocedural analysis, so you pay build time cost at the benefit of not having to add asserts or attribute annotations.
  2. Add debug asserts and debug only attributes for making analysis faster, at the expense of adding and maintaining annotations.
juliusfriedman commented 4 years ago

@mavasani I don't see how this is not better:

[OSPlatformSupportAttribute(new MinSupportedPlatformAttribute(){ Platform, Version }, new NotSupportedPlatform(){ Platform, Version}), new MaxSupportedPlatformAttribute(){ Platform, Version)]
int SomeMethod(int param){  /**/ }

This gives you more flexibility and the ability to have all the information in one place rather than several.


public sealed class OSPlatformSupportAttribute: System.Attribute  {
//This is not allowed I guess... https://sharplab.io/#gist:d062656f543144e19805a3b4d5d80ab8
 PlatformSupportAttribute[] PlatformSupportAttributes {get; protected set;}
}

public abstract class PlatformSupportAttribute: System.Attribute  
{  
    int m_Major, m_Minor;
    public bool IsSupported {get; protected set;} = true;
    public string Platform {get; protected set;}
    public Version Version {get;} => return new Version(m_Major,m_Minor);

    protected PlatformSupportAttribute(string platform, int major, int minor, bool isSupported = true)  
    {  
        if(string.IsNullOrWhiteSpace(platform) throw new InvalidOperationException($"{nameof(platform)} cannot be null or consist of only whitespace");
        Platform = platform;
        IsSupported = isSupported;  
        m_Major = major;  
        m_Minor = minor;
    }  
}  

public sealed class MinSupportedPlatformAttribute: PlatformSupportAttribute
{     
    public MinSupportedPlatformAttribute(string platform, int major, int minor, , bool isSupported = true) : base(platform, major, minor, isSupported)  
    { 
    }  
}  

public sealed class MaxSupportedPlatformAttribute: PlatformSupportAttribute
{     
    public MaxSupportedPlatformAttribute(string platform, int major, int minor, bool isSupported = true) : base(platform, major, minor, isSupported)  
    { 
    }  
}  

public sealed class NotSupportedPlatformAttribute: PlatformSupportAttribute
{     
    public NotSupportedPlatformAttribute(string platform, int major, int minor) : base(platform, major, minor, false)  
    { 
    }  
}

This could be extended for when the OS is patched live or even for CPUSupport etc very easily while proposed implementation cannot.

Please reconsider.

mavasani commented 4 years ago

@juliusfriedman my comment was not related to your suggestion on actual attributes to use for annotating platform dependent operations. It was regarding the kind of dataflow analysis to perform when detecting if a platform dependent operation was invoked in correct context, when user has performed the platform checks in a helper method or stored it into a field or property.

jkotas commented 4 years ago

@juliusfriedman You suggestion does not compile as written. System.Version and arrays of non-primitive types are not valid fields in custom attributes.

juliusfriedman commented 4 years ago

@jkotas that's unfortunate, it can either be resolved with a custom version struct or more likley additional members which exspose a version property publicly.

+protected int m_Major,m_Minor,m_Build,m_Patch;
+public Version => new Version (m_Major, m_Minor);//Not sure about build and patch here

See also:

https://sharplab.io/#gist:d062656f543144e19805a3b4d5d80ab8

If you really can't have arrays there as in my gist than have a method on the type which returns an array and that array should be able to be baked into the assembly if static.

+ IEnumerable<PlatformSupportAttributes> GetPlatformSupportAttributes()