Skip to content
New issue

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

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

Already on GitHub? # to your account

Mark existing Windows-specific APIs without a version number #40095

Closed
terrajobst opened this issue Jul 29, 2020 · 4 comments · Fixed by #40375
Closed

Mark existing Windows-specific APIs without a version number #40095

terrajobst opened this issue Jul 29, 2020 · 4 comments · Fixed by #40375
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.Runtime.InteropServices code-analyzer Marks an issue that suggests a Roslyn analyzer
Milestone

Comments

@terrajobst
Copy link
Contributor

terrajobst commented Jul 29, 2020

We'd like change our stance on marking the existing Windows-specific not as windows7.0 but as just windows.

From the spec:

The lowest version of Windows that we support with .NET Core is Windows 7. Also, we generally don't expose functionality that requires a higher version of Windows.

We originally said we'll mark these APIs as windows7.0 but this would mean that callers have to call guard these APIs with a 7.0 version check too, which isn't really necessary. But what's worse is that many developers already have written code that checks for the OS but not version, and due to the version support that is perfectly correct code.

We decided that it's better for our analyzer to special case version-less checks and let it be equivalent of a check for 0.0. We also decided that applying the attribute without a version is the same as stating the API was introduced in 0.0.

The net effect is that consumers of the existing Windows-specific APIs will get away with just checking for the OS. So the existing code in DoSomething() will not cause a warning when we add the annotation to TheRegistry():

[SupportedOSPlatform("windows")]
public void TheRegistry() { ... }

public void DoSomething()
{
    if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows))
        TheRegistry();
}

Moving forward, we'll only tag Windows-specific APIs without version if they are supported by Windows 7 or earlier. APIs requiring newer OS versions will be marked with the corresponding OS version.

/cc @jeffhandley @eerhardt @buyaa-n @bartonjs

@terrajobst terrajobst added area-System.Runtime.InteropServices api-ready-for-review API is ready for review, it is NOT ready for implementation labels Jul 29, 2020
@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added the untriaged New issue has not been triaged by the area owner label Jul 29, 2020
@terrajobst terrajobst added code-analyzer Marks an issue that suggests a Roslyn analyzer and removed untriaged New issue has not been triaged by the area owner labels Jul 29, 2020
@terrajobst terrajobst added this to the 5.0.0 milestone Jul 29, 2020
@terrajobst terrajobst added the blocking Marks issues that we want to fast track in order to unblock other important work label Jul 29, 2020
@jeffhandley
Copy link
Member

/cc @adamsitnik

@terrajobst
Copy link
Contributor Author

@richlander @mhutch

@terrajobst terrajobst added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for review, it is NOT ready for implementation blocking Marks issues that we want to fast track in order to unblock other important work labels Jul 30, 2020
@terrajobst
Copy link
Contributor Author

terrajobst commented Jul 30, 2020

Video

  • Looks good
  • We should consider re-baselining when we bump the minimum Windows version, for similar benefits for net-new code.
  • We should consider having an analyzer that flags checks for a higher version than what the guarded code actually needs.
[SupportedOSPlatform("windows")]
public void TheRegistry() { ... }

public void DoSomething()
{
    if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows))
        TheRegistry();
}

@terrajobst
Copy link
Contributor Author

@adamsitnik, please replace

[MinimumOSPlatform("windows7.0")]

with

[SupportedOSPlatform("windows")]

# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
api-approved API was approved in API review, it can be implemented area-System.Runtime.InteropServices code-analyzer Marks an issue that suggests a Roslyn analyzer
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants