Add nullable annotations for XmlDataDocument #41541

terrajobst · 2020-08-28T22:59:55Z

No description provided.

ghost · 2020-08-28T22:59:58Z

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

pgovind · 2020-08-28T23:05:12Z

src/libraries/System.Data.Common/src/System/Xml/DataPointer.cs

                if (rowElem == null)
                {
-                    return null;
+                    return null!;


This might cause a NRE if someone calls Row.Something. We should make the property return a DataRow? instead I think.

Why is this ! needed?

Because the smart person that I am I only changed the return type of the property 😟

Fixed

eerhardt · 2020-08-28T23:08:50Z

src/libraries/System.Data.Common/src/System/Xml/XmlDataDocument.cs

@@ -21,13 +18,13 @@ namespace System.Xml
    [Obsolete("XmlDataDocument class will be removed in a future release.")]
    public class XmlDataDocument : XmlDocument
    {
-        private DataSet _dataSet;
+        private DataSet _dataSet = null!;


Can you use [MemberNotNull] on the Init and AttachDataSet methods instead? You can get rid of this null! if you do.

The same for _pointers, _columnChangeList, _foliationLock and _attrXml.

I tried that. The problem is that there is this constructor as well:

internal XmlDataDocument(XmlImplementation imp) : base(imp) { }

For now, I've null! initialized the values there.

We may not be entirely consistent, but anywhere we've used = null!; on fields we've tried to add a comment next to it indicating why.

The problem is that there is this constructor as well

What ensures _dataSet is set to non-null when that ctor is used? Or the field is never used when that ctor is used?

What ensures _dataSet is set to non-null when that ctor is used? Or the field is never used when that ctor is used?

I believe the field is never used in that instance. It's instantiated by XmlDataImplementation which exposes it as XmlDocument.

eerhardt · 2020-08-28T23:09:20Z

src/libraries/System.Data.Common/src/System/Xml/XmlDataDocument.cs


-        private DataSetMapper _mapper;
-        internal Hashtable _pointers;         // Hastable w/ all pointer objects used by this XmlDataDocument. Hashtable are guaranteed to work OK w/ one writer and mutiple readers, so as long as we guarantee
+        private DataSetMapper _mapper = null!;


Same here - use [MemberNotNull] on the Init method

#41541 (comment)

eerhardt · 2020-08-28T23:09:46Z

src/libraries/System.Data.Common/src/System/Xml/DataDocumentXPathNavigator.cs

@@ -50,7 +47,7 @@ public override string Value
            get
            {
                XPathNodeType xnt = _curNode.NodeType;
-                return xnt == XPathNodeType.Element || xnt == XPathNodeType.Root ? _curNode.InnerText : _curNode.Value;
+                return xnt == XPathNodeType.Element || xnt == XPathNodeType.Root ? _curNode.InnerText : _curNode.Value!;


What's protecting _curNode.Value from being null? (here and below)

Quite frankly I don't know. But I can't make the property nullable because the base definition isn't nullable. But changing the code to return an empty string when _curNode.Value is null seems worse than potentially lying.

But I can't make the property nullable because the base definition isn't nullable

Does that mean the base is defined incorrectly?

Looking through the code it seems like other overrides return an empty string instead, so it seems the intent was non-nullable.

carlossanlop · 2020-08-28T23:21:40Z

src/libraries/System.Data.Common/src/System/Xml/BaseTreeIterator.cs

@@ -49,7 +46,7 @@ internal bool NextRightRowElement()
        // Returns true if the current node is on a row element (head of a region)


Line 16 of this file: CurrentNode should probably be nullable. This is an internal class, and is the base class for other two classes, which are in this same PR.
At some point, the underlying private field for the currentNode is set to null. For example, line 61 of RegionIterator.cs.

Line 91 in RegionIterator.cs is doing the same, setting the value to null.

TreeIterator.cs seems to be doing the same.

Good point. Since this follows the typical iterator pattern, I have annotated the NextXxx methods with [MemberNotNullIf(true, CurrentNode)]. Sadly this doesn't do much for us because the compiler still can't infer that iter.CurrentNode isn't null. for cases like this:

for (bool fMore = iter.NextRowElement(); fMore; fMore = iter.NextRowElement()) { rowElem = (XmlBoundElement)(iter.CurrentNode!); EnsureDisconnectedDataRow(rowElem); }

src/libraries/System.Data.Common/src/System/Xml/DataPointer.cs

src/libraries/System.Data.Common/src/System/Xml/DataSetMappper.cs

pgovind · 2020-08-29T00:15:20Z

src/libraries/System.Data.Common/src/System/Xml/XPathNodePointer.cs

@@ -67,9 +64,10 @@ private XPathNodeType DecideXPNodeTypeForTextNodes(XmlNode node)
            //the function can only be called on text like nodes.
            Debug.Assert(XmlDataDocument.IsTextNode(node.NodeType));
            XPathNodeType xnt = XPathNodeType.Whitespace;
-            while (node != null)
+            XmlNode? n = null;


Why change node to n here? I think we should revert this

Because node is a parameter, which is non-nullable. The code assigns to the parameter, which then can include null values as the loop goes. And the assert would null ref when one passes null to the method. So in order to have a nullable variable for the loop but keep the parameter as non-nullable I had to introduce a local 😢

pgovind · 2020-08-29T00:23:38Z

src/libraries/System.Data.Common/src/System/Xml/XPathNodePointer.cs

@@ -339,7 +337,7 @@ internal string Value
                }
                else if (_column.ColumnMapping == MappingType.Attribute || _fOnValue)
                {
-                    DataRow row = Row;
+                    DataRow row = Row!;


This is a possible NRE. Row currently could return a null! and the next line will fail.

The way I understand the code is that when _column is non-null, Row will have a value (see the definition of Row). Sadly I don't know how to express this invariant. Marking Row as non-null seems worse though.

pgovind · 2020-08-29T00:24:08Z

src/libraries/System.Data.Common/src/System/Xml/XPathNodePointer.cs

@@ -376,7 +374,7 @@ internal string InnerText
                }
                else
                {
-                    DataRow row = Row;
+                    DataRow row = Row!;


Another possible NRE from using the Row property

See #41541 (comment)

carlossanlop · 2020-08-29T00:34:06Z

src/libraries/System.Data.Common/src/System/Xml/XPathNodePointer.cs

            if (_column != null)
            {
-                rowElem = _node as XmlBoundElement;
+                rowElem = (XmlBoundElement)_node;


I mean, technically the change is not really wrong (aside from the fact that now it could throw if the cast fails), but it's not a change related to nullability.
Up to you if you want to revert it.

This still represents a change of behavior from returning null to throwing InvalidCastException plus the method is already expected to return null.

If we lack code coverage, we would not catch if this is an invasive change regardless of the assertions.

The previous code was this:

rowElem = _node as XmlBoundElement; Debug.Assert(rowElem != null);

If the cast doesn't succeed, the next line asserts

If the _node is null, the cast will succeed but the next line asserts

In release builds, a null reference would be returned

My reasoning was: a hard cast seems more desirable because at least that makes the code fail consistently, rather than asserting in debug builds an null referencing in release builds.

But if you guys feel we shouldn't do this, I can revert.

src/libraries/System.Data.Common/src/System/Xml/XmlDataDocument.cs

src/libraries/System.Data.Common/ref/System.Data.Common.cs

stephentoub · 2020-08-30T10:24:47Z

src/libraries/System.Data.Common/src/System/Xml/BaseTreeIterator.cs

+                Debug.Assert(CurrentNode != null);
+                return true;
+            }
+            return false;


I would expect a simpler change would still satisfy the compiler:

return CurrentNode is XmlBoundElement be && be.Row != null;

Does it not?

It does. For some reason, this doesn't work:

XmlBoundElement? be = CurrentNode as XmlBoundElement; return ((be != null) && (be.Row != null));

Fixed

For some reason, this doesn't work

The compiler doesn't track the relationship between variables for nullability.

stephentoub · 2020-08-30T10:32:15Z

src/libraries/System.Data.Common/src/System/Xml/XmlDataDocument.cs

+        [MemberNotNull(nameof(_columnChangeList))]
+        [MemberNotNull(nameof(_mapper))]
+        [MemberNotNull(nameof(_foliationLock))]
+        [MemberNotNull(nameof(_attrXml))]


You can also use the ctor that takes a params array (though you'll likely also need to suppress a CLS compliance warning).

(Random side question: why do we get CLS compliance warnings on attributes of non-public members?)

AFAIK CLS compliance is about consistent attribution and doesn't take visibility into consideration. If you say "the entire assembly/type is CLS compliant", then the compiler will force you to mark the ones that aren't, no matter their visibility. Presumably, that was done to make the rules simpler.

You can also use the ctor that takes a params array (though you'll likely also need to suppress a CLS compliance warning).

I noticed the CLS compliance warning and looked around what other parts did. The other locations in S.D.C seemed use multiple applications, so that's what I did. Do you want me to change them all?

AFAIK CLS compliance is about consistent attribution and doesn't take visibility into consideration.

That isn't true for method return types:

Do you want me to change them all?

You don't need to. I was just calling out the alternative.

AFAIK CLS compliance is about consistent attribution and doesn't take visibility into consideration.

That isn't true for method return types:

OK, now I'm confused :-)

eerhardt · 2020-08-31T19:40:41Z

src/libraries/System.Data.Common/src/System/Xml/XmlDataDocument.cs

@@ -2539,7 +2537,7 @@ public XmlDataDocument() : base(new XmlDataImplementation())
        {
            Init();
            AttachDataSet(new DataSet());
-            _dataSet.EnforceConstraints = false;
+            _dataSet!.EnforceConstraints = false;


(nit) if AttachDataSet was annotated with [MemberNotNull(nameof(_dataSet))], this ! wouldn't be needed.

Good call! Fixed.

eerhardt · 2020-08-31T19:43:37Z

src/libraries/System.Data.Common/src/System/Xml/XmlDataDocument.cs

-
+            // This constructor is used by XmlDataImplementation.CreateDocument(), which
+            // exposes it as XmlDocument. The methods using these fields are never called.
+            _dataSet = null!;


Do we need both the = null! here and in the field initializers at the top of the class?

private DataSet _dataSet = null!;

My apologies, I thought I reverted them already, which I didn't. Fixed.

eerhardt · 2020-08-31T20:04:05Z

src/libraries/System.Data.Common/src/System/Xml/XmlDataDocument.cs

-        }
-
+            // This constructor is used by XmlDataImplementation.CreateDocument(), which
+            // exposes it as XmlDocument. The methods using these fields are never called.


I'm not sure if this comment is 100% accurate. From looking at the code, it looks like XmlDataImplementation.CreateDocument() can be called publicly:

using System.Xml; var x = new XmlDataDocument(); x.AppendChild(x.CreateElement("h")); var y = (XmlDataDocument)x.Implementation.CreateDocument(); Console.WriteLine(y.DataSet?.ToString() ?? "<null>");

The code above prints <null>.

That is fair. But a document constructed from there will also null ref, for example:

y.GetRowFromElement(y.DocumentElement);

Without some surgery I don't believe an XmlDataDocument that was created via XmlDataImplementation.CreateDocument() can be used safely today 😢

eerhardt · 2020-08-31T20:26:40Z

src/libraries/System.Data.Common/src/System/Xml/XPathNodePointer.cs

@@ -67,9 +64,10 @@ private XPathNodeType DecideXPNodeTypeForTextNodes(XmlNode node)
            //the function can only be called on text like nodes.
            Debug.Assert(XmlDataDocument.IsTextNode(node.NodeType));
            XPathNodeType xnt = XPathNodeType.Whitespace;
-            while (node != null)
+            XmlNode? n = null;


Doesn't this need to be XmlNode? n = node;? As written, I don't think you will ever get into the loop.

Damn. Good catch! Fixed.

eerhardt

This looks good.

pgovind

LGTM now.

jozkee

Other than this comment, LGTM.

In order to match with src

* 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 <[email protected]> Co-authored-by: David Cantú <[email protected]>

roji · 2020-09-04T09:15:36Z

@terrajobst sorry for not reviewing, personal issues... Not sure I'd have a lot to contribute on the XML stuff anyway :)

Add nullable annotations for XmlDataDocument

c19f67e

terrajobst added area-System.Data area-System.Xml labels Aug 28, 2020

terrajobst requested review from roji and a team August 28, 2020 22:59

pgovind reviewed Aug 28, 2020

View reviewed changes

carlossanlop mentioned this pull request Aug 28, 2020

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

Merged

14 tasks

eerhardt reviewed Aug 28, 2020

View reviewed changes

carlossanlop reviewed Aug 28, 2020

View reviewed changes

pgovind reviewed Aug 28, 2020

View reviewed changes

src/libraries/System.Data.Common/src/System/Xml/DataPointer.cs Outdated Show resolved Hide resolved

carlossanlop reviewed Aug 28, 2020

View reviewed changes

src/libraries/System.Data.Common/src/System/Xml/DataPointer.cs Show resolved Hide resolved

carlossanlop reviewed Aug 28, 2020

View reviewed changes

src/libraries/System.Data.Common/src/System/Xml/DataSetMappper.cs Show resolved Hide resolved

pgovind reviewed Aug 29, 2020

View reviewed changes

src/libraries/System.Data.Common/src/System/Xml/DataSetMappper.cs Outdated Show resolved Hide resolved

pgovind reviewed Aug 29, 2020

View reviewed changes

carlossanlop reviewed Aug 29, 2020

View reviewed changes

jozkee reviewed Aug 29, 2020

View reviewed changes

src/libraries/System.Data.Common/src/System/Xml/XmlDataDocument.cs Show resolved Hide resolved

jozkee reviewed Aug 29, 2020

View reviewed changes

src/libraries/System.Data.Common/ref/System.Data.Common.cs Outdated Show resolved Hide resolved

Address code review feedback

4a283e4

stephentoub reviewed Aug 30, 2020

View reviewed changes

More code review feedback

d633f65

terrajobst requested review from carlossanlop, stephentoub, pgovind and jozkee August 31, 2020 17:36

terrajobst requested a review from eerhardt August 31, 2020 17:36

eerhardt reviewed Aug 31, 2020

View reviewed changes

Address Eric's feedback

b169a62

eerhardt approved these changes Aug 31, 2020

View reviewed changes

Annotate _dataSet as not null when AttachDataSet is called

c4cacc0

pgovind approved these changes Aug 31, 2020

View reviewed changes

jozkee approved these changes Aug 31, 2020

View reviewed changes

Fix GetElementById return type annotation

9ad4719

In order to match with src

terrajobst merged commit 381c164 into dotnet:master Sep 1, 2020

terrajobst deleted the nullable-xmldatadocument branch September 1, 2020 01:09

ghost locked as resolved and limited conversation to collaborators Dec 7, 2020

		@@ -49,7 +46,7 @@ internal bool NextRightRowElement()
		// Returns true if the current node is on a row element (head of a region)

Add nullable annotations for XmlDataDocument #41541

Add nullable annotations for XmlDataDocument #41541

Conversation

terrajobst commented Aug 28, 2020

ghost commented Aug 28, 2020

pgovind Aug 28, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

terrajobst Aug 29, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

terrajobst Aug 29, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlossanlop Aug 28, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

terrajobst Aug 29, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlossanlop Aug 29, 2020 • edited Loading

Choose a reason for hiding this comment

jozkee Aug 29, 2020 • edited Loading

Choose a reason for hiding this comment

terrajobst Aug 29, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eerhardt left a comment • edited Loading

Choose a reason for hiding this comment

pgovind left a comment

Choose a reason for hiding this comment

jozkee left a comment

Choose a reason for hiding this comment

roji commented Sep 4, 2020

pgovind Aug 28, 2020 •

edited

Loading

terrajobst Aug 29, 2020 •

edited

Loading

terrajobst Aug 29, 2020 •

edited

Loading

carlossanlop Aug 28, 2020 •

edited

Loading

terrajobst Aug 29, 2020 •

edited

Loading

carlossanlop Aug 29, 2020 •

edited

Loading

jozkee Aug 29, 2020 •

edited

Loading

terrajobst Aug 29, 2020 •

edited

Loading

eerhardt left a comment •

edited

Loading