Roslyn-analyzers: Platform compat analyzer guard method not working when OSPlatform.Create(string) used

[buyaa-n]

Analyzer CA1416 Platform compat analyzer

Describe the bug

class Program
{
    static void Main(string[] args)
    {
        if (!RuntimeInformation.IsOSPlatform(OSPlatform.Create("windows")))
        {
            Foo(); // not supposed to warn
        }
    }

    [UnsupportedOSPlatform("windows")]
    public static void Foo() {}
}

Expected behavior

Not warn

Actual behavior

Warning

I think we want to warn it in RC2

[mavasani]

NOTE: Implementing a special case where Create call is an argument would be easy to handle in the analyzer's OperationVisitor. However, implementing full fledged dataflow support for tracking OSPlatform values is slightly more complicated, though doable. For example:

class Program
{
    static void M(bool isWindows)
    {
        var windows = OSPlatform.Create("windows");
        var linux = OSPlatform.Create("linux");
        var platform = isWindows ? windows : linux;  // Needs tracking how `OSPlatform` instances flows, we already have this information with the underlying PointsToAnalysisResult though
        if (!RuntimeInformation.IsOSPlatform(platform))
        {
            Foo(); // not supposed to warn
        }
    }

    [UnsupportedOSPlatform("linux")]
    [UnsupportedOSPlatform("windows")]
    public static void Foo() {}
}

@buyaa-n I think the fix would be pass in DataFlowAnalysisContext.PointsToAnalysisResult to the below call: https://github.com/dotnet/roslyn-analyzers/blob/58c58c1064658da9e6376649138cea03253031cb/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/InteropServices/PlatformCompatabilityAnalyzer.OperationVisitor.cs#L39. The decoding method can query the points to result to get the underlying PointsToAbstractValue, which has the underlying creation locations for all possible values. For the above example, it will be the IInvocationOperations for the OSPlatform.Create calls, so you should be able to look for constant string argument to the invocation to decode the platform.

Let me know if you need further pointers.

[mavasani]

Ah, actually OSPlatform is a struct so we will not have this information in the PointsToAnalysisResult. Handling the non-trival case like above will be bit more tricky. Probably we can enhance ValueContentAnalysis for it. Let me know if the feature request here is just for the simple case you mentioned or full fledged flow analysis of OSPlatform values.

[buyaa-n]

NOTE: Implementing a special case where Create call is an argument would be easy to handle in the analyzer's OperationVisitor. However, implementing full fledged dataflow support for tracking OSPlatform values is slightly more complicated, though doable.

I was planning to add the argument support in OperationVisitor, didn't think about supporting the value in local

[buyaa-n]

Ah, actually OSPlatform is a struct so we will not have this information in the PointsToAnalysisResult. Handling the non-trival case like above will be bit more tricky. Probably we can enhance ValueContentAnalysis for it. Let me know if the feature request here is just for the simple case you mentioned or full fledged flow analysis of OSPlatform values.

This is a bug reported from customer, for now just the simple case, guess in the future we might want to handle the local cache, CC @pranavkm is the local cache is important for you case?

[pranavkm]

I think making the trivial case work would suffice. It unblocks using this in libraries that cross-compile which right now would require complicated #ifdef shennanigans.

[mavasani]

@pranavkm FYI the fix should even handle non-trivial cases mentioned above. You can pickup the next package that gets uploaded here: https://dev.azure.com/dnceng/public/_packaging?_a=package&feed=dotnet5&package=Microsoft.CodeAnalysis.NetAnalyzers&protocolType=NuGet&version=5.0.0-rc2.20453.1&view=versions. It should take about 20-30 minutes for the new package to show up.

[mavasani]

Also tagging @jeffhandley