Skip to content

XmlSerializer support for IsDynamicCodeSupported=false #59386

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
eerhardt merged 5 commits into from
Sep 23, 2021
Merged

XmlSerializer support for IsDynamicCodeSupported=false #59386

Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
9a82ac3
XmlSerializer support for IsDynamicCodeSupported=false
eerhardt Sep 21, 2021
ccb3aa0
Fix a bug in XmlSerializer.CanDeserialize when in ReflectionOnly mode.
eerhardt Sep 21, 2021
cdd1d14
Port UAP code for CanDeserialize
eerhardt Sep 21, 2021
f1b0cac
PR feedback
eerhardt Sep 22, 2021
d6fb135
Add a linker test to ensure linker option '--enable-opt sealer' works…
eerhardt Sep 22, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 24 additions & 2 deletions src/libraries/System.Private.Xml/src/System/Xml/Serialization/XmlSerializer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -194,7 +194,10 @@ public XmlSerializer(XmlTypeMapping xmlTypeMapping)
if (xmlTypeMapping == null)
throw new ArgumentNullException(nameof(xmlTypeMapping));

_tempAssembly = GenerateTempAssembly(xmlTypeMapping);
if (Mode != SerializationMode.ReflectionOnly)
{
_tempAssembly = GenerateTempAssembly(xmlTypeMapping);
}
_mapping = xmlTypeMapping;
}

Expand All @@ -218,6 +221,12 @@ public XmlSerializer(Type type, string? defaultNamespace)
_primitiveType = type;
return;
}

if (Mode == SerializationMode.ReflectionOnly)
{
return;
}

_tempAssembly = s_cache[defaultNamespace, type];
if (_tempAssembly == null)
{
Expand Down Expand Up @@ -270,7 +279,10 @@ public XmlSerializer(Type type, XmlAttributeOverrides? overrides, Type[]? extraT
DefaultNamespace = defaultNamespace;
_rootType = type;
_mapping = GenerateXmlTypeMapping(type, overrides, extraTypes, root, defaultNamespace);
_tempAssembly = GenerateTempAssembly(_mapping, type, defaultNamespace, location);
if (Mode != SerializationMode.ReflectionOnly)
Copy link
Member

@StephenMolloy StephenMolloy Sep 22, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit, because we want to restructure this RefEmit/Reflection-Based dichotomy in 7.0... but SOAP mappings also trigger using reflection-based serialization. Perhaps consider using if (!ShouldUseReflectionBasedSerialization(_mapping)) in these three constructors?

Copy link
Member Author

@eerhardt eerhardt Sep 22, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we are late in 6.0, I wanted to make the least risk change that fixes the issue. The least risk (IMO) is to use what worked in UAP and in the NativeAOT branch.

This suggestion would affect SOAP in "normal" applications, not just when IsDynamicCodeSupported=false. I can log an issue for someone to investigate taking this approach in 7.0.

{
_tempAssembly = GenerateTempAssembly(_mapping, type, defaultNamespace, location);
}
}

[RequiresUnreferencedCode("calls ImportTypeMapping")]
Expand Down Expand Up @@ -534,6 +546,16 @@ public virtual bool CanDeserialize(XmlReader xmlReader)
{
return _tempAssembly.CanRead(_mapping, xmlReader);
}
else if (ShouldUseReflectionBasedSerialization(_mapping) || _isReflectionBasedSerializer)
Copy link
Member Author

@eerhardt eerhardt Sep 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@HongGit @StephenMolloy - one XmlSerializer\ReflectionOnly test was failing with the other changes because CanDeserialize was returning false when _primitiveType == null and _tempAssembly == null. So I wrote this logic to handle that case. Can you take a look?

I plan on porting this PR back to 6.0, so it would be good to get eyes on it.

Copy link
Member Author

@eerhardt eerhardt Sep 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MichalStrehovsky - any thoughts on why that XmlSerializer\ReflectionOnly test doesn't fail in runtimelab's NativeAOT branch?

Copy link
Member

@MichalStrehovsky MichalStrehovsky Sep 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't run those tests in the NativeAOT branch. The set of fixes we have in the NativeAOT branch comes from manually porting the UWP ifdefs from an older version of the source code.

Looks like I missed this path.

This is how it looked like:

https://github.com/dotnet/corefx/blob/e31a2b7f3bdf985f56798df530daa4c06fd80376/src/System.Private.Xml/src/System/Xml/Serialization/XmlSerializer.cs#L682-L700

Copy link
Member Author

@eerhardt eerhardt Sep 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @MichalStrehovsky. I took the UAP logic (using ReflectionMethodEnabled) and brought it into this PR.

Copy link
Member

@StephenMolloy StephenMolloy Sep 22, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Logically speaking, the conditions for Serialize()/Deserialize() to try the reflection-based route are exactly what is in this 'else' statement. And we check this before checking for _tempAssembly, so I would move this condition ahead in the pecking order, between _primitiveType and _tempAssembly.

I would also note that when matching this condition, we will try to do reflection-based serialization, period. No fallback. So I might consider just returning true, since returning ReflectionMethodEnabled does not account for SOAP mappings. My only concern would be that I don't know how robust the reflection-based serialization will be, and we might perhaps be saying "yes we can deserialize" when in fact we will fail when trying reflection-based serialization. (Of course, it looks like we were probably still saying 'true' for all reflection-based scenarios before, so ¯\_ (ツ)_/¯ ) @mconnew, any thoughts on this?

Copy link
Member Author

@eerhardt eerhardt Sep 22, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so I would move this condition ahead in the pecking order

I can switch the if conditions around so they match Deserialize. That makes sense to me. Originally I did it this way "just in case" it doesn't break someone when _tempAssembly is available. But it should logically match Deserialize.

I would also note that when matching this condition, we will try to do reflection-based serialization, period. No fallback. So I might consider just returning true, since returning ReflectionMethodEnabled

Honestly - the Mode property in this code is confusing/convoluted. Does anyone actually set it to CodeGenOnly or PreGenOnly? I don't see any code (other than tests) doing that.

Since Mode can never be anything other than ReflectionOnly or ReflectionAsBackup, I can just return true here. But if we did want to respect Mode for CodeGenOnly or PreGenOnly, I think it should logically use ReflectionMethodEnabled. You are right that if you had a SOAP mapping, and call Deserialize with Mode == CodeGenOnly | PreGenOnly, nothing is checking the Mode, so it should work.

Copy link
Member

@StephenMolloy StephenMolloy Sep 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well... um... yeah. Like I said, XmlSerializer has been cobbled together from many different iterations on previous platforms. So it's kind of a mess. Reflection-only is a .Net Core thing I believe - just kind of stapled on to the mess that was already XmlSerializer moving through various runtimes. As we've hinted at previously, we do hope to clean this all up in 7.0 as we try to move to reflection-based serialization as the primary mode of operation rather than a niche alternative.

{
XmlMapping mapping = GetMapping();
ElementAccessor element = mapping.Accessor;
string elementNamespace = element.Form == XmlSchemaForm.Qualified ? element.Namespace! : string.Empty;

return mapping.IsReadable &&
mapping.GenerateSerializer &&
xmlReader.IsStartElement(element.Name, elementNamespace);
}
else
{
return false;
Expand Down