Runtime: Nullability annotations in XElement constructor

[olmobrutall]

After downloading the latest .Net 5 SDK (Preview 2) I'm starting to update my code to it.

I'm using nullable reference types.

I think the nullability annotations could be improved in https://github.com/dotnet/runtime/blob/035b729d829368c2790d825bd02db14f0c0fd2ea/src/libraries/System.Private.Xml.Linq/src/System/Xml/Linq/XElement.cs#L95

C# public XElement(XName name, params object[] content) : this(name, (object)content) { } //Currently public XElement(XName name, params object?[] content) : this(name, (object)content) { } //Fixed

[msftbot[bot]]

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

[danmosemsft]

@krwq

[jeffhandley]

Here's the bar we're using for whether or not to take nullable annotation fixes in 5.0:

1. Must not introduce new warnings. A few examples (but this is not exhaustive):
   
   
   
   • Changing the return type of a non-virtual member from nullable to non-nullable is OK; the other direction is NOT OK
   
   
   • Changing the return type of a virtual member is NOT OK
   
   
   • Changing a method parameter from non-nullable to nullable is OK; the other direction is NOT OK
   
   
2. Must be related to annotations made during 5.0 (not in 3.0)
3. Must be based on customer feedback
4. Must affect a commonly-used API

Reviewing this issue, it meets that bar.

1. This relaxes warnings
2. This API was annotated in 5.0
3. This is based on customer feedback (thanks, @olmobrutall!)
4. apisof.net shows 38% of scanned projects use XElement and 14% use this specific constructor (I would consider that to be commonly-used)

@krwq if you agree with this fix, I would support taking it to tactics for a 5.0 GA port this week.

[germanftorres]

The XElement constructor that accepts an array of objects is very useful for building documents, specially for conditionally adding xml nodes to a parent node. If the element is null, nothing is added to the XElement.

Looking forward for having this solved!

var ele = new XElement("FlightRequest",
    new XElement("From", "MAD"),
    new XElement("To", "SFO"),

    // this line gives warning CS8604
    !string.IsNullOrEmpty(carrier) ? new XElement("Carrier", carrier) : null
);

[krwq]

cc: @Jozkee who annotated XLinq

[krwq]

quickly looking at the code I think the code should be fine with accepting null as array elements so if you find this scenario common I'm fine with this change

[Jozkee]

Should we expand this to other signatures containing params object[] as argument? At a first glance, their impl. looks capable of handling null elements.

https://source.dot.net/#System.Private.Xml.Linq/System/Xml/Linq/XElement.cs,986
https://source.dot.net/#System.Private.Xml.Linq/System/Xml/Linq/XElement.cs,1023
https://source.dot.net/#System.Private.Xml.Linq/System/Xml/Linq/XContainer.cs,193

There may be more cases.

[krwq]

yes, we should back track and find all instances and be consistent across APIs

[jeffhandley]

As noted on #43717, this fix will target 6.0 and won't meet the bar for getting merged into 5.0 at this point. Thank you again for reporting it, @olmobrutall; I'm sorry for the inconvenience that will exist here.

[olmobrutall]

It's a pity... not for me, I've already updated to .Net 5 preview, but for any other project using Linq to Xml with NRT.

But just to have an idea of the impact, check out this commit https://github.com/signumsoftware/extensions/commit/cdba46fbbc27cd367b2110bf3c9ced323a1ba31d and search for null!. I count 94 only in this repo, all related to this issue.

Maybe this example can be of use to escalate it and get into .Net 5. Nobody wants a .Net 5.1 just for one ?.

[jeffhandley]

I appreciate the extra data, @olmobrutall. I double-checked and got confirmation that this doesn't meet our 5.0 bar at this stage.

I recognize the frustration this will cause for you and many others, but we won't be able to fix it until 6.0. We expect there will be other fixes to nullable annotations that we'll make in 6.0 too, which we'll be accumulating in a single document. We anticipated that we would make mistakes in our annotations, and we're grateful that you reported this one as quickly as you did so that others will be able to discover the existing issue and know that they can apply a ! here and still pass in null elements.