Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Developers need more flexibility to annotate their code for platform supported/unsupported scenarios to reduce issues warnings noise #44922

Closed
1 task done
jeffhandley opened this issue Nov 19, 2020 · 5 comments
Assignees
Labels
area-Meta Cost:M Work that requires one engineer up to 2 weeks Priority:2 Work that is important, but not critical for the release Team:Libraries User Story A single user-facing feature. Can be grouped under an epic.
Milestone

Comments

@jeffhandley
Copy link
Member

jeffhandley commented Nov 19, 2020

The Platform Compatibility Analyzer was introduced in .NET 5.0, raising new diagnostics when APIs are referenced that are unsupported on targeted platforms. The analyzer relies on the [SupportedOSPlatform] and [UnsupportedOSPlatform] attributes, which can be applied to assemblies, types, and members. These annotations only provide a mechanism for marking an API as entirely supported/unsupported--there is currently no way to annotate an API as partially or conditionally supported on a platform.

Conditionally-Supported Platform Annotations

With a mechanism for marking an API as being conditionally supported/unsupported on a platform, we could improve annotations for APIs such as MemoryMappedFile.CreateNew(), where calls with a named memory mapped file (that is, a non-null mapName) are supported on Windows operating systems only.

This could involve creating new attributes such as [SupportedOSPlatformIfNull], [SupportedOSPlatformIfNotNull], [UnsupportedOSPlatformIfNull], and [UnsupportedOSPlatformIfNotNull]. Occurrences of conditional platform support should be researched to determine what types of conditions need to be modeled in such attributes.

Platform-Guard Assertion Annotations

The analyzer already recognizes platform guards using the methods on OperatingSystem, such as OperatingSystem.IsWindows and OperatingSystem.IsWindowsVersionAtLeast. However, the analyzer does not recognize other guard methods or even helper methods that assert platform guards. Expanding this support could involve creating new attributes that indicate that an API asserts platform checks the same way the APIs on OperatingSystem do, potentially even conditionally.

One example is in Thread, there is an internal field for IsThreadStartSupported that indicates whether or not the current platform supports calls to Thread.Start, regardless of which platforms are targeted. Annotations could allow this to be modeled for the analyzer to warn if Threat.Start is called without first checking the corresponding IsThreadStartSupported member.

Additionally, it's expected that projects will commonly create helper methods that wrap around the OperatingSystem calls and these are not currently recognized through the flow analysis in the analyzer. Helper methods that perform platform assertion could be annotated to inform the analyzer that invocation or specific return values indicate platform support.

  • Conditionally-Supported Platform Annotations removed from scope
  • Platform-Guard Assertion Annotations
@jeffhandley jeffhandley added this to the 6.0.0 milestone Nov 19, 2020
@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added the untriaged New issue has not been triaged by the area owner label Nov 19, 2020
@jeffhandley jeffhandley added Priority:2 Work that is important, but not critical for the release and removed untriaged New issue has not been triaged by the area owner labels Nov 19, 2020
@marek-safar marek-safar changed the title Support conditional platform supported/unsupported scenarios and platform assertion helper methods Developers have need more flexibility to annotate their code for platform supported/unsupported scenarios to reduce issues warnings noise Nov 27, 2020
@marek-safar marek-safar added the User Story A single user-facing feature. Can be grouped under an epic. label Nov 27, 2020
@marek-safar marek-safar changed the title Developers have need more flexibility to annotate their code for platform supported/unsupported scenarios to reduce issues warnings noise Developers need more flexibility to annotate their code for platform supported/unsupported scenarios to reduce issues warnings noise Nov 27, 2020
@buyaa-n buyaa-n added the Cost:M Work that requires one engineer up to 2 weeks label Jan 14, 2021
@jeffhandley
Copy link
Member Author

Related: #47593

@buyaa-n
Copy link
Contributor

buyaa-n commented Mar 2, 2021

Conditionally-Supported Platform Annotations

This could involve creating new attributes such as [SupportedOSPlatformIfNull], [SupportedOSPlatformIfNotNull], [UnsupportedOSPlatformIfNull], and [UnsupportedOSPlatformIfNotNull].

Thanks, I think we can have more generalized attribute to support null, not null, or other values

From the conditional platform support occurrences I found from Mark Windows-specific APIs design doc there is usually APIs are supported only on windows when a parameter is not null, some depend on enum value provided.

[AttributeUsage(AttributeTargets.Constructor | AttributeTargets.Method | System.AttributeTargets.Property, AllowMultiple = true, Inherited = false)]
public sealed class SupportedOSPlatformWhenAttribute : OSPlatformAttribute 
{
	public SupportedOSPlatformWhenAttribute(string platformName, string parameterName, string value) : base(platformName) { }
}

[AttributeUsage(AttributeTargets.Constructor | AttributeTargets.Method |  System.AttributeTargets.Property, AllowMultiple = true, Inherited = false)]
public sealed class UnsupportedOSPlatformUnlessAttribute : OSPlatformAttribute 
{
	public UnsupportedOSPlatformUnlessAttribute(string platformName, string parameterName, string value) : base(platformName) { }
}}

Usage examples:

  1. All of the MemoryMappedFile .Create* overload are supported only on windows mapName is not null
[SupportedOSPlatformWhen("windows", "mapName", "notnull")] 
public static MemoryMappedFile CreateNew(string? mapName, long capacity) { throw null; }

void UsageSample ()
{
    var mmf =  MemoryMappedFile.CreateNew("testmap", 10000); // warns 
    var mmf2 = MemoryMappedFile.CreateNew(null, 10000); // not warn 
} 
  1. PipeStream.set_ReadMode(PipeTransmissionMode) works only on Windows the value is PipeTransmissionMode.Message.
public virtual PipeTransmissionMode ReadMode 
{
    get { ... }
    [SupportedOSPlatformWhen("windows", "value", "Message")]
    set { ... }
}

void UsageSample ()
{
    var pipeStream = new PipeStream(); 
    pipeStream.ReadMode  = PipeTransmissionMode.Message; // warn 
    pipeStream.ReadMode  = PipeTransmissionMode.Byte; // not warn 
} 
  1. SemaphoreSlim.Wait(int millisecondsTimeout) is unsupported on browser, but when the millisecondsTimeout == 0 it can work on browser.
[UnsupportedOSPlatformUnless("browser", "millisecondsTimeout", "0")]
public bool Wait(int millisecondsTimeout)
{
    return Wait(millisecondsTimeout, CancellationToken.None);
}

void UsageSample (SemaphoreSlim lock, int timeout)
{
     lock.Wait(timeout); // warn 
     lock.Wait(timeout, CancellationToken.None); // warn 
}

void UsageSample (SemaphoreSlim lock)
{
     lock.Wait(0); // note warn 
     lock.Wait(0, CancellationToken.None); // not warn 
}

Note:

  • PipeTransmissionMode.Message is already annotated with [SupportedOSPlatform("windows")]
  • Did not see any example requiring null value
  • At first, i had UnsupportedOSPlatformWhen attribute, but it didn't make sense for unsupported scenario which represent an exclusion, i didn't found any real example that need UnsupportedOSPlatformWhen attibute, instead, I propose UnsupportedOSPlatformUnless which will express exclusion of the parameter value

cc @bartonjs @stephentoub @terrajobst

@buyaa-n
Copy link
Contributor

buyaa-n commented Mar 2, 2021

Platform-Guard Assertion Annotations

The analyzer already recognizes platform guards using the methods on OperatingSystem, such as OperatingSystem.IsWindows and OperatingSystem.IsWindowsVersionAtLeast. However, the analyzer does not recognize other guard methods or even helper methods that assert platform guards. Expanding this support could involve creating new attributes that indicate that an API asserts platform checks the same way the APIs on OperatingSystem do, potentially even conditionally.

