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

[CA1416] Allow child APIs attribute override assembly attribute in targeted builds of cross-platform assemblies #5023

Closed
ManickaP opened this issue Apr 15, 2021 · 3 comments · Fixed by dotnet/runtime#53626

Comments

@ManickaP
Copy link
Member

Internally used [SupportedOSPlatform] will not raise any warnings due to build automatically adding assembly-wide [SupportedOSPlatform("...")] for the currently build platform.

Repro:

[SupportedOSPlatform("windows")]
[SupportedOSPlatform("linux")]
[SupportedOSPlatform("macos")]
private async ValueTask<(HttpConnectionBase connection, bool isNewConnection)>
    GetHttp3ConnectionAsync(HttpRequestMessage request, HttpAuthority authority, CancellationToken cancellationToken) { ... }
    
public async ValueTask<(HttpConnectionBase connection, bool isNewConnection)>
    GetConnection(...)
{
   ...
   // This line should cause a warning on Android/tvOS/iOS etc. but doesn't
   return GetHttp3ConnectionAsync(request, authority, cancellationToken);
}

Potential solution:

  • the nested SupportedOS should behave as intersection --> supported are only OSes named on all levels
  • the nested UnsupportedOS should behave as union --> unsupported are all OSes from all levels
    In other words, narrowing the lists of supported OSes with nested attributes.

Also consider that the list of operating systems in the attributes is behaving differently from the platform guards. Example:

if (OperatingSystem.IsLinux())
{
    // true for Linux distributions as well as Android, see: https://github.com/dotnet/runtime/blob/01b7e73cd378145264a7cb7a09365b41ed42b240/eng/targetframeworksuffix.props
}

[SupportedOSPlatform("Linux")]
public class A
{
    // true only for Linux distributions, not for Android.
}

This inconsistency confused me quite a bit since the guards used in the code must use different list of OSes than what is used in the attributes to achieve the same. This might cause some headaches to our customers in the future.

This is a spin off from conversation in dotnet/runtime#49261, see this comment dotnet/runtime#49261 (comment)

cc: @marek-safar @terrajobst @jeffhandley @buyaa-n

@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

@buyaa-n buyaa-n transferred this issue from dotnet/runtime Apr 15, 2021
@buyaa-n buyaa-n self-assigned this Apr 15, 2021
@mavasani mavasani added Area-Microsoft.CodeAnalysis.NetAnalyzers Bug The product is not behaving according to its current intended design labels Apr 22, 2021
@mavasani mavasani added this to the .NET 6.x milestone Apr 22, 2021
@buyaa-n buyaa-n added Enhancement and removed Bug The product is not behaving according to its current intended design labels Apr 26, 2021
@buyaa-n buyaa-n changed the title [Platform Compatibility Analyzer] SupportedOSPlatform doesn't work for internal APIs [CA1416] Allow child APIs attribute override for targeted builds in cross-platform assemblies Apr 26, 2021
@buyaa-n
Copy link
Contributor

buyaa-n commented Apr 26, 2021

This is an expected scenario by design, child APIs should not extend parent support. SupportedOSPlatform illustrates that the API/assembly only reachable for that platform and it should not have any API supported on other platforms inside it. But this is happening in runtime for targeted cross platform builds which is not expected to happen in normal applications. Ideally, all platform specific implementation should be separated from cross platform src. Though that causing code duplication some level, the API owner should choose if separation is worth doing. With the implementation of dotnet/runtime#43971 we should warn whenever child API tries to extend the parent support list. But as you can see from this issue this rule is not followed in runtime repo and in most cases we don't want to separate the code, including the Unsupported attribute inconsistency. In short, dotnet/runtime#43971 is not implemented because of this conflicting issue.

Even though it is an expected scenario by design it is unfortunate to not get any warning about this inconsistency or possible reference of incompatible APIs within the same assembly

I think we need some way to distinguish between real platform specific and cross-platform with platform specific builds. I am thinking to add [SupportedOSPlatform("CrossPlatofrm")] attribute for targeted builds of cross-platform assembly to allow child member attributes to override the parent. This approach could help with implementing dotnet/runtime#43971 appropriately without causing unwanted noise/false positives
CC @jeffhandley @terrajobst

@buyaa-n buyaa-n changed the title [CA1416] Allow child APIs attribute override for targeted builds in cross-platform assemblies [CA1416] Allow child APIs attribute override assembly attribute in targeted builds of cross-platform assemblies Apr 26, 2021
@buyaa-n
Copy link
Contributor

buyaa-n commented May 21, 2021

I have tried the approach using SupportedOSPlatform("CrossPlatform") attribute which worked well for the main purpose but is causing side effects in further analysis and needs more tweak to get rid of it. Even i was able to exclude all side effects it doesn't look like an efficient way of handling this, feels a bit hacky.

So i decided to use MSBuild property <CompilerVisibleProperty Include="PlatformNeutralAssembly" /> instead, which we will be added into targeted builds of cross-platform assembly, and as it is not an attribute it will not affect the further analysis.

Further in the child APIs should not extend parent support analyzer if the violation occurs we could suggest adding this property to suppress the warning if it that was the intention or remove the child attribute violation

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment