Fix NRT annotations for XmlProcessingInstruction (#73687)

* Fix NRT annotations for XmlProcessingInstruction

* fix comment, replace assert with ArgumentNullException.ThrowIfNull

* add argument name to ArgumentException, fix comment

src/libraries/System.Private.Xml/src/System/Xml/Core/XmlTextWriter.cs

@@ -1693,7 +1693,7 @@ private void InternalWriteName(string name, bool isNCName)
         // all valid name characters at that position. This can't be changed because of backwards compatibility.
         private void ValidateName(string name, bool isNCName)
         {
-            if (name == null || name.Length == 0)
+            if (string.IsNullOrEmpty(name))
             {
                 throw new ArgumentException(SR.Xml_EmptyName);
             }

src/libraries/System.Private.Xml/src/System/Xml/Dom/XmlDocument.cs

@@ -727,8 +727,9 @@ public virtual XmlEntityReference CreateEntityReference(string name)
 
         // Creates a XmlProcessingInstruction with the specified name
         // and data strings.
-        public virtual XmlProcessingInstruction CreateProcessingInstruction(string target, string data)
+        public virtual XmlProcessingInstruction CreateProcessingInstruction(string target, string? data)
         {
+            ArgumentNullException.ThrowIfNull(target);
             return new XmlProcessingInstruction(target, data, this);
         }
 

src/libraries/System.Private.Xml/src/System/Xml/Dom/XmlProcessingInstruction.cs

@@ -14,94 +14,93 @@ public class XmlProcessingInstruction : XmlLinkedNode
         private readonly string _target;
         private string _data;
 
-        protected internal XmlProcessingInstruction(string target, string data, XmlDocument doc) : base(doc)
+        protected internal XmlProcessingInstruction(string target, string? data, XmlDocument doc) : base(doc)
         {
-            _target = target;
-            _data = data;
-        }
+            ArgumentNullException.ThrowIfNull(target);
 
-        // Gets the name of the node.
-        public override string Name
-        {
-            get
+            if (target.Length == 0)
             {
-                if (_target != null)
-                    return _target;
-                return string.Empty;
+                throw new ArgumentException(SR.Xml_EmptyName, nameof(target));
             }
-        }
 
-        // Gets the name of the current node without the namespace prefix.
-        public override string LocalName
-        {
-            get { return Name; }
+            _target = target;
+            _data = data ?? string.Empty;
         }
 
-        // Gets or sets the value of the node.
+        /// <inheritdoc />
+        public override string Name => _target;
+
+        /// <inheritdoc />
+        public override string LocalName => Name;
+
+        /// <inheritdoc />
         [AllowNull]
         public override string Value
         {
-            get { return _data; }
-            set { Data = value!; } //use Data instead of data so that event will be fired
+            get => _data;
+            set { Data = value; } // uses Data instead of data so that event will be fired and null will be normalized to empty string
         }
 
         // Gets the target of the processing instruction.
-        public string? Target
-        {
-            get { return _target; }
-        }
+        public string Target => _target;
 
         // Gets or sets the content of processing instruction,
         // excluding the target.
+        [AllowNull]
         public string Data
         {
-            get { return _data; }
+            get => _data;
             set
             {
                 XmlNode? parent = ParentNode;
-                XmlNodeChangedEventArgs? args = GetEventArgs(this, parent, parent, _data, value, XmlNodeChangedAction.Change);
+                string val = value ?? string.Empty;
+                XmlNodeChangedEventArgs? args = GetEventArgs(this, parent, parent, _data, val, XmlNodeChangedAction.Change);
+
                 if (args != null)
+                {
                     BeforeEvent(args);
-                _data = value;
+                }
+
+                _data = val;
+
                 if (args != null)
+                {
                     AfterEvent(args);
+                }
             }
         }
 
-        // Gets or sets the concatenated values of the node and
-        // all its children.
+        /// <inheritdoc />
+        [AllowNull]
         public override string InnerText
         {
-            get { return _data; }
-            set { Data = value; } //use Data instead of data so that change event will be fired
+            get => _data;
+            set { Data = value; } // uses Data instead of data so that event will be fired and null will be normalized to empty string
         }
 
-        // Gets the type of the current node.
-        public override XmlNodeType NodeType
-        {
-            get { return XmlNodeType.ProcessingInstruction; }
-        }
+        /// <inheritdoc />
+        public override XmlNodeType NodeType => XmlNodeType.ProcessingInstruction;
 
-        // Creates a duplicate of this node.
+        /// <inheritdoc />
         public override XmlNode CloneNode(bool deep)
         {
             Debug.Assert(OwnerDocument != null);
             return OwnerDocument.CreateProcessingInstruction(_target, _data);
         }
 
-        // Saves the node to the specified XmlWriter.
+        /// <inheritdoc />
         public override void WriteTo(XmlWriter w)
         {
             w.WriteProcessingInstruction(_target, _data);
         }
 
-        // Saves all the children of the node to the specified XmlWriter.
+        /// <inheritdoc />
         public override void WriteContentTo(XmlWriter w)
         {
             // Intentionally do nothing
         }
 
-        internal override string XPLocalName { get { return Name; } }
-        internal override XPathNodeType XPNodeType { get { return XPathNodeType.ProcessingInstruction; } }
+        internal override string XPLocalName => Name;
+        internal override XPathNodeType XPNodeType => XPathNodeType.ProcessingInstruction;
     }
 }

