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

ILLink annotations related to ISerializable and IBindingListView in System.Data.Common #55394

Merged
merged 1 commit into from
Jul 10, 2021

Conversation

krwq
Copy link
Member

@krwq krwq commented Jul 9, 2021

Per offline conversations ISerializable related warnings will be suppressed since adding RUC on them would require also adding RUC on the interface or on all ctors of the class which would cause many unnecessary warnings and binary serialization is about to be obsoleted anyway and is unsafe regardless of trimming.

@krwq krwq requested review from eerhardt and vitek-karas July 9, 2021 11:15
@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.

@ghost
Copy link

ghost commented Jul 9, 2021

Tagging subscribers to this area: @roji, @ajcvickers
See info in area-owners.md if you want to be subscribed.

Issue Details

Per offline conversations ISerializable related warnings will be suppressed since adding RUC on them would require also adding RUC on the interface or on all ctors of the class which would cause many unnecessary warnings.

Author: krwq
Assignees: -
Labels:

area-System.Data, new-api-needs-documentation

Milestone: -

@@ -268,6 +268,8 @@ protected DataSet(SerializationInfo info, StreamingContext context, bool Constru
DeserializeDataSet(info, context, remotingFormat, schemaSerializationMode);
}

[UnconditionalSuppressMessage("ReflectionAnalysis", "IL2026:RequiresUnreferencedCode",
Justification = "Binary serialization is unsafe in general and is planned to be obsoleted. We do not want to mark interface or ctors of this class as unsafe as that would show many unnecessary warnings elsewhere.")]
Copy link
Member

Choose a reason for hiding this comment

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

@vitek-karas @MichalStrehovsky @marek-safar - are you OK with these suppressions? According to https://github.com/dotnet/designs/blob/main/accepted/2020/better-obsoletion/binaryformatter-obsoletion.md#entirety-of-binaryformatter-type-obsolete-as-warning, these GetObjectData methods are scheduled for obsoletion in .NET 7, along with ISerializable. So going through the work of marking ISerializable.GetObjectData as [RUC] just doesn't seem worth it right now.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Spoke with Eric offline about this. Just to clarify: ISerializable is distinct from BinaryFormatter, though the two are commonly used together. Our primary focus is on removing BinaryFormatter from the ecosystem. That doc also talks about removing ISerializable from the ecosystem, but there's less immediate urgency around this, so the timelines might not match up 100%.

Regardless, I think suppressing this is the right move for now. 👍

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.

This LGTM

@krwq krwq merged commit 413f0de into dotnet:main Jul 10, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Aug 9, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants