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

[API Proposal]: Add back ObsoletedInOSPlatformAttribute for .NET 7 #68557

Closed
Tracked by #14548
rolfbjarne opened this issue Apr 26, 2022 · 13 comments · Fixed by #71846
Closed
Tracked by #14548

[API Proposal]: Add back ObsoletedInOSPlatformAttribute for .NET 7 #68557

rolfbjarne opened this issue Apr 26, 2022 · 13 comments · Fixed by #71846
Labels
api-approved API was approved in API review, it can be implemented area-System.Runtime
Milestone

Comments

@rolfbjarne
Copy link
Member

rolfbjarne commented Apr 26, 2022

Background and Motivation

[This is pretty much a copy of #47601, with an additional aggravating scenario we've run into]

API Proposal

Original specs

This was removed in 5.0 RC: dotnet/docs#20635

ObsoletedInOSPlatformAttribute was removed, as this annotation is not needed at this time

With the addition of mobile platforms we think the need/time has come :-)

API Usage

Apple commonly obsoletes API and provide better alternatives for them. The headers (both C and Objective-C) includes macros that help developers (and the Xcode IDE) select the best API for the OS version being targeted.

The legacy Xamarin SDKs (iOS/Mac) annotate the API with the similar attributes. They are:

  1. [Introduced]: specifies in which OS version an API was introduced. This maps perfectly to [SupportedOSPlatform ("ios10.0")]
  2. [Obsoleted]: Apple will often make API obsolete, and sometimes tell the user which API they're to use instead. This might map to [UnsupportedOSPlatform ("ios15.0")], but there's no way to specify a custom message.
  3. [Deprecated]: Some time after an API becomes obsolete, they might become deprecated (which generally means they will be rejected if submitted to one of the Apple App Stores). In this case Apple might also give a message telling the user the better API to use. This might map to [UnsupportedOSPlatform ("ios15.0")], but there's no way to map a custom message.
  4. [Unavailable]: specifies an API which is not available anymore (and which we can't remove because it would be a breaking change). This maps perfectly to [UnsupportedOSPlatform ("ios")]

The problematic attributes are [Obsoleted] and [Deprecated], because the [UnsupportedOSPlatform] attribute doesn't let us give the user a custom message (such as telling them what to do instead).

We've opted for using the [Obsolete] attribute instead (as was suggested in #47601): [Obsolete ("Starting with iOS 15.0, use XYZ instead")].

The problem is the following scenario:

  1. Developer needs to keep their app working an older iOS version, say iOS 14.
  2. Apple obsoleted some API in iOS 15, providing an alternative.

The only way to make the build warning-free, is by doing something like this:

#pragma warning disable 618
    CalliOS14API ();
#pragma warning restore 618

adding a version check won't change anything:

    if (checkForiOS15) {
        CalliOS15API ();
    } else {
#pragma warning disable 618
        CalliOS14API ();
#pragma warning restore 618
    }

I believe that adding back the [ObsoletedInOSPlatform] (which had a Message property) would allow us to improve this, so that app developers won't get warning about deprecated code that will never run on platforms where the API is deprecated due to version checks.

Ref: xamarin/xamarin-macios#10170

For reference, we got this design from how Apple/clang declares it: https://clang.llvm.org/docs/AttributeReference.html#availability

Attribute Explanation
introduced=version The first version in which this declaration was introduced.
deprecated=version The first version in which this declaration was deprecated, meaning that users should migrate away from this API.
 obsoleted=version  The first version in which this declaration was obsoleted, meaning that it was removed completely and can no longer be used. 
 unavailable This declaration is never available on this platform.

Alternative Designs

Alternatively it might be possible to add a Message property to the [UnsupportedOSPlatform] attribute.

Risks

The lack of a solution means that users will have a hard time making their build warning-free.

@rolfbjarne rolfbjarne added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Apr 26, 2022
@dotnet-issue-labeler
Copy link

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.

@ghost
Copy link

ghost commented Apr 26, 2022

Tagging subscribers to this area: @dotnet/area-system-runtime
See info in area-owners.md if you want to be subscribed.

Issue Details

Background and Motivation

[This is pretty much a copy of #47601, with an additional aggravating scenario we've run into]

API Proposal

Original specs

This was removed in 5.0 RC: dotnet/docs#20635

ObsoletedInOSPlatformAttribute was removed, as this annotation is not needed at this time

With the addition of mobile platforms we think the need/time has come :-)

API Usage

Apple commonly obsoletes API and provide better alternatives for them. The headers (both C and Objective-C) includes macros that help developers (and the Xcode IDE) select the best API for the OS version being targeted.

The legacy Xamarin SDKs (iOS/Mac) annotate the API with the similar attributes. They are:

  1. [Introduced]: specifies in which OS version an API was introduced. This maps perfectly to [SupportedOSPlatform ("ios10.0")]
  2. [Obsoleted]: Apple will often make API obsolete, and sometimes tell the user which API they're to use instead. This might map to [UnsupportedOSPlatform ("ios15.0")], but there's no way to specify a custom message.
  3. [Deprecated]: Some time after an API becomes obsolete, they might become deprecated (which generally means they will be rejected if submitted to one of the Apple App Stores). In this case Apple might also give a message telling the user the better API to use. This might map to [UnsupportedOSPlatform ("ios15.0")], but there's no way to map a custom message.
  4. [Unavailable]: specifies an API which is not available anymore (and which we can't remove because it would be a breaking change). This maps perfectly to [UnsupportedOSPlatform ("ios")]

The problematic attributes are [Obsoleted] and [Deprecated], because the [UnsupportedOSPlatform] attribute doesn't let us give the user a custom message (such as telling them what to do instead).

We've opted for using the [Obsolete] attribute instead (as was suggested in #47601): [Obsolete ("Starting with iOS 15.0, use XYZ instead")].

The problem is the following scenario:

  1. Developer needs to keep their app working an older iOS version, say iOS 14.
  2. Apple obsoleted some API in iOS 15, providing an alternative.

The only way to make the build warning-free, is by doing something like this:

#pragma warning disable 618
    CalliOS14API ();
#pragma warning restore 618

adding a version check won't change anything:

    if (checkForiOS15) {
        CalliOS15API ();
    } else {
#pragma warning disable 618
        CalliOS14API ();
#pragma warning restore 618
    }

I believe that adding back the [ObsoletedInOSPlatform] (which had a Message property) would allow us to improve this, so that app developers won't get warning about deprecated code that will never run on platforms where the API is deprecated due to version checks.

Ref: xamarin/xamarin-macios#10170

Alternative Designs

Alternatively it might be possible to add a Message property to the [UnsupportedOSPlatform] attribute.

Risks

The lack of a solution means that users will have a hard time making their build warning-free.

Author: rolfbjarne
Assignees: -
Labels:

api-suggestion, area-System.Runtime, untriaged

Milestone: -

@AaronRobinsonMSFT
Copy link
Member

@rolfbjarne Seems like this has some impact on macios in .NET 7. I'm not seeing any strong feedback either way here. Would you like me to mark this as blocking for .NET 7 so we can get traction on the API review front?

@dalexsoto
Copy link
Member

@AaronRobinsonMSFT please do, we would love to get this review and discussed and of course if stars align make it happen! Thank you!

@AaronRobinsonMSFT AaronRobinsonMSFT added blocking Marks issues that we want to fast track in order to unblock other important work api-ready-for-review API is ready for review, it is NOT ready for implementation and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation untriaged New issue has not been triaged by the area owner labels May 12, 2022
@AaronRobinsonMSFT AaronRobinsonMSFT added this to the 7.0.0 milestone May 12, 2022
@danmoseley
Copy link
Member

@buyaa-n

@buyaa-n
Copy link
Member

buyaa-n commented May 13, 2022

Alternatively it might be possible to add a Message property to the [UnsupportedOSPlatform] attribute.

If we use same attribute for all how do we know/distinguish if it was obsoleted or deprecated or unavailable? Message should contain all that info?

Also tagging @terrajobst @jeffhandley

@rolfbjarne
Copy link
Member Author

Alternatively it might be possible to add a Message property to the [UnsupportedOSPlatform] attribute.

If we use same attribute for all how do we know/distinguish if it was obsoleted or deprecated or unavailable? Message should contain all that info?

The mapping would be as follows:

  • "Obsoleted" and "Deprecated": [UnsupportedOSPlatform ("osWithVersion")] (say [UnsupportedOSPlatform ("ios14.0", "Use Api.XYZ instead")]
  • "Unavailable": [UnsupportedOSPlatform ("osWithoutVersion")] (for instance [UnsupportedOSPlatform ("ios", "Use Api.XYZ instead")])

So no, we wouldn't be able to distinguish between "Obsoleted" and "Deprecated", but it solves part of the problem: it lets us tell the user which API to use instead (or whatever else we want to tell them), and the user would be able to fix the warning by adding a version check to their code (which I think is the bigger pain point).

That said, I would still like an [ObsoletedInOSPlatform], for a couple of reasons:

  • It lets us distinguish between "Obsoleted" and "Deprecated".
  • Using the word Unsupported is not accurate for these API: the API works, it's supported, but it's just recommended (by Apple) to not use it. In fact iOS has APIs that were already deprecated from the very first iOS release, and they're still around.

@terrajobst
Copy link
Member

terrajobst commented May 16, 2022

It sounds like Apple's definition of obsoleted is "treat it is as removed as we don't accept new submission to the app store using it".

If that's the case, then it seems like we'd like this mapping:

Attribute Mapping
introduced=version [SupportedOSPlatform(version)]
deprecated=version,message [ObsoletedInOSPlatform(version, Message=message)]
obsoleted=version,message  [UnsupportedOSPlatform(version, Message=message)]
unavailable [UnsupportedOSPlatform(platform)]

Did I get that right? Assuming yes, then we should add an optional Message property to UnsupportedOSPlatformAttribute too:

namespace System.Runtime.Versioning;

public partial class UnsupportedOSPlatformAttribute : OSPlatformAttribute
{
    public string Message { get; set; }
}

[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; }
}

@rolfbjarne
Copy link
Member Author

@terrajobst yes, that's correct. It's a bit unfortunate that there are mismatched definitions of the word "Obsolete" (it appears in different rows in the table), although I'm not sure what we can do about it. One idea would be to name our attribute DeprecatedInOSPlatform instead.

@bartonjs
Copy link
Member

bartonjs commented May 19, 2022

Video

Generally looks good as proposed, except

  • we synchronized the AttributeTargets with what we have on SupportedOSPlatformAttribute.
  • we made UnsupportedOSPlatformAttribute.Message be get-only with a new ctor.
namespace System.Runtime.Versioning
{
    public partial class UnsupportedOSPlatformAttribute : OSPlatformAttribute
    {
        public UnsupportedOSPlatformAttribute(string platformName, string? message);

        public string? Message { get; }
    }

    [AttributeUsage(AttributeTargets.Assembly |
                    AttributeTargets.Class |
                    AttributeTargets.Constructor |
                    AttributeTargets.Enum |
                    AttributeTargets.Event |
                    AttributeTargets.Field |
                    AttributeTargets.Interface |
                    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; }
    }
}

@bartonjs bartonjs added api-approved API was approved in API review, it can be implemented and removed blocking Marks issues that we want to fast track in order to unblock other important work api-ready-for-review API is ready for review, it is NOT ready for implementation labels May 19, 2022
@dalexsoto
Copy link
Member

Hello @bartonjs / @terrajobst

Just a small ping to see if it has been any progress on this? I don't want it to be forgotten :) plus we have to do some work on our end once this is implemented.

Thank you!

@jeffhandley
Copy link
Member

Thanks for the ping on this @dalexsoto. @buyaa-n, @terrajobst, and I have reached out to @rolfbjarne to discuss some details on moving this forward.

@jeffhandley
Copy link
Member

@rolfbjarne will target Preview 7 for the attribute. @buyaa-n will target RC1 for the analyzer. The attribute will be applied to the iOS APIs during RC1 as well (likely done by @chamons).

rolfbjarne added a commit to rolfbjarne/runtime that referenced this issue Jul 8, 2022
rolfbjarne added a commit to rolfbjarne/runtime that referenced this issue Jul 8, 2022
As described here:

dotnet#68557 (comment)

@buyaa-n will implement the changes required in the analyzer (target is RC1).

Fixes dotnet#68557.
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Jul 8, 2022
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Jul 8, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Aug 8, 2022
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

Successfully merging a pull request may close this issue.

9 participants