Allow using SupportedOSPlatformAttribute on interfaces

[spouliot]

Background and Motivation

This is a slight addition to the existing [SupportedOSPlatform] attribute type.

For Xamarin (iOS, tvOS, macOS and Catalyst) we expose Objective-C protocols as .net interfaces. However the current implementation of SupportedOSPlatformAttribute [1] does not let us annotate interfaces.

Apple does include versioning information for their protocols and the current version of Xamarin SDK do support this feature (with our own attributes).

The lack of support for annotating interface is blocking the completion of xamarin/xamarin-macios#10170

[1] https://docs.microsoft.com/en-us/dotnet/api/system.runtime.versioning.supportedosplatformattribute?view=net-5.0

Proposed API

Add AttributeTargets.Interface to SupportedOSPlatformAttribute

[System.AttributeUsage(System.AttributeTargets.Assembly | System.AttributeTargets.Class |
System.AttributeTargets.Constructor | System.AttributeTargets.Enum | System.AttributeTargets.Event |
System.AttributeTargets.Field | System.AttributeTargets.Method | System.AttributeTargets.Module |
+ System.AttributeTargets.Interface |
System.AttributeTargets.Property | System.AttributeTargets.Struct, AllowMultiple=true, Inherited=false)]
public sealed class SupportedOSPlatformAttribute : System.Runtime.Versioning.OSPlatformAttribute

Usage Examples

This is how an existing generated interface for a protocol looks like:

[Introduced (PlatformName.WatchOS, 6,0, ObjCRuntime.PlatformArchitecture.All)]
[Introduced (PlatformName.iOS, 7,0, ObjCRuntime.PlatformArchitecture.All)]
[Protocol (Name = "AVPlayerItemOutputPushDelegate", WrapperType = typeof (AVPlayerItemOutputPushDelegateWrapper))]
[ProtocolMember (IsRequired = false, IsProperty = false, IsStatic = false, Name = "OutputSequenceWasFlushed", Selector = "outputSequenceWasFlushed:", ParameterType = new Type [] { typeof (AVPlayerItemOutput) }, ParameterByRef = new bool [] { false })]
public partial interface IAVPlayerItemOutputPushDelegate : INativeObject, IDisposable
{
}

And we would like to be able to do this for .net6

[SupportedOSPlatform ("watchos6.0")]
[SupportedOSPlatform ("ios7.0")]
[Protocol (Name = "AVPlayerItemOutputPushDelegate", WrapperType = typeof (AVPlayerItemOutputPushDelegateWrapper))]
[ProtocolMember (IsRequired = false, IsProperty = false, IsStatic = false, Name = "OutputSequenceWasFlushed", Selector = "outputSequenceWasFlushed:", ParameterType = new Type [] { typeof (AVPlayerItemOutput) }, ParameterByRef = new bool [] { false })]
public partial interface IAVPlayerItemOutputPushDelegate : INativeObject, IDisposable
{
}

Alternative Designs

Annotating only interface members is possible but:

• this requires changes to the generator;
• there are thousands of extra annotations that will increase the platform assemblies, affecting build and deploy times;

Risks

The existing analyzers might need to consider interfaces (if they do not scan them already).

[dotnet-issue-labeler]

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

[spouliot]

c.c. @terrajobst - it looks like we missed the lack of interface while reviewing https://github.com/dotnet/designs/blob/main/accepted/2020/platform-checks/platform-checks.md#attributes

[danmoseley]

Cc @jeffhandley

[jeffhandley]

This came up in the API review for the attributes during 5.0; we decided at that time to intentionally exclude interfaces. We concluded that there's nothing about an interface that is truly platform-specific, but instead it's the implementations of the interface that would be platform-specific. It sounds like you have a scenario that might disprove that assertion.

Is it correct that no classes actually implement IAVPlayerItemOutputPushDelegate and the consuming code only ever references the interface?

/cc @buyaa-n

[spouliot]

there's nothing about an interface that is truly platform-specific

Agreed. I understand that, from a .net perspective, its neither platform specific nor does it need availability attributes.

But ObjC protocols are not quite interfaces but they are the closest match there is in .net.

Is it correct that no classes actually implement IAVPlayerItemOutputPushDelegate and the consuming code only ever references the interface?

Yes, but IAVPlayerItemOutputPushDelegate was a bad example. I picked it up because it was on top of the alphabetical order :-)

Protocols are used for different things. When they ends with name suffixes like Delegate then it's largely because they are to be provided for by developer. In such cases the Xamarin SDK do provide concrete types to simplify their uses (and those would have the version attributes).

Other protocols are not meant to be user-created. In such cases you consume them thru the interface. Most of the time you can guess the availability thru the property/method that expose the interface. But since ObjC is weakly-typed some API support more than one protocol type for a single API (generally the version information is the key to provide the correct one).

Finally there's the assembly bloating issue. Our logic is to annotate types (classes, interfaces, struct and enums) so that members inherit this information (and do not each require additional attributes). Only the difference (a newer or obsoleted/deprecated member) requires extra annotations.

[terrajobst]

What @jeffhandley said. We disabled it on interfaces to communicate/shape the intent. However, I've got no problem with relaxing that if that's useful for iOS.

[jeffhandley]

I've got no problem with relaxing that if that's useful for iOS.

Me either.

@buyaa-n -- will the Platform Compatibility Analyzer will work with interfaces as implemented, or will changes be required in the analyzer to handle platform attributes on them?

[buyaa-n]

