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

Guarding calls to platform-specific APIs #33331

Closed
terrajobst opened this issue Mar 7, 2020 · 50 comments
Closed

Guarding calls to platform-specific APIs #33331

terrajobst opened this issue Mar 7, 2020 · 50 comments
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.Runtime
Milestone

Comments

@terrajobst
Copy link
Member

terrajobst commented Mar 7, 2020

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

@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added area-System.Runtime untriaged New issue has not been triaged by the area owner labels Mar 7, 2020
@terrajobst terrajobst added api-ready-for-review and removed area-System.Runtime untriaged New issue has not been triaged by the area owner labels Mar 7, 2020
@terrajobst terrajobst self-assigned this Mar 7, 2020
@terrajobst terrajobst added this to the 5.0 milestone Mar 7, 2020
@terrajobst terrajobst added area-System.Runtime untriaged New issue has not been triaged by the area owner labels Mar 7, 2020
@bartonjs
Copy link
Member

bartonjs commented Mar 7, 2020

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

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

nil4 commented Mar 7, 2020

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

marek-safar commented Mar 7, 2020

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

jkotas commented Mar 7, 2020

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. #32575 has recent example.

@scalablecory
Copy link
Contributor

Will this API work correctly in Windows too?

@davidsh
Copy link
Contributor

davidsh commented Mar 7, 2020

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

jkotas commented Mar 7, 2020

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

svick commented Mar 8, 2020

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

nil4 commented Mar 8, 2020

@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 #32575 as an example:

OperatingSystem ver = Environment.OSVersion;
if (ver.Platform == PlatformID.Win32NT && ver.Version < new Version(10, 0, 19041, 0))

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

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

jkotas commented Mar 9, 2020

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

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

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

@tannergooding
Copy link
Member

tannergooding commented Mar 9, 2020

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

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

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

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

spouliot commented Mar 9, 2020

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 terrajobst added api-needs-work API needs work before it is approved, it is NOT ready for implementation and removed api-ready-for-review labels Mar 13, 2020
@bartonjs
Copy link
Member

bartonjs commented Jul 1, 2020

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

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 added a commit that referenced this issue Jul 1, 2020
* Add platform-specific attributes

Spec #33331

* Convert to xml doc

* Update src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/MinimumOSAttribute.cs

Co-authored-by: Jeremy Barton <jbarton@microsoft.com>

* Update src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/ObsoletedInPlatformAttribute.cs

Co-authored-by: Jeff Handley <jeffhandley@users.noreply.github.com>

* Address code review

* Add to ref assembly, test and fix build errors

* Fix spacing, revert unwanted changes

* Namespace was wrong, updated

Co-authored-by: Jeremy Barton <jbarton@microsoft.com>
Co-authored-by: Jeff Handley <jeffhandley@users.noreply.github.com>
Co-authored-by: Buyaa Namnan <bunamnan@microsoft.com>
akoeplinger added a commit that referenced this issue Jul 10, 2020
* Add OSPlatform entries for iOS/tvOS/watchOS/Android

Implements the non-controversial new OSPlatform members from #33331

* Add tests for new OSPlatform entries
@jeffhandley
Copy link
Member

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

jkotas commented Jul 10, 2020

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

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

jkotas commented Jul 10, 2020

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

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

buyaa-n commented Jul 10, 2020

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

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

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

buyaa-n commented Jul 11, 2020

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

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

mavasani commented Jul 11, 2020

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

juliusfriedman commented Jul 11, 2020

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

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

jkotas commented Jul 11, 2020

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

@juliusfriedman
Copy link
Contributor

juliusfriedman commented Jul 11, 2020

@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()

kevinwkt pushed a commit to kevinwkt/runtimelab that referenced this issue Jul 15, 2020
* Add platform-specific attributes

Spec dotnet/runtime#33331

* Convert to xml doc

* Update src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/MinimumOSAttribute.cs

Co-authored-by: Jeremy Barton <jbarton@microsoft.com>

* Update src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/ObsoletedInPlatformAttribute.cs

Co-authored-by: Jeff Handley <jeffhandley@users.noreply.github.com>

* Address code review

* Add to ref assembly, test and fix build errors

* Fix spacing, revert unwanted changes

* Namespace was wrong, updated

Co-authored-by: Jeremy Barton <jbarton@microsoft.com>
Co-authored-by: Jeff Handley <jeffhandley@users.noreply.github.com>
Co-authored-by: Buyaa Namnan <bunamnan@microsoft.com>
@ghost ghost locked as resolved and limited conversation to collaborators Dec 10, 2020
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-System.Runtime
Projects
None yet
Development

No branches or pull requests