Skip to content
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

Jump to bottom

Address remaining warnings from DataContractSerialization #51200

Merged
10 commits merged into from
Apr 19, 2021
Merged

Address remaining warnings from DataContractSerialization #51200

10 commits merged into from
Apr 19, 2021

Conversation

joperezr
Copy link
Member

Contributes to #45623

This Resolves the remaining warnings on System.Private.DataContractSerialization.

cc: @eerhardt @vitek-karas

@joperezr joperezr added area-Serialization linkable-framework Issues associated with delivering a linker friendly framework labels Apr 13, 2021
@joperezr joperezr requested a review from eerhardt April 13, 2021 22:06
@ghost
Copy link

ghost commented Apr 13, 2021 •
edited by joperezr
Loading

Tagging subscribers to 'linkable-framework': @eerhardt, @vitek-karas, @LakshanF, @tannergooding, @sbomer
See info in area-owners.md if you want to be subscribed.

Issue Details

Contributes to #45623

This Resolves the remaining warnings on System.Private.DataContractSerialization.

cc: @eerhardt @vitek-karas

Author: joperezr
Assignees: -
Labels:

area-Serialization, linkable-framework
Milestone: -

In reply to: 819084419

@dotnet-issue-labeler
Copy link

dotnet-issue-labeler bot commented Apr 13, 2021 •
edited by joperezr
Loading

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

In reply to: 819084428

@joperezr joperezr requested a review from mconnew April 13, 2021 22:07
@joperezr
Copy link
Member Author

joperezr commented Apr 13, 2021 •
edited
Loading

FYI: @mconnew

In reply to: 819084588

eerhardt
eerhardt approved these changes Apr 16, 2021
View reviewed changes
Copy link
Member

@eerhardt eerhardt left a comment

Choose a reason for hiding this comment

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

Looks good. Just some minor-ish comments.

...ystem.Private.DataContractSerialization/src/System/Runtime/Serialization/ReflectionReader.cs Outdated Show resolved Hide resolved
...System.Private.DataContractSerialization/src/System/Runtime/Serialization/AccessorBuilder.cs Outdated Show resolved Hide resolved
...System.Private.DataContractSerialization/src/System/Runtime/Serialization/AccessorBuilder.cs Outdated Show resolved Hide resolved
...Private.DataContractSerialization/src/System/Runtime/Serialization/CollectionDataContract.cs Outdated Show resolved Hide resolved
mconnew
mconnew reviewed Apr 16, 2021
View reviewed changes
...em.Private.DataContractSerialization/src/System/Runtime/Serialization/XmlObjectSerializer.cs

public abstract class XmlObjectSerializer
{
[RequiresUnreferencedCode(DataContractJsonSerializer.SerializerTrimmerWarning)]
public abstract void WriteStartObject(XmlDictionaryWriter writer, object? graph);
Copy link
Member

Choose a reason for hiding this comment

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

This isn't necessarily true. This class is public abstract and may not require unreferenced code. WCF carries an implementation of this abstract type and customers might in their own code too. The WCF implementation does delegate work to XmlSerializer (which doesn't implement this abstract class) and doesn't reference anything from DCS or DCJS.

Copy link
Member

Choose a reason for hiding this comment

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

The way the API is shaped - object? graph - doesn't lead to being trim compatible. The trimmer has no way of knowing statically which Types are being passed into WriteStartObject. Therefore, it doesn't know what members on what Types should be preserved.

Even if you looked at the type of variables:

Customer c = GetSomeCustomer();
serializer.WriteStartObject(writer, c);

You don't know statically if c is concretely a Customer or a type derived from Customer.

@mconnew
Copy link
Member

mconnew commented Apr 16, 2021

@StephenMolloy as he owns DCS and DCJS

@ghost
Copy link

ghost commented Apr 19, 2021

Hello @joperezr!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@ghost ghost merged commit 989ba28 into dotnet:main Apr 19, 2021
@runfoapp runfoapp bot mentioned this pull request Apr 20, 2021
Closed
@eerhardt eerhardt mentioned this pull request Apr 29, 2021
Closed
@ghost ghost locked as resolved and limited conversation to collaborators May 19, 2021
@karelz karelz added this to the 6.0.0 milestone May 20, 2021
@joperezr joperezr deleted the SPDCS2 branch August 11, 2021 21:00
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@vitek-karas vitek-karas vitek-karas left review comments

@eerhardt eerhardt eerhardt approved these changes

@agocke agocke agocke left review comments

@mconnew mconnew mconnew approved these changes

Assignees
No one assigned
Labels
area-Serialization linkable-framework Issues associated with delivering a linker friendly framework new-api-needs-documentation
Projects
None yet
Milestone
6.0.0
Development

Successfully merging this pull request may close these issues.
6 participants
@joperezr @mconnew @agocke @eerhardt @vitek-karas @karelz