Add trimmer annotations to System.Data.Common #52046

krwq · 2021-04-29T10:46:46Z

No description provided.

dotnet-issue-labeler · 2021-04-29T10:46:51Z

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: 829128156

ghost · 2021-04-29T10:46:54Z

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

Issue Details

Author:	krwq
Assignees:	-
Labels:	`area-System.Data`, `new-api-needs-documentation`
Milestone:	-

Wraith2 · 2021-04-29T22:37:46Z

What is the process for finding what needs annotation for trimming?

eerhardt · 2021-04-29T22:56:03Z

What is the process for finding what needs annotation for trimming?

For assemblies in the netcoreapp shared framework, we have a build step that runs the trimming tool in "analysis mode", and spits out all the warnings. We've baselined the warnings in ILLink.Supressions.xml files in the repo, and are actively working on removing the suppressions and addressing the warnings (e.g. this PR).

For libraries outside of the shared framework, we are still working through the user experience. See https://github.com/dotnet/docs/blob/main/docs/core/deploying/prepare-libraries-for-trimming.md for more info.

For whole applications, you publish your app with -p:PublishTrimmed=true, and it spits out the warnings for all code used in your app.

eerhardt · 2021-04-29T22:57:02Z

    public string ColumnName { get { throw null; } [System.Diagnostics.CodeAnalysis.RequiresUnreferencedCodeAttribute("Members from serialized types or types used in expressions may be trimmed if not referenced directly.")] set { } }

Setting the ColumnName is unsafe?

In reply to: 829648326

Refers to: src/libraries/System.Data.Common/ref/System.Data.Common.cs:125 in 4872c21. [](commit_id = 4872c21, deletion_comment = False)

eerhardt · 2021-04-29T22:58:08Z

    [System.Diagnostics.CodeAnalysis.RequiresUnreferencedCodeAttribute("Members from serialized types or types used in expressions may be trimmed if not referenced directly.")]

Is there any way we can annotate the dataType to make this safe?

In reply to: 829648848

Refers to: src/libraries/System.Data.Common/ref/System.Data.Common.cs:103 in 4872c21. [](commit_id = 4872c21, deletion_comment = False)

eerhardt · 2021-04-29T22:59:12Z

    [System.Diagnostics.CodeAnalysis.RequiresUnreferencedCodeAttribute("Members from serialized types or types used in expressions may be trimmed if not referenced directly.")]

How come Adding and Removing from a collection is unsafe? This message doesn't really tell me what is the issue.

Refers to: src/libraries/System.Data.Common/ref/System.Data.Common.cs:177 in 4872c21. [](commit_id = 4872c21, deletion_comment = False)

src/libraries/System.Data.Common/src/ILLink/ILLink.Suppressions.xml

src/libraries/System.Data.Common/src/System/Data/DataSet.cs

src/libraries/System.Data.Common/src/System/Data/DataView.cs

ghost · 2021-04-29T23:10:43Z

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

Issue Details

Author:	krwq
Assignees:	-
Labels:	`area-System.Data`, `linkable-framework`
Milestone:	-

eerhardt

@krwq - overall, I think this is a good start. However, I think we may be marking too many constructors as RequiresUnreferencedCode - ex. DataColumn and DataSet. I wonder if there is something that can be done so we don't have to mark the constructors as unsafe, but instead marking the methods that are unsafe.

krwq · 2021-04-30T08:53:08Z

    public string ColumnName { get { throw null; } [System.Diagnostics.CodeAnalysis.RequiresUnreferencedCodeAttribute("Members from serialized types or types used in expressions may be trimmed if not referenced directly.")] set { } }
Setting the ColumnName is unsafe?

Refers to: src/libraries/System.Data.Common/ref/System.Data.Common.cs:125 in 4872c21. [](commit_id = 4872c21, deletion_comment = False)

I think I marked one of the events it executed as RUC at first and then reverted it but this was left as RUC. Reverting

    [System.Diagnostics.CodeAnalysis.RequiresUnreferencedCodeAttribute("Members from serialized types or types used in expressions may be trimmed if not referenced directly.")]
How come Adding and Removing from a collection is unsafe? This message doesn't really tell me what is the issue.

Refers to: src/libraries/System.Data.Common/ref/System.Data.Common.cs:177 in 4872c21. [](commit_id = 4872c21, deletion_comment = False)

