fix: Cecil serialization inheritance cutoff with complex base types

[Eideren]

PR Details

The binary serializer can lose serialized data when round-tripped with a specific inheritance setup;

[DataContract]
public class D : C<AnyType>{ public bool bill; }

public class C<T> : B<T>{}
public class B<T> : A{}

[DataContract(Inherited = true)]
public class A : EntityComponent{}

The following logic fails to capture B<T>

stride/sources/core/Stride.Core.AssemblyProcessor/Serializers/CecilSerializerContext.cs

Lines 198 to 204 in 37bbf0c

	SerializableTypeInfo parentSerializableTypeInfo;
	var parentType = ResolveGenericsVisitor.Process(type, type.Resolve().BaseType);
	if (!parentType.ContainsGenericParameter() && (parentSerializableTypeInfo = GenerateSerializer(parentType, false, profile)) != null &&
	parentSerializableTypeInfo.SerializerType != null)
	{
	serializableTypeInfo.ComplexSerializerProcessParentType = true;
	}

Which in turn means that all serialized data contained in its base types would be discarded, most obvious with the component Id inherited from EntityComponent, which right now shows up as a new Id on every save caused by the asset collision Id de-duplication using binary serialization through the Clone function:

stride/sources/assets/Stride.Core.Assets/Analysis/AssetCollision.cs

Lines 33 to 48 in 37bbf0c

	public static void Clean(Package package, ICollection<AssetItem> inputItems, ICollection<AssetItem> outputItems, AssetResolver assetResolver, bool cloneInput, bool removeUnloadableObjects)
	{
	if (inputItems == null) throw new ArgumentNullException(nameof(inputItems));
	if (outputItems == null) throw new ArgumentNullException(nameof(outputItems));
	if (assetResolver == null) throw new ArgumentNullException(nameof(assetResolver));
	// Check that all items are non-null
	if (inputItems.Any(item => item == null))
	{
	throw new ArgumentException("List cannot contain null items", nameof(inputItems));
	}
	var items = inputItems;
	if (cloneInput)
	{
	items = inputItems.Select(item => item.Clone(flags: removeUnloadableObjects ? AssetClonerFlags.RemoveUnloadableObjects : AssetClonerFlags.None)).ToList();

Related Issue

Kind of hard to tell

Types of changes

• Docs change / refactoring / dependency upgrade
• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)

Checklist

• My change requires a change to the documentation.
• I have added tests to cover my changes.
• All new and existing tests passed.
• I have built and run the editor to try this change out.

[manio143]

Nice solution. Just a question - the check on generic type inside the for loop where we continue - what scenario is that skipping?

[Kryptos-FR]

Nice solution. Just a question - the check on generic type inside the for loop where we continue - what scenario is that skipping?

Would be nice to add a few comments on those lines that explain what the algorithm is doing.

[Eideren]

Added a couple of comments to clarify.
@manio143 from what I can tell, this is a fallback for when ResolveGenericsVisitor.Process failed to resolve, which afaict happens when trying to serialize B as an open type with an inheritance like the following:

class A<T>{}
class B<T> : A<T>{}

Not 100% sure as I haven't tested it, here's the source for that method:

stride/sources/core/Stride.Core.AssemblyProcessor/ResolveGenericsVisitor.cs

Lines 27 to 44 in beb2972

	public static TypeReference Process(TypeReference context, TypeReference type)
	{
	if (type == null)
	return null;
	var parentContext = context;
	GenericInstanceType genericInstanceTypeContext = null;
	while (parentContext != null)
	{
	genericInstanceTypeContext = parentContext as GenericInstanceType;
	if (genericInstanceTypeContext != null)
	break;
	parentContext = parentContext.Resolve().BaseType;
	}
	if (genericInstanceTypeContext == null || genericInstanceTypeContext.ContainsGenericParameter())
	return type;

[xen2]

Would it be easy to add a test (in https://github.com/stride3d/stride/blob/master/sources/core/Stride.Core.Tests/TestSerialization.cs) so that we make sure to not break it in the future? (i.e. if we switch to Roslyn)
If not trivial, no problem.

[Eideren]

Looked into it - in that particular test context, cecil fails to generate a serializer for the class even with the exact same type definitions I used at edit and runtime to validate the changes in this PR. I don't have a lot of time to dig too deep into why the test fails while the real one passes, so I'll have to leave it as is for now

[xen2]

Thanks for giving it a try, all good then!