[Platform Compatibility Analyzer] Nested APIs shouldn't be allowed to expand the platform set

[terrajobst]

Repro

[SupportedOSPlatform("ios")]
class Some
{    
    [SupportedOSPlatform("windows")]
    public static void Api() {}
}

Expected

'Api' cannot widen the set of supported platforms of its containing symbol Some.

Work Items

Each of these should produce separate diagnostic ids:

• Inconsistent List section of the Advanced Scenarios of Combining Attributes, when conflicting attributes are applied at the same level
• Nested APIs shouldn't be allowed to expand the platform set and version support

[carlossanlop]

Talked with @buyaa-n and since this proposal would require a new ruleID, we think it should go through API review, so I'm moving it to the runtime repo.

[carlossanlop]

@buyaa-n will help group this issue with these two others into one proposal:
https://github.com/dotnet/roslyn-analyzers/issues/4014
dotnet/roslyn-analyzers#4015

[buyaa-n]

Category: Interoperability
Suggested Severity: Info

This analyzer might be implemented as part of #45851 and should cover the following main scenarios:

• Nested APIs shouldn't be allowed to expand the platform set and version support

  • Note: this is happening in runtime for multitargeted projects when SDK adds [SupportedOSPlatform] attribute for the targeted builds

• It should flag Inconsistent list determined by the following rule:

  • If either [SupportedOSPlatform] or [UnsupportedOSPlatform] attributes are present, we group all attributes by OS platform identifier:
    • Allow list. If the lowest version for each OS platform is a [SupportedOSPlatform] attribute, the API is considered to only be supported by the listed platforms and unsupported by all other platforms.
    • Deny list. If the lowest version for each OS platform is a [UnsupportedOSPlatform] attribute, then the API is considered to only be unsupported by the listed platforms and supported by all other platforms.
    • Inconsistent list. If for some platforms the lowest version attribute is [SupportedOSPlatform] while for others it is [UnsupportedOSPlatform], the analyzer will produce a warning on the API definition because the API is attributed inconsistently.

• We can add multiple attributes for an API with the same platform, with the following rule:

  • Allow list could have only one [SupportedOSPlatform] attribute and an optional [UnsupportedOSPlatform] attribute with > version. This implies that if an API was supported on a platform then could have unsupported later, but it cannot be re-supported back
  • Deny list. could have only two [UnsupportedOSPlatform] attributes and one [SupportedOSPlatform] attribute. This implies that an API was unsupported for a platform and later added a support, then can be unsupported back. But after that, it cannot be re-supported back

[buyaa-n]

Examples:

• Nested APIs shouldn't be allowed to expand the platform set and version support

[SupportedOSPlatform("ios")]
class Some
{    
    [SupportedOSPlatform("windows")] // Warn: 'Api' cannot widen the set of supported platforms of its containing symbol `Some`.
    public static void Api() {}
}

• Inconsistent list

[UnsupportedOSPlatform("browser")] // Warn: `Inconsistent platform 'browser' found in the Supported platforms list`
[SupportedOSPlatform("windows")]
[UnsupportedOSPlatform("windows10.0.19041")]
public void Api1();
// Fixer might suggest remove `[UnsupportedOSPlatform("browser")] `

 Valid consistent list example

[SupportedOSPlatform("browser")] // No warning
[SupportedOSPlatform("windows")]
[UnsupportedOSPlatform("windows10.0.19041")]
public void Api1();

• Multiple attributes for same platform
  • Allow list

  [SupportedOSPlatform("ios12.0")] // If two SupportedOSPlatform attributes applied, the one with lowest version accepted
  [SupportedOSPlatform("ios15.0")] // Warns:  `Platform 'ios' is already supported from version 12.0`
  public void OnlyWorksOniOS();
  
  [SupportedOSPlatform("ios12.0")] // If two SupportedOSPlatform attributes applied, the one with lowest version accepted
  [UnsupportedOSPlatform("ios14.0")]
  [SupportedOSPlatform("ios15.0")] // Warns:  `Platform 'ios' is unsupported from version 14.0 cannot re supported back`
  public void OnlyWorksOniOS();

  • Deny list

  [UnsupportedOSPlatform("windows")]
  [SupportedOSPlatform("windows8.0")]
  [UnsupportedOSPlatform("windows10.0.19041")]
  [SupportedOSPlatform("windows10.0.2004")] // Warns: `Platform 'windows' is unsupported from version 10.0.19041 cannot re supported back`
  public void WorksOnWindows8To10();
  
  [UnsupportedOSPlatform("windows")]
  [UnsupportedOSPlatform("windows10.0.19041")] // Warns: `Platform 'windows' is unsupported for all versions`
  [SupportedOSPlatform("windows10.0.2004")]
  public void WorksOnWindows10();

Fixer can suggest removing the invalid attribute

[terrajobst]

Video

Makes sense. Two thoughts:

1. Should be a warning and on by-default.
2. The analyzer today can only handle an API coming and going out of support. In principle, there is nothing ill-defined about an API being re-supported after it went out of support, but that seems rare. It feels OK to treat this as a warning until we need to support it.

[jeffhandley]

Throughout .NET 6, we've seen less evidence that we need to have an analyzer warning about this. Our rules for this scenario have continued to be refined, and we have intentionally opened up some scenarios where nested APIs can expand the parent set of platforms--particularly when dealing with cross-platform/platform-neutral libraries.

As a result, I'm going to close this issue; we can revisit after .NET 6 if appropriate.