Fix Activator.CreateInstance(GetType(), ...) related ILLink warnings in System.Data.Common #54680
Conversation
System.Data.Common
krwq
added
the
linkable-framework
|
Tagging subscribers to 'linkable-framework': @eerhardt, @vitek-karas, @LakshanF, @sbomer, @joperezr Issue Details
|
|
Note regarding the 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. |
| @@ -202,7 +203,7 @@ public DataTableMappingCollection TableMappings | |||
| [Obsolete("CloneInternals() has been deprecated. Use the DataAdapter(DataAdapter from) constructor. https://go.microsoft.com/fwlink/?linkid=14202")] | |||
| protected virtual DataAdapter CloneInternals() | |||
| { | |||
| DataAdapter clone = (DataAdapter)Activator.CreateInstance(GetType(), System.Reflection.BindingFlags.Public | System.Reflection.BindingFlags.Instance, null, null, CultureInfo.InvariantCulture, null)!; | |||
| DataAdapter clone = (DataAdapter)Activator.CreateInstance(GetType())!; | |||
| @@ -2304,6 +2305,8 @@ public void AcceptChanges() | |||
|
|
|||
| // Prevent inlining so that reflection calls are not moved to caller that may be in a different assembly that may have a different grant set. | |||
| [MethodImpl(MethodImplOptions.NoInlining)] | |||
| [UnconditionalSuppressMessage("ReflectionAnalysis", "IL2026:RequiresUnreferencedCode", | |||
There was a problem hiding this comment.
What is going to keep the parameterless constructor alive if some 3rd party code inherits from DataTable?
I do not think DynamicallyAccessedMembers applies to child types. This UnconditionalSuppressMessage does not look 100% correct to me.
There was a problem hiding this comment.
There are 2 different concerns at play here.
What is going to keep the parameterless constructor alive if some 3rd party code inherits from DataTable?
The [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicParameterlessConstructor | DynamicallyAccessedMemberTypes.NonPublicConstructors)] attribute on the DataTable class itself preserves the parameterless ctor for DataTable and all derived types it marks. See dotnet/linker#1929.
This UnconditionalSuppressMessage does not look 100% correct to me.
This suppress message actually suppressing a warning that is 1 step down from the problem at hand. Because this change is adding DynamicallyAccessedMembers(NonPublicConstructors) to the DataTable class itself, and the DataTable class has non-public ctors that are marked with RequiresUnreferencedCode (specifically, the SerializationInfo, StreamingContext ctor), the linker is now issuing a new warning that it looks like we are reflecting against this "unsafe" serialization ctor. That is the warning that is being suppressed here. It is fine to suppress the warning because this code will never find that ctor, since that ctor has parameters and this is looking for a parameterless ctor.
There was a problem hiding this comment.
Ah ok. Thank you for the explanation.
There was a problem hiding this comment.
AFAIK DynamicallyAccessedMembers put on a class flows into derived classes. Suppression is because Activator.CreateInstance(GetType()) doesn't look at the arguments so if any of the ctors has RUC it will show you the warning (here we have SerializationContext ctor with RUC) but we know we can only hit parameterles constructor and SerializationContext ctor will never get executed. cc: @vitek-karas
There was a problem hiding this comment.
Both @eerhardt and @krwq are correct. The semantic of putting DynamicallyAccessedMembers on a type are such that it applies to the type and all the derived types (in case of an interface it applies to all implementations of the interface). The behavior is not automatic, the attribute is only applied if trimmer can find a use case for it. The use case currently is a call to variable.GetType() where variable is of static type TBase and TBase has DynamicallyAccessedMembers (or it derives from type which has it).
The second part about the need for suppression is also correct and it's basically a current limitation. The DynamicallyAccessedMemberTypes only has PublicParameterlessConstructor, PublicConstructors and NonPublicConstructors. So it can't express the exact requirements of "public or non-public parameterless constructor". So the current way to do this is to specify PublicParameterlessConstructor | NonPublicConstructors which will mark additional non-public constructors with parameters. In this case one of them has a RUC and so trimmer reports a warning.
There's a discussion about adding some way to specify "public or non-public parameterless constructor" here: dotnet/linker#1688
ghost
locked as resolved and limited conversation to collaborators
No description provided.