Handle guarded maccatalyst attribute issue that suppressed by call site by buyaa-n · Pull Request #7569 · dotnet/roslyn-analyzers

buyaa-n · 2025-02-10T07:11:00Z

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.

One way to solve this is to restore the suppressed maccatalyst support in case ios support is not suppressed.

In this PR we are ignoring maccatalys15.0 part of the guard when DoSomething() originally had maccatalyst attribute that is suppressed by the call site.

See more details from #7239 (comment)

Fixes #7239,
Fixed #6955,
Fixes #7530

codecov · 2025-02-10T07:35:27Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.84%. Comparing base (cbcc6dd) to head (7a03d3d).
Report is 54 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #7569      +/-   ##
==========================================
- Coverage   96.84%   96.84%   -0.01%     
==========================================
  Files        1215     1215              
  Lines      307561   307646      +85     
  Branches     9513     9515       +2     
==========================================
+ Hits       297846   297925      +79     
- Misses       7376     7382       +6     
  Partials     2339     2339

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

rolfbjarne · 2025-02-10T11:04:28Z

I tried to debug #7530, and ended up in the some code paths @MartyIX did here, so that issue might be related - but that issue is not using maccatalyst at all, so there might be something else going on (or maybe there are two different issues in more or less the same code?)

buyaa-n · 2025-02-10T20:25:45Z

I tried to debug #7530, and ended up in the some code paths @MartyIX did here, so that issue might be related - but that issue is not using maccatalyst at all, so there might be something else going on (or maybe there are two different issues in more or less the same code?)

That is a good finding, thank you for letting me know. I see now it is not ios/maccatalyst specific issue, it can also happen for guard APIs that has more than 1 guard attribute and one of the attributes suppressed by call site. I have updated this PR to skip guards for any platform that has been suppressed by call site and added corresponding test

tannergooding · 2025-02-10T21:16:26Z

