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

Add back ObsoletedInOSPlatformAttribute for net6 #47601

Closed
spouliot opened this issue Jan 28, 2021 · 14 comments
Closed

Add back ObsoletedInOSPlatformAttribute for net6 #47601

spouliot opened this issue Jan 28, 2021 · 14 comments
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Runtime
Milestone

Comments

@spouliot
Copy link
Contributor

spouliot commented Jan 28, 2021

Background and Motivation

Proposed API

Original specs

This was removed in 5.0 RC

dotnet/docs#20635 mention

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

With the addition of Xamarin SDK I think the need/time has come :-)

Usage Examples

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 existing Xamarin SDKs (iOS/Mac) annotate the API with the similar attributes. This allow developers to identify which API are obsolete, well before they become deprecated (which generally means they will be rejected if submitted to one of the Apple App Stores).

Current Dotnet (planned)
[Introduced (PlatformName.iOS, 7,0)] [SupportedOSPlatform ("ios7.0")]
[Obsoleted (PlatformName.iOS, 12,1)] [ObsoletedInOSPlatform ("ios12.1")]
[Deprecated (PlatformName.iOS, 14,3)] [UnsupportedOSPlatform ("ios14.3")]
[Unavailable (PlatformName.iOS)] [UnsupportedOSPlatform ("ios")]

The removal of ObsoletedInOSPlatform is blocking the completion of xamarin/xamarin-macios#10170

Alternative Designs

None

Risks

The lack of this attribute means the analyzer can't have feature parity (regression) compared to the existing features available on VS.

@spouliot spouliot added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Jan 28, 2021
@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.

@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged New issue has not been triaged by the area owner label Jan 28, 2021
@spouliot
Copy link
Contributor Author

c.c. @terrajobst

spouliot added a commit to spouliot/xamarin-macios that referenced this issue Feb 4, 2021
This moves our current/legacy attributes to the ones added in dotnet 5 [1].

Short Forms (only in bindings)

| Old                                   | New                                 |
|---------------------------------------|-------------------------------------|
| [iOS (7,0)]                           | [SupportedOSPlatform ("ios7.0")]    |
| [NoIOS]                               | [UnsupportedOSPlatform ("ios")]     |

Long Forms

| Old                                   | New                                 |
|---------------------------------------|-------------------------------------|
| [Introduced (PlatformName.iOS, 7,0)]  | [SupportedOSPlatform ("ios7.0")]    |
| [Obsoleted (PlatformName.iOS, 12,1)]  | [Obsolete (...)]                    |
| [Deprecated (PlatformName.iOS, 14,3)] | [UnsupportedOSPlatform ("ios14.3")] |
| [Unavailable (PlatformName.iOS)]      | [UnsupportedOSPlatform ("ios")]     |

Other changes

* `[SupportedOSPlatform]` and `[UnsupportedOSPlatform]` are not allowed on `interface` [2] which means they cannot be used for protocols. This is currently handled by inlining the existing attributes on all members.
* `[ObsoletedInOSPlatform]` was removed in net5 RC. This PR is now mapping the existing attributes to `[Obsolote]`, however multiple ones cannot be added so they need to be platform specific.

Still Missing

* [ ] Generator deduplication of type/members attributes for interfaces (due to [2])
* [ ] Manual bindings
* [ ] Introspection tests updates

References

