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

[release/5.0] Backport nullability annotations to release/5.0 #41348

Merged
merged 14 commits into from
Sep 1, 2020

Conversation

carlossanlop
Copy link
Member

@carlossanlop carlossanlop commented Aug 25, 2020

Do not merge yet.

This PR will backport the following master PRs (unchecked = still in review):

@carlossanlop carlossanlop added this to the 5.0.0 milestone Aug 25, 2020
@carlossanlop carlossanlop self-assigned this Aug 25, 2020
@carlossanlop carlossanlop added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Aug 25, 2020
@danmoseley danmoseley changed the title Backport nullability annotations to release/5.0 [release/5.0] Backport nullability annotations to release/5.0 Aug 25, 2020
@krwq
Copy link
Member

krwq commented Aug 26, 2020

@carlossanlop please also add #41315

@carlossanlop
Copy link
Member Author

@carlossanlop Carlos Sanchez Lopez FTE please also add #41315

No need, @krwq, because @danmosemsft automatically backported it with the bot comment: 2aa627b

@carlossanlop carlossanlop force-pushed the nullabilityBackport50 branch from 684965b to 3f2d49b Compare August 26, 2020 18:02
@carlossanlop
Copy link
Member Author

I force-updated this branch to the latest changes in dotnet/release5.0 , you can see change #41315 is already included in the commit history.

@krwq
Copy link
Member

krwq commented Aug 27, 2020

FYI I've added:

#41438 Globally enable nullability in XML
#41382 Fix null check in S.Diagnostics.Contract regressed by nullability annotations

to your list

Copy link
Member

@roji roji left a comment

Choose a reason for hiding this comment

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

OK for the System.Data parts, thanks!

@jkotas
Copy link
Member

jkotas commented Aug 27, 2020

@krwq Should #41382 be treated separately? #41382 is a real bug that needs to be fixed for 5.0 vs. the rest is just adding more annotations that may or may not meet the bar for .NET 5 at this point.

@carlossanlop carlossanlop force-pushed the nullabilityBackport50 branch from 3f2d49b to e7afa73 Compare August 27, 2020 17:18
@jeffhandley
Copy link
Member

@krwq Should #41382 be treated separately? #41382 is a real bug that needs to be fixed for 5.0 vs. the rest is just adding more annotations that may or may not meet the bar for .NET 5 at this point.

We've gotten tactics approval to merge this PR in on Monday (August 31st) with however far we get through the remaining annotations--including them all in RC1. But it's certainly better to separate the bug fix into a separate PR.

@carlossanlop can you remove #41382 from this PR and create a separate port PR for that please? The port bot should be able to do its magic on that one.

@carlossanlop carlossanlop force-pushed the nullabilityBackport50 branch from ad4191d to 7c99012 Compare August 28, 2020 17:30
jozkee and others added 4 commits August 28, 2020 15:07
…t#40744)

* Add nullability annotations to Xml.Linq project

* Fix misplaced assertion

* Address feedback

* Add missing NotNullIfNotNull attributes to operators
* Enable nullability on System.Xml.XPath

* Switch XPathException ctor's message parameter to string?
Prashanth Govindarajan and others added 9 commits August 28, 2020 15:07
* src files annotated

* ref file

* Address feedback
* Nullable: System.Xml, part 7 (Serialization)

* Fix build errors on Release (ifdef-ed code)

* fix incidental product bug

* address PR feedback

* Make XmlSerializer._rootType nullable
* Globally enable nullability in XML

* remove double #nullable enable in the file
…net#41083)

* Port nullability annotations to System.Xml.ReaderWriter contract assembly

* Port annotations of recently updated members in Xml/Serialization

* Fix anontations on XmlElement discovered while updating System.Data.Common Xml related annotations

* Fix Xml related annotations on System.Data.Common

* address feedback

Co-authored-by: Krzysztof Wicher <kwicher@microsoft.com>
…otnet#41474)

* Port nullability annotations to refs XmlDocument and XmlSerializer

* Fix new System.Data.Common nullability related errors

* Switch nullability of parameter in SoapElementAttribute ctor
dotnet#41086)

* Enable nullability on System.Xml.XPath.XDocument src

* Port nullability annotations to System.Xml.XPath.XDocument

* Port nullability annotations to System.Xml.XDocument

* Add ? to XmlAttribute argument in int? explicit operator
…elements (dotnet#41488)

* Switch Markup property to XmlNode?[]? to reflect it can contain null elements

* Update ref
…Runtime.Serialization.Json (dotnet#41476)

* Initial nullable annotations of System.Private.DataContractSerialization

Contributes to dotnet#2339

* Mark DataMember.Name as non-nullable.

* Fix a few simple nullable compile errors.

* Assert attributes is non-null in XmlObjectSerializerReadContext

* Ensure XmlObjectSerializerContext.serializer is never null

* Fix a few simple nullable errors

* Remove any checks that DataMember.MemberInfo can be null.

* Mark EnumDataContract.Members as non-nullable.

Fix nullable errors in SchemaExporter.

* Correctly annotate CollectionDataContract.IsCollectionOrTryCreate.

* Assert DataContractResolver is non-null.

* Suppress dotnet#41465

* Update System.Runtime.Serialization.Json ref source for nullable annotations.

* Update System.Runtime.Serializaiton.Xml ref source for nullable annotations.

* Update for Xml.ReaderWriter nullable annotations

* Work around compiler issue.

* Fix test failure.

Reference compiler issue in TODO comment.

* Revert nullable suppression now that XmlSchemaAppInfo.Markup is annotated correctly.

* Fix build for latest annotations in master.

* PR feedback round 1

* Address PR feedback round 2
@carlossanlop carlossanlop force-pushed the nullabilityBackport50 branch from 48125da to 4969648 Compare August 28, 2020 22:08
@ericstj
Copy link
Member

ericstj commented Sep 1, 2020

I have reviewed all current commits in this PR as well as linked PR #41541. This is approved for release/5.0 (previously approved by Tactics). You are good to go from my side assuming a simple cherry-pick for that last PR. I'll also be online to have another look if you'd like.

* Add nullable annotations for XmlDataDocument

* Address code review feedback

* More code review feedback

* Address Eric's feedback

* Annotate _dataSet as not null when AttachDataSet is called

* Fix GetElementById return type annotation

In order to match with src

Co-authored-by: Jeff Handley <jeff.handley@microsoft.com>
Co-authored-by: David Cantú <dacantu@microsoft.com>
@carlossanlop carlossanlop added auto-merge and removed NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) labels Sep 1, 2020
@ghost
Copy link

ghost commented Sep 1, 2020

Hello @carlossanlop!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@ericstj ericstj removed the auto-merge label Sep 1, 2020
@ericstj
Copy link
Member

ericstj commented Sep 1, 2020

Removing auto-merge since it actually squashes where we want this PR to be merged not squashed.

@carlossanlop carlossanlop merged commit fe9059e into dotnet:release/5.0 Sep 1, 2020
@carlossanlop carlossanlop deleted the nullabilityBackport50 branch September 1, 2020 03:54
@ghost ghost locked as resolved and limited conversation to collaborators Dec 7, 2020
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.

10 participants