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

runtime/src/libraries/System.Private.Xml.Linq/src/System/Xml/Linq/XElement.cs

Line 95 in 035b729

public XElement(XName name, params object[] content) : this(name, (object)content) { }

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

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

[danmoseley]

@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.