Move XElement ILLinkTrim.xml file to only be used during the library build

[eerhardt]

Today, we have an internal XElement constructor that isn't invoked directly:

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

Lines 134 to 137 in 1d70b31

	internal XElement()
	: this("default")
	{
	}

However, we need to tell the IL Linker that it needs to preserve this constructor:

runtime/src/libraries/System.Private.Xml.Linq/src/ILLinkTrim.xml

Lines 3 to 5 in 1d70b31

	<type fullname="System.Xml.Linq.XElement">
	<!-- needed by serialization -->
	<method name=".ctor" />

because it is used by the XmlSerializer when generating code. See #20445, #21454, and dotnet/corefx#19244 for some information about this.

However, XmlSerializer is already special casing creating an XElement when it generates IL:

runtime/src/libraries/System.Private.Xml/src/System/Xml/Serialization/XmlSerializationWriterILGen.cs

Lines 2394 to 2418 in 1d70b31

	// Special case XElement
	// codegen the same as 'internal XElement : this("default") { }'
	if (type.FullName == "System.Xml.Linq.XElement")
	{
	Type xName = type.Assembly.GetType("System.Xml.Linq.XName");
	if (xName != null)
	{
	MethodInfo XName_op_Implicit = xName.GetMethod(
	"op_Implicit",
	CodeGenerator.StaticBindingFlags,
	new Type[] { typeof(string) }
	);
	ConstructorInfo XElement_ctor = type.GetConstructor(
	CodeGenerator.InstanceBindingFlags,
	new Type[] { xName }
	);
	if (XName_op_Implicit != null && XElement_ctor != null)
	{
	ilg.Ldstr("default");
	ilg.Call(XName_op_Implicit);
	ilg.New(XElement_ctor);
	return;
	}
	}
	}

The only place this constructor is truly necessary is when XmlSerializer generates C# code.

runtime/src/libraries/System.Private.Xml/src/System/Xml/Serialization/XmlSerializationWriter.cs

Lines 1939 to 1965 in 1d70b31

	internal string GetStringForCreateInstance(string escapedTypeName, bool useReflection, bool ctorInaccessible, bool cast, string arg)
	{
	if (!useReflection && !ctorInaccessible)
	return "new " + escapedTypeName + "(" + arg + ")";
	return GetStringForCreateInstance(GetStringForTypeof(escapedTypeName, useReflection), cast && !useReflection ? escapedTypeName : null, ctorInaccessible, arg);
	}
	internal string GetStringForCreateInstance(string type, string cast, bool nonPublic, string arg)
	{
	StringBuilder createInstance = new StringBuilder();
	if (cast != null && cast.Length > 0)
	{
	createInstance.Append('(');
	createInstance.Append(cast);
	createInstance.Append(')');
	}
	createInstance.Append(typeof(Activator).FullName);
	createInstance.Append(".CreateInstance(");
	createInstance.Append(type);
	createInstance.Append(", ");
	string bindingFlags = typeof(BindingFlags).FullName;
	createInstance.Append(bindingFlags);
	createInstance.Append(".Instance | ");
	createInstance.Append(bindingFlags);
	createInstance.Append(".Public | ");
	createInstance.Append(bindingFlags);
	createInstance.Append(".CreateInstance");

Here the XmlSerializer relies on the fact that there is an internal, parameterless constructor for XElement.

Investigating an early Blazor on .NET 5 app, leaving XElement rooted is causing roughly 500KB out of 4MB of unnecessary IL code to be left in the application, since this one constructor will bring in a large amount of XML code.

We should change the C# generation to special case XElement (just like we did for the IL generation case), and have it generate new XElement("default") instead of calling Activator.CreateInstance. This will allow us to remove this "dead" constructor from XElement, and it will allow us to delete the ILLinkTrim.xml file. This way the linker will never have to worry about preserving the code (in the shared framework, or a linked app).

We should make the default constructor public, so the linker knows if it can trim it successfully or not. If no one is calling the constructor, it can be trimmed.

We should rename the ILLinkTrim.xml file to ILLinkTrim_LibraryBuild.xml so it gets used only while building the System.Private.Xml.Linq assembly. While linking/trimming a complete application, the constructor will be preserved if it is needed by the generated code since the generated code uses this pattern:

o.@Element = (global::System.Xml.Linq.XElement)ReadSerializable(( System.Xml.Serialization.IXmlSerializable)System.Activator.CreateInstance(typeof(global::System.Xml.Linq.XElement), System.Reflection.BindingFlags.Instance | System.Reflection.BindingFlags.Public | System.Reflection.BindingFlags.CreateInstance | System.Reflection.BindingFlags.NonPublic, null, new object[0], null), true

The linker will be able to recognize Activator.CreateInstance(typeof(global::System.Xml.Linq.XElement) and preserve the constructors on XElement.

cc @krwq @mconnew @StephenMolloy

Tagging subscribers to this area: @buyaa-n, @krwq
Notify danmosemsft if you want to be subscribed.

[stephentoub]

I like the plan, but it does mean that previously generated C# code might break. What's been our general stance on compatibility for XmlSerializer-generated code? Hopefully we'd be willing to say that if you want to use it with .NET 5 and you're (de)serializing XElements, you need to re-generate the serialization code, but I don't know enough about use cases to know whether that's feasible.

[eerhardt]

Nice catch on breaking a previously generated serialization assembly running on .NET 5. I added the breaking-change label to call it out.
Even though it is an internal constructor, and we're using private reflection to call it, it is us who are using private reflection against ourselves.

[danmoseley]

@marklio

[marklio]

Adding @PriyaPurkayastha

How do we feel about folks who code-generated XmlSerializers and compiled them into NuGet packages? It's hard to query the data for arbitrary Activator.CreateInstance calls for XElement. Are there other calls that would be made by such serializers that would be relatively unique to those scenarios? Then, we could know the extent of the break.

On the surface, the benefit here seems super low-value in comparison the the downside of the break.

[stephentoub]

(whatever we do here, we should fix the generator to stop generating such private reflection into persisted code/assemblies)

[StephenMolloy]

@mconnew ?

[eerhardt]

Another option here I discussed with @ericstj is that if we made XElement's parameterless constructor public this would all go away as well. We could mark it as EditorBrowsableState.Never and doc it as "This method is not intended to be called from consumer code.". That seems like a pretty reasonable way forward here that would resolve the linkability concern.

@krwq @ericstj - thoughts?

[krwq]

I don't mind making it public for serialization purposes but I'm against adding EditorBrowsableState, not a fan of hidden APIs (although I'll leave the decision about attribute for API reviewers)

[stephentoub]

Isn't exposing it but not hiding it a pit of failure? Seems like it wasn't exposed because you want to guide developers constructing instances to always pass an element name, and the public parameterless constructor will be the first thing someone sees, and very difficult to use correctly.