src/libraries/System.Private.Xml/tests/XmlDocument/XmlDocumentTests/CreateProcessingInstruction.cs

@@ -7,14 +7,33 @@ namespace System.Xml.Tests
 {
     public class CreateProcessingInstruction
     {
-        [Fact]
-        public static void BasicCreate()
+        [Theory]
+        [InlineData("bar", "foo", "<?bar foo?>")]
+        [InlineData("bar", "", "<?bar ?>")]
+        [InlineData("bar", null, "<?bar ?>")]
+        [InlineData("foo.bar", null, "<?foo.bar ?>")]
+        [InlineData("foo:bar", null, "<?foo:bar ?>")]
+        public static void ProcessingInstructionCanBeCreatedAndSerialized(string target, string? data, string expectedOutput)
         {
             var xmlDocument = new XmlDocument();
-            var newNode = xmlDocument.CreateProcessingInstruction("bar", "foo");
+            var newNode = xmlDocument.CreateProcessingInstruction(target, data);
 
-            Assert.Equal("<?bar foo?>", newNode.OuterXml);
+            Assert.Equal(expectedOutput, newNode.OuterXml);
             Assert.Equal(XmlNodeType.ProcessingInstruction, newNode.NodeType);
         }
+
+        [Fact]
+        public static void NullTargetThrows()
+        {
+            var xmlDocument = new XmlDocument();
+            Assert.Throws<ArgumentNullException>(() => xmlDocument.CreateProcessingInstruction(null, "anyData"));
+        }
+
+        [Fact]
+        public static void EmptyTargetThrows()
+        {
+            var xmlDocument = new XmlDocument();
+            Assert.Throws<ArgumentException>(() => xmlDocument.CreateProcessingInstruction("", "anyData"));
+        }
     }
 }

src/libraries/System.Private.Xml/tests/XmlDocument/XmlNodeTests/ValueTests.cs

@@ -137,5 +137,60 @@ public static void DocumentFragment()
             Assert.Null(node.Value);
             Assert.Throws<InvalidOperationException>(() => node.Value = "some value");
         }
