Add feature switch to disable DataSet XML serialization #107713
Conversation
| @@ -14,6 +14,7 @@ configurations but their defaults might vary as any SDK can set the defaults dif | |||
| | VerifyDependencyInjectionOpenGenericServiceTrimmability | Microsoft.Extensions.DependencyInjection.VerifyOpenGenericServiceTrimmability | When set to true, DependencyInjection will verify trimming annotations applied to open generic services are correct. | | |||
| | _AggressiveAttributeTrimming | System.AggressiveAttributeTrimming | When set to true, aggressively trims attributes to allow for the most size savings possible, even if it could result in runtime behavior changes | | |||
| | _ComObjectDescriptorSupport | System.ComponentModel.TypeDescriptor.IsComObjectDescriptorSupported | When set to true, supports creating a TypeDescriptor based view of COM objects. | | |||
| | _DataSetXmlSerializationSupport | System.Data.DataSet.XmlSerializationSupport | When set to false, DataSet implementation of IXmlSerializable will throw instead of using trim-incompatible XML serialization. | | |||
There was a problem hiding this comment.
Do we have some process to decide if something is a public or non-public switch? The "has been disabled in the app configuration" in the exception message sounds like something that we might want to leave configurable. If someone turns it on, my understanding is they'll get trimming warnings, so it should be fine.
There was a problem hiding this comment.
It's true that there will be trim warnings if someone turns it on. I made this one non-public because I don't think there's much value in toggling it in a non-trimmed app - it feels more like a behavior that should be automatically disabled when trimming, but is otherwise not super interesting.
There was a problem hiding this comment.
My question is more whether someone could have been using it with TrimMode=partial and wants to re-enable it. (My assumption is that this doesn't reflect on our IsTrimmable assemblies, but on user code.)
We do document BuiltInComInteropSupport and enabling that one is a lot more scarier. (I always wondered why we did that.)
There was a problem hiding this comment.
I would consider that an unsupported scenario where we provide an undocumented escape hatch - in that case I think it's fine for the escape hatch to be non-public. Maybe BuiltInComInteropSupport should have been non-public too.
Curious what you think - should it be public? Or should the exception message say something else?
There was a problem hiding this comment.
I'm questioning this because this might be the first feature switch PR that I'm tagged on adding a non-public switch. We have others that feel in the same category and are not underscored (JsonSerializerIsReflectionEnabledByDefault comes to mind). Enabling them with full trimming will likely break the app but the story might be different with partial trimming.
It comes down to whether we would consider this supported with partial trimming (to the extent that we say it's supported for JsonSerializerIsReflectionEnabledByDefault, EnableCppCLIHostActivation, EnableUnsafeBinaryFormatterInDesigntimeLicenseContextSerialization, etc.).
dotnet/docs#41739 (comment) touched on that a bit but I think we'll want to have some better guidelines around this.
I don't feel strongly however. I'm not a fan of warning-disabled partial trimming, but we have appmodels that still set it.
There was a problem hiding this comment.
Thanks for bringing it up - I think it warrants further discussion. I left some thoughts in dotnet/docs#41739 (comment).
I'll leave this one non-public here per my comments, but if we reach a different agreement in that discussion I'll go back and make it public.
Fixes dotnet#107704 by introducing a feature switch that makes the DataSet and DataTable implementation of IXmlSerializable throw. This prevents unactionable warnings from the DataSet/DataTable implementation details when both DataSet and IXmlSerializable happen to be used in an app. The downside of this approach is that the trim warnings are replaced by a runtime failure. The advantage is that it eliminates unactionable warnings. It is preferable over annotating IXmlSerializable methods because that interface is not inherently trim-incompatible (some implementations might be compatible).
SDK change corresponding to dotnet/runtime#107713.
Fixes dotnet#107704 by introducing a feature switch that makes the DataSet and DataTable implementation of IXmlSerializable throw. This prevents unactionable warnings from the DataSet/DataTable implementation details when both DataSet and IXmlSerializable happen to be used in an app. The downside of this approach is that the trim warnings are replaced by a runtime failure. The advantage is that it eliminates unactionable warnings. It is preferable over annotating IXmlSerializable methods because that interface is not inherently trim-incompatible (some implementations might be compatible).
Fixes #107704 by introducing a feature switch that makes the DataSet and DataTable implementation of IXmlSerializable throw. This prevents unactionable warnings from the DataSet/DataTable implementation details when both DataSet and IXmlSerializable happen to be used in an app.
The downside of this approach is that the trim warnings are replaced by a runtime failure. But I believe this is better than the currently available alternatives (annotating IXmlSerializable or the current unactionable warnings).
If folks are on board with this approach, I'll make a corresponding SDK change to add the RuntimeHostConfigurationOption.