* [1] xamarin#10170
* [2] dotnet/runtime#47599
* [3] dotnet/runtime#47601
spouliot added a commit to xamarin/xamarin-macios that referenced this issue Apr 10, 2021
…te` (#10580)

This moves our current/legacy attributes to the ones added in dotnet 5 [1].

Short Forms (only in bindings)

| Old                                   | New                                 |
|---------------------------------------|-------------------------------------|
| [iOS (7,0)]                           | [SupportedOSPlatform ("ios7.0")]    |
| [NoIOS]                               | [UnsupportedOSPlatform ("ios")]     |

Long Forms

| Old                                   | New                                 |
|---------------------------------------|-------------------------------------|
| [Introduced (PlatformName.iOS, 7,0)]  | [SupportedOSPlatform ("ios7.0")]    |
| [Obsoleted (PlatformName.iOS, 12,1)]  | [Obsolete (...)]                    |
| [Deprecated (PlatformName.iOS, 14,3)] | [UnsupportedOSPlatform ("ios14.3")] |
| [Unavailable (PlatformName.iOS)]      | [UnsupportedOSPlatform ("ios")]     |

Other changes

* `[SupportedOSPlatform]` and `[UnsupportedOSPlatform]` are not allowed on `interface` [2] which means they cannot be used for protocols. This is currently handled by inlining the existing attributes on all members.
* `[ObsoletedInOSPlatform]` was removed in net5 RC. This PR is now mapping the existing attributes to `[Obsolote]`, however multiple ones cannot be added so they need to be platform specific.

Remaining work (manual bindings update) tracked in #11055

References

* [1] #10170
* [2] dotnet/runtime#47599
* [3] dotnet/runtime#47601
@steveisok
Copy link
Member

@marek-safar Thoughts?

@steveisok steveisok removed the untriaged New issue has not been triaged by the area owner label Jun 28, 2021
@marek-safar marek-safar added the untriaged New issue has not been triaged by the area owner label Jun 29, 2021
@marek-safar
Copy link
Contributor

It has been sitting here for 6 months so this is for @jeffhandley to prioritize now.

@tannergooding tannergooding removed the untriaged New issue has not been triaged by the area owner label Jul 12, 2021
@tannergooding tannergooding added this to the Future milestone Jul 12, 2021
@jeffhandley
Copy link
Member

During .NET 6, we ended up investing our time with the Platform Compatibility Analyzer into several other stories that had higher priorities and more compelling scenarios; as a result, we were not able to evaluate this story for .NET 6.

While I understand that we don't have parity with previous functionality, I'm unconvinced that the [ObsoletedInOSPlatform] attribute adds significant value beyond [UnsupportedOSPlatformAttribute].

  1. When an API is announced as being deprecated, does the announcement typically come with the mention of which version the API becomes unsupported in? If so, would simply adding an [UnsupportedOSPlatform] attribute with the future version yield the desired experience?
  2. The analyzer's logic is already quite complex, ensuring it can handle supported/unsupported attributes at any scope and potentially with multiple version ranges, and now also with recognition of the relationship between iOS/MacCatalyst (through the guard attributes), etc. I'm reluctant to have us introduce another dimension to this where there's a relationship between supported/obsoleted/unsupported that needs to be understood and enforced.
  3. How would we suggest developers react to diagnostics from such a deprecated API? Would those deprecations need to have custom messages with them?

@terrajobst
Copy link
Member

Agreed. That being said, I think it's reasonable to assume we'll be able to provide this for .NET 7.

@jeffhandley
Copy link
Member

Before committing to it, I'd like to understand the value it provides more thoroughly. I expect it to have a large cost.

@spouliot
Copy link
Contributor Author

spouliot commented Aug 5, 2021

How would we suggest developers react to diagnostics from such a deprecated API? Would those deprecations need to have custom messages with them?

Almost :-) It's for obsoleted, not deprecated, API.

Most obsoleted API comes with replacement(s), which are part of the existing (pre-net6) attributes. E.g.

[Obsoleted (PlatformName.iOS, 10,0, message: "Replaced by 'GColorConversionInfoTriple'.")]

The current IDE tooling is showing this to developers at edit-time (not at build-time).

For net6 we're mapping obsoleted API to [Obsolete] - xamarin/xamarin-macios#10580 (comment)

This is not a perfect match since the platform is not clearly identified in the metadata. That means the message needs to be different to identify the platform/version where the obsolescence starts. Also since only a single [Obsolete] can be present on each member. This leads to code like this...

#if __IOS__
[Obsolete ("Starting with ios14.0 Use 'CKQuerySubscriptionOptions' instead.", DiagnosticId = "BI1234", UrlFormat = "https://github.com/xamarin/xamarin-macios/wiki/Obsolete")]
#endif
#if __MACOS__
[Obsolete ("Starting with macos10.16 Use 'CKQuerySubscriptionOptions' instead.", DiagnosticId = "BI1234", UrlFormat = "https://github.com/xamarin/xamarin-macios/wiki/Obsolete")]
#endif

which is not a problem for the generated code, but not very pleasant when manual bindings are needed.

It's also reported at build-time, but with the recent ObsoleteAttribute enhancement it's now possible/easy to ignore them.

Agreed. That being said, I think it's reasonable to assume we'll be able to provide this for .NET 7.

Before committing to it, I'd like to understand the value it provides more thoroughly. I expect it to have a large cost.

I'm not sure what the reaction to this change will be... it's a very old feature, there will be some muscle memory to retrain. I'm hoping people will prefer the new approach, since it has more tooling available and will be identical across all platforms. Anyway it's a bit too late to change anything so I suggest we keep our ears open for customer feedback.

@jeffhandley
Copy link
Member

Thanks, @spouliot! I'll go ahead and close this, but we can certainly reopen it if we get feedback indicating we should revisit it.

@jeffhandley jeffhandley modified the milestones: Future, 6.0.0 Aug 5, 2021
@mandel-macaque
Copy link
Member

For future references, the following is a real example of what we have to do know: https://github.com/xamarin/xamarin-macios/pull/12365/files#diff-72574ede623de97723139c5b253475575f186431bd438834f73af5052587e91aR582

As you can see, is very verbose and error prone :(

@jeffhandley
Copy link
Member

@mandel-macaque Thanks for sharing that example; that's helpful.

If we were to do [ObsoletedInOSPlatform], I see a difference in what the experience would be. That attribute would end up producing a single message that combines all of the platforms together, like [SupportedOSPlatform] and [UnsupportedOSPlatform] do today. Therefore your multiple attributes would all combine together for a single message, instead of having a different message per platform.

If you took the same approach of having a single, combined message, you'd having a single [Obsolete] attribute such as:

[Obsolete ("This API is removed starting in: maccatalyst15.0, ios15.0, tvos15.0, macos12.0. Please do not use.", DiagnosticId = "BI1234", UrlFormat = "https://github.com/xamarin/xamarin-macios/wiki/Obsolete")]

Since that's the experience you'd get through [ObsoletedInOSPlatform], would you consider combining the [Obsolete] attributes into one like that now? If so, then I think [Obsolete] and [ObsoletedInPlatform] would have equivalent ergonomics.

@ghost ghost locked as resolved and limited conversation to collaborators Sep 7, 2021
@dotnet dotnet unlocked this conversation Sep 30, 2021
@jpobst
Copy link
Contributor

jpobst commented Sep 30, 2021

Context: dotnet/android#6349

We are just noticing that we have this same issue in Xamarin - Android.

Previously we have shipped N copied of Mono.Android.dll where N is each supported Android API level, so each level you target is able to have the correct [Obsolete] attributes for that level.

Example: DevicePolicyManager.clearDeviceOwnerApp (string) was deprecated in API-26 (Android 8.0).

Thus ReferenceAssemblies\Microsoft\Framework\MonoAndroid\v5.0\Mono.Android.dll looks like this:

public virtual void ClearDeviceOwnerApp (string packageName) { ... }

while ReferenceAssemblies\Microsoft\Framework\MonoAndroid\v8.0\Mono.Android.dll looks like this:

[Obsolete ("deprecated")]
public virtual void ClearDeviceOwnerApp (string packageName) { ... }

In .NET6 we are shipping a single Mono.Android.dll, using [SupportedOSPlatform] to decorate if a method was added in a later API level. However, we do not have a way to express if a method was deprecated in a later API level.

Currently this method shows as [Obsolete] even if you are using <SupportedOSPlatformVersion>21</SupportedOSPlatformVersion>.

@ghost ghost locked as resolved and limited conversation to collaborators Nov 25, 2021
@rolfbjarne
Copy link
Member

Here's an unfortunate developer 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.

If we want to provide a good message to the user when they use the old API (telling them about the new API in iOS 15), we currently have to use the Obsolete attribute.

However, this means that the user have to do something like this to have a warning-free build:

#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 a Message property on the UnsupportedOSPlatformAttribute might make it possible to improve this (and make the Obsolete attribute obsolete 😉) - although I haven't tested how using API with [UnsupportedOSPlatform ("ios15.0")] is reported when used in code that's executed on older iOS versions.

Put another way: using [Obsolete] will always show the warning, even in cases where we know the user should be able to use it (on an older OS version).

@rolfbjarne
Copy link
Member

I've created an issue to request this API for .NET 7: #68557.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Runtime
Projects
None yet
Development

No branches or pull requests

10 participants