+
+        [Fact]
+        public static void CreateProcessingInstructionTranslatesNullIntoEmptyString()
+        {
+            var xmlDocument = new XmlDocument();
+            XmlProcessingInstruction pi = xmlDocument.CreateProcessingInstruction("test", null);
+            Assert.Equal(string.Empty, pi.Data);
+            Assert.Equal(string.Empty, pi.Value);
+            Assert.Equal(string.Empty, pi.InnerText);
+        }
+
+        [Fact]
+        public static void SettingNullDataTranslatesIntoEmptyString()
+        {
+            var xmlDocument = new XmlDocument();
+            XmlProcessingInstruction pi = xmlDocument.CreateProcessingInstruction("test", "foo");
+            Assert.Equal("foo", pi.Data);
+            Assert.Equal("foo", pi.Value);
+            Assert.Equal("foo", pi.InnerText);
+
+            pi.Data = null;
+            Assert.Equal(string.Empty, pi.Data);
+            Assert.Equal(string.Empty, pi.Value);
+            Assert.Equal(string.Empty, pi.InnerText);
+        }
+
+        [Fact]
+        public static void SettingNullValueTranslatesIntoEmptyString()
+        {
+            var xmlDocument = new XmlDocument();
+            XmlProcessingInstruction pi = xmlDocument.CreateProcessingInstruction("test", "foo");
+            Assert.Equal("foo", pi.Data);
+            Assert.Equal("foo", pi.Value);
+            Assert.Equal("foo", pi.InnerText);
+
+            pi.Value = null;
+            Assert.Equal(string.Empty, pi.Data);
+            Assert.Equal(string.Empty, pi.Value);
+            Assert.Equal(string.Empty, pi.InnerText);
+        }
+
+        [Fact]
+        public static void SettingNullInnerTextTranslatesIntoEmptyString()
+        {
+            var xmlDocument = new XmlDocument();
+            XmlProcessingInstruction pi = xmlDocument.CreateProcessingInstruction("test", "foo");
+            Assert.Equal("foo", pi.Data);
+            Assert.Equal("foo", pi.Value);
+            Assert.Equal("foo", pi.InnerText);
+
+            pi.InnerText = null;
+            Assert.Equal(string.Empty, pi.Data);
+            Assert.Equal(string.Empty, pi.Value);
+            Assert.Equal(string.Empty, pi.InnerText);
+        }
     }
 }

src/libraries/System.Xml.ReaderWriter/ref/System.Xml.ReaderWriter.cs

@@ -343,7 +343,7 @@ public partial class XmlDocument : System.Xml.XmlNode
         public virtual System.Xml.XmlNode CreateNode(string nodeTypeString, string name, string? namespaceURI) { throw null; }
         public virtual System.Xml.XmlNode CreateNode(System.Xml.XmlNodeType type, string name, string? namespaceURI) { throw null; }
         public virtual System.Xml.XmlNode CreateNode(System.Xml.XmlNodeType type, string? prefix, string name, string? namespaceURI) { throw null; }
-        public virtual System.Xml.XmlProcessingInstruction CreateProcessingInstruction(string target, string data) { throw null; }
+        public virtual System.Xml.XmlProcessingInstruction CreateProcessingInstruction(string target, string? data) { throw null; }
         public virtual System.Xml.XmlSignificantWhitespace CreateSignificantWhitespace(string? text) { throw null; }
         public virtual System.Xml.XmlText CreateTextNode(string? text) { throw null; }
         public virtual System.Xml.XmlWhitespace CreateWhitespace(string? text) { throw null; }
@@ -738,13 +738,15 @@ public partial class XmlParserContext
     }
     public partial class XmlProcessingInstruction : System.Xml.XmlLinkedNode
     {
-        protected internal XmlProcessingInstruction(string target, string data, System.Xml.XmlDocument doc) { }
+        protected internal XmlProcessingInstruction(string target, string? data, System.Xml.XmlDocument doc) { }
+        [System.Diagnostics.CodeAnalysis.AllowNullAttribute]
         public string Data { get { throw null; } set { } }
+        [System.Diagnostics.CodeAnalysis.AllowNullAttribute]
         public override string InnerText { get { throw null; } set { } }
         public override string LocalName { get { throw null; } }
         public override string Name { get { throw null; } }
         public override System.Xml.XmlNodeType NodeType { get { throw null; } }
-        public string? Target { get { throw null; } }
+        public string Target { get { throw null; } }
         [System.Diagnostics.CodeAnalysis.AllowNullAttribute]
         public override string Value { get { throw null; } set { } }
         public override System.Xml.XmlNode CloneNode(bool deep) { throw null; }