Add nullable annotations for XmlDataDocument (#41541)

* 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]>
dotnet · Sep 1, 2020 · 74dc622 · 74dc622
1 parent 4969648
commit 74dc622
Show file tree

Hide file tree

Showing 12 changed files with 411 additions and 408 deletions.
diff --git a/src/libraries/System.Data.Common/ref/System.Data.Common.cs b/src/libraries/System.Data.Common/ref/System.Data.Common.cs
@@ -3536,7 +3536,6 @@ public enum StorageState
         UnmanagedBuffer = 2,
     }
 }
-#nullable disable
 namespace System.Xml
 {
     [System.ObsoleteAttribute("XmlDataDocument class will be removed in a future release.")]
@@ -3546,17 +3545,16 @@ public XmlDataDocument() { }
         public XmlDataDocument(System.Data.DataSet dataset) { }
         public System.Data.DataSet DataSet { get { throw null; } }
         public override System.Xml.XmlNode CloneNode(bool deep) { throw null; }
-        public override System.Xml.XmlElement CreateElement(string prefix, string localName, string namespaceURI) { throw null; }
+        public override System.Xml.XmlElement CreateElement(string? prefix, string localName, string? namespaceURI) { throw null; }
         public override System.Xml.XmlEntityReference CreateEntityReference(string name) { throw null; }
-        protected override System.Xml.XPath.XPathNavigator CreateNavigator(System.Xml.XmlNode node) { throw null; }
-        public override System.Xml.XmlElement GetElementById(string elemId) { throw null; }
+        protected override System.Xml.XPath.XPathNavigator? CreateNavigator(System.Xml.XmlNode node) { throw null; }
+        public override System.Xml.XmlElement? GetElementById(string elemId) { throw null; }
         public System.Xml.XmlElement GetElementFromRow(System.Data.DataRow r) { throw null; }
         public override System.Xml.XmlNodeList GetElementsByTagName(string name) { throw null; }
-        public System.Data.DataRow GetRowFromElement(System.Xml.XmlElement e) { throw null; }
+        public System.Data.DataRow? GetRowFromElement(System.Xml.XmlElement? e) { throw null; }
         public override void Load(System.IO.Stream inStream) { }
         public override void Load(System.IO.TextReader txtReader) { }
         public override void Load(string filename) { }
         public override void Load(System.Xml.XmlReader reader) { }
     }
 }
-#nullable enable
diff --git a/src/libraries/System.Data.Common/src/System/Xml/BaseTreeIterator.cs b/src/libraries/System.Data.Common/src/System/Xml/BaseTreeIterator.cs
@@ -1,8 +1,8 @@
 // Licensed to the .NET Foundation under one or more agreements.
 // The .NET Foundation licenses this file to you under the MIT license.
 
-// TODO: Enable after System.Private.Xml is annotated
-#nullable disable
+using System.Diagnostics;
+using System.Diagnostics.CodeAnalysis;
 
 namespace System.Xml
 {
@@ -16,11 +16,15 @@ internal BaseTreeIterator(DataSetMapper mapper)
             this.mapper = mapper;
         }
 
-        internal abstract XmlNode CurrentNode { get; }
+        internal abstract XmlNode? CurrentNode { get; }
 
+        [MemberNotNullWhen(true, nameof(CurrentNode))]
         internal abstract bool Next();
+
+        [MemberNotNullWhen(true, nameof(CurrentNode))]
         internal abstract bool NextRight();
 
+        [MemberNotNullWhen(true, nameof(CurrentNode))]
         internal bool NextRowElement()
         {
             while (Next())
@@ -33,6 +37,7 @@ internal bool NextRowElement()
             return false;
         }
 
+        [MemberNotNullWhen(true, nameof(CurrentNode))]
         internal bool NextRightRowElement()
         {
             if (NextRight())
@@ -47,10 +52,10 @@ internal bool NextRightRowElement()
         }
 
         // Returns true if the current node is on a row element (head of a region)
+        [MemberNotNullWhen(true, nameof(CurrentNode))]
         internal bool OnRowElement()
         {
-            XmlBoundElement be = CurrentNode as XmlBoundElement;
-            return (be != null) && (be.Row != null);
+            return CurrentNode is XmlBoundElement be && be.Row != null;
         }
     }
 }
diff --git a/src/libraries/System.Data.Common/src/System/Xml/DataDocumentXPathNavigator.cs b/src/libraries/System.Data.Common/src/System/Xml/DataDocumentXPathNavigator.cs
@@ -1,9 +1,6 @@
 // Licensed to the .NET Foundation under one or more agreements.
 // The .NET Foundation licenses this file to you under the MIT license.
 
-// TODO: Enable after System.Private.Xml is annotated
-#nullable disable
-
 using System.Xml.XPath;
 
 #pragma warning disable 0618 // ignore obsolete warning about XmlDataDocument
@@ -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!;
             }
         }
 
@@ -73,7 +70,7 @@ public override string GetAttribute(string localName, string namespaceURI)
             }
 
             _temp.MoveTo(_curNode);
-            return _temp.MoveToAttribute(localName, namespaceURI) ? _temp.Value : string.Empty;
+            return _temp.MoveToAttribute(localName, namespaceURI) ? _temp.Value! : string.Empty;
         }
 
         public override string GetNamespace(string name) => _curNode.GetNamespace(name);
@@ -129,7 +126,7 @@ public override bool MoveTo(XPathNavigator other)
         {
             if (other != null)
             {
-                DataDocumentXPathNavigator otherDataDocXPathNav = other as DataDocumentXPathNavigator;
+                DataDocumentXPathNavigator? otherDataDocXPathNav = other as DataDocumentXPathNavigator;
                 if (otherDataDocXPathNav != null && _curNode.MoveTo(otherDataDocXPathNav.CurNode))
                 {
                     _doc = _curNode.Document;
@@ -146,7 +143,7 @@ public override bool IsSamePosition(XPathNavigator other)
         {
             if (other != null)
             {
-                DataDocumentXPathNavigator otherDataDocXPathNav = other as DataDocumentXPathNavigator;
+                DataDocumentXPathNavigator? otherDataDocXPathNav = other as DataDocumentXPathNavigator;
                 if (otherDataDocXPathNav != null &&
                     _doc == otherDataDocXPathNav.Document && _curNode.IsSamePosition(otherDataDocXPathNav.CurNode))
                 {
@@ -158,16 +155,16 @@ public override bool IsSamePosition(XPathNavigator other)
 
         //the function is only called for XPathNodeList enumerate nodes and
         // shouldn't be promoted to frequently use because it will cause foliation
-        XmlNode IHasXmlNode.GetNode() => _curNode.Node;
+        XmlNode IHasXmlNode.GetNode() => _curNode.Node!;
 
-        public override XmlNodeOrder ComparePosition(XPathNavigator other)
+        public override XmlNodeOrder ComparePosition(XPathNavigator? other)
         {
             if (other == null)
             {
                 return XmlNodeOrder.Unknown; // this is what XPathDocument does.
             }
 
-            DataDocumentXPathNavigator otherDataDocXPathNav = other as DataDocumentXPathNavigator;
+            DataDocumentXPathNavigator? otherDataDocXPathNav = other as DataDocumentXPathNavigator;
 
             return otherDataDocXPathNav == null || otherDataDocXPathNav.Document != _doc ?
                 XmlNodeOrder.Unknown :