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.

Sign up for GitHub

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

Jump to bottom

Add nullability annotations to System.Private.Xml.Linq project #40744

Merged
merged 4 commits into from
Aug 20, 2020
Merged

Add nullability annotations to System.Private.Xml.Linq project #40744

merged 4 commits into from
Aug 20, 2020

Conversation

jozkee
Copy link
Member

@jozkee jozkee commented Aug 13, 2020

Contributes to #2339

@jozkee jozkee added this to the 5.0.0 milestone Aug 13, 2020
@jozkee jozkee requested review from krwq, buyaa-n and a team August 13, 2020 00:07
@jozkee jozkee self-assigned this Aug 13, 2020
@ghost
Copy link

ghost commented Aug 13, 2020

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

krwq
krwq reviewed Aug 13, 2020
View reviewed changes
src/libraries/System.Private.Xml/src/System/Xml/XPath/XPathNavigator.cs
@@ -1206,7 +1206,7 @@ public virtual object Evaluate(string xpath)
return Evaluate(XPathExpression.Compile(xpath), null);
}

public virtual object Evaluate(string xpath, IXmlNamespaceResolver resolver)
public virtual object Evaluate(string xpath, IXmlNamespaceResolver? resolver)
Copy link
Member

Choose a reason for hiding this comment

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

should all resolvers above also be nullable?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, and also in other places where a IXmlNamespaceResolver is taken, not sure if everywhere but we should be able to take null in other cases, e.g:

runtime/src/libraries/System.Private.Xml/src/System/Xml/Core/XmlReader.cs

Line 381 in f0461cb

public virtual object ReadContentAs(Type returnType, IXmlNamespaceResolver namespaceResolver)

The docs also mention that the param can be null https://docs.microsoft.com/dotnet/api/system.xml.xmlreader.readcontentas?view=netcore-3.1

Copy link
Member

Choose a reason for hiding this comment

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

Do the other places IXmlNamespaceResolver is taken still need to be updated to be nulalble?

Copy link
Member Author

@jozkee jozkee Aug 20, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

XmlReader.ReadContentAs(Type, IXmlNamespaceResolver) is yet as not-nullable so I assume is the same for the other cases if they exists.

Copy link
Member Author

Choose a reason for hiding this comment

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

I did a scan through the methods that can take IXmlNamespaceResolver and found a few ones that could be changed to IXmlNamespaceResolver?. I submitted #41079 with the changes.

@jaredpar jaredpar mentioned this pull request Aug 13, 2020
Closed
@jozkee jozkee force-pushed the nullability_xpath4 branch from 66f7021 to 43ba8e1 Compare August 18, 2020 22:49
@jozkee jozkee mentioned this pull request Aug 20, 2020
Merged
@jozkee jozkee changed the title Add nullability annotations to Xml.Linq project Add nullability annotations to System.Private.Xml.Linq project Aug 20, 2020
krwq
krwq approved these changes Aug 20, 2020
View reviewed changes
Copy link
Member

@krwq krwq left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for fixing, please ensure build errors are fixed or known issues

@jozkee
Copy link
Member Author

jozkee commented Aug 20, 2020

CI issues are unrelated, see #41006.

@jozkee jozkee merged commit 9d26606 into dotnet:master Aug 20, 2020
@jozkee jozkee deleted the nullability_xpath4 branch August 20, 2020 08:56
@carlossanlop carlossanlop mentioned this pull request Aug 25, 2020
Merged
14 tasks
carlossanlop pushed a commit to carlossanlop/runtime that referenced this pull request Aug 28, 2020
@jozkee @carlossanlop
Add nullability annotations to System.Private.Xml.Linq project (dotne…
c131a1a 
…t#40744)

* Add nullability annotations to Xml.Linq project

* Fix misplaced assertion

* Address feedback

* Add missing NotNullIfNotNull attributes to operators
@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.
Reviewers

@jeffhandley jeffhandley jeffhandley left review comments

@krwq krwq krwq approved these changes

@buyaa-n buyaa-n Awaiting requested review from buyaa-n

Assignees

@jozkee jozkee

Labels
area-System.Xml
Projects
None yet
Milestone
5.0.0
Development

Successfully merging this pull request may close these issues.
3 participants
@jozkee @krwq @jeffhandley