[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.

The suppression here sounds wrong in the first place, potentially due to the initial SupportedOSPlatform set being inconsistent. That is, as per https://learn.microsoft.com/en-us/dotnet/standard/analyzers/platform-compat-analyzer#advanced-scenarios-for-attribute-combinations

The consideration here it that [SupportedOSPlatform("ios13.0")] indicates that both ios13.0+ and maccatalyst are supported. You've then separately defined [SupportedOSFormat("maccatalyst13.0")] which leaves an inconsistency in what is or isn't supported for maccatalyst, as you're simultaneously saying "all versions" and "only 13.0+".

The rules are pretty clear about only allowing Supported + Unsupported in the scenario where the latter must be a higher version. Trying to say that a range of versions is unsupported isn't allowed.

tannergooding · 2025-02-10T21:37:07Z

It's also pretty clear about how it works for ios+maccatalyst here: https://learn.microsoft.com/en-us/dotnet/standard/analyzers/platform-compat-analyzer#platform-inclusion

Namely that if you want to say that ios is supported but maccatalyst is not, you need [SupportedOSPlatform("ios")] + [UnsupportedOSPlatform("maccatalyst")]

At best I could see this maybe being a special circumstance (that should be documented) where you are required to have all three of [SupportedOSPlatform("ios13.0"] + [UnsupportedOSPlatform("maccatalyst")] + [SupportedOSPlatform("maccatalyst13.0")].

But this still leads to a type of inconsistency because there's no way to distinguish a caller that validated they were ios vs one that was actually maccatalyst. You can maybe rationalize the scenarios, but its quite a bit more complex and doesn't play out nicely

buyaa-n · 2025-02-10T23:35:10Z

The suppression here sounds wrong in the first place, potentially due to the initial SupportedOSPlatform set being inconsistent. That is, as per https://learn.microsoft.com/en-us/dotnet/standard/analyzers/platform-compat-analyzer#advanced-scenarios-for-attribute-combinations

The consideration here it that [SupportedOSPlatform("ios13.0")] indicates that both ios13.0+ and maccatalyst are supported. You've then separately defined [SupportedOSFormat("maccatalyst13.0")] which leaves an inconsistency in what is or isn't supported for maccatalyst, as you're simultaneously saying "all versions" and "only 13.0+".

The rules are pretty clear about only allowing Supported + Unsupported in the scenario where the latter must be a higher version. Trying to say that a range of versions is unsupported isn't allowed.

I think you are mixing the steps. By design the warnings can be suppressed by 2 means: call site attributes or guard methods.

Here is how the the analyzer works in brief:

It collect all platform attributes of the called API, then it collects call site platform attributes. Here if there is [SupportedOSPlatform("ios")] it converted into 2 attributes [SupportedOSPlatform("ios")] and [SupportedOSPlatform("maccatalyst")] in case it is not have [SupportedOSPlatform("maccatalyst")] attribute with different version or [UnsupportedOSPlatform("maccatalyst")] etc. i.e. the scenario you are mentioning takes place here as you mentioned.
It compares all call site and called API attributes and removes API attributes that suppressed by call site. Here where that maccatalyst support is being removed and my previous fix made here too.
Now all attributes that are not suppressed by call site left, we do flow analysis in this step to check if the called API is guarded somehow. Here the guard methods could be guarding more than 1 platform and one of the might be suppressed by call site. The fix here skipping that guard platform if it was in original attributes and could have been suppressed by the guard either.

tannergooding · 2025-02-11T17:03:08Z

I think you are mixing the steps. By design the warnings can be suppressed by 2 means: call site attributes or guard methods.

I don't see that in here, which notably is also missing nuance on ios+maccatalyst. The public documentation does cover what's in the design and further augments it for ios+maccatalyst

By the documentation https://learn.microsoft.com/en-us/dotnet/standard/analyzers/platform-compat-analyzer#platform-inclusion:

The OperatingSystem.IsIOS() and OperatingSystem.IsIOSVersionAtLeast methods check not only the iOS platform, but also the MacCatalyst platform.

[SupportedOSPlatform("iOS")] implies that the API is supported on iOS and also on its superset platform, MacCatalyst. You can use the [UnsupportedOSPlatform("MacCatalyst")] attribute to exclude this implied support.

[UnsupportedOSPlatform("iOS")] implies that the API is not supported on iOS and MacCatalyst. You can use the [SupportedOSPlatform("MacCatalyst")] attribute to exclude this implied lack of support.

This means that by default the following

[SupportedOSPlatform("ios13.0")]
[SupportedOSPlatform("maccatalyst13.0")]

is expanding to:

[SupportedOSPlatform("ios13.0")]
[SupportedOSPlatform("maccatalyst")]
[SupportedOSPlatform("maccatalyst13.0")]

Per the documentation, this is then equivalent to simply the following, as maccatalyst is a superset of maccatalyst13.0:

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

You can only exclude a given version if you do [UnsupportedOSPlatform("maccatalyst")]. But, given https://learn.microsoft.com/en-us/dotnet/standard/analyzers/platform-compat-analyzer#advanced-scenarios-for-attribute-combinations which specifies further rules for supported/unsupported mixing; it should not be valid to say something was supported, then unsupported, then supported again later:

sic. Supported only list: ... The optional [UnsupportedOSPlatform] attributes for each platform can only have higher version of the minimum supported version, which denotes that the API is removed starting from the specified version.

sic. Unsupported only list: ... The list could have [SupportedOSPlatform] attribute with the same platform but a higher version, which denotes that the API is supported starting from that version.

So the original sample for Tapped has nonsensical specification, because [SupportedOSPlatform("maccatalyst13.0")] is unnecessary. At best you could try to define it as:

[SupportedOSPlatform("ios13.0")]
[UnsupportedOSPlatform("maccatalyst")]
[SupportedOSPlatform("maccatalyst13.0")]

But this doesn't seem to be a scenario that's supported by the documentation and appears to be considered "inconsistent" instead.

rolfbjarne · 2025-02-11T19:20:10Z

This means that by default the following

[SupportedOSPlatform("ios13.0")]
[SupportedOSPlatform("maccatalyst13.0")]

is expanding to:

[SupportedOSPlatform("ios13.0")]
[SupportedOSPlatform("maccatalyst")]
[SupportedOSPlatform("maccatalyst13.0")]

It doesn't look like the documentation says how the "ios" attribute expands when an OS version is present, but I believe the behavior would be that "ios13.0" expands to "maccatalyst13.0", not "maccatalyst" (the latter doesn't make sense IMHO, if that's the way it works it sounds like a bug).

That would mean that this:

[SupportedOSPlatform("ios13.0")]
[SupportedOSPlatform("maccatalyst13.0")]

expands to (at least conceptually):

[SupportedOSPlatform("ios13.0")]
[SupportedOSPlatform("maccatalyst13.0")]
[SupportedOSPlatform("maccatalyst13.0")]

i.e. "maccatalyst13.0" is repeated.

tannergooding · 2025-02-11T19:32:41Z

It doesn't look like the documentation says how the "ios" attribute expands when an OS version is present, but I believe the behavior would be that "ios13.0" expands to "maccatalyst13.0", not "maccatalyst" (the latter doesn't make sense IMHO, if that's the way it works it sounds like a bug).

I don't believe this is how SupportedOSPlatformGuard works, which is how IsIOSVersion ends up propagating the info. We have precisely [SupportedOSPlatformGuard("maccatalyst")] and I'm not aware of any support for indicating that a version should be propagated or flow through from one to the other.

If it is, this is something that needs explicit documentation because its not what's specified today or what's shown in the examples in the documentation

buyaa-n · 2025-02-11T22:53:10Z

This means that by default the following
[SupportedOSPlatform("ios13.0")]
[SupportedOSPlatform("maccatalyst13.0")]
is expanding to:
[SupportedOSPlatform("ios13.0")]
[SupportedOSPlatform("maccatalyst")]
[SupportedOSPlatform("maccatalyst13.0")]
It doesn't look like the documentation says how the "ios" attribute expands when an OS version is present, but I believe the behavior would be that "ios13.0" expands to "maccatalyst13.0", not "maccatalyst" (the latter doesn't make sense IMHO, if that's the way it works it sounds like a bug).

That would mean that this:
[SupportedOSPlatform("ios13.0")]
[SupportedOSPlatform("maccatalyst13.0")]
expands to (at least conceptually):
[SupportedOSPlatform("ios13.0")]
[SupportedOSPlatform("maccatalyst13.0")]
[SupportedOSPlatform("maccatalyst13.0")]
i.e. "maccatalyst13.0" is repeated.

[SupportedOSPlatform("ios13.0")]

is simply expands to:

[SupportedOSPlatform("ios13.0")]
[SupportedOSPlatform("maccatalyst13.0")]

For:

[SupportedOSPlatform("ios13.0")]
[SupportedOSPlatform("maccatalyst13.0")]

its is also expands to:

[SupportedOSPlatform("ios13.0")]
[SupportedOSPlatform("maccatalyst13.0")]

[SupportedOSPlatform("maccatalyst13.0")] adding is just redundant here. maccatalyst attribute only affects if it has lower version or it was UnsupportedOSPlatform platform attribute. i.e.:

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

just expands to:

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

not

[SupportedOSPlatform("ios13.0")]
[SupportedOSPlatform("maccatalyst")]
[SupportedOSPlatform("maccatalyst13.0")] // this will not be added to the attributes list

buyaa-n · 2025-02-11T22:57:15Z

It doesn't look like the documentation says how the "ios" attribute expands when an OS version is present, but I believe the behavior would be that "ios13.0" expands to "maccatalyst13.0", not "maccatalyst" (the latter doesn't make sense IMHO, if that's the way it works it sounds like a bug).

I don't believe this is how SupportedOSPlatformGuard works, which is how IsIOSVersion ends up propagating the info. We have precisely [SupportedOSPlatformGuard("maccatalyst")] and I'm not aware of any support for indicating that a version should be propagated or flow through from one to the other.

If it is, this is something that needs explicit documentation because its not what's specified today or what's shown in the examples in the documentation

The version is propagated:

if (OperatingSystem.IsIOSVersionAtLeast(15,0))

check same as checking:

if (OperatingSystem.IsIOSVersionAtLeast(15,0) || OperatingSystem.IsMaccatalysVersionAtLeast(15,0))

because of [SupportedOSPlatformGuard("maccatalyst")] attribute on OperatingSystem.IsIOSVersionAtLeast method

buyaa-n · 2025-02-11T23:08:01Z

I think you are mixing the steps. By design the warnings can be suppressed by 2 means: call site attributes or guard methods.

I don't see that in here, which notably is also missing nuance on ios+maccatalyst. The public documentation does cover what's in the design and further augments it for ios+maccatalyst

The call site suppression scenario is not what you are thinking Tanner. Lets try it without guard methods:

class TestType
{
    private void CallSiteHasNoAttributes()
    {
        DoSomething();       // would warn here that 'DoSomething' is supported on ios14.0 and maccatalyst all versions
    }

    [SupportedOSPlatform("ios13.0")] 
    [SupportedOSPlatform("maccatalyst13.0")] // this is redundant
    private void CallSiteSupportedIos13AndMaccatalyst13()
    {
        DoSomething();             // would warn here that 'DoSomething' is supported on ios14.0
    }

    [SupportedOSPlatform("ios14.0")] 
    private void CallSiteSupportedIos14AndMaccatalyst14()
    {
        DoSomething();             // now warning as call site suppressed all required attributes warning
    }

    [SupportedOSPlatform("ios14.0")]
    [SupportedOSPlatform("maccatalyst")] 
    public void DoSomething() {}    // 'DoSomething' method supported on ion14 and maccatalyst all versions
}

buyaa-n · 2025-02-11T23:14:15Z

Now with guard the bug shows up:

    [SupportedOSPlatform("ios13.0")] 
    [SupportedOSPlatform("maccatalyst13.0")]
    private void CallSiteSupportedIos13AndMaccatalyst13()
    {
        DoSomething();             // would warn here that 'DoSomething' is supported on ios14.0

        if (OperatingSystem.IsIOSVersionAtLeast(15,0)) // checks if ios15.0 || maccatalys15.0 or above
           DoSomething(); // 'DoSomething' attribute list only had ios14.0, but it is reachable also on maccatalys15.0 so warns
    }

Hope it helps for understanding the issue

rolfbjarne

I tested this PR on our local repository, and it fixes all we know about. How soon can this be released? 😄

tannergooding · 2025-02-17T18:01:42Z

The call site suppression scenario is not what you are thinking Tanner. Lets try it without guard methods:

What I'm pointing out here is that we have a set of documentation and a set of attributes/behaviors that are accessible to users. This PR is trying to customize a very particular path which doesn't align with what is in the existing documentation/behaviors, this could be a gap in the documentation but its something that also needs to be fixed then.

Such a fix should also not be specific to maccatalyst or ios, it should be a general fix against SupportedOSPlatformGuard and should equally work if in the future we added something like [SupportedOSPlatformGuard("xbox")] to say IsWindowsVersionAtLeast, without in turn having to go add more custom code. Such a change should likewise be explicitly documented as a special case of how SupportedOSPlatformGuard behaves when applied to the built-in Is*VersionAtLeast checks in that it matches and that it matches the checked version. Such behavior should be documented as not being definable to regular users, that is there is no way for a user to define a custom guard API that indicates that version x of OS a also lines up to version x of OS b

buyaa-n · 2025-02-17T20:18:48Z

What I'm pointing out here is that we have a set of documentation and a set of attributes/behaviors that are accessible to users. This PR is trying to customize a very particular path which doesn't align with what is in the existing documentation/behaviors, this could be a gap in the documentation but its something that also needs to be fixed then.

Apparently your concern were about evaluating the attributes combinations, I think I have answered most of it with above comment, I have answered the remaining question in below comment. Here I want to mention that the bug was caused by call site suppressions not because of attributes combinations. Also this fix is not doing anything that is not aligned with doc, though the doc did not cover all edge case scenarios handling.

Such a fix should also not be specific to maccatalyst or ios, it should be a general fix against SupportedOSPlatformGuard and should equally work if in the future we added something like [SupportedOSPlatformGuard("xbox")] to say IsWindowsVersionAtLeast, without in turn having to go add more custom code.

True, my fix initially was ios, maccatalyst specific, @rolfbjarne reported above that it was not ios, maccatalyst specific see #7530. And my last commit 1b5fece made it generic fix for any platform.

Such a change should likewise be explicitly documented as a special case of how SupportedOSPlatformGuard behaves when applied to the built-in Is*VersionAtLeast checks in that it matches and that it matches the checked version.

Yes, the document https://learn.microsoft.com/en-us/dotnet/standard/analyzers/platform-compat-analyzer#platform-inclusion did not mention about the version, it could updated, but for me the version match kind a obvious.

Such behavior should be documented as not being definable to regular users, that is there is no way for a user to define a custom guard API that indicates that version x of OS a also lines up to version x of OS b

Not sure if I understood your point, but user can define a custom guard API with any version

roslyn-analyzers/src/NetAnalyzers/UnitTests/Microsoft.NetCore.Analyzers/InteropServices/PlatformCompatibilityAnalyzerTests.GuardedCallsTests.cs

Lines 5423 to 5425 in 1b5fece

    
               [SupportedOSPlatformGuard (""macos15.0"")] 
        
               [SupportedOSPlatformGuard (""tvos15.0"")] 
        
               public bool IsAtLeast => false;

buyaa-n · 2025-02-17T20:30:39Z

You can only exclude a given version if you do [UnsupportedOSPlatform("maccatalyst")]. But, given https://learn.microsoft.com/en-us/dotnet/standard/analyzers/platform-compat-analyzer#advanced-scenarios-for-attribute-combinations which specifies further rules for supported/unsupported mixing; it should not be valid to say something was supported, then unsupported, then supported again later:

sic. Supported only list: ... The optional [UnsupportedOSPlatform] attributes for each platform can only have higher version of the minimum supported version, which denotes that the API is removed starting from the specified version.

sic. Unsupported only list: ... The list could have [SupportedOSPlatform] attribute with the same platform but a higher version, which denotes that the API is supported starting from that version.

Good point, Advanced scenarios for attribute combinations is the base scenario that applies for all platforms, Platform inclusion part is a special case for related platforms like ios and maccatalyst. Probably we should have added Platform inclusion doc after Advanced scenarios for attribute combinations as an exclusion for those scenarios

tannergooding · 2025-02-18T03:11:03Z

@buyaa-n My whole line of questioning here stems from 2 points.

The behavior, to me, is non-obvious. This is due to a lack of documentation around the topic on the .NET side. Additionally, there seems to be a lack of documentation from Apple in the relationship between iOS and MacCatalyst versions. You've indicated that the behavior is intentional, which is fine. I'm just trying to call out that it's not documented and that in turn is causing some of the questioning/confusion.
The actual bug here seems to be that we are suppressing maccatalyst at all in the first place. This makes the "fix" here confusing, because it seems rather like a hack/workaround the actual underlying issue. The suppressing at all seems incorrect given how you've stated these checks work and isn't how I'd expect us to be tracking the "what's supported" for a given scope.

To elaborate on 2, lets consider for a second that what you've indicated is correct and that iosX.Y.Z implies strictly on a 1-to-1 basis maccatalystX.Y.Z.

Now within this, lets consider the following code:

[SupportedOSPlatform("ios13.0")] 
[SupportedOSPlatform("maccatalyst13.0")]
private void A()
{
    C();
}

[SupportedOSPlatform("ios13.0")] 
[SupportedOSPlatform("maccatalyst13.0")]
private void B()
{
    if (OperatingSystem.IsIOSVersionAtLeast(15,0))
       C();
}

[SupportedOSPlatform(""ios14.0"")]
[SupportedOSPlatform(""maccatalyst"")]
[SupportedOSPlatform(""windows"")]
public void C() {}

For both A and B we should presumably be building a list of supported platforms and the versions they map to. Logically we would first encounter ios13.0, so we would add an entry ios and a version 13.0. Because of the specialized mapping between these, we would then also add maccatalyst and a version 13.0. We then encounter the second attribute maccatalyst13.0 so we would try to add the key and find we already have an entry and that the new version (13) is not lesser (they are the same), so the tracked existing version supported is preserved as is. This gives us { ["ios"] = 13.0.0, ["maccatalyst"] = 13.0.0 }

For C we have a similar process. We encounter ios and add 14.0, we additionally add maccatalyst and 14.0 due to the special relationship, we then encounter maccatalyst again and should be updating the version this time because no version (logically 0.0.0 is less than the existing (13.0.0), and finally we encounter windows with no version so we add that. This should give us { ["ios"] = 14.0.0, ["maccatalyst"] = 0.0.0, ["windows"] = 0.0.0 }.

Then we should process A where we should encounter the call to C. So we should iterate the scopes of the callee (C) as compared to the current scope of the caller (A). We should see that ios exists in both and that the version of the caller (13) is less than that of the callee (14) so we should warn. We then continue iterating the scopes to find maccatalyst and see that the version of the caller (13) is greater than that of the callee (0) so we don't warn on this one. We then finish the iteration and see that windows is defined by the callee but not the caller, so therefore no check is needed as its not part of the union of supported sets.

For B we should do a similar process. But first we encounter the IsIOSVersionAtLeast(15,0) check which temporarily modifies the scope, as if it were a stack. That is, we should first validate that the platform being checked (ios) is part of the parent scope (scope of B), it is so we then check if the version (15) is greater than what the parent scope requires (13). If either of these were false, we'd presumably want to warn as the check is "unreachable" (a check for IsWindows is nonsensical if B only supports ios+maccatalyst, likewise a check for IsIOSVersionAtLeast(12) is nonsensical if B said the minimum supported was ios13.0). This guard check being sensible and given what it validates is the minimum supported version logically builds a new scope { ["ios"] = 15.0.0, ["maccatalyst"] = 15.0.0 } that is pushed onto the supported scope stack until the end of the block that it guards. We then encounter the call to C within that block and follow the same set of checks we did for A, but this time ios 14 < 15, maccatalyst 0 < 15, and windows isn't in the union, so no warnings are emitted.

This same premise of building up a dictionary that tracks the platform name to version supported works and then logically tracking a "stack" of these support scopes per block works trivially and extends well to the whole premise of SupportedOSPlatform and UnsupportedOSPlatform and it never requires a "suppression" or addition back of the entries. You simply build up the dictionary, taking the minimum versions, and you clone the dictionary for the new scope if a guard check is introduced. You validate correctness by taking a union of the keys (platform names) and comparing versions. This also allows you to easily check for inconsistency, warn for entries that are duplicate or unnecessary, warn for guard checks that are unreachable, etc

buyaa-n · 2025-02-18T20:54:19Z

The behavior, to me, is non-obvious. This is due to a lack of documentation around the topic on the .NET side. Additionally, there seems to be a lack of documentation from Apple in the relationship between iOS and MacCatalyst versions. You've indicated that the behavior is intentional, which is fine. I'm just trying to call out that it's not documented and that in turn is causing some of the questioning/confusion.

All scenarios are listed in the issue dotnet/runtime#53084 (comment), could be added more info in the official doc

The actual bug here seems to be that we are suppressing maccatalyst at all in the first place. This makes the "fix" here confusing, because it seems rather like a hack/workaround the actual underlying issue. The suppressing at all seems incorrect given how you've stated these checks work and isn't how I'd expect us to be tracking the "what's supported" for a given scope.

Call site suppression is part of the original design not a bug. How it implemented might not ideal, but it is how it is written originally. I would say the fix made here more like surgical fix for existing implementation than a hack.

For both A and B we should presumably be building a list of supported platforms and the versions they map to. Logically we would first encounter ios13.0, so we would add an entry ios and a version 13.0. Because of the specialized mapping between these, we would then also add maccatalyst and a version 13.0. We then encounter the second attribute maccatalyst13.0 so we would try to add the key and find we already have an entry and that the new version (13) is not lesser (they are the same), so the tracked existing version supported is preserved as is. This gives us { ["ios"] = 13.0.0, ["maccatalyst"] = 13.0.0 }

For C we have a similar process. We encounter ios and add 14.0, we additionally add maccatalyst and 14.0 due to the special relationship, we then encounter maccatalyst again and should be updating the version this time because no version (logically 0.0.0 is less than the existing (13.0.0), and finally we encounter windows with no version so we add that. This should give us { ["ios"] = 14.0.0, ["maccatalyst"] = 0.0.0, ["windows"] = 0.0.0 }.

Then we should process A where we should encounter the call to C. So we should iterate the scopes of the callee (C) as compared to the current scope of the caller (A). We should see that ios exists in both and that the version of the caller (13) is less than that of the callee (14) so we should warn. We then continue iterating the scopes to find maccatalyst and see that the version of the caller (13) is greater than that of the callee (0) so we don't warn on this one. We then finish the iteration and see that windows is defined by the callee but not the caller, so therefore no check is needed as its not part of the union of supported sets.

The current implementation works exactly like you wrote on above 3 paragraphs

For B we should do a similar process. But first we encounter the IsIOSVersionAtLeast(15,0) check which temporarily modifies the scope, as if it were a stack. That is, we should first validate that the platform being checked (ios) is part of the parent scope (scope of B), it is so we then check if the version (15) is greater than what the parent scope requires (13). If either of these were false, we'd presumably want to warn as the check is "unreachable" (a check for IsWindows is nonsensical if B only supports ios+maccatalyst, likewise a check for IsIOSVersionAtLeast(12) is nonsensical if B said the minimum supported was ios13.0). This guard check being sensible and given what it validates is the minimum supported version logically builds a new scope { ["ios"] = 15.0.0, ["maccatalyst"] = 15.0.0 } that is pushed onto the supported scope stack until the end of the block that it guards. We then encounter the call to C within that block and follow the same set of checks we did for A, but this time ios 14 < 15, maccatalyst 0 < 15, and windows isn't in the union, so no warnings are emitted.

The current implementation pretty much works like this, except it first checks the call site suppression. I do not recall all reasons for choosing this approach, I believe one reasons was the flow analysis is quite expensive, wanted to run it with the least number of invocations.

This same premise of building up a dictionary that tracks the platform name to version supported works and then logically tracking a "stack" of these support scopes per block works trivially and extends well to the whole premise of SupportedOSPlatform and UnsupportedOSPlatform and it never requires a "suppression" or addition back of the entries. You simply build up the dictionary, taking the minimum versions, and you clone the dictionary for the new scope if a guard check is introduced. You validate correctness by taking a union of the keys (platform names) and comparing versions. This also allows you to easily check for inconsistency, warn for entries that are duplicate or unnecessary, warn for guard checks that are unreachable, etc

FYI information current implementation builds a dictionary that tracks the platform name to version supported and checks the platform/versions as you mentioned, plus with many other edge case scenarios. It does remove call-site or guard suppressed platforms but never add it back (except my closed PR tried fix the issue by adding maccatalyst back).

Overall you suggestion of checking the guards and call site suppression together could work, though it needs a lot of refactoring that will be more difficult to review and could cause another bug, especially in case if the fix needs to be serviced 9.0 and/or 8.0

* Allow for specifying a local path to the analyzer assemblies. * Update the api availability test using locally built analyzers that includes dotnet/roslyn-analyzers#7569, because the public analyzers have a bug we run into *a lot*. * Write more values in GeneratedMSBuildEditorConfig.editorconfig. This is be required in newer versions of the platform availability analyzer.

MartyIX · 2025-03-03T06:52:37Z

@tannergooding Does the previous post answer your questions?

Is there a way to move this one forward? The posts in this thread are detailed, perhaps they can be just copied to the PR (or simply a link can be pasted there?) because it contains non-trivial / non-obvious knowledge that would be great to keep for future sets of eyes.

edit: I noticed that @rolfbjarne actually already uses this PR here dotnet/macios#22219 (comment).

MartyIX · 2025-03-26T10:23:11Z

@tannergooding Any way to move this forward?

tannergooding · 2025-03-26T17:13:34Z

Someone with permissions to merge to roslyn-analyzers will need to give a final review, sign-off, and do the actual merge.

CC. @jeffhandley

jingliancui · 2025-03-30T10:55:53Z

Hi @ViktorHofer could you or any one from roslyn team can review please?

ViktorHofer · 2025-03-30T15:38:05Z

Hi @ViktorHofer could you or any one from roslyn team can review please?

cc @CyrusNajmabadi @jaredpar

jaredpar · 2025-04-01T00:15:48Z

@arunchndr

arunchndr · 2025-04-01T23:20:32Z

@carlossanlop would you like to approve and merge since this is in NetAnalyzers?

MartyIX · 2025-04-09T07:47:14Z

@carlossanlop Friendly ping

…39_6955

MartyIX · 2025-04-22T08:03:45Z

@buyaa-n

I believe that

Fixes #7239, #6955, #7530

must be

Fixes #7239,
Fixed #6955,
Fixes #7530

in the original post to let GitHub close the issues automatically.

MartyIX · 2025-05-03T11:24:52Z

@carlossanlop @jeffhandley @CyrusNajmabadi Could anyone approve or share concerns?

MartyIX · 2025-06-19T09:33:01Z

Hi @ViktorHofer could you or any one from roslyn team can review please?

cc @CyrusNajmabadi @jaredpar

Friendly ping :)

jaredpar · 2025-06-19T15:34:39Z

@MartyIX these analyzers are primarily owned by the .NET runtime team and will need their sign off.

@ericstj, @jeffhandley

ViktorHofer · 2025-06-19T16:55:35Z

@MartyIX sorry for the delay here. I wasn't clear on ownership back when you reached out to me. Yes this is NetAnalyzers which my team owns. I'll chat with others about this and get back to you.

tannergooding

The changes LGTM given the explanation above.

Long term I think it would be good to go and do a refactoring to get this into the general shape I suggested, as I think it would simplify the overall logic and make this much easier to understand. However, we shouldn't block this PR on that happening.

Handle guarded maccatalyst attribute issue that suppressed by call site

f121b81

buyaa-n requested a review from a team as a code owner February 10, 2025 07:11

github-actions bot added the Community label Feb 10, 2025

buyaa-n force-pushed the bug_7239_6955 branch from 3265f07 to f121b81 Compare February 10, 2025 07:11

buyaa-n mentioned this pull request Feb 10, 2025

Restore suppressed maccatalyst for proper flow analysis #7539

Closed

Skip any guarded platform that was suppressed by call site

1b5fece

rolfbjarne approved these changes Feb 17, 2025

View reviewed changes

rolfbjarne mentioned this pull request Feb 20, 2025

[src/tests] Misc updates to the platform analyzer tests. dotnet/macios#22219

Merged

jingliancui approved these changes Mar 30, 2025

View reviewed changes

Merge branch 'main' of github.com:dotnet/roslyn-analyzers into bug_72…

7a03d3d

…39_6955

rolfbjarne mentioned this pull request Apr 22, 2025

CA1416 when property accessor is unsupported on Mac Catalyst dotnet/sdk#50337

Open

rolfbjarne mentioned this pull request May 5, 2025

CA1416 with guard attributes #7647

Closed

rolfbjarne mentioned this pull request May 15, 2025

CA1416 with guard attributes when non-taken path has UnsupportedOSPlatform attribute #7661

Open

tannergooding approved these changes Jun 20, 2025

View reviewed changes

ViktorHofer approved these changes Jun 20, 2025

View reviewed changes

ViktorHofer merged commit 977baf6 into dotnet:main Jun 20, 2025
10 checks passed

buyaa-n deleted the bug_7239_6955 branch June 20, 2025 19:08

MartyIX mentioned this pull request Jul 18, 2025

Bug: Platform Analyzer (CA1416) ignores OperatingSystem.IsIOSVersionAtLeast guard clause dotnet/maui#30681

Open

Conversation

buyaa-n commented Feb 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Feb 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

rolfbjarne commented Feb 10, 2025

Uh oh!

buyaa-n commented Feb 10, 2025

Uh oh!

tannergooding commented Feb 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tannergooding commented Feb 10, 2025

Uh oh!

buyaa-n commented Feb 10, 2025

Uh oh!

tannergooding commented Feb 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rolfbjarne commented Feb 11, 2025

Uh oh!

tannergooding commented Feb 11, 2025

Uh oh!

buyaa-n commented Feb 11, 2025

Uh oh!

buyaa-n commented Feb 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

buyaa-n commented Feb 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

buyaa-n commented Feb 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rolfbjarne left a comment

Choose a reason for hiding this comment

Uh oh!

tannergooding commented Feb 17, 2025

Uh oh!

buyaa-n commented Feb 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

buyaa-n commented Feb 17, 2025

Uh oh!

tannergooding commented Feb 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

buyaa-n commented Feb 18, 2025

Uh oh!

MartyIX commented Mar 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MartyIX commented Mar 26, 2025

Uh oh!

tannergooding commented Mar 26, 2025

Uh oh!

jingliancui commented Mar 30, 2025

Uh oh!

ViktorHofer commented Mar 30, 2025

Uh oh!

jaredpar commented Apr 1, 2025

Uh oh!

arunchndr commented Apr 1, 2025

Uh oh!

MartyIX commented Apr 9, 2025

Uh oh!

MartyIX commented Apr 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MartyIX commented May 3, 2025

Uh oh!

MartyIX commented Jun 19, 2025

Uh oh!

jaredpar commented Jun 19, 2025

Uh oh!

ViktorHofer commented Jun 19, 2025

Uh oh!

buyaa-n commented Feb 10, 2025 •

edited

Loading

codecov bot commented Feb 10, 2025 •

edited

Loading

tannergooding commented Feb 10, 2025 •

edited

Loading

tannergooding commented Feb 11, 2025 •

edited

Loading

buyaa-n commented Feb 11, 2025 •

edited

Loading

buyaa-n commented Feb 11, 2025 •

edited

Loading

buyaa-n commented Feb 11, 2025 •

edited

Loading

buyaa-n commented Feb 17, 2025 •

edited

Loading

tannergooding commented Feb 18, 2025 •

edited

Loading

MartyIX commented Mar 3, 2025 •

edited

Loading

MartyIX commented Apr 22, 2025 •

edited

Loading