Preserve types for serialization by sbomer · Pull Request #1932 · dotnet/linker

sbomer · 2021-03-31T18:43:38Z

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)

marek-safar · 2021-04-07T11:17:50Z

+			DataContractSerializer,
+		}
+
+		static bool IsPreservedSerializationAttribute (ICustomAttributeProvider provider, CustomAttribute attribute, out SerializerKind serializerKind)


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.

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.

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.

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.

vitek-karas · 2021-04-15T13:08:09Z

+- 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).


What happens if I put [DataMember] on a private field - I would assume we preserve it... but this sentence seems to say otherwise.

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.

Fixed in the latest iteration (see the commit comments and doc updates)

vitek-karas · 2021-04-15T13:34:43Z

 		List<ISubStep> on_events;

-		protected MarkSubStepsDispatcher (IEnumerable<ISubStep> subSteps)
+		public MarkSubStepsDispatcher (IEnumerable<ISubStep> subSteps)


Is this necessary for the rest of the change - I could not see how we need that.

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 <vitek.karas@microsoft.com>

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

vitek-karas

Nice!

* 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 <vitek.karas@microsoft.com> * Update docs/serialization.md Co-authored-by: Vitek Karas <vitek.karas@microsoft.com> * Update src/linker/Linker.Steps/DiscoverSerializationHandler.cs Co-authored-by: Vitek Karas <vitek.karas@microsoft.com> * Update src/linker/Linker.Steps/DiscoverSerializationHandler.cs Co-authored-by: Vitek Karas <vitek.karas@microsoft.com> * 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 <vitek.karas@microsoft.com> Commit migrated from dotnet/linker@b3725ed

sbomer changed the title ~~[WIP] Preserve types for serialization~~ Preserve types for serialization Apr 6, 2021

sbomer marked this pull request as ready for review April 6, 2021 22:30

sbomer requested a review from marek-safar as a code owner April 6, 2021 22:30

sbomer requested review from agocke and vitek-karas April 6, 2021 22:30

marek-safar reviewed Apr 7, 2021

View reviewed changes

sbomer added 11 commits April 7, 2021 08:38

Preserve types for XML serializer

131ebd2

Move TypePreserve.All

602beef

Add doc about serialization handling

ed908b7

Adjust comments

b7fa14d

Remove dataflow type discovery

53fff61

Add more tests, update doc

9df99e9

Add logic for DataContractSerializer

02bb7bb

Remove IsActiveFor check

e53eb79

Fix comments

02f80bf

Update docs

8e090d9

PR feedback

89a8cc5

- Remove redundant usings - Make MarkSubStepsDispatcher non-abstract, make ctor public - PreserveSerialization -> PreserveSerializationSubStep - SerializationHelper -> SerializationMarker

sbomer force-pushed the serialization branch from b5a72ee to 89a8cc5 Compare April 7, 2021 15:39

agocke reviewed Apr 12, 2021

View reviewed changes

Comment thread docs/serialization.md Outdated

Comment thread src/linker/Linker/Driver.cs Outdated

sbomer added 7 commits April 13, 2021 11:36

Only scan types that are already marked

a4d269e

Fix test

51ac2cd

Activate serialization logic only for ctor calls

9e2c0c0

With this change the serialization heuristics will mark types only if there was a call to a relevant serialization constructor.

PR feedback

69beabb

Update docs

PR feedback

224c822

Change flag to --disable-serialization-discovery, and add a test to check that disabling works as expected.

Add missing changes

a01788e

vitek-karas reviewed Apr 15, 2021

View reviewed changes

sbomer and others added 3 commits April 15, 2021 08:37

Update docs/serialization.md

256c0a4

Co-authored-by: Vitek Karas <vitek.karas@microsoft.com>

Update docs/serialization.md

018376e

Co-authored-by: Vitek Karas <vitek.karas@microsoft.com>

Update src/linker/Linker.Steps/DiscoverSerializationHandler.cs

f524817

Co-authored-by: Vitek Karas <vitek.karas@microsoft.com>

sbomer and others added 2 commits April 15, 2021 08:38

Update src/linker/Linker.Steps/DiscoverSerializationHandler.cs

b59e3cb

Co-authored-by: Vitek Karas <vitek.karas@microsoft.com>

vitek-karas approved these changes Apr 15, 2021

View reviewed changes

Handle DataContractJsonSerializer

3a4fb83

vitek-karas reviewed Apr 16, 2021

View reviewed changes

Comment thread src/linker/Linker.Steps/DiscoverSerializationHandler.cs

Mention DataContractJsonSerializer in docs

7ec7c41

sbomer merged commit b3725ed into dotnet:main Apr 16, 2021

MichalStrehovsky mentioned this pull request Jul 27, 2022

Port serialization discovery logic from illink dotnet/runtime#72910

Closed

sbomer mentioned this pull request Jul 27, 2022

Consider turning off serialization discovery by default #2924

Closed

Uh oh!

Conversation

sbomer commented Mar 31, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sbomer Apr 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vitek-karas left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

sbomer commented Mar 31, 2021 •

edited

Loading

sbomer Apr 13, 2021 •

edited

Loading