Preserve types for serialization #1932
Conversation
sbomer
changed the title
| DataContractSerializer, | ||
| } | ||
|
|
||
| static bool IsPreservedSerializationAttribute (ICustomAttributeProvider provider, CustomAttribute attribute, out SerializerKind serializerKind) |
There was a problem hiding this comment.
I don't think we should use attributes to detect which serializer is needed. We want to trim all these and other serialization-related attributes whenever it's possible.
There was a problem hiding this comment.
The reason I'm checking for attributes is because we're concerned about back-compat with xamarin-android, and they used to do this. If we're ok with relaxing this, one idea is to make it conditional on marking a serializer ctor.
There was a problem hiding this comment.
If we're ok with relaxing this, one idea is to make it conditional on marking a serializer ctor.
The context back then was different so whatever logic that makes more sense in .NET6 world is preferable.
There was a problem hiding this comment.
Changed so that we only mark things if we marked a serializer ctor (but the attributes are still used to discover types to consider as "serialization roots"). The heuristics also have been relaxed to mark fewer things.
- Remove redundant usings - Make MarkSubStepsDispatcher non-abstract, make ctor public - PreserveSerialization -> PreserveSerializationSubStep - SerializationHelper -> SerializationMarker
Instead of trying to conservatively match the xamarin-android behavior, this now does something more "correct" without building in too much serializer-specific logic. Specifically: - Don't preserve methods - Don't preserve static members - Don't preserve private members
With this change the serialization heuristics will mark types only if there was a call to a relevant serialization constructor.
Update docs
Change flag to --disable-serialization-discovery, and add a test to check that disabling works as expected.
| - Public instance fields | ||
| - Public parameterless instance constructors | ||
|
|
||
| Note that in general, private members and static members are not preserved, nor are methods or events (other than the mentioned constructor). |
There was a problem hiding this comment.
What happens if I put [DataMember] on a private field - I would assume we preserve it... but this sentence seems to say otherwise.
There was a problem hiding this comment.
Currently it causes the type to be considered a root, but the field won't actually be kept. I think I can fix this without building serializer-specific logic into the recursive phase by marking such members up-front.
There was a problem hiding this comment.
Fixed in the latest iteration (see the commit comments and doc updates)
| List<ISubStep> on_events; | ||
|
|
||
| protected MarkSubStepsDispatcher (IEnumerable<ISubStep> subSteps) | ||
| public MarkSubStepsDispatcher (IEnumerable<ISubStep> subSteps) |
There was a problem hiding this comment.
Is this necessary for the rest of the change - I could not see how we need that.
There was a problem hiding this comment.
It was based on PR feedback in a previous iteration that used MarkSubStepsDispatcher for the serializer logic. I think it's an improvement, but I can separate it if you prefer - LMK.
Co-authored-by: Vitek Karas <[email protected]>
Co-authored-by: Vitek Karas <[email protected]>
Co-authored-by: Vitek Karas <[email protected]>
Co-authored-by: Vitek Karas <[email protected]>
To ensure private attributed members get marked, this changes the "root" tracking to track members rather than types. Marking a root member for serialization will now ensure that the member and its type (for fields/properties) also get marked recursively. Similar behavior applies for static members, methods, and events. Also address other feedback: - Update docs to describe .ctor-based activation - Clarify kept vs considered root - RecursiveType -> SerializedRecursiveType - Add comment about array type handling
* Preserve types for XML serializer * Move TypePreserve.All * Add doc about serialization handling * Adjust comments * Remove dataflow type discovery * Add more tests, update doc * Add logic for DataContractSerializer * Remove IsActiveFor check * Fix comments * Update docs * PR feedback - Remove redundant usings - Make MarkSubStepsDispatcher non-abstract, make ctor public - PreserveSerialization -> PreserveSerializationSubStep - SerializationHelper -> SerializationMarker * Update heuristics to be less conservative Instead of trying to conservatively match the xamarin-android behavior, this now does something more "correct" without building in too much serializer-specific logic. Specifically: - Don't preserve methods - Don't preserve static members - Don't preserve private members * Only scan types that are already marked * Fix test * Activate serialization logic only for ctor calls With this change the serialization heuristics will mark types only if there was a call to a relevant serialization constructor. * PR feedback Update docs * PR feedback Change flag to --disable-serialization-discovery, and add a test to check that disabling works as expected. * Add missing changes * Update docs/serialization.md Co-authored-by: Vitek Karas <[email protected]> * Update docs/serialization.md Co-authored-by: Vitek Karas <[email protected]> * Update src/linker/Linker.Steps/DiscoverSerializationHandler.cs Co-authored-by: Vitek Karas <[email protected]> * Update src/linker/Linker.Steps/DiscoverSerializationHandler.cs Co-authored-by: Vitek Karas <[email protected]> * PR feedback To ensure private attributed members get marked, this changes the "root" tracking to track members rather than types. Marking a root member for serialization will now ensure that the member and its type (for fields/properties) also get marked recursively. Similar behavior applies for static members, methods, and events. Also address other feedback: - Update docs to describe .ctor-based activation - Clarify kept vs considered root - RecursiveType -> SerializedRecursiveType - Add comment about array type handling * Handle DataContractJsonSerializer * Mention DataContractJsonSerializer in docs Co-authored-by: Vitek Karas <[email protected]> Commit migrated from dotnet/linker@b3725ed
4 participants
This adds a flag to keep types for serialization under certain conditions. The details are described in the included docs. See also the comments in SerializationHelper.cs for rationale and a comparison with xamarin-android.
@vitek-karas in particular we discussed only preserving the discovered types instead of marking them, but xamarin-android used to mark types in non-SDK "link" assemblies (in ApplyPreserveAttribute, which uses this logic from ApplyPreserveAttributeBase for example). So to match that I am marking all discovered types for now, which should enable deserialization scenarios - PTAL and let me know if this seems reasonable to you.
@LakshanF PTAL (I can't add you to the reviewers)