dotnet / maui

.NET MAUI is the .NET Multi-platform App UI, a framework for building native device applications spanning mobile, tablet, and desktop.
https://dot.net/maui
MIT License
22.07k stars 1.73k forks source link

OperatingSystem.IsIOSVersionAtLeast() iOS/MacCatalyst mismatch makes it impossible to guard some features #21106

Closed bzd3y closed 7 months ago

bzd3y commented 7 months ago

Description

This might not be a bug in Maui itself and instead belong in some other dotnet repository, but I wasn't sure where it should go.   Further, I'm not sure if this is an issue with how some of the classes/methods are annotated with the SupportedOSPlatform or if the problem is with the analyzer or whatever is processing the guard conditions and the SupportedOSPlatforms on a class/method.

An example is a method like PHImageManager.RequestImageDataAndOrientation(). Using it unguarded will produce the following warning. This warning is correct for iOS, but not for Mac Catalyst. At the very least the warning message is wrong.

This call site is reachable on: 'iOS' 11.0 and later, 'maccatalyst' 13.0 and later. 'PHImageManager.RequestImageDataAndOrientation(PHAsset, PHImageRequestOptions?, PHImageManagerRequestImageDataHandler)' is only supported on: 'ios' 13.0 and later.

Adding a guard like if (OperatingSystem.IsIOSVersionAtLeast(13)) (or it seems anything else, even checking for Mac Catalyst) does not remove the warning.

This method replaces the deprecated method RequestImageData(), which is annotated with:

        [ObsoletedOSPlatform("ios13.0", "Use 'RequestImageDataAndOrientation (PHAsset asset, [NullAllowed] PHImageRequestOptions options, PHImageManagerRequestImageDataHandler resultHandler)' instead.")]
        [ObsoletedOSPlatform("tvos13.0", "Use 'RequestImageDataAndOrientation (PHAsset asset, [NullAllowed] PHImageRequestOptions options, PHImageManagerRequestImageDataHandler resultHandler)' instead.")]
        [ObsoletedOSPlatform("maccatalyst13.1", "Use 'RequestImageDataAndOrientation (PHAsset asset, [NullAllowed] PHImageRequestOptions options, PHImageManagerRequestImageDataHandler resultHandler)' instead.")]
        [SupportedOSPlatform("maccatalyst")]
        [SupportedOSPlatform("ios")]
        [SupportedOSPlatform("tvos")]
        [UnsupportedOSPlatform("macos")]

The supported method RequestImageDataAndOrientation() is annotated with:

        [SupportedOSPlatform("tvos13.0")]
        [SupportedOSPlatform("ios13.0")]
        [SupportedOSPlatform("maccatalyst")]
        [SupportedOSPlatform("macos")]

OperatingSystem.IsIOSVersionAtLeast() is annotated with (I don't know if this matters):

        /// <summary>
        /// Check for the iOS/MacCatalyst version (returned by 'libobjc.get_operatingSystemVersion') with a >= version comparison. Used to guard APIs that were added in the given iOS release.
        /// </summary>
        [SupportedOSPlatformGuard("maccatalyst")]

So it looks like the cause of the problem might be anything annotated with [SupportedOSPlatform("maccatalyst")] without an explicit version.

The entire BGTaskScheduler class is another example of this.

Maybe slightly unrelated, but the annotations for RequestImageData() also just seems to be wrong. It is Obsoleted on "maccatalyst13.1", but Apple's documentation actually says that it is deprecated for versions of everything 13.0+ except MacCatalyst 13.1+ (I have no idea if their documentation is correct, though). So it looks like removing the [ObsoletedOSPlatform("maccatalyst13.1"...] and changing the "maccatalyst" Supported to [SupportedOSPlatform("maccatalyst13.1")] might fix the warning. But I have no idea if that will correctly fix the runtime operation of IsIOSVersionAtLeast().

Similarly, the BGTaskScheduler documentation says that it is supported on iOS 13.0+, but Mac Catalyst 13.1+.

It appears that when the "maccatalyst" version isn't explicitly indicated that it is inferred from the "ios" version. I don't know if that is generally correct/valid. Probably not, since it looks like the earliest that MacCatalyst was aligned with iOS was Mac Catalyst 13.1 which might align with iOS 13.0.

So either way, the Mac Catalyst versions for certain classes/methods seem like they needs to explicitly indicate "maccatalyst13.1".

Or maybe "maccatalyst" should infer "maccatalyst13.1" from "ios13.0"? But I also don't know if that is valid.

To me it seems that the ideal fix would be for the OperatingSystem.IsIOS*() methods to not look at "maccatalyst" at all (or to explicitly indicate to do it or not to do it with an overload/optional bool parameter). That would (I think) fix this problem but also allow developers to have finer control over guarding features between the two platforms and/or the mobile or desktop idioms.

But maybe that is a breaking change? If so, then the method could have an optional parameter or an overload that defaults to true so that existing code would still work how it worked before (which I believe is wrong anyway, at least for anything "maccatalyst13.0") but moving forward developers would have more control.

As an example of why this seems broken either way: BGTaskScheduler guarded with IsIOSVersionAtLeast(13) would get run on "macctalyst13.0" even though it is supposedly isn't supported.

So even though this is a breaking change, because the iOS versions and Mac Catalyst versions don't line up, I'm not sure it can really be avoided.

Steps to Reproduce

  1. Clone the reproduction repository.
  2. Try to build the OSVersion project.
  3. Look at the Error List window and there should be 3 warnings.

Link to public reproduction project repository

https://github.com/bzd3y/ReproductionProject

Version with bug

8.0.7 SR2

Is this a regression from previous behavior?

Not sure, did not test other versions

Last version that worked well

Unknown/Other

Affected platforms

iOS

Affected platform versions

iOS 13.0, MacCatalyst 13.0

Did you find any workaround?

Sort of. The warnings can of course be suppressed, but I'm not sure that IsIOS*() will work at runtime.

They can also be "hidden" by adding something like [SupportedOSPlatform("maccatalyst13.1")] to my method or class. But I also don't think that affects the runtime operation.

Relevant log output

No response

drasticactions commented 7 months ago

Description

This might not be a bug in Maui itself and instead belong in some other dotnet repository, but I wasn't sure where it should go.  

https://github.com/xamarin/xamarin-macios or https://github.com/dotnet/runtime, this repo doesn't control that tooling and warnings. @rolfbjarne What do you think?

bzd3y commented 7 months ago

@drasticactions @rolfbjarne Yeah, sorry. In my opinion the problem probably is in Microsoft.iOS.dll (and maybe related libraries), so wherever that is. I guess that is xamarin-macios? I hadn't looked into how all this was working and my brain has moved past anything "Xamarin", which wasn't a great assumption.

Another example that might help is the enum UIKit.UIDatePickerStyle. From Apple's documentation it looks like it is iOS 13.4 and MacCatalyst 13.4 and it is annotated with:

    [SupportedOSPlatform("ios13.4")]
    [SupportedOSPlatform("maccatalyst")]
    [UnsupportedOSPlatform("tvos")]

So I get a warning for UIDatePickerStyle.Wheel, like the other things I have mentioned. When I saw that the entire enum is annotated that way, I realized that I am not getting a warning for the property I am setting itself, UIDatePicker.PreferredDatePickerStyle. (I'm not even sure that I need this code in Maui...)

That is annotated with:

    [SupportedOSPlatform("maccatalyst13.4")]
    [SupportedOSPlatform("ios13.4")]
    [UnsupportedOSPlatform("tvos")]

With an explicit Mac Catalyst version. So it does seem that giving these things an explicit Mac Catalyst version might be the solution.

Something to maybe note is that per Apple's documentation the iOS and Mac Catalyst versions do align in this case, so there is no inferring the version from "ios". I realize now that I just assumed that and entirely made that concept up.

But that makes me think that it really is just a matter of any time there is an explicit "ios" version annotation, the "maccatalyst" annotation needs to be explicit as well, which, again, seems like it would be in something like Microsoft.iOS.dll.

rolfbjarne commented 7 months ago

So it looks like the cause of the problem might be anything annotated with [SupportedOSPlatform("maccatalyst")] without an explicit version.

That's correct, the attribute without an explicit version is supposed to mean "all versions".

I've filed a bug with the analyzer for this issue here: https://github.com/dotnet/roslyn-analyzers/issues/7239

Maybe slightly unrelated, but the annotations for RequestImageData() also just seems to be wrong. It is Obsoleted on "maccatalyst13.1", but Apple's documentation actually says that it is deprecated for versions of everything 13.0+ except MacCatalyst 13.1+ (I have no idea if their documentation is correct, though).

That's most likely because someone at Apple forgot to list the API as deprecated on Mac Catalyst. This happens a lot in their APIs: they deprecate some APIs on some, but not all platforms (in particular on platforms that were introduced after the API in question - for instance Mac Catalyst came out years after this particular photo API). In most cases this seems to be a mistake unless it's obviously correct, so we're usually deprecating all platforms in these cases.

Similarly, the BGTaskScheduler documentation says that it is supported on iOS 13.0+, but Mac Catalyst 13.1+.

Mac Catalyst 13.0 was a weird version, and we don't support targeting it, which is why most of our annotations say Mac Catalyst 13.1+ (not quite correct in that the API might be available on that version of Mac Catalyst, but you won't be able to run your app on that Mac Catalyst version because we don't support it).

Sort of. The warnings can of course be suppressed, but I'm not sure that IsIOS*() will work at runtime.

This is a problem with the warning, not the code, so the code will run as expected.

Note that IsIOSVersionAtLeast is weird, in that it works like "IsMacCatalystVersionAtLeast" when executing on Mac Catalyst.

jfversluis commented 7 months ago

Hope that clears it up and for the analyzer a bug has been logged to follow up. Thanks for bringing this to our attention.

Since there is nothing more to do on the .NET MAUI side for this, closing it here.

bzd3y commented 7 months ago

Great. It's good to know that the runtime methods should work. Thanks everybody.