CA-1418 Validate platform string

[buyaa-n]

Validates platform string

Provided in constructor arguments [SupportedOSPlatform("platform")], [UnsupportedOSPlatform("platform")] attributes and OperatingSystem.IsOSPlatform("platform"), OperatingSystem.IsOSPlatformVersionAtLeast("platform", 1) methods

Fixes dotnet/runtime#45851

• Validates if platform name is known. The know platform names 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 add custom platform name
• Optionally validates version part if provided

Example:

class Some
{    
    public static void GuardedCall()
    {
        if (OperatingSystem.IsOSPlatform("x")) // The platform 'x' is not a known platform name
            SomeType.Api1();

        if (OperatingSystem.IsOSPlatformVersionAtLeast("x", 1)) // The platform 'x' is not a known platform name
            SomeType.Api1();
    }
}

class SomeType
{
    [SupportedOSPlatform("x")] // The platform 'x' is not a known platform name
    public static void Api1() { }

    [UnsupportedOSPlatform("y")] // The platform 'y' is not a known platform name
    public static void Api2() { }

    // The '7' is not a valid version. Please provide an input having two to four numbers separated with a dot.
    [UnsupportedOSPlatform("windows7")] 
    public event EventHandler Windows7Event;
}

[jeffhandley]

Looks good to me. I just have a question about the versions that might have been covered during the API review of this already: Should we respect the number of version segments each OSPlatform supports? Several only support 3 segments; some don't support versions at all.

[buyaa-n]

Looks good to me. I just have a question about the versions that might have been covered during the API review of this already: Should we respect the number of version segments each OSPlatform supports? Several only support 3 segments; some don't support versions at all.

I don't think that is covered during API review, i am not sure if that information for all OS are available somewhere, if we could get that info we can add that check

[jeffhandley]

If the platform was found in the OperatingSystem.Is<Platform> check, we could check to see if there's a ...VersionAtLeast function as well. If not, versions aren't supported; if so, then the min/max numbers of parameters would define the supported segments.

[Youssef1313]

AFAIK, some editorconfig options doesn't do anything without something like [*.cs].

@mavasani Can you confirm whether this is correct for this specific option?

If so, then there is a test gap.

[buyaa-n]

AFAIK, some editorconfig options doesn't do anything without something like [*.cs].

hm, not sure, i was using it like this previously and it still works here too

[codecov]

Codecov Report

Merging #4838 (6428d82) into release/6.0.1xx-preview2 (3c2b67e) will decrease coverage by 0.00%.
The diff coverage is 97.58%.

@@                     Coverage Diff                      @@
##           release/6.0.1xx-preview2    #4838      +/-   ##
============================================================
- Coverage                     95.61%   95.61%   -0.01%     
============================================================
  Files                          1167     1169       +2     
  Lines                        266899   267438     +539     
  Branches                      16134    16174      +40     
============================================================
+ Hits                         255206   255716     +510     
- Misses                         9613     9619       +6     
- Partials                       2080     2103      +23     

[buyaa-n]

If the platform was found in the OperatingSystem.Is<Platform> check, we could check to see if there's a ...VersionAtLeast function as well. If not, versions aren't supported; if so, then the min/max numbers of parameters would define the supported segments.

@jeffhandley added validation for the number of version segments each OSPlatform supports, as version is optional we need only a max number of parameters. You might want to review messaging, sample messages are below cc @terrajobst :

// versions are not supported case:
[SupportedOSPlatform("Linux4.1")|}] // The platform 'Linux' is not versioned, please remove version part
public void SupportedLinux_41() { }

// a platform support Max 4 version segments
[SupportedOSPlatform("Android4.8.1.2.3")|}] // The '4.8.1.2.3' is not a valid version. Please provide an input having 2 to 4 numbers separated with a dot.
public void SupportedOSPlatformAndroid5Parts() { }

// a platform support Max 3 version segments
[UnsupportedOSPlatform("Ios14.1.2.3")] // The '14.1.2.3' is not a valid version. Please provide an input having 2 to 3 numbers separated with a dot.
public void UnsupportedOSPlatformIos4PartsInvalid() { }

[jeffhandley]

I suggest the following messages. Each also shows using lower-case for the platform names in the messages to recommend that practice.

// versions are not supported case:
[SupportedOSPlatform("Linux4.1")|}] // Version '4.1' is not valid for the platform 'linux'. Do not use versions for this platform.
public void SupportedLinux_41() { }

// a platform support Max 4 version segments
[SupportedOSPlatform("Android4.8.1.2.3")|}] // Version '4.8.1.2.3' is not valid for the platform 'android'. Use a version with 2-4 parts for this platform.
public void SupportedOSPlatformAndroid5Parts() { }

// a platform support Max 3 version segments
[UnsupportedOSPlatform("Ios14.1.2.3")] // Version '14.1.2.3' is not valid for the platform 'ios'. Use a version with 2-3 parts for this platform.
public void UnsupportedOSPlatformIos4PartsInvalid() { }

// a platform supporting Max 2 version segments
[UnsupportedOSPlatform("foo1.2.3"] // Version '1.2.3' is not valid for the platform 'foo'. Use a version with 2 parts for this platform.

[jeffhandley]

I have a couple suggestions and I also commented with suggested messages, but otherwise looks good to me. Once my feedback is addressed, I approve without needing to re-review.

[buyaa-n]

I suggest the following messages. Each also shows using lower-case for the platform names in the messages to recommend that practice.

Thanks, I like the messages except the lower casing part, isn't it better to report back the platform names as it is provided by the user for expressing that there is nothing wrong with the platform name itself?

[jeffhandley]

Thanks, I like the messages except the lower casing part, isn't it better to report back the platform names as it is provided by the user for expressing that there is nothing wrong with the platform name itself?

Fair enough; thanks for considering it.

[mavasani]

This will throw if the argument kind is array: http://sourceroslyn.io/#Microsoft.CodeAnalysis/Symbols/TypedConstant.cs,79

I would suggest adding a .Kind != TypedConstantKind.Array before accessing .Value

[buyaa-n]

This is called only if the attribute were SupportedOSPlatform or UnsupportedOSPlatform attribute, which only accepts a string argument, if anyone calls them with an array type there will be a compiler error, so i think that is unnecessary, do you think that check is still needed?