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 Add 'CrossPlatform' as special case allowing override parent support #5094

Closed

Conversation

buyaa-n
Copy link
Member

@buyaa-n buyaa-n commented May 10, 2021

Fixes #5023

Allow child APIs attribute override assembly attribute in targeted builds of cross-platform assemblies

By the 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. We have planned to warn if it happens by dotnet/runtime#43971, but this scenario is happening in runtime for targeted cross-platform builds. Ideally, all platform-specific implementation should be separated from cross-platform src. Though that causing code duplication at some level, so most developers would not like to apply strict separation of code.

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

After some discussion, we decided to add a special case [SupportedOSPlatform("CrossPlatofrm")] attribute for targeted builds of cross-platform assembly to allow child member attributes to override the parent. To distinguish between real platform-specific build and cross-platform with platform-specific builds. (We might change "CrossPlatofrm" into "Neutral" or something like that)

CC @jeffhandley @terrajobst

@buyaa-n buyaa-n requested a review from a team as a code owner May 10, 2021 23:06
@codecov
Copy link

codecov bot commented May 10, 2021

Codecov Report

Merging #5094 (fd19701) into release/6.0.1xx (15dd5af) will increase coverage by 0.00%.
The diff coverage is 99.40%.

@@                Coverage Diff                @@
##           release/6.0.1xx    #5094    +/-   ##
=================================================
  Coverage            95.56%   95.57%            
=================================================
  Files                 1203     1203            
  Lines               274837   274997   +160     
  Branches             16653    16663    +10     
=================================================
+ Hits                262657   262825   +168     
+ Misses                9970     9967     -3     
+ Partials              2210     2205     -5     

@buyaa-n buyaa-n changed the base branch from release/6.0.1xx-preview4 to release/6.0.1xx May 11, 2021 19:00
@buyaa-n buyaa-n marked this pull request as draft May 14, 2021 00:57
@buyaa-n
Copy link
Member Author

buyaa-n commented May 19, 2021

Using SupportedOSPlatform attribute for handling this is causing side effects in further analysis and needs more hack like fixes to get rid of it. I realize we can use just an MSBuild property for this and raised a new PR with implementation, so closing this PR

@buyaa-n buyaa-n closed this May 19, 2021
@buyaa-n buyaa-n deleted the cross_platform_specialcase branch May 19, 2021 04:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants