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.

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

Resolve ILLink warnings on System.Private.DataContractSerialization Part 1; Json) #50619

Merged
3 commits merged into from
Apr 8, 2021

Conversation

joperezr
Copy link
Member

@joperezr joperezr commented Apr 1, 2021

This is part 1 of resolving the warnings for System.Private.DataContractSerialization. It is covering all of the warnings from the Json side, the next step will be to cover the Xml side.

Given the space is serialization, these changes mostly are about adding RequiresUnreferencedCode to all the stack that uses reflection to serialize/deserialize objects comming from the input.

cc: @eerhardt @krwq @vitek-karas

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

ghost commented Apr 1, 2021

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

This is part 1 of resolving the warnings for System.Private.DataContractSerialization. It is covering all of the warnings from the Json side, the next step will be to cover the Xml side.

Given the space is serialization, these changes mostly are about adding RequiresUnreferencedCode to all the stack that uses reflection to serialize/deserialize objects comming from the input.

cc: @eerhardt @krwq @vitek-karas

Author: joperezr
Assignees: -
Labels:

area-Serialization, linkable-framework

Milestone: -

@dotnet-issue-labeler
Copy link

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.

@joperezr
Copy link
Member Author

joperezr commented Apr 2, 2021

As said above, the initial approach was to just mark everything as RequiredUnreferencedCode given the fact that this is a serializer, but due to the nature of DataContractSerialization and the fact that you have to always create the serializer passing in the expected types in the object graph, I'm checking to see if we can actually annotate this accordingly in order to make this serializer safe. I'll try a few things and update the PR if necessary

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.

Just one suggestion to factor the Delegate GetMethod("Invoke") out into a helper method. Once that is addressed this looks good.

@ghost
Copy link

ghost commented Apr 7, 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 f22214b into dotnet:main Apr 8, 2021
@ghost ghost locked as resolved and limited conversation to collaborators May 8, 2021
@karelz karelz added this to the 6.0.0 milestone May 20, 2021
@joperezr joperezr deleted the SPDCS1 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.
Labels
area-Serialization linkable-framework Issues associated with delivering a linker friendly framework new-api-needs-documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants