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

OS compatibility analyzer #37359

Closed
buyaa-n opened this issue Jun 3, 2020 · 6 comments
Closed

OS compatibility analyzer #37359

buyaa-n opened this issue Jun 3, 2020 · 6 comments
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-Meta code-analyzer Marks an issue that suggests a Roslyn analyzer code-fixer Marks an issue that suggests a Roslyn code fixer
Milestone

Comments

@buyaa-n
Copy link
Contributor

buyaa-n commented Jun 3, 2020

OS compatibility analyzer

Based on design spec Annotating platform-specific APIs and detecting its use

Requirements

Goals

  • Developers get diagnostics when they inadvertently use OS-specific APIs.
    • The developer doesn't have to resort to explicit suppressions to indicate intent. Idiomatic gestures, such as guarding the call with an OS check or marking a method as OS-specific, automatically suppress these diagnostics. However, explicit suppressions via the usual means (NoWarn, #pragma) will also work.
  • The analyzer helps library authors to express their intent so that their consumers can also benefit from these diagnostics.
    • Library authors can annotate their OS-specific APIs without taking a dependency.
    • Library consumers don't have to install a NuGet package to get diagnostics when using OS-specific APIs
  • The analyzer must satisfy the quality bar in order to be turned on by default
    • No false positives, low noise.

Non-Goals

  • Modelling partially portable APIs (that is APIs are applicable to more than one platform but not all platforms).
    • It is still in question, we might decide to support it.
  • Will not diagnose non OS-related compatibility, such as .Net framework, UWP, deprecated APIs etc., not same as Platform Compatibility Analyzer
  • Shipping the platform-specific analyzer and/or annotations out-of-band.

Expectations:

  • Guarding calls to platform-specific APIs is approved and implemented issue
    • It is merged into preview 8.
  • All OS-dependent APIs in runtime have annotated with OSPlatformVersionAttribute accordingly
    • In runtime repo windows specific APIs now attributed with MinimumOSPlatform("Windows7.0").
    • No other attribution done yet
  • Roslyn analyzer provide access to TFM and TargetPlatformMinVersion
    • It is now available in roslyn-analyzers, we can consume it as .editconfig options.
    • We don't need this now asMSBuild emits the assembly level attribute now

Diagnostic properties

  • Category: Interoperability
  • Severity: Warning
  • isEnabledByDefault: true
  • Id: CA1416
  • Title: Call of platform dependent API
  • Description: Using platform dependendent API on a component makes the code no longer work across all platforms.

Finding Diagnostics

The analyzer could start analyzing diagnostics for any method | event | property | enum invocation, check if they have annotated with any OSPlatformAttribute.

  • if an assembly or a class has marked with any OSPlatformAttribute that attribute applies to all members defined within the assembly/class. So if a member has no OS dependent attributes defined we will lookup for derived dependency from containing class or assembly level.
  • if an assembly, a class, and its members all have its own OSPlatformAttribute then the innermost member’s OS version would be counted for diagnostics, i.e if any OSPlatformVersionAttribute dependency found for a member we will not lookup for a derived dependencies from containing class or assembly.

In general, the analyzer can produce three diagnostics:

  1. If the member has an MinimumOSPlatformAttribute which haven't supppressed => warn with '{API}' requires {OS} {OS version} or later.* (Fixer could suggest for adding MinimumOSPlatformAttribute or platform guard RuntimeInformation.IsOSPlatformOrLater(…))
  2. If the member has an ObsoletedInOSPlatformVersionAttribute which haven't supppressed => warn with '{API}' has been deprecated since {OS} {OS version}. (Fixer could suggest for adding ObsoletedInOSPlatformVersionAttribute or platform guarding method RuntimeInformation. IsOSPlatformEarlierThan(…)
  3. If the member has a RemovedInOSPlatformVersionAttribute which haven't supppressed => warn with '{API}' has been removed since {OS} {OS version}. (Fixer could suggest for adding RemovedInOSPlatformVersionAttribute or platform guarding method RuntimeInformation. IsOSPlatformEarlierThan(…) )

Detailed Scenarios for version check:

  1. If invoked member has MinimumOSPlatformAttribute("Windows", 10, 1, 2, 3) attribute and call site had guarded with MinimumOSPlatformAttribute("Windows", x, y, z, r) then
Dependency Call site Warn Fix
10.1.2.3 10.1.2.3 false no
10.1.2.3 10.1.3.3 false no
10.1.2.3 10.1.3.1 false no
10.1.2.3 11.1.2.3 false no
10.1.2.3 10.2.2.0 false no
10.1.2.3 10.1.1.0 true 10.1.2.3 (IsOSPlatformOrLater)
10.1.2.3 10.1.1.4 true 10.1.2.3 (IsOSPlatformOrLater)
10.1.2.3 10.1.0.1 true 10.1.2.3 (IsOSPlatformOrLater)
10.1.2.3 8.1.1.4 true 10.1.2.3 (IsOSPlatformOrLater)
  1. If invoked member has ObsoletedInOSPlatformVersion("Windows", 10, 1, 2, 3) attribute and call site had guarded with ObsoletedInOSPlatformVersion("Windows", x, y, z, r) then
Dependency Call site Warn Fix
10.1.2.3 10.1.2.3 true no
10.1.2.3 10.1.3.3 true 10.1.2.3 (IsOSPlatformEarlierThan)
10.1.2.3 10.1.3.1 true 10.1.2.3 (IsOSPlatformEarlierThan)
10.1.2.3 11.1.2.3 true 10.1.2.3 (IsOSPlatformEarlierThan)
10.1.2.3 10.2.2.0 true 10.1.2.3 (IsOSPlatformEarlierThan)
10.1.2.3 10.1.1.0 false no
10.1.2.3 10.1.1.4 false no
10.1.2.3 10.1.0.1 false no
10.1.2.3 8.1.1.4 false no
  • This applies the same for RemovedInOSPlatformVersion attribute

Automatic suppression via Platform context or platform guard methods

Diagnostics can be suppressed by call site annotated with the same attribute with the correct version or a call to guard methods:

  1. Call-site application of OSPlatformAttribute to the containing member, type, module, or assembly related to the current diagnostic.
  2. If the call-site is guarded by a platform check (RuntimeInformation.IsOSPlatformOrLater() or RuntimeInformation.IsOSPlatformEarlierThan()) related to the current diagnostic.
  • if (1) call-site's containing method, type, module, or assembly's OSPlatformVersionAttribute type and osPlatform string matches check the versions to determine if context suppresses the diagnostic.

  • If (1) not suppressing the diagnostic, determine if the call-site is guarded by a platform check using flow analysis.

Sample: diagnostic suppressed by (2) a call to RuntimeInformation.IsOSPlatformOrLater(). This should work for simple cases where the call is contained inside an if check but also when the check causes control flow to never reach the call:

public void Case1()
{
    if (RuntimeInformation.IsOSPlatformOrLater(OSPlatform.iOS, 13))
        AppleApi();
}

public void Case2()
{
    if (!RuntimeInformation.IsOSPlatformOrLater(OSPlatform.iOS, 13))
        return;

    AppleApi();
}

Diagnostics (2) and (3) are analogous except the guard is provided by RuntimeInformation.IsOSPlatformEarlierThan().

Code Fixers

  • Suggest wrapping statement in platform guard
  • Suggest annotating the method with an attribute
    All diagnostics can be suppressed by surround the call site using a platform guard. The fixer should offer surrounding the call with an if check using the appropriate platform-guard. It's not necessary that the end result compiles. It is the user's responsibility to adjust control flow and data flow to ensure variables are declared and initialized appropriately. The value that the fixer provides here is educational and avoids having to look up the correct version numbers.
    For diagnostic (1) we should also offer a fixer that allows it to be marked as platform-specific by applying AddedInOSPlatformVersionAttribute. If the attribute is already applied, it should offer to change the platform and version.
@buyaa-n buyaa-n added api-needs-work API needs work before it is approved, it is NOT ready for implementation area-Meta code-analyzer Marks an issue that suggests a Roslyn analyzer code-fixer Marks an issue that suggests a Roslyn code fixer labels Jun 3, 2020
@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added the untriaged New issue has not been triaged by the area owner label Jun 3, 2020
@buyaa-n buyaa-n changed the title OS-specific APIs analyzer OS compatibility analyzer Jun 3, 2020
@buyaa-n buyaa-n added this to the 5.0 milestone Jun 3, 2020
@buyaa-n buyaa-n removed the untriaged New issue has not been triaged by the area owner label Jun 3, 2020
@buyaa-n buyaa-n added api-ready-for-review and removed api-needs-work API needs work before it is approved, it is NOT ready for implementation labels Jun 26, 2020
@buyaa-n
Copy link
Contributor Author

