Nullable annotations: XmlNode.AppendChild

[jods4]

After migration to .NET 5.0 we got tons of nullable warnings (that we treat as errors) because XmlNode.AppendChild was annotated as returning a possibly null XmlNode?.

I looked at the source code and I think this is technically correct but would happen only if you pass an empty xml fragment as a parameter.

Because of this edge case, tons of common valid code like this one creates warning:

var doc = new XmlDocument();
var root = doc.AppendChild(doc.CreateElement("root")); // XmlNode?
root.AppendChild(doc.CreateElement("child")); // warning!

This is really counter-productive because it creates tons of "incorrect" (in the sense that it won't happen at runtime) warnings in common, idiomatic, valid code.

One way to fix this -- if you want to keep the "possibly null" annotation -- could be to create a more specific overload that takes XmlElement as a parameter and then just calls AppendChild(XNode) but with a non-null assertion added.

Or achieve the same with a magic attribute (not sure if it's possible)?

[ghost]

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

Issue Details

---

After migration to .NET 5.0 we got tons of nullable warnings (that we treat as errors) because `XmlNode.AppendChild` was annotated as returning a possibly null `XmlNode?`. I looked at the source code and I think this is technically correct but would happen _only if you pass an empty xml fragment_ as a parameter. Because of this edge case, tons of common valid code like this one creates warning: ```csharp var doc = new XmlDocument(); var root = doc.AppendChild(doc.CreateElement("root")); // XmlNode? root.AppendChild(doc.CreateElement("child")); // warning! ``` This is really counter-productive because it creates tons of "incorrect" (in the sense that it won't happen at runtime) warnings in common, idiomatic, valid code. One way to fix this -- if you want to keep the "possibly null" annotation -- could be to create a more specific overload that takes `XmlElement` as a parameter and then just calls `AppendChild(XNode)` but with a non-null assertion added. Or achieve the same with a magic attribute (not sure if it's possible)?

Author:	jods4
Assignees:	-
Labels:	`area-System.Xml`, `untriaged`
Milestone:	-

[krwq]

Thanks @jods4! I agree this is a pain point. I do not promise we will fix it in this release but I hope we will at least figure out some generic solution for APIs like this so we know what the next steps should be (I want to make sure we clarify the guidelines about this).

For now I'd recommend you to create an extension method which returns non-nullable.

One possible solution for this specific problem could be to make APIs like AppendChild be able to deal with null values so at least the most common scenarios don't produce warnings

[jods4]

@krwq Thank you, sounds good.

I apologize but I'll take this opportunity to go on a short rant.

I would love to move my piece of code away from XmlDocument and co. and use the XDocument apis, which are better and more modern.

Sadly it seems MS stopped working on XML technologies a long time ago (no adoption of old standards such as newer xslt, xpath, etc.; no perf work despite everything else in .net core getting overhauls; etc.).

In this instance, I'm stuck with XmlDocument because MS never even ported signatures (XmlDsigEnvelopedSignatureTransform and co.) from XmlDocument to the new Linq to XML. So Linq to XML is not even on par with the "obsolete" XmlDocument.

I'm also a web developer and I know XML is not "modern" or "trendy" those days, and it hasn't been for a long time now. But the reality is that many large industries such as finance or healthcare heavily rely on XML as their document format. Think about SVG, MusicXML, OpenDocument/Office, MathML. Look at ISO 20022, it defines 695 different XML messages and it is the de facto standard for everyone in banking.

XML is not a cool kid anymore but it is very, very relevant in large businesses and it would be nice if it got a little more .net love.

Sorry for the rant. 🐹 You're doing an amazing job on .net those days.

[krwq]

@jods4 I understand the frustration. See https://github.com/dotnet/runtime/issues/14819 for the explanation for why this is not being worked on. In short we've done investigation on that but we've found that there is relatively little gain and a lot of work (we do not have data on that many people using newer version to justify the cost). I'd recommend you to respond to that thread with any more data and upvote and perhaps it will be reconsidered one day.

When speaking about SignedXml we can't really remove old APIs and replace them with new ones as that would break a lot of people and having double APIs would be painful.

On top of that we recommend to use SignedCms rather than SignedXml which works on bytes and does not try to reason about the structure of the data

[jods4]

@krwq Thanks for the feedback. In that issue XmlPrime is mentionned and it looks like a good (commercial) alternative for newer XQuery, XPath, XSLT, XSD specs.

To be fair, even though I'm working with a lot of XML documents I don't have much use for those newer techs, so I'm not gonna push this with an upvote.

If that's ever possible I'd rather see MS improve the fundamentals of working with XML documents, for example:

• Updated xsd tooling: https://developercommunity.visualstudio.com/idea/426740/better-xsd-xml-tooling.html
• This guy is keeping "linq to xsd" alive (originally created by MS) and it's a great tool: https://github.com/mamift/LinqToXsdCore
• Feature parity with original XML apis, notably EncryptedXml and SignedXml

With respect to that last point:

On top of that we recommend to use SignedCms rather than SignedXml which works on bytes and does not try to reason about the structure of the data

Sadly in my case I have no choice. We're consuming public API from banks and they require EncryptedXml + SignedXml + ISO 20022. I'm not gonna name the banks but these are brand new API, not legacy stuff. Nobody talks about XML but it's still well alive.

Thanks again for the feedback.

[krwq]

I've took a quick look at this and I think we can close this as by design. Here is the scenario:

XmlDocument doc = new();
doc.LoadXml("<root></root>");

XmlDocumentFragment fragment = doc.CreateDocumentFragment();
//fragment.InnerXml = "<foo/>";
XmlNode? node = doc.DocumentElement!.AppendChild(fragment);
Console.WriteLine(node != null); // false with commented out code, true when uncommented
Console.WriteLine(doc.OuterXml);

while this is unfortunate it is technically correct and I'm not sure about any good way to fix this other than forbidding document fragments