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.

Sign up for GitHub

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

Restore suppressed maccatalyst for proper flow analysis #7539

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Restore suppressed maccatalyst for proper flow analysis #7539

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b7c38f4
Restore suppressed maccatalyst for proper flow analysis
buyaa-n Jan 19, 2025
6561e7d
msbuild /t:pack changes
buyaa-n Jan 20, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions ...alyzers/Core/Microsoft.NetCore.Analyzers/InteropServices/PlatformCompatibilityAnalyzer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,8 @@ public sealed partial class PlatformCompatibilityAnalyzer : DiagnosticAnalyzer
private const string macOS = nameof(macOS);
private const string OSX = nameof(OSX);
private const string MacSlashOSX = "macOS/OSX";
private const string ios = nameof(ios);
private const string maccatalyst = nameof(maccatalyst);
private static readonly Version EmptyVersion = new(0, 0);

internal static readonly DiagnosticDescriptor OnlySupportedCsReachable = DiagnosticDescriptorHelper.Create(
Expand Down Expand Up @@ -1692,6 +1694,18 @@ private static bool IsNotSuppressedByCallSite(SmallDictionary<string, Versions>
}
}
}

// if operation had ios and maccatalyst attibutes, and only maccatalyst has supppressed by callsite, we need to
// put that back else it would cause an issue in flow analysis when `OperatingSystem.IsIOSVersionAtLeast`
// guard is used, because the IsIOSVersionAtLeast method guards ios and maccatalyst both
if (notSuppressedAttributes.ContainsKey(ios) &&
!notSuppressedAttributes.ContainsKey(maccatalyst) &&
operationAttributes.TryGetValue(maccatalyst, out Versions? macVersion))
{
Versions diagnosticAttribute = new Versions();
CopyAllAttributes(diagnosticAttribute, macVersion);
notSuppressedAttributes[maccatalyst] = diagnosticAttribute;
}
Comment on lines +1698 to +1708
Copy link
Member

@tannergooding tannergooding Jan 23, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@buyaa-n buyaa-n Jan 23, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@buyaa-n buyaa-n Jan 23, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

return !notSuppressedAttributes.IsEmpty;
Expand Down
1 change: 0 additions & 1 deletion src/NetAnalyzers/RulesMissingDocumentation.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,4 +3,3 @@
Rule ID | Missing Help Link | Title |
--------|-------------------|-------|
CA2023 | <https://learn.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca2023> | Invalid braces in message template |
CA2024 | <https://learn.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca2024> | Do not use 'StreamReader.EndOfStream' in async methods |
24 changes: 24 additions & 0 deletions ...NetCore.Analyzers/InteropServices/PlatformCompatibilityAnalyzerTests.GuardedCallsTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5338,6 +5338,30 @@ class MyType { }
await VerifyAnalyzerCSAsync(source, s_msBuildPlatforms);
}

[Fact]
public async Task MacCatalystWithMandatorySupportFound()
{
var source = @"
using System;
using System.Runtime.Versioning;
class TestType
{
[SupportedOSPlatform(""ios13.0"")]
[SupportedOSPlatform(""maccatalyst13.0"")]
private void Tapped()
{
if (OperatingSystem.IsIOSVersionAtLeast(15,0))
DoSomething();
}
[SupportedOSPlatform(""ios14.0"")]
[SupportedOSPlatform(""maccatalyst"")]
public void DoSomething() {}
}";

string msBuildPlatforms = "build_property.TargetFramework=net8.0-maccatalyst13;";
await VerifyAnalyzerCSAsync(source, msBuildPlatforms);
}

private readonly string TargetTypesForTest = @"
namespace PlatformCompatDemo.SupportedUnupported
{
Expand Down