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

Build Threading.Tasks.DataFlow and ComponentModel.Annotations for NetCoreAppCurrent #48667

Merged
merged 2 commits into from
Feb 26, 2021

Conversation

eerhardt
Copy link
Member

Fix #48662

This involved fixing a few nullable annotation errors in System.Threading.Tasks.DataFlow.

cc @ericstj

@ghost
Copy link

ghost commented Feb 23, 2021

Tagging subscribers to this area: @safern, @ViktorHofer
See info in area-owners.md if you want to be subscribed.

Issue Details

Fix #48662

This involved fixing a few nullable annotation errors in System.Threading.Tasks.DataFlow.

cc @ericstj

Author: eerhardt
Assignees: -
Labels:

area-Infrastructure-libraries

Milestone: -

@ericstj
Copy link
Member

ericstj commented Feb 23, 2021

@ViktorHofer and I were just discussing this the other day.

There's no requirement to target $(NetCoreAppCurrent) just because some assembly is part of the shared framework. In fact we try to avoid configurations that don't ship in the package since they create a risk for a package downgrading functionality.

What benefit are we getting by targeting NetCoreAppCurrent? It looks like nullable annotations? Could we get the same benefit by ensuring the references are nullable annotated?

@eerhardt
Copy link
Member Author

What benefit are we getting by targeting NetCoreAppCurrent?

  1. The nullable annotations working correctly.
  2. @LakshanF was annotating System.Threading.Tasks.DataFlow for linker annotations and needed to reference UnconditionalSuppressMessageAttribute, which is only in net5.0+. So he added the file as a source linked file, which compiles the attribute as internal. This then broke the linker because it was picking up the internal version of UnconditionalSuppressMessageAttribute and erroring with:
D:\work\Core\changes\runtime\src\libraries\System.Private.CoreLib\src\ILLink\ILLink.Suppressions.Shared.xml : error IL20
52: Property 'Scope' could not be found [D:\work\Core\changes\runtime\src\libraries\src.proj]
D:\work\Core\changes\runtime\src\libraries\System.Private.CoreLib\src\ILLink\ILLink.Suppressions.Shared.xml : error IL20
52: Property 'Target' could not be found [D:\work\Core\changes\runtime\src\libraries\src.proj]

Having to compile internal attributes in the shared framework assemblies seems busted. Any shared framework assembly that needs to use those attributes should be able to pick those attributes up from the public API.

cc @vitek-karas

@safern
Copy link
Member

safern commented Feb 23, 2021

Could we get the same benefit by ensuring the references are nullable annotated?

We don't get the same benefit as .NET Standard and .NET Framework are not annotated, so you don't get data flow analysis on stuff like Debug.Assert or string.IsNullOrEmpty nor any of the APIs that affect data flow analysis that are inbox on those frameworks as their ref assemblies are not annotated. This leads annotating libraries very hard and basically it is just throwing a bunch of !s across the code. That is why we decided to disable nullable warnings for anything non netcoreapp.

@ericstj
Copy link
Member

ericstj commented Feb 24, 2021

That's a bummer. So long as we have these configurations we should be really careful about them being noticeably different from the package. At the moment we only have code review to catch significant differences.

Copy link
Member

@safern safern left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks reasonable

Copy link
Member

@stephentoub stephentoub left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dataflow nullable annotation changes LGTM

@eerhardt
Copy link
Member Author

Looks like I missed a DefineConstants when building for netcoreapp. (Thanks for the catch, unit tests!)

Not a huge change, but if anyone wants to take another look here.

@eerhardt
Copy link
Member Author

Failures are #1488

@eerhardt eerhardt merged commit 2e2d7bd into dotnet:master Feb 26, 2021
@eerhardt eerhardt deleted the Fix48662 branch February 26, 2021 02:39
@ghost ghost locked as resolved and limited conversation to collaborators Mar 28, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Threading.Tasks.DataFlow and ComponentModel.Annotations are not building NetCoreAppCurrent
5 participants