search

dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
15.27k stars 4.73k forks source link

XmlSerializer.GenerateSerializer should not be in System.Private.Xml #1413

Open stephentoub opened 5 years ago

stephentoub commented 5 years ago

System.Private.Xml contains all of the code for generating an assembly as the core logic behind the sgen tool: https://github.com/dotnet/corefx/tree/master/src/Microsoft.XmlSerializer.Generator The vast majority of the functionality here isn't used by anything other than sgen, which calls XmlSerializer.GenerateSerializer via reflection, and is the only caller of that method. This functionality should be removed from XmlSerializer in System.Private.Xml and moved into sgen.

tamlin-mike commented 5 years ago

I maintain code that uses System.Xml.Serialization.XmlSerializer.GenerateSerializer (directly, not using reflection) for a case sgen is unable to handle.

Could completely ripping this function out break f.ex. the Microsoft.XmlSerializer.Generator nuget package?

StephenBonikowsky commented 4 years ago

@mconnew Please comment on this issue and resolve as appropriate.

eerhardt commented 4 years ago

I maintain code that uses System.Xml.Serialization.XmlSerializer.GenerateSerializer (directly, not using reflection)

@tamlin-mike - can you explain this more? Are you only doing that on the .NET Framework? This method is internal in .NET Core.

https://github.com/dotnet/runtime/blob/4f9ae42d861fcb4be2fcd5d3d55d5f227d30e723/src/libraries/System.Private.Xml/src/System/Xml/Serialization/XmlSerializer.cs#L596

tamlin-mike commented 4 years ago

This method is internal in .NET Core.

Yeah, that turned out to be the problem. It was public in Framework. Unfortunately the code started to depend on that functionality. Then the need arose to use some other assembly compiled to netstandard 2.0 IIRC, and much gnashing of teeth ensued. Ended up throwing the MS tool out and writing our own serialization generator (-frontend) to get the required cross-platform compatibility we needed (fw + netstd). Unfortunately that now depends on that older specific version of the mentioned nuget package -- that will likely be removed sooner or later, or become incompatible with later versions of some runtime -- so probability is high we'll either be forced to rewrite it all from scratch or simply burn that use of XML at the stake. To quote Alien "Nuke it from space. Only way to be sure".

eerhardt commented 4 years ago

When I spoke with @mconnew it seemed that this issue would take a lot of work, and is pretty low on the priority list.

With #35547, we have addressed the size concern with having to root XmlSerializer.GenerateSerializer. I'm going to remove the linkable-framework label, as this doesn't issue no longer needs to be tracked in the linkablity work.

KalleOlaviNiemitalo commented 1 year ago

https://github.com/dotnet/runtime/issues/56589 seems to be a duplicate of this issue.

Microsoft.XmlSerializer.Generator has "rollForward": "LatestMajor", which makes it use the XmlSerializer.GenerateSerializer method of the highest installed version of .NET Runtime (PR https://github.com/dotnet/runtime/pull/40216, causes https://github.com/dotnet/runtime/issues/90913). If the XmlSerializer.GenerateSerializer method were deleted from a future version of .NET Runtime, then installing that would indeed break the build of projects that use an older version of Microsoft.XmlSerializer.Generator, even if those projects also use an older version of .NET SDK.

I think the C# code generation should be moved into the Microsoft.XmlSerializer.Generator package. The XmlSerializer.GenerateSerializer method would then be called only by older versions of Microsoft.XmlSerializer.Generator, and its implementation should be reverted to a maximally compatible version, perhaps to how it was in .NET Core 3.1.0. Each project that wants Microsoft.XmlSerializer.Generator to generate code that uses newer features of C# or .NET Runtime, such as Span\, would have to upgrade Microsoft.XmlSerializer.Generator.