Port nullability annotations to System.Xml.ReaderWriter contract

[jozkee]

Contributes to #2339

[ghost]

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

[eerhardt]

    string System.Xml.IXmlNamespaceResolver.LookupNamespace(string prefix) { throw null; }

Should this return string? ? The non-interface implementation method can return null above.

---

Refers to: src/libraries/System.Xml.ReaderWriter/ref/System.Xml.ReaderWriter.cs:1073 in a2a0ead. [](commit_id = a2a0ead2ac98db85b3d8bcb5de769dafdc7ca897, deletion_comment = False)

[eerhardt]

    public virtual bool IsStartElement(string localname, string ns) { throw null; }

Should ns be string? ns ? I believe this allows for null.

---

Refers to: src/libraries/System.Xml.ReaderWriter/ref/System.Xml.ReaderWriter.cs:825 in a2a0ead. [](commit_id = a2a0ead2ac98db85b3d8bcb5de769dafdc7ca897, deletion_comment = False)

[eerhardt]

    public void WriteAttributeString(string? prefix, string localName, string? ns, string? value) { }

How come this overload has string? value, but the 2 above it have string value?

---

Refers to: src/libraries/System.Xml.ReaderWriter/ref/System.Xml.ReaderWriter.cs:1249 in a2a0ead. [](commit_id = a2a0ead2ac98db85b3d8bcb5de769dafdc7ca897, deletion_comment = False)

[jozkee]

As per documentation:

If the attribute replaces an existing attribute with the same name, the old XmlAttribute is returned; otherwise, null is returned.

Suggested change

	public virtual System.Xml.XmlAttribute SetAttributeNode(System.Xml.XmlAttribute newAttr) { throw null; }
	public virtual System.Xml.XmlAttribute? SetAttributeNode(System.Xml.XmlAttribute newAttr) { throw null; }

[krwq]

I don't believe this actually returns null - looking at the implementation it returns old node if it already exists and new node if it doesn't

[jozkee]

I changed it because is virtual and someone could override it to return null; looking at "find all references" in VS, we don't even dereference the method's return value so it doesn't really affect libraries code if the method returns null or not.

If you feels like is pointless to make it nullable I can undo this change.

[krwq]

string namespaceURI is correct here as it will throw but I think this is incidental and due to bug in the code, I think we should ideally fix the bug and change annotation (not in this PR)

[krwq]

Most likely couple of namespaceResolvers below should be nullable, i.e.:
ReadElementContentAs, ReadElementContentAsAsync

[krwq]

Seems it will throw InvalidOperationException in some cases when this is null

[jozkee]

Yes, that's why I didn't change those signatures on #41079

[krwq]

LGTM after fixing/resolving comments

[krwq]

    public virtual bool IsStartElement(string localname, string ns) { throw null; }

Should ns be string? ns ? I believe this allows for null.

Refers to: src/libraries/System.Xml.ReaderWriter/ref/System.Xml.ReaderWriter.cs:825 in a2a0ead. [](commit_id = a2a0ead, deletion_comment = False)

I think this will always return false if you pass null. In all other places null implies string.Empty - I think it might be better to leave it as non-null

[eerhardt]

I think this will always return false if you pass null. In all other places null implies string.Empty - I think it might be better to leave it as non-null

You're right. I looked at the implementation and this appears broken to me. We should leave it as you have it now, and I opened #41461 for us to consider to make it nullable in the future.