will the Platform Compatibility Analyzer will work with interfaces as implemented, or will changes be required in the analyzer to handle platform attributes on them

I don't think there is any change required, but interface attributes would only work for the interface references, not for derived types inherited from that interface

Finally there's the assembly bloating issue. Our logic is to annotate types (classes, interfaces, struct and enums) so that members inherit this information (and do not each require additional attributes). Only the difference (a newer or obsoleted/deprecated member) requires extra annotations.

The attributes are not inherited,

runtime/src/libraries/System.Private.CoreLib/src/System/Runtime/Versioning/PlatformAttributes.cs

Line 63 in 1b1ff80

AllowMultiple = true, Inherited = false)]

so in this case, you will still need to add additional attributes for each required members inherited from the attributed types

[terrajobst]

@spouliot

Finally there's the assembly bloating issue. Our logic is to annotate types (classes, interfaces, struct and enums) so that members inherit this information (and do not each require additional attributes). Only the difference (a newer or obsoleted/deprecated member) requires extra annotations.

Are we sure there is significant bloat? Attribute blobs are interned and shared between applications.

[spouliot]

so in this case, you will still need to add additional attributes for each required members inherited from the attributed types

right :-( this differs even more from our current attributes and tooling...

Are we sure there is significant bloat?

It made a difference in our current attributes, but those are also using string messages. I'm doing more measurements using the new, message-less, attributes...

Attribute blobs are interned and shared between applications.

applications ? you mean it exists once in metadata and each type/member can reference the same entry (when identical) right ?

[terrajobst]

Attribute blobs are interned and shared between applications.

applications ? you mean it exists once in metadata and each type/member can reference the same entry (when identical) right ?

Yes. The attribute applications point to an entry in the blob table that contains the data passed to it. So long the values are the same between applications even strings should be fine because you only pay for them once.

[spouliot]

The attribute applications

I think we have different definition of application :-)

An early check seems to add about 40kb to the assembly (not everything is updated). Considering the 1000s of bindings that's likely just metadata entries pointing to the same (limited set of) data and not real duplication.

[buyaa-n]

@buyaa-n -- will the Platform Compatibility Analyzer will work with interfaces as implemented, or will changes be required in the analyzer to handle platform attributes on them?

I don't think there is any change required, but interface attributes would only work for the interface references, not for derived types inherited from that interface

I have tested by adding AttributeTargets.Interface target to the SupportedOSPlatform and UnsupportedOSPlatform attributes, here is how the analyzer works with interfaces and its children:

    [SupportedOSPlatform("linux")]
    [SupportedOSPlatform("windows")]
    [UnsupportedOSPlatform("windows10.0")]
    public interface IWindowsOnly
    {
        void Derived1();
        void Derived2();
    }

    public class Child : IWindowsOnly
    {
        public void Derived1() { }

        [UnsupportedOSPlatform("linux")]
        public void Derived2() { }

        public void NonDerived() { }
    }

    public class Test
    {
        void Callsite()
        {
            IWindowsOnly test = new Child();
            test.Derived1(); // 'IWindowsOnly.Derived1()' is only supported on 'linux', 'windows' 10.0 and before.
            test.Derived2(); // 'IWindowsOnly.Derived2()' is only supported on 'linux', 'windows' 10.0 and before.

            var child = new Child();
            child.Derived1(); // no warning
            child.Derived2(); // 'Child.Derived2()' is unsupported on 'linux'. (only warn for the attribute within Child)
            child.NonDerived(); // no warning
        }
    }

[jeffhandley]

Thanks, @buyaa-n. I assume no changes were necessary in the analyzer itself to achieve that result; let me know if that is not correct.

@spouliot, would this behavior of the analyzer match your expectations?

@terrajobst, do you have concerns about misleading users (to think attributes are inherited) here, or do you think we (can) cover this well enough in documentation that no form of based types/interfaces is recognized?

[buyaa-n]

Thanks, @buyaa-n. I assume no changes were necessary in the analyzer itself to achieve that result; let me know if that is not correct

That is correct

[spouliot]

@spouliot, would this behavior of the analyzer match your expectations?

Yes :-)

If you use a protocol (interface in C#) provided by the OS then you'll have the warnings. If you implement the interface then it's up to you (the implementer) to add the attributes if/when required.

Thanks @buyaa-n !

[terrajobst]

Video

- We should also add it to `UnsupportedOSPlatformAttribute`

• Our analyzer should probably warn when types implement the interface but aren't marked as platform-specific for at least the version that the interface is constrained to.
  • If types don't want that, they can suppress the warning and implement the interface methods with guards

namespace System.Runtime.Versioning
{
    [AttributeUsage(Assembly | Class | Constructor | Enum | Event | Field | Method | Module |
                    Interface /* new */ |
                    Property | Struct, AllowMultiple=true, Inherited=false)]
    public partial class SupportedOSPlatformAttribute
    {
    }

    [AttributeUsage(Assembly | Class | Constructor | Enum | Event | Field | Method | Module |
                    Interface /* new */ |
                    Property | Struct, AllowMultiple=true, Inherited=false)]
    public partial class UnsupportedOSPlatformAttribute 
    {
    }
}

[buyaa-n]

• Our analyzer should probably warn when types implement the interface but aren't marked as platform-specific for at least the version that the interface is constrained to.
  • If types don't want that, they can suppress the warning and implement the interface methods with guards

That is probably what we should do instead of: Consider flag platform specific constructor called implicitly in derived types