buyaa-n commented Jun 26, 2020

@buyaa-n
Copy link
Contributor Author

buyaa-n commented Aug 11, 2020

Checklist for all new requirements related to applying new design requirements. Related issue #39989

  • Rename MinimumOSPlatformAttribute to SupportedOSPlatformAttribute and RemovedInOSPlatformAttribute to UnsupportedOSPlatformAttribute, update all references and tests

Update the semantics of these attributes as follows:

  • An API that doesn't have any of these attributes is considered supported by all platforms.
  • 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.
  • Both attributes can be instantiated without version numbers. This means the version number is assumed to be 0.0. This simplifies guard clauses, see examples below for more details.
  • [ObsoletedInOSPlatform] continuous to require a version number.
  • [ObsoletedInOSPlatform] by itself doesn't imply support. However, it doesn't make sense to apply [ObsoletedInOSPlatform] unless that platform is supported.
  • Add support for version less attribute like: [UnsupportedOSPlatform("windows")] and add/update tests
  • Use appropriate caching and refactor code accordingly as we need to keep track of multiple attribute
    • maybe cache unattributed symbols too, as next time it would need another lookup for attribute
  • Add support for RuntimeInformation.IsOSPlatform(OSPlatform.Windows) and other new guard methods. We'll change the analyzer to consider following guard clauses as equivalent:
if (RuntimeInformation.IsPlatform(OSPlatform.Windows))
{
    WindowsSpecificApi();
}
if (OperatingSystem.IsOSPlatform("windows"))
{
    WindowsSpecificApi();
}
if (OperatingSystem.IsWindows())
{
    WindowsSpecificApi();
}
if (OperatingSystem.IsWindowsVersionAtLeast(0))
{
    WindowsSpecificApi();
}
  • We have removed the new guard method IsOSPlatformEarlierThan(OSPlatform.Windows, 10), instead add support for the alternative of it: if (OperatingSystem.IsWindows() && !OperatingSystem.IsWindowsVersionAtLeast(10)). Update all guards tests using IsOSPlatformEarlierThan(...)
  • Add OR || operator support for cases like
// On Windows, the API was unsupported until version 10.0.19041.
// The API is considered supported everywhere else without constraints.
[UnsupportedOSPlatform("windows")]
[SupportedOSPlatform("windows10.0.19041")]
public extern void Api();

public void Api_Usage()
{
    if (!OperatingSystem.IsWindows() ||
         OperatingSystem.IsWindowsVersionAtLeast(10, 0, 19041))
    {
        Api();
    }
}
  • Add diagnostics for invalid platform string and add corresponding tests
  • Add diagnostics for invalid attributes combinations and add corresponding tests
  • Add supported attributes coming from Analyzer config options and add corresponding tests - This one not have to be in 5.0

@joperezr
Copy link
Member

@buyaa-n I suppose this is still for 5.0? If the change won't be cherry-picked into a release branch (which probably won't given that this is an analyzer) should we change the milestone?

@jeffhandley
Copy link
Member

We just sent the email to seek approval to merge dotnet/roslyn-analyzers#3917 in for RC1. So hopefully we'll be able to close this very soon. 🤞

@buyaa-n
Copy link
Contributor Author

buyaa-n commented Aug 25, 2020

Yes it is for 5.0 and yes it will be merged into roslyn-analyzers repo, not runtime. The PR is about to be merged in the RC1 branch of that repo.

@buyaa-n
Copy link
Contributor Author

buyaa-n commented Aug 26, 2020

The analyzer merged, closing

@buyaa-n buyaa-n closed this as completed Aug 26, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 8, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-Meta code-analyzer Marks an issue that suggests a Roslyn analyzer code-fixer Marks an issue that suggests a Roslyn code fixer
Projects
None yet
Development

No branches or pull requests

5 participants