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.

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

Update the flow analysis result evaluation to account the call site reachability #5571

Merged
merged 3 commits into from
Oct 1, 2021

Conversation

buyaa-n
Copy link
Member

@buyaa-n buyaa-n commented Sep 30, 2021

Customer Impact

This fixes a customer-reported issue related to a combination of SupportedOSPlatform attributes along with flow analysis handling of OperatingSystem guard methods. With this edge case bug, we report a false-positive diagnostic on a call site, indicating that the call site is reachable on a version of the platform that differs from what is actually reachable.

Here is the specific scenario that was reported:

using System.Runtime.Versioning;

[SupportedOSPlatform("android23.0")]
[SupportedOSPlatform("ios13.0")]
[SupportedOSPlatform("windows8.1")]
[UnsupportedOSPlatform("MacCatalyst13.0")]
class MyUsage
{
    public void M()
    {
        if (!OperatingSystem.IsWindows() || OperatingSystem.IsWindowsVersionAtLeast(10, 0, 10240))
        {
            // FALSE POSITIVE DIAGNOSTIC REPORTED
            // CA1416: This call site is reachable on: 'android' 23.0 and later, 'ios' 13.0 and later,
            // 'windows' 8.1 and later. 'MyType' is only supported on: 'windows' 10.0.10240 and later.
            var t = new MyType();
        }
    }
}
[SupportedOSPlatform("android23.0")]
[SupportedOSPlatform("ios13.0")]
[SupportedOSPlatform("windows10.0.10240")]
class MyType { }

It is caused by the logic we use for evaluating the flow analysis result, we suppress warnings with 2 steps:

  1. Call site attributes -> with this one the [SupportedOSPlatform("android23.0")] [SupportedOSPlatform("ios13.0")] attributes are suppressed (removed), therefore only unmatching [SupportedOSPlatform("windows10.0.10240")] will be left for MyType
  2. Check if the flow analysis result !Windows() || Windows10OrLater() is guarding the API call, from the OR conditionals the !Windows() is not guarding the API for the remaining attribute [SupportedOSPlatform("windows10.0.10240")] therefore there is a warning

The issue is we are not accounting for the call site reachability when evaluating the flow analysis result. The fix here is now accounting for that in the flow analysis result evaluation.

Testing

We have thorough unit test coverage for related scenarios, and new tests were added for these edge cases.

Risk

Low. Minimal performance implications of adding additional code during flow analysis evaluation of which platforms/versions can reach the call site.

@terrajobst
Copy link
Member

/cc @dotMorten

@jeffhandley
Copy link
Member

I will send an email to Tactics to request approval of this change for 6.0 GA.

Copy link
Member

@terrajobst terrajobst left a comment

Choose a reason for hiding this comment

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

Haven't looked at the implementation ('cause I lost my PhD in flow analysis) but the tests looked good :-)

@jmarolf
Copy link
Contributor

jmarolf commented Oct 1, 2021

@jeffhandley this was approved so I am enabling auto-merge on this PR.

@buyaa-n buyaa-n merged commit bd21c21 into dotnet:release/6.0.1xx Oct 1, 2021
@buyaa-n buyaa-n deleted the account_callsite_on_guard branch October 1, 2021 02:35
@dotMorten
Copy link

Thanks everyone for fixing this so quick. My code base is gonna be a lot happier now 😁

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants