Handle guarded maccatalyst attribute issue that suppressed by call site #7569
Conversation
buyaa-n
force-pushed
the
bug_7239_6955
branch
from
3265f07 to
f121b81
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #7569 +/- ##
=======================================
Coverage 96.54% 96.54%
=======================================
Files 1454 1454
Lines 352505 352590 +85
Branches 11478 11480 +2
=======================================
+ Hits 340329 340417 +88
+ Misses 9262 9259 -3
Partials 2914 2914 |
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 |
The suppression here sounds wrong in the first place, potentially due to the initial The consideration here it that The rules are pretty clear about only allowing |
|
It's also pretty clear about how it works for Namely that if you want to say that At best I could see this maybe being a special circumstance (that should be documented) where you are required to have all three of But this still leads to a type of inconsistency because there's no way to distinguish a caller that validated they were |
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:
|
I don't see that in here, which notably is also missing nuance on By the documentation https://learn.microsoft.com/en-us/dotnet/standard/analyzers/platform-compat-analyzer#platform-inclusion:
This means that by default the following is expanding to: Per the documentation, this is then equivalent to simply the following, as You can only exclude a given version if you do
So the original sample for But this doesn't seem to be a scenario that's supported by the documentation and appears to be considered "inconsistent" instead. |
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): i.e. "maccatalyst13.0" is repeated. |
I don't believe this is how 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 |
is simply expands to: For: its is also expands to:
just expands to: not |
The version is propagated: check same as checking: because of |
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
} |
|
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 |
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 |
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.
True, my fix initially was
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.
Not sure if I understood your point, but user can define a custom guard API with any 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 |
|
@buyaa-n My whole line of questioning here stems from 2 points.
To elaborate on 2, lets consider for a second that what you've indicated is correct and that 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 For Then we should process For 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 |
All scenarios are listed in the issue dotnet/runtime#53084 (comment), could be added more info in the official doc
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.
The current implementation works exactly like you wrote on above 3 paragraphs
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.
FYI information current implementation builds a 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.
[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 whenDoSomething()called above it become only supported on ios14.0 butOperatingSystem.IsIOSVersionAtLeast(15,0)is reachable on ios15.0 and maccatalys15.0 therefore warns.One way to solve this is to restore the suppressed
maccatalystsupport in caseiossupport is not suppressed.In this PR we are ignoring
maccatalys15.0part of the guard whenDoSomething()originally hadmaccatalystattribute that is suppressed by the call site.See more details from #7239 (comment)
Fixes #7239, #6955, #7530