Repository navigation
Guarding calls to platform-specific APIs #33331
Description
Activity
I feel like the method name needs something about the versioning. Like
IsOSPlatformVersionOrNewer; orIsMinimumOSPlatformVersion. Because, as written, I could see people writing it thinking it's exact, and where they should have lightup they have fallback.Reacted by Levi Broderick, pinkfloydx33, Ganbarukamo41, Dan Moseley and Alexander Köplinger- Reacted by Immo Landwerth
Could the two overloads be replaced by a single method taking a
System.Versionargument? e.g.public static bool IsOSVersionAtLeast(OSPlatform osPlatform, Version minimumVersion);
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
Reacted by Sebastien PouliotWhat 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.
Will this API work correctly in Windows too?
Reacted by David Shulman51 remaining items
Cache the result of the "is current platform check?" in the
OSPlatformconstructor in abool? TheIsOSPlatformchecks that we care about can then just check this bool without doing any expensive string comparisons, etc.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; } }
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)
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() { }
I really think a set of
PlatformSupportAttributesdervived fromPlatformSupportAttributeis better than a single attribute here,That array given to a single
PlatformSupportAttributeas I indicated above and it solves more problems than this does today.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 thatCurrently 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.
Reacted by Buyaa NamnanNote that the analysis understands Debug asserts. I would personally recommend we take route similar to dataflow analysis for C# nullable reference types:
- Default analysis to scope of the current method. No interprocedural analysis by default.
- 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.
- 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:
- Enable interprocedural analysis, so you pay build time cost at the benefit of not having to add asserts or attribute annotations.
- Add debug asserts and debug only attributes for making analysis faster, at the expense of adding and maintaining annotations.
Reacted by Jeff Handley@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
CPUSupportetc very easily while proposed implementation cannot.Please reconsider.
@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.
Reacted by Julius R Friedman@juliusfriedman You suggestion does not compile as written.
System.Versionand arrays of non-primitive types are not valid fields in custom attributes.@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()- added a commit that references this issue
on Jul 15, 2020 - ghost locked as resolved and limited conversation to collaborators
on Dec 10, 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.OSVersionin favor ofRuntimeInformation.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
This design allows us to encapsulate the version comparison, i.e. the "and later" part.
Usage: Recording Project Properties
The SDK already generates a file called
AssemblyInfo.cswhich 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):Usage: Guarding Platform-Specific APIs
NSFizzBuzzis 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 usingIsOSPlatformOrLater:Usage: Declaring Platform-Specific APIs
The
RemovedInPlatformAttributeandObsoletedInPlatformAttributewill primarily be used by the OS bindings to indicatewhether a given API shouldn't be used any more.
The
MinimumPlatformAttributewill be for two things: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
NSFizzBuzzAPI to be complex. User code might want to encapsulate it's usage in a helper type:As far as the analyzer is concerned,
NSFizzBuzzHelpercan 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 onNSFizzBuzzHelper.@dotnet/fxdc @mhutch