Adding - I've tried to remove RUC there couple of times but seems like it needs to read the default value for the record which is unsafe as in some cases it might be reading some data through reflection.
Removing - this one is weird: it needs to clear errors on columns which triggers the RecordChanged event on the table which trggers selection RecordChanged which triggers IndexListChangedInternal on the DataView and that triggers couple of things which are marked as RUC - possibly none of that ever gets executed in that code path but I wasn't able to easily tell so I left it as RUC

@krwq - overall, I think this is a good start. However, I think we may be marking too many constructors as RequiresUnreferencedCode - ex. DataColumn and DataSet. I wonder if there is something that can be done so we don't have to mark the constructors as unsafe, but instead marking the methods that are unsafe.

For DataColumn I believe only some constructors are unsafe and DataSet is mainly suffering because of inheritance... perhaps one solution could be to mark GetObjectData as RUC and suppress the warning about not matching inherited -- but then that might actually allow to bypass the message in case where you would get error but that still might be better than making more common case suffer

krwq · 2021-04-30T13:15:50Z

    [System.Diagnostics.CodeAnalysis.RequiresUnreferencedCodeAttribute("Members from serialized types or types used in expressions may be trimmed if not referenced directly.")]
Is there any way we can annotate the dataType to make this safe?

Refers to: src/libraries/System.Data.Common/ref/System.Data.Common.cs:103 in 4872c21. [](commit_id = 4872c21, deletion_comment = False)

I think DynamicallyAccessedMemberTypes.PublicProperties should fix this specific constructor but not the other as the other one is unsafe because of expression

krwq · 2021-05-07T12:58:53Z

@eerhardt I've removed RUCs from two DataSet ctors but after I did investigation and some backtracing on them it didn't really help much with removing any other public RUCs, I only was able to remove it from couple of internal/private members.

eerhardt · 2021-05-07T14:57:25Z

    [System.Diagnostics.CodeAnalysis.RequiresUnreferencedCodeAttribute("Members might be trimmed for some column data types.")]

What about adding an existing DataColumn to a collection is unsafe?

Refers to: src/libraries/System.Data.Common/ref/System.Data.Common.cs:177 in 33b3e45. [](commit_id = 33b3e45, deletion_comment = False)

eerhardt · 2021-05-07T14:57:41Z

    [System.Diagnostics.CodeAnalysis.RequiresUnreferencedCodeAttribute("Members might be trimmed for some column data types.")]

Is this necessary to be marked as RUC?

In reply to: 834485445

Refers to: src/libraries/System.Data.Common/ref/System.Data.Common.cs:180 in 33b3e45. [](commit_id = 33b3e45, deletion_comment = False)

eerhardt · 2021-05-07T14:58:49Z

    [System.Diagnostics.CodeAnalysis.RequiresUnreferencedCodeAttribute("Members might be trimmed for some column data types.")]

What about removing columns from the collection is unsafe?

Refers to: src/libraries/System.Data.Common/ref/System.Data.Common.cs:192 in 33b3e45. [](commit_id = 33b3e45, deletion_comment = False)

krwq · 2021-05-25T13:54:20Z

@eerhardt there is a merge conflict between this PR and #52629 - all string changes @safern has made were already in this PR but there is a diff between RequiresUnreferencedCode (@safern's) vs RequiresUnreferencedCodeAttribute (mine). I'm not sure if you're reviewing incremental or full review so will not resolve the conflict right now as the easiest path for me is to squash commits and take my changes on the ref file and that would destroy possibility of incremental review. Please let me know if you're ok with squashing or prefer to review first

src/libraries/System.Data.Common/src/System/Data/DataColumn.cs

src/libraries/System.Data.Common/src/System/Data/Common/DbProviderFactories.cs

eerhardt · 2021-05-26T23:29:24Z

    [System.Diagnostics.CodeAnalysis.RequiresUnreferencedCodeAttribute("Members from serialized types or types used in expressions may be trimmed if not referenced directly")]

These should have different messages on them.

In reply to: 849186450

Refers to: src/libraries/System.Data.Common/ref/System.Data.Common.cs:2543 in e63d347. [](commit_id = e63d347, deletion_comment = False)

eerhardt · 2021-05-26T23:30:49Z

    public static explicit operator byte[]? (System.Data.SqlTypes.SqlBinary x) { throw null; }

It might be good to do all this formatting separately.... It's causing a lot of noise in this PR

Refers to: src/libraries/System.Data.Common/ref/System.Data.Common.cs:2613 in e63d347. [](commit_id = e63d347, deletion_comment = True)

eerhardt · 2021-05-26T23:31:06Z

    [System.Diagnostics.CodeAnalysis.RequiresUnreferencedCodeAttribute("Members from serialized types or types used in expressions may be trimmed if not referenced directly")]

Should have a different message

In reply to: 849187167

Refers to: src/libraries/System.Data.Common/ref/System.Data.Common.cs:3681 in e63d347. [](commit_id = e63d347, deletion_comment = False)

src/libraries/System.Data.Common/src/System/Data/LinqDataView.cs

joperezr · 2021-05-27T22:57:58Z

    [System.Diagnostics.CodeAnalysis.RequiresUnreferencedCodeAttribute("Members from serialized types or types used in expressions may be trimmed if not referenced directly")]

How come setting a field is unsafe, but the Field methods above getting it back aren't? I would have expected that T above to be annotated in order for the values to be preserved.

Refers to: src/libraries/System.Data.Common/ref/System.Data.Common.cs:421 in e63d347. [](commit_id = e63d347, deletion_comment = False)

joperezr · 2021-05-27T22:58:14Z

    [System.Diagnostics.CodeAnalysis.RequiresUnreferencedCodeAttribute("Members from serialized types or types used in expressions may be trimmed if not referenced directly")]

Can't you annotate the T to have it be safe?

Refers to: src/libraries/System.Data.Common/ref/System.Data.Common.cs:423 in e63d347. [](commit_id = e63d347, deletion_comment = False)

joperezr · 2021-05-27T23:01:35Z

    [System.Diagnostics.CodeAnalysis.RequiresUnreferencedCodeAttribute("Members from serialized types or types used in expressions may be trimmed if not referenced directly")]

Having the same message in all of the RUC attributes could be confusing. It is great to try to reuse when possible and when it makes sense, but these messeges will be public-facing so we should try to keep them be understandable from the API called perspective

Refers to: src/libraries/System.Data.Common/ref/System.Data.Common.cs:592 in e63d347. [](commit_id = e63d347, deletion_comment = False)

joperezr · 2021-05-27T23:03:42Z

    [System.Diagnostics.CodeAnalysis.RequiresUnreferencedCode("Matching overriden method. This cannot be called directly due to explicit implementation")]

not sure how user friendly this message is

Refers to: src/libraries/System.Data.Common/ref/System.Data.Common.cs:1120 in e63d347. [](commit_id = e63d347, deletion_comment = False)

joperezr · 2021-05-28T19:19:58Z

src/libraries/System.Data.Common/src/System/Data/DataSet.cs

@@ -273,6 +278,7 @@ public virtual void GetObjectData(SerializationInfo info, StreamingContext conte
 protected virtual void InitializeDerivedDataSet() { }

 // Serialize all the tables.
+ [RequiresUnreferencedCode(RequiresUnreferencedCodeMessage)]


Correct me if I'm wrong but this is only unsafe if the serializationformat is not xml right since in that case it would use BF. If so, would it make sense to add it to the message so it can be suppressed upstack when we know that the format is xml?

No, XML can contain filter expressions which are unsafe

joperezr · 2021-05-28T19:21:19Z

src/libraries/System.Data.Common/src/System/Data/DataSet.cs

@@ -367,6 +374,7 @@ internal void DeserializeDataSet(SerializationInfo info, StreamingContext contex
 }

 // Deserialize schema.
+ [RequiresUnreferencedCode(RequiresUnreferencedCodeMessage)]


Same comment here, seems like the only unsafe path is BinaryFormatter.Deserialize right?

as above (XML itself can contain filter expressions which are unsafe)

src/libraries/System.Data.Common/src/System/Data/Common/DataStorage.cs

joperezr · 2021-05-28T19:27:17Z

src/libraries/System.Data.Common/src/System/Data/Common/DbProviderFactories.cs

@@ -188,7 +193,7 @@ private static DbProviderFactory GetFactoryInstance(Type providerFactoryClass)
 return (DbProviderFactory)factory;
 }

-
+ [RequiresUnreferencedCode(DataSet.RequiresUnreferencedCodeMessage)]


Same here, we should probably adjust message to just say here that the reason why this is unsafe is because we are calling Type.GetType(string) #Closed

src/libraries/System.Data.Common/src/System/Data/XMLSchema.cs

src/libraries/System.ComponentModel.TypeConverter/src/System/ComponentModel/ITypedList.cs

src/libraries/System.Data.Common/src/System/Data/Common/DbProviderFactories.cs

src/libraries/System.Data.Common/src/System/Data/Common/DBSchemaRow.cs

eerhardt

This is getting really close @krwq. Thanks for the good work here.

Just a few more comments. Once those are addressed I think this will be ready to merge.

src/libraries/System.Data.Common/src/System/Data/Common/DbProviderFactories.cs

src/libraries/System.Data.Common/src/System/Data/DataColumn.cs

src/libraries/System.Data.Common/src/System/Data/DataSet.cs

src/libraries/System.Data.Common/src/System/Data/DataTable.cs

src/libraries/System.Data.Common/src/System/Data/Select.cs

src/libraries/System.Data.Common/src/System/Data/Filter/BinaryNode.cs

eerhardt

This looks good. Let's get this merged. Thanks for the work.

Add trimmer annotations to System.Data.Common

4872c21

krwq requested a review from eerhardt April 29, 2021 10:46

dotnet-issue-labeler bot added area-System.Data new-api-needs-documentation labels Apr 29, 2021

eerhardt reviewed Apr 29, 2021

View reviewed changes

src/libraries/System.Data.Common/src/ILLink/ILLink.Suppressions.xml Outdated Show resolved Hide resolved

eerhardt reviewed Apr 29, 2021

View reviewed changes

src/libraries/System.Data.Common/src/System/Data/DataSet.cs Outdated Show resolved Hide resolved

eerhardt reviewed Apr 29, 2021

View reviewed changes

src/libraries/System.Data.Common/src/System/Data/DataView.cs Outdated Show resolved Hide resolved

eerhardt added linkable-framework Issues associated with delivering a linker friendly framework and removed new-api-needs-documentation labels Apr 29, 2021

eerhardt reviewed Apr 29, 2021

View reviewed changes

runfoapp bot mentioned this pull request Apr 30, 2021

LocalEndPointTestIPv6Sync.TcpAcceptSocket_WhenServerBoundToSpecificAddress_LocalEPIsSame failing #52141

Closed

krwq added 2 commits May 5, 2021 13:42

Removes some RUCs in DataColumn and DataColumnCollection

928b9d6

Remove RUC from DataSet ctors

33b3e45

krwq added 2 commits May 25, 2021 12:50

Remove many RUCs per Eric's comments

610fa5b

Fix RUCs in DataTableCollection, DataViewManager, DataViewSetting

5e0810b

joperezr reviewed May 26, 2021

View reviewed changes

src/libraries/System.Data.Common/src/System/Data/DataColumn.cs Outdated Show resolved Hide resolved

eerhardt reviewed May 26, 2021

View reviewed changes

src/libraries/System.Data.Common/src/System/Data/Common/DbProviderFactories.cs Outdated Show resolved Hide resolved

eerhardt reviewed May 26, 2021

View reviewed changes

src/libraries/System.Data.Common/src/System/Data/LinqDataView.cs Outdated Show resolved Hide resolved

joperezr reviewed May 28, 2021

View reviewed changes

src/libraries/System.Data.Common/src/System/Data/Common/DataStorage.cs Outdated Show resolved Hide resolved

joperezr reviewed May 28, 2021

View reviewed changes

src/libraries/System.Data.Common/src/System/Data/XMLSchema.cs Outdated Show resolved Hide resolved

eerhardt mentioned this pull request Jun 1, 2021

Fully annotate JsonNode for trimmability #53184

Merged

address feedback

3bd477a

eerhardt reviewed Jun 2, 2021

View reviewed changes

src/libraries/System.ComponentModel.TypeConverter/src/System/ComponentModel/ITypedList.cs Outdated Show resolved Hide resolved

eerhardt reviewed Jun 2, 2021

View reviewed changes

src/libraries/System.Data.Common/src/System/Data/Common/DbProviderFactories.cs Outdated Show resolved Hide resolved

eerhardt reviewed Jun 2, 2021

View reviewed changes

src/libraries/System.Data.Common/src/System/Data/Common/DBSchemaRow.cs Outdated Show resolved Hide resolved

Address some missed comments

6c1d670

eerhardt reviewed Jun 8, 2021

View reviewed changes

src/libraries/System.Data.Common/src/System/Data/Filter/BinaryNode.cs Outdated Show resolved Hide resolved

eerhardt mentioned this pull request Jun 8, 2021

Opaque warnings for unsupported features without managed callers dotnet/linker#2004

Open

krwq and others added 3 commits June 9, 2021 10:06

apply feedback

51c5b4f

apply remaining comments

8a62d95

Fix justification

37871e8

eerhardt approved these changes Jun 10, 2021

View reviewed changes

krwq merged commit c2f559c into dotnet:main Jun 11, 2021

ghost locked as resolved and limited conversation to collaborators Jul 11, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add trimmer annotations to System.Data.Common #52046

Add trimmer annotations to System.Data.Common #52046

krwq commented Apr 29, 2021

dotnet-issue-labeler bot commented Apr 29, 2021 •

edited by krwq

Loading

ghost commented Apr 29, 2021

Wraith2 commented Apr 29, 2021

eerhardt commented Apr 29, 2021

eerhardt commented Apr 29, 2021 •

edited by krwq

Loading

eerhardt commented Apr 29, 2021 •

edited by krwq

Loading

eerhardt commented Apr 29, 2021

ghost commented Apr 29, 2021

eerhardt left a comment

krwq commented Apr 30, 2021

krwq commented Apr 30, 2021

krwq commented May 7, 2021

eerhardt commented May 7, 2021

eerhardt commented May 7, 2021 •

edited

Loading

eerhardt commented May 7, 2021

krwq commented May 25, 2021

eerhardt commented May 26, 2021 •

edited by krwq

Loading

eerhardt commented May 26, 2021

eerhardt commented May 26, 2021 •

edited

Loading

joperezr commented May 27, 2021

joperezr commented May 27, 2021

joperezr commented May 27, 2021

joperezr commented May 27, 2021

joperezr May 28, 2021

krwq Jun 2, 2021

joperezr May 28, 2021

krwq Jun 4, 2021

joperezr May 28, 2021 •

edited by eerhardt

Loading

eerhardt left a comment

eerhardt left a comment

Add trimmer annotations to System.Data.Common #52046

Add trimmer annotations to System.Data.Common #52046

Conversation

krwq commented Apr 29, 2021

dotnet-issue-labeler bot commented Apr 29, 2021 • edited by krwq Loading

ghost commented Apr 29, 2021

Wraith2 commented Apr 29, 2021

eerhardt commented Apr 29, 2021

eerhardt commented Apr 29, 2021 • edited by krwq Loading

eerhardt commented Apr 29, 2021 • edited by krwq Loading

eerhardt commented Apr 29, 2021

ghost commented Apr 29, 2021

eerhardt left a comment

Choose a reason for hiding this comment

krwq commented Apr 30, 2021

krwq commented Apr 30, 2021

krwq commented May 7, 2021

eerhardt commented May 7, 2021

eerhardt commented May 7, 2021 • edited Loading

eerhardt commented May 7, 2021

krwq commented May 25, 2021

eerhardt commented May 26, 2021 • edited by krwq Loading

eerhardt commented May 26, 2021

eerhardt commented May 26, 2021 • edited Loading

joperezr commented May 27, 2021

joperezr commented May 27, 2021

joperezr commented May 27, 2021

joperezr commented May 27, 2021

joperezr May 28, 2021

Choose a reason for hiding this comment

krwq Jun 2, 2021

Choose a reason for hiding this comment

joperezr May 28, 2021

Choose a reason for hiding this comment

krwq Jun 4, 2021

Choose a reason for hiding this comment

joperezr May 28, 2021 • edited by eerhardt Loading

Choose a reason for hiding this comment

eerhardt left a comment

Choose a reason for hiding this comment

eerhardt left a comment

Choose a reason for hiding this comment

dotnet-issue-labeler bot commented Apr 29, 2021 •

edited by krwq

Loading

eerhardt commented Apr 29, 2021 •

edited by krwq

Loading

eerhardt commented Apr 29, 2021 •

edited by krwq

Loading

eerhardt commented May 7, 2021 •

edited

Loading

eerhardt commented May 26, 2021 •

edited by krwq

Loading

eerhardt commented May 26, 2021 •

edited

Loading

joperezr May 28, 2021 •

edited by eerhardt

Loading