Refering to unknown platform names should result in warnings

[terrajobst]

We use string literals for OS platform names. We should validate that the name is known. The list is the combined set from two places:

1. OperatingSystem.Is(PlatformName)[VersionAtLeast](). That's the target framework's known set of platforms.
2. The project's MSBuild item group SupportedPlatform. This is the project's specific knowledge of known platforms. This allows class library authors to target an older version of .NET that can still light up on later versions with more platform support.

Note: The diagnostics should be raised for the definitions the attributes are applied to, not use sites of the APIs. IOW, APIs with unknown platform annotations should be treated as having no attributes applied. This avoids cascading errors for the author.

Note: We should treat the universal OperatingSystem.IsPlatform() and OperatingSystem.IsPlatformVersionAtLeast() similar to attributes in that if we can't statically determine its value or if the value isn't in the known set, we should provide a warning.

Repro

class Some
{    
    public static void UseSite()
    {
        SomeType.Api1();
        SomeType.Api2();
        SomeType.Api3();
    }

    public static void GuardedCall()
    {
        if (OperatingSystem.IsOSPlatform("x")) // $1
            SomeType.Api1();

        if (OperatingSystem.IsOSPlatformVersionAtLeast("x", 1)) // $2
            SomeType.Api1();
    }
}

class SomeType
{
    [SupportedOSPlatform("x")] // $3
    public static void Api1() { }

    [UnsupportedOSPlatform("y")] // $4
    public static void Api2() { }

    [UnsupportedOSPlatform("z")] // $5
    public static void Api3() { }
}

Expected

$1: The platform 'x' isn't a known platform name.
$2: The platform 'x' isn't a known platform name.
$3: The platform 'x' isn't a known platform name.
$4: The platform 'y' isn't a known platform name.
$5: The platform 'z' isn't a known platform name.

[carlossanlop]

@buyaa-n @terrajobst Was this already approved in the runtime repo in the API proposal meeting? If yes, can you please link the issue number? It can be confusing to have proposals open in this repo and in runtime at the same time. I think we should keep all proposals in the runtime repo.

[buyaa-n]

@carlossanlop when discussed offline i told you this will be covered with #43971, as they could/would be one analyzer, but this issue description has more context about the proposed new analyzer, so i am transferring this one into runtime and prepare it for API review

[Dotnet-GitSync-Bot]

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

[jeffhandley]

Hey @terrajobst -- @marek-safar mentioned the potential for custom platform names that we would want to avoid producing warnings for. What's the authoritative way to determine if a platform name is "known"?

[terrajobst]

I'm not convinced we need this yet. Platforms are rarely added and usually coincide with .NET releases which gives us the ability to add new methods on the OperatingSystem class. This time, we're talking about macOS Catalyst, for example.

The benefit of validating user typos outweighs the (at this point) theoretical benefit of more flexibility for us IMHO.

[marek-safar]

I think the list should be somehow open. If someone would like to experiment with porting .NET to a new platform in runtimelab if should not be hit with hard to fix warnings.

[buyaa-n]

I think the list should be somehow open.

I think the project's MSBuild item group <SupportedPlatform/> could be used for that

[terrajobst]

I think the project's MSBuild item group <SupportedPlatform/> could be used for that

That's not a bad idea actually, I like that.

[buyaa-n]

Category: Interoperability
Suggested severity: Info

This analyzer should also cover scenario in dotnet/roslyn-analyzers#4015 provide definition-site diagnostics if the syntax of the string passed to the attributes isn't of a valid platform name followed by an optional version.

The version part should be a valid parsable version (2-4 numbers separated by a dot), I don't think we should/could determine the correctness of versions.

Flag:

[SupportedOSPlatform("window7.0")] // Invalid platform
public static void Api1() { } // The platform 'window' isn't a known platform name.

[SupportedOSPlatform("windows10.0.")] // the number is not valid
public static void Api1() { } // The version part '10.0.' isn't a parsable version. Please provide an input having two to four version components.

[SupportedOSPlatform("windows 10.0")] // cannot have space or any other separators
public static void Api1() { } // The platform 'windows 10.0' isn't a known platform name.

[SupportedOSPlatform("10.0windows")] // platform and version order invalid
public static void Api1() { } // The platform '10.0windows' isn't a known platform name.

[SupportedOSPlatform("windows10")] // one number is not valid
public static void Api1() { } // The version part '10' isn't a parsable version. Please provide an input having two to four version components.

// Fixer might suggest add a dot and 0: [SupportedOSPlatform("windows10.0")] 

[UnsupportedOSPlatform("windows10.0.0.1.2")] // 5 numbers, not valid
public static void Api2() { } // The version part '10.0.0.1.2' isn't a parsable version. Please provide an input having two to four version components

// Fixer might suggest to remove extra number parts: [UnsupportedOSPlatform("windows10.0.0.1")]

Do Not Flag:

[SupportedOSPlatform("windows10.0")] // two numbers separated by dot
public static void Api1() { } // No warning

[SupportedOSPlatform("windows9.0")] // Inexistent version will not be flagged
public static void Api1() { } // No warning

[UnsupportedOSPlatform("windows10.0.1")] // 3 numbers, valid
public static void Api2() { }

[UnsupportedOSPlatform("Windows10.0.0.1")] // 4 numbers, valid
public static void Api3() { }

[SupportedOSPlatform("windows")] // version less, valid
public static void Api1() { } 

[SupportedOSPlatform("WINDOWS")] // case insensitive
public static void Api1() { } 

I am not sure if we need a fixer, for not known platform names fixer doesn't make sense, for invalid versions we could suggest add missing .0 or remove extra number parts

[terrajobst]

Video

The severity should be a warning and on by-default. It seems this can't produce false positives, especially with the escape mechanism of adding a net-new platform via the SupportedPlatform item group in the project file. /cc @marek-safar

[buyaa-n]

Solved with dotnet/roslyn-analyzers#4838