One example is in Thread, there is an internal field for IsThreadStartSupported that indicates whether or not the current platform supports calls to Thread.Start, regardless of which platforms are targeted. Annotations could allow this to be modeled for the analyzer to warn if Threat.Start is called without first checking the corresponding IsThreadStartSupported member.

Additionally, it's expected that projects will commonly create helper methods that wrap around the OperatingSystem calls and these are not currently recognized through the flow analysis in the analyzer. Helper methods that perform platform assertion could be annotated to inform the analyzer that invocation or specific return values indicate platform support.

Based on the flow analysis API usage I think we do not need additional attributes for covering these, I propose to just add corresponding [SupportedOSPlatform] or [UnsupportedOSPlatform] attributes for those helper methods or Field/Propert/Enum. When the analyzer found them in the conditional statement it will evaluate its attribute to figure out what attribute it is guarding. The only requirement is the API should return boolean (same for Field/Property).

Example: 1. For IsThreadStartSupported field mentioned above:

#if !TARGET_BROWSER
    [UnsupportedOSPlatform("browser")] // The analyzer would only evaluate an immediate attribute(s)
    internal const bool IsThreadStartSupported = true;
#endif

protected internal override void QueueTask(Task task)
{
    if (Thread.IsThreadStartSupported) // This will be evaluated same as `!OperatingSystem.IsBrowser()`
    {
        // Call unsupported on browser APIs safely
        new Thread(s_longRunningThreadWork)
        {
            IsBackground = true,
            Name = ".NET Long Running Task"
        }.UnsafeStart(task);
    }
}
  1. Helper method:
[SupportedOSPlatform("windows10.0")]
public bool  IsWindows10OrAbove()
{ 
     return OperatingSystem.IsWindowsVersionAtLeast(10);
}

[SupportedOSPlatform("windows10.0")]
public bool  Windows10OnlyAPI() { }

void Sample ()
{
    if (IsWindows10OrAbove()) // work same as OperatingSystem.IsWindowsVersionAtLeast(10)
    {
        Windows10OnlyAPI();
    }
}
  1. More advanced case: type check as a guard: even we have assembly level [SupportedOSPlatform("windows")], if we want to use the type/API as a guard we need to add the inline attribute - Good to have
[SupportedOSPlatform("windows")]
public partial class WindowsIdentity : System.Security.Claims.ClaimsIdentity, System.IDisposable, ...
{ ... }

void Sample ()
{
    var identity = _negotiateState.GetIdentity();
    if (identity is WindowsIdentity winIdentity) // work same as OperatingSystem.IsWindows()
    {
        ClaimsPrincipal user = new WindowsPrincipal(winIdentity);
    }
}

@buyaa-n
Copy link
Contributor

buyaa-n commented Apr 20, 2021

Notes from the last meeting:

  1. For Conditionally-Supported Platform Annotations the attributes i proposed because there are not many cases detected for use, so we do not want to add another attribute if the number of cases is less than 20, instead we will hard code those APIs in the analyzer and special case. The number of cases I have seen until now is around 10-15, we can find more while resolving As a developer, I get compile time diagnostics for unsupported APIs #47228, so for now, i am leaving this scenario as blocked by As a developer, I get compile time diagnostics for unsupported APIs #47228
  2. For Platform-Guard Assertion Annotations the idea sounds good, but we prefer to use different attributes for guards. I will create an issue for new attributes proposal

@jeffhandley
Copy link
Member Author

Given the assessment @buyaa-n provided for "Conditionally-Supported Platform Attributes," I'm going to edit this story to remove that from scope. With the Platform-Guard attributes in place and in use, we can then mark this story as completed.

@ghost ghost locked as resolved and limited conversation to collaborators Jun 18, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Meta Cost:M Work that requires one engineer up to 2 weeks Priority:2 Work that is important, but not critical for the release Team:Libraries User Story A single user-facing feature. Can be grouped under an epic.
Projects
No open projects
Development

No branches or pull requests

5 participants