Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add option to ignore reference cycles on serialization #46101
Add option to ignore reference cycles on serialization #46101
Changes from all commits
152db42
f980ad0
0eaf149
2e55d73
1905462
aafaf2f
b562579
216caae
75ef177
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we have this
ReferenceEqualsWrapper
type?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Stack<T>.Contains
uses anobject.Equals(object)
comparison and there is no way to tell the method to use a different comparison (in our case, we need reference equality comparison).Therefore we use
Stack<ReferenceEqualsWrapper>
where ReferenceEqualsWrapper is a thin struct wrapper for the objects in the graph that overridesobject.Equals
to callobject.ReferenceEquals
using the underlying_object
in the wrapper.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Opposed to this, we can avoid forcing reference equality and rely on object.Equals, which is what Newtonsoft does (see JamesNK/Newtonsoft.Json#401 (comment)).
Also, Newtonsoft offers
JsonSerializerSettings.EqualityComparer
which can be used to allow users to specify their own equality comparison for this case.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer the current choice of forcing reference equality for this feature since it aligns with Preserve, which uses
ReferenceEqualityComparer
.If requested by users, we can consider adding an API to a future extensibility model which allows users to specify an equality comparer to use with IgnoreCycles.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it, thanks. This doesn't affect behavior when this feature is off (happy path), but are there any perf issues with struct copies when inserting/checking/extracting from the stack?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We only pass reference-types to the stack, in the case of boxed structs, there is no copy semantics applying given that we never unbox.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A variable named
success
already exists (but is declared below this code) in this context.runtime/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonConverterOfT.cs
Line 384 in 148bc08
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider making this one
success
(and the othersuccess2
) since this appears first; or using an unassigned local higher in the method which can be shared by both call sites.