CA1416 handle call-site empty sets causing warnings when TFM attributes applied

[buyaa-n]

Related to dotnet/runtime#44231

By design, SDK adds SuuportedOSPlatform attribute with the targeted OS platform from TFMs, which works well for normal condition but for multitargeted env like runtime repo it is causing warnings mentioned in dotnet/runtime#44231 and requiring clear separation of platform-specific APIs which forcing developers to duplicate their code to conform to the strict supported-platform name rules. After evaluating of possible solution and several discussions we have decided to resolve it at the analyzer level. The TFM target attributes are causing inconsistent scenarios in the call site and the real attributes within that assembly are being ignored or malfunctioned which causing the warnings, this shouldn't happen in normal development. By the main rule of "Child API attributes cannot widen parent or containing type attributes" when the inconsistency happens in the call site and call site evaluated to an empty set of support not gonna warn within that empty set context.

cc @jeffhandley @terrajobst

Examples:

[SupportedOSPlatform("windows7")]
class Test1
{
    [UnsupportedOSPlatform("windows9")]
    void StillSupportedOnWindows7and8()
    {
        SupportedOnWindows10(); // Warning; calling from Windows 7-8 is still supported but it will fail
    }
    [UnsupportedOSPlatform("windows7")]
    void NotSupportedAnywhere()
    {
        SupportedOnWindows10(); // No warning; no valid call sites
    }
    [SupportedOSPlatform("windows10")]
    void SupportedOnWindows10()
    {
    }
}
[SupportedOSPlatform("windows")]
[SupportedOSPlatform("macos")]
class Test2
{
    [SupportedOSPlatform("macos")]
    class Test2MacOs
    {
        [SupportedOSPlatform("windows")]
        void WindowsOnly()
        {
            // No valid call sites; no warnings no matter what
        }
        [SupportedOSPlatform("linux")]
        void LinuxOnly()
        {
            // No valid call sites; no warnings no matter what
        }
    }
}

[buyaa-n]

Because of attributes kept in the cache i needed to add Call-site info for subsequent calls made within that call site

[codecov]

Codecov Report

Merging #4740 (f59f285) into master (33582d0) will decrease coverage by 0.03%.
The diff coverage is 94.69%.

@@            Coverage Diff             @@
##           master    #4740      +/-   ##
==========================================
- Coverage   95.71%   95.68%   -0.04%     
==========================================
  Files        1196     1193       -3     
  Lines      271237   271512     +275     
  Branches    16386    16411      +25     
==========================================
+ Hits       259611   259788     +177     
- Misses       9477     9620     +143     
+ Partials     2149     2104      -45     

[mavasani]

@buyaa-n just trying to understand the expected semantics for the below example you provided:

[SupportedOSPlatform("windows")]
[SupportedOSPlatform("macos")]
class Test2
{
    [SupportedOSPlatform("macos")]
    class Test2MacOs
    {
        [SupportedOSPlatform("windows")]
        void WindowsOnly()
        {
            // No valid call sites; no warnings no matter what
        }
        [SupportedOSPlatform("linux")]
        void LinuxOnly()
        {
            // No valid call sites; no warnings no matter what
        }
    }
}

Applying [SupportedOSPlatform("macos")] attribute on Test2MacOs but not [SupportedOSPlatform("windows")] is equivalent to [UnsupportedOSPlatform("windows")] being applied to Test2MacOs. Due to this, WindowsOnly method with [SupportedOSPlatform("windows")] becomes a method with an empty set of supported attributes, hence has no valid call sites. Is this a correct analysis?

[buyaa-n]

Applying [SupportedOSPlatform("macos")] attribute on Test2MacOs but not [SupportedOSPlatform("windows")] is equivalent to [UnsupportedOSPlatform("windows")] being applied to Test2MacOs. Due to this, WindowsOnly method with [SupportedOSPlatform("windows")] becomes a method with an empty set of supported attributes, hence has no valid call sites. Is this a correct analysis?

Yes that is correct, it is equivalent to [UnsupportedOSPlatform("windows")] being applied

[jeffhandley]

@buyaa-n Do I interpret this change correctly in that there's no assembly-level special-casing of the empty set behavior and that this is the desired "correct" fix to achieve empty set detection?

With these changes, how many of the dotnet/runtime browser-build warnings does this clean up, and how many are left?

[buyaa-n]

@buyaa-n Do I interpret this change correctly in that there's no assembly-level special-casing of the empty set behavior and that this is the desired "correct" fix to achieve empty set detection?

Yes it does not include my previous solution with assembly-level special-casing, it is based on our last discussion the desired "correct" fix to achieve empty set detection

With these changes, how many of the dotnet/runtime browser-build warnings does this clean up, and how many are left?

All are cleaned up, no one left

[jeffhandley]

Yes it does not include my previous solution with assembly-level special-casing, it is based on our last discussion the desired "correct" fix to achieve empty set detection

💯 /cc @terrajobst -- the correct fix is in here, so no need for a follow-up item

All are cleaned up, no one left

Great!

[mavasani]

Just curious - does the reverse annotation work? i.e. if I mark the containing assembly or type as unsupported for a platform A and just a single method/property inside it as supported, will we get warnings inside that method/property if it has guarded runtime checks within it?

[buyaa-n]

Just curious - does the reverse annotation work? i.e. if I mark the containing assembly or type as unsupported for a platform A and just a single method/property inside it as supported

Only parent attribute would work, as a child cannot extend parent support supported attribute on M2 will be ignored

[UnsupportedOSPlatform(""Browser"")] // Deny list => unsupported only on browser but supported on all others 
public class Test
{
    [SupportedOSPlatform(""browser"")] // child cannot extend parent support, annotation willl be ignored
    public void M2 ()  // the above annotation doesn't produce empty set as parent only unsupported on browser but supported on all other 
    {
         // All platforms except browser can access here, so we would warn for platform-specific APIs usage 
    }
}

will we get warnings inside that method/property if it has guarded runtime checks within it?

Good question, this combination is not producing the empty set, so we should warn

[mavasani]

Why is this the case? If I had a type such that only one of the method supported a platform, why would I be required to add the supported attribute on the whole type as opposed to just the method?

[buyaa-n]

Right, as you mentioned it is not a meaningful or realistic scenario, but it is happening in multitargeted builds for runtime repo. We have assemblies for multiple platforms where some APIs annotated as platform-specific, the assembly doesn't have any platform attributes but it does build for each platform specified:

 <TargetFrameworks>net5.0-windows;net5.0-Unix;net5.0-tvOS;net5.0-Browser</TargetFrameworks>

For example, when it builds for windows net5.0-windows target MSBuild adds assembly-level [SupportedOSPlatform("windows")] attributes then the individual APIs having different platforms support would produce this scenario and we see warnings within their body for those targeted builds, this is the main reason we are doing this change, we don't want to warn in this case

[mavasani]

LGTM. Have some minor questions, but none of those seem blocking.