Restore suppressed maccatalyst for proper flow analysis

[buyaa-n]

class TestType
{
    [SupportedOSPlatform("ios13.0")]
    [SupportedOSPlatform("maccatalyst13.0")]  // THIS suppresses lower versions
    private void Tapped()
    {
        if (OperatingSystem.IsIOSVersionAtLeast(15,0))
           DoSomething(); // when it called here only [SupportedOSPlatform(""ios14.0"")] left in the list
                         // as OperatingSystem.IsIOSVersionAtLeast(15,0) applies to ios15.0 and maccatalys15.0 warns 
    }

    [SupportedOSPlatform("ios14.0")]
    [SupportedOSPlatform("maccatalyst")]   // when version less or <= 13.0 then suppressed by callsite attribute 
    public void DoSomething() {}
}

[SupportedOSPlatform("maccatalyst")] is suppressed (removed) by call site [SupportedOSPlatform("maccatalyst13.0")] attribute, but [SupportedOSPlatform("ios14.0")] is not suppressed by [SupportedOSPlatform("ios13.0")] because the version is higher. So when DoSomething() called above it become only supported on ios14.0 but OperatingSystem.IsIOSVersionAtLeast(15,0) is reachable on ios15.0 and maccatalys15.0 therefore warns.

I think the best way to solve this is to restore the suppressed maccatalyst support in case ios support is not suppressed.

See more details from #7239 (comment)

Fixes #7239

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.50%. Comparing base (45caa45) to head (6561e7d).
Report is 5 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #7539   +/-   ##
=======================================
  Coverage   96.50%   96.50%           
=======================================
  Files        1452     1452           
  Lines      347566   347594   +28     
  Branches    11416    11417    +1     
=======================================
+ Hits       335415   335446   +31     
+ Misses       9255     9252    -3     
  Partials     2896     2896           

[MartyIX]

cc @tannergooding Would you mind taking a look at this one given your recent work on the analyzer?

[tannergooding]

I don't think I quite understand this or why it is specific to maccatalyst; it seems like a wider issue.

In general a SupportedOSPlatform("x") attribute says that the API is supported on x. If that includes a version then it means it is only supported on that version of x or later. Inversely UnsupportedOSPlatform("x") attribute indicates the API is not supported on x or later.

If there are only supported attributes then platforms other than those listed are automatically deemed unsupported. If there are only unsupported attributes then platforms other than those listed are automatically deemed supported. If there is a mix of supported/unsupported attributes then there are some rules on whether its considered allow, deny, or inconsistent.

In the scenario given, we only have supported and so the API in question works on either ios or maccatalyst. This means that a guard that only checks IsIOSVersionAtLeast(15, 0) should be passing for an API marked [SupportedOSPlatform("ios14.0")] irrespectively of any other SupportedOSPlatform in the list; adding back maccatalyst should not be required.

The fact that IsIOSVersionAtLeast also functions as a guard for maccatalyst via the SupportedOSPlatformGuard should be irrespective, as it should logically be treated the same as IsIOSVersionAtLeast(..) || IsMacCatalyst(); which is that it functions as two guards under an or condition.

This change however is making it seem more like its functioning as IsIOSVersionAtLeast(..) && IsMacCatalyst() which seems like it is strictly incorrect and likely to lead to other bugs/issues

I would likewise expect this issue to reproduce for any other SupportedOSPlatformGuard usage like this scenario, it wouldn't be strictly limited to ios+maccatalyst

[MartyIX]

Just to check: Have you had a chance to read #7239 (comment) ?

[tannergooding]

Yes, but that comment doesn't cover why this is the "right fix". I think rather the actual issue is elsewhere and is what is causing the undesired behavior (that is, which is functionally treating it as an && instead of an || condition).

[buyaa-n]

The fact that IsIOSVersionAtLeast also functions as a guard for maccatalyst via the SupportedOSPlatformGuard should be irrespective, as it should logically be treated the same as IsIOSVersionAtLeast(..) || IsMacCatalyst(); which is that it functions as two guards under an or condition.

It is works as you mentioned i.e. IsIOSVersionAtLeast(..) || IsMacCatalystVersionAtLeast() and the condition means the guarded code path is reachable on IOS or MacCatalyst, but the DoSomething() method at that point only supported on ios14.0, when call site also reachable on maccatalyst, therefore we need to put the original maccatalyst support back

This change however is making it seem more like its functioning as IsIOSVersionAtLeast(..) && IsMacCatalyst() which seems like it is strictly incorrect and likely to lead to other bugs/issues

The change doesn't affect the flow analysis condition. It changes (actually restores) the supported attribute list of method DoSomething() to it supported on IsIOS14.0 && MacCatalyst

[buyaa-n]

I don't think I quite understand this or why it is specific to maccatalyst; it seems like a wider issue.

In general a SupportedOSPlatform("x") attribute says that the API is supported on x. If that includes a version then it means it is only supported on that version of x or later. Inversely UnsupportedOSPlatform("x") attribute indicates the API is not supported on x or later.

If there are only supported attributes then platforms other than those listed are automatically deemed unsupported. If there are only unsupported attributes then platforms other than those listed are automatically deemed supported. If there is a mix of supported/unsupported attributes then there are some rules on whether its considered allow, deny, or inconsistent.

In the scenario given, we only have supported and so the API in question works on either ios or maccatalyst. This means that a guard that only checks IsIOSVersionAtLeast(15, 0) should be passing for an API marked [SupportedOSPlatform("ios14.0")] irrespectively of any other SupportedOSPlatform in the list; adding back maccatalyst should not be required.

You are right, that is how the analyzer works for all other unrelated platforms. But ios and maccatalyst are related: ios is maccatalyst but maccatalyst is not ios. Analyzer recognize such relation using SupportedOSPlatformGuard attribute on guard methods, currently the attribute only applied on IsIOS... methods. There is lots of rules around how to handle this relation, check the original issue requirement for more info

[MartyIX]

@tannergooding Did the explanation from @buyaa-n resolve your concerns?

[buyaa-n]

@tannergooding I put up another solution that ignores maccatalys15.0 part from IsIOSVersionAtLeast(15, 0) guard when evaluating the flow analysis if the called API originally had maccatalyst attribute and versions are compatible. PTAL #7569

[tannergooding]

Sorry, I had missed this. I'm still not sure it was the right approach.

The existing guards shouldn't require inserting new platforms into the list; otherwise this is a general problem that applies to the attributes and isn't specific to just ios/maccatalyst.

We should be able to correctly model the relationship without special-casing this specific scenario in the analyzer; otherwise we have a gap somewhere.

[buyaa-n]

I am gonna close this as #7569 is better solution