Skip to content

Conversation

madelson
Copy link
Contributor

Came across this when reading through the code during a debug session. Most of these checks (including the similar ones in ListDictionaryInternal) were removed some time ago, but this one wasn't behind a #if, so perhaps there's a good reason to keep it around.

Figured I'd file a PR just in case there was interest in cleaning this up.

From a quick search it appears this may be the last usage of SR.Argument_NotSerializable, so possibly that could be removed too if this is accepted.

@ghost ghost added community-contribution Indicates that the PR has been added by a community member area-System.Collections labels Jan 20, 2023
@ghost
Copy link

ghost commented Jan 20, 2023

Tagging subscribers to this area: @dotnet/area-system-collections
See info in area-owners.md if you want to be subscribed.

Issue Details

Came across this when reading through the code during a debug session. Most of these checks (including the similar ones in ListDictionaryInternal) were removed some time ago, but this one wasn't behind a #if, so perhaps there's a good reason to keep it around.

Figured I'd file a PR just in case there was interest in cleaning this up.

From a quick search it appears this may be the last usage of SR.Argument_NotSerializable, so possibly that could be removed too if this is accepted.

Author: madelson
Assignees: -
Labels:

area-System.Collections, community-contribution

Milestone: -

@StephenMolloy
Copy link
Member

This is really more @GrabYourPitchforks's call. I don't have a problem with removing these checks. It makes this consistent with ListDictionaryInternal which should be the goal since they are used as complements.

However, there is one of two issues with this proposed change.

  1. The same checks should be removed from this[]{ set; } - again, to be consistent with ListDictionaryInternal.

or

  1. These checks should stay in EmptyReadOnlyDictionaryInternal and be added back to ListDictionaryInternal, because Exception and it's data container are serializable and these exceptions should probably be thrown when adding unserializable data rather than at serialization time. (They used to be part of ListDictionaryInternal hidden behind an #if some time ago.)

But whichever resolution is decided, I think the important thing is to keep consistent behavior between the two collection types - which are only used for Exception data as far as I can tell.

@ghost
Copy link

ghost commented Mar 23, 2023

Tagging subscribers to this area: @dotnet/area-system-runtime
See info in area-owners.md if you want to be subscribed.

Issue Details

Came across this when reading through the code during a debug session. Most of these checks (including the similar ones in ListDictionaryInternal) were removed some time ago, but this one wasn't behind a #if, so perhaps there's a good reason to keep it around.

Figured I'd file a PR just in case there was interest in cleaning this up.

From a quick search it appears this may be the last usage of SR.Argument_NotSerializable, so possibly that could be removed too if this is accepted.

Author: madelson
Assignees: -
Labels:

area-Serialization, area-System.Runtime, community-contribution

Milestone: -

@jkotas
Copy link
Member

jkotas commented Apr 24, 2023

added back to ListDictionaryInternal, because Exception

This would be a breaking change. I do not think we can justify making this breaking change now that these checks were removed years ago.

Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

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

Thanks

@jkotas jkotas merged commit 6abedb8 into dotnet:main Apr 25, 2023
@ghost ghost locked as resolved and limited conversation to collaborators May 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Runtime community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants