PlatformCompatibilityAnalyzer: Assertions should work the same way as nullable

[pranavkm]

Consider:

string? foo = SomeFunction();
Debug.Assert(foo != null);
// After this point, foo is considered non-null
foo.ToString(); // Does not trigger a nullable warning

Debug.Assert(OperatingSystem.IsWindows());
new SomeUnsupportedAPI(); // Results in a warning.

[mavasani]

@buyaa-n I thought we did have tests for this case? If not, can we add a regression test that fails? Also you may want to pull in the latest bits from RC2 branch to confirm it still repros.

[buyaa-n]

We have a test for one or two cases, don't have for all guard methods. Will check this out, thanks

[buyaa-n]

@mavasani it works for checking the Supported platform Debug.Assert(OperatingSystem.IsWindows());, but not working when using ! operator for checking Unsupported platform. Flow analysis not returning any result in case we assert with not Debug.Assert(!OperatingSystem.IsWindows());. Is that by design, could we fix it?

[buyaa-n]

Sample test

using System;
using System.Diagnostics;
using System.Runtime.Versioning;

class Test
{
    void M1()
    {
        Debug.Assert(OperatingSystemHelper.IsWindows());
        new ClassSupportedWindows();
        
        [|SupportedWindows10()|];
        [|UnsupportedWindows()|];
    }

    void M2()
    {
        Debug.Assert(OperatingSystemHelper.IsOSPlatformVersionAtLeast(""Windows"", 10, 2));
        SupportedWindows10();
        new ClassSupportedWindows();

        [|UnsupportedWindows()|];
    }

    void M3()
    {
        Debug.Assert(!OperatingSystemHelper.IsWindows());
        [|SupportedWindows10()|];
        [|new ClassSupportedWindows()|];

        UnsupportedWindows(); // fails
    }

    [SupportedOSPlatform(""Windows10.0"")]
    void SupportedWindows10() { }

    [SupportedOSPlatform(""Windows"")]
    class ClassSupportedWindows { }

    [UnsupportedOSPlatform(""Windows"")]
    void UnsupportedWindows() { }
}

[mavasani]

@buyaa-n Debug.Assert(!OperatingSystem.IsWindows()); only tells us that current platform is not Windows, but can be anything else. Unless we have a way to mark an API as Supported on Windows and Unsupported on every other platform, I am not sure if assert really helps add any guard. In short, do we have a real world scenario affected by this case that we are trying to fix? Otherwise, I would rather wait for one...

[mavasani]

@pranavkm Which version of the analyzer assembly/package are you using? If you are hitting this on the latest version, it might very well be in context of other code in the same method. Would you be able to provide us a standalone repro where you hit this? If not, based on @buyaa-n's comment we will not be able to make further progress.

@buyaa-n we should still create a test-only PR to add the above test.

[buyaa-n]

@buyaa-n Debug.Assert(!OperatingSystem.IsWindows()); only tells us that current platform is not Windows, but can be anything else. Unless we have a way to mark an API as Supported on Windows and Unsupported on every other platform, I am not sure if assert really helps add any guard.

@mavasani when and API has [UnsupportedOSPlatform(""platform"")] attribute it means the attribute only not supported on that platform but supported on all others. The only way for guarding an unsupported API call is to use to call corresponding OperatingSystem check methid with ! operator. It is working well with if conditional check

In short, do we have a real world scenario affected by this case that we are trying to fix? Otherwise, I would rather wait for one...

[UnsupportedOSPlatform("Windows")] might be not that realistic scenario but it is important for Browser wasm cases. There is many APIs not supported only on "browser" which is being annotated accordingly in runtime repo. It would be useful if Debug.Assert support not operator

@buyaa-n we should still create a test-only PR to add the above test.

Sure i can add the test

[mavasani]

It is working well with if conditional check

Interesting, then I am wondering why it doesn't work for assert case as both should go down the same code path. Let me check...

[mavasani]

Thanks @buyaa-n - I have opened #4196 to fix the issue you cited.

@pranavkm I am still unable to reproduce the original issue that you reported.

[pranavkm]

• git clone https://github.com/pranavkm/repro.git
• git checkout platformcompatanalyzer
• dotnet build

I'm using SDK v5.0.100-rc.2.20471.7.

[mavasani]

@pranavkm Seems like the warning is by design as you asserting you are on Windows code path and then invoking an API marked as unsupported on Windows?

Did you mean to have Debug.Assert(!OperatingSystem.IsWindows());, which does generate a false positive, as noted by @buyaa-n and fixed with #4196

[buyaa-n]

Did you mean to have Debug.Assert(!OperatingSystem.IsWindows());

Yeah, i thought @pranavkm forgot to add the ! operator, otherwise it will suppress warning only for [SupportedOSPlatform(""Windows"")] which is working as expected

[pranavkm]

Fixing the bug doesn't help:

[mavasani]

@pranavkm You will need to wait for #4196 for the fix to the negation within Debug.Assert scenario.

[mavasani]

Fixed in release/dotnet5-rc2 with #4196