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

As a developer, I get compile time diagnostics for unsupported APIs #47228

Closed
terrajobst opened this issue Jan 20, 2021 · 11 comments
Closed

As a developer, I get compile time diagnostics for unsupported APIs #47228

terrajobst opened this issue Jan 20, 2021 · 11 comments
Assignees
Labels
area-Meta code-analyzer Marks an issue that suggests a Roslyn analyzer Cost:M Work that requires one engineer up to 2 weeks Priority:1 Work that is critical for the release, but we could probably ship without User Story A single user-facing feature. Can be grouped under an epic.
Milestone

Comments

@terrajobst
Copy link
Member

terrajobst commented Jan 20, 2021

In .NET 5, we introduced a built-in platform analyzer that will check whether you call platform-specific APIs. However, this doesn't cover APIs that always throw PlatformNotSupportedException. We should mark those APIs in some fashion as well, for example, by applying ObsoleteAttribute.

We should make sure whatever mechanism we choose for detecting those APIs, that the docs also reflect this.

An example of an unmarked APIs is CSharpCodeGenerator.CreateFromBatch().

@terrajobst terrajobst transferred this issue from dotnet/core Jan 20, 2021
@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged New issue has not been triaged by the area owner label Jan 20, 2021
@terrajobst terrajobst added code-analyzer Marks an issue that suggests a Roslyn analyzer User Story A single user-facing feature. Can be grouped under an epic. Priority:1 Work that is critical for the release, but we could probably ship without area-Meta and removed untriaged New issue has not been triaged by the area owner labels Jan 20, 2021
@dotnet dotnet deleted a comment from dotnet-issue-labeler bot Jan 20, 2021
@danmoseley
Copy link
Member

@jeffhandley is this one something you're going to drive? If so we can set you or someone as assignee I think.

@jeffhandley jeffhandley added this to the 6.0.0 milestone Jan 21, 2021
@jeffhandley jeffhandley self-assigned this Jan 21, 2021
@jeffhandley
Copy link
Member

As was mentioned in #42967, we have over 4000 instances of throwing PlatformNotSupportedException. We would need to review that list and for each occurrence determine:

  1. Should there be a [SupportedOSPlatform] or [UnsupportedOSPlatform] attribute applied?
  2. Should there be an [Obsolete] attribute?
    • This would apply if the member is no longer supported in .NET, regardless of platform

I suspect we'd want to divide this list up by area so that owners across the areas could review and address the instances in their areas. That could be done either before or after the implementation of an analyzer.

We were pondering in #42967 whether or not an analyzer for doing this would be valuable beyond the runtime team, or if this is one we should build directly into the runtime repo. I'm unsure on that myself.

@jeffhandley jeffhandley added the Cost:M Work that requires one engineer up to 2 weeks label Jan 21, 2021
@terrajobst
Copy link
Member Author

Talked to @jeffhandley today. #42967 is really about us finding the issues; while that's useful that's not actually what this issue is about. This issue is about the user's experience when they call such an API. They should get a warning.

@jeffhandley
Copy link
Member

#44916 is also filed, and this one would involve reviewing the 4000 occurrences of PNSE that exist.

@buyaa-n
Copy link
Contributor

buyaa-n commented Feb 18, 2021

  1. Should there be an [Obsolete] attribute?
    • This would apply if the member is no longer supported in .NET, regardless of platform

If we apply for those [Obsolete] attribute what we would do in case the API get supported on any of the OS in .Net core? For example i see an issue for adding support for the CSharpCodeGenerator.CreateFromBatch(). Can we remove the attribute?

@buyaa-n
Copy link
Contributor

buyaa-n commented Mar 3, 2021

Could not we use the [SupportedOSPlatform] or [UnsupportedOSPlatform] attributes with framework names (with optional version)? (especially in case we want the PCA analyzer warn for their usages)

For example, by my understanding the above CSharpCodeGenerator.FromFileBatch() API is unsupported on .NetCore (or it is only supported on .Net framework)

[UnsupportedOSPlatform("netcoreapp")] //[UnsupportedOSPlatform("All")]
// or [SupportedOSPlatform("net46")]
private CompilerResults FromFileBatch(CompilerParameters options, string[] fileNames)
{
    throw new PlatformNotSupportedException();
}

@buyaa-n
Copy link
Contributor

buyaa-n commented Apr 6, 2021

[Brief Meeting Note]:
We have 3 options for handling APIs not supported in .NetCore but having open issue for future support:

  1. If we decide not to support the API we can close the related issue and add [Obsolete] to the API
  2. Add support for AllPlatforms scenario to PCA analyzer and annotate the API with [UnsupportedOSPlatform("AllPlatforms")]
  3. Leave the API un annotated until we decide if we really gonna add support for this

For the 2 API we discussed in the meeting, Regex.RegexAssemblyCompiler() and CodeDom.CSharpCodeGenerator we are leaving them unannotated until we decide to not support for sure. (Did not choose option 2 because there are too few cases found, only 2 for now)

@krwq
Copy link
Member

krwq commented Jul 22, 2021

@buyaa-n what's the current status of this?

@buyaa-n
Copy link
Contributor

buyaa-n commented Jul 22, 2021

@buyaa-n what's the current status of this?

Previously detected PNSEs on cross-platform builds and filed issues with findings, yesterday started the work detecting PNSEs on targeted builds and will file/update issues with findings. Overall this issue will cover the detecting PNSEs and that work will be done within 6.0. But some of the related issues might not make 6.0, depends on the area owners

@buyaa-n
Copy link
Contributor

buyaa-n commented Aug 10, 2021

My PNSE analysis is done and most findings are reported with separate issues referenced above. There is few APIs having an issue for future support which we decided not to annotate, for now, I did not file issue for them since the discussion we had in #50535. Here is the list of those APIs:

Assembly API Progress Issue for support
System.CodeDom 'CSharpCodeGenerator.FromFileBatch(CompilerParameters, string[])' unconditionally throws PNSE, VB version too No progress #18768
System.Text.RegularExpressions 'RegexAssemblyCompiler.Save()' throws PNSE and has no annotation No progress #19838
System.Configuration.ConfigurationManager 'RsaProtectedConfigurationProvider' all APIs unconditionally throws PNSE in PR #19838

Some of them were missing info in the doc that they are not supported in .Net Core, a PR for updating them is up

With that, I think we can close this issue now what do you think @terrajobst @jeffhandley?

@jeffhandley
Copy link
Member

Thanks, @buyaa-n!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Meta code-analyzer Marks an issue that suggests a Roslyn analyzer Cost:M Work that requires one engineer up to 2 weeks Priority:1 Work that is critical for the release, but we could probably ship without 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