-
Notifications
You must be signed in to change notification settings - Fork 965
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 trimming annotations #7256
Add trimming annotations #7256
Conversation
First jab at #4649 |
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.
Could you please describe the developer flow (that is, what steps are to surface the issue) and the testing steps (i.e., the results before and after)?
Thanks
src/System.Windows.Forms/src/System/Windows/Forms/BindingSource.cs
Outdated
Show resolved
Hide resolved
@agocke does it look like we're moving in the right direction? |
I think I should start with dotnet/runtime#61874 (comment) suggestions and create baseline first, and then chip-off from that issues in subsequence PR like this one. |
Thank you for the link. Sounds good👍 |
src/System.Windows.Forms/src/System/Windows/Forms/DataGridViewButtonColumn.cs
Outdated
Show resolved
Hide resolved
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.
Fails to compile
internal const string FilterRequiresUnreferencedCodeMessage = "The public parameterless constructor or the 'Default' static field may be trimmed from the Attribute's Type."; | ||
|
||
internal const string AttributesRequiresUnreferencedCodeMessage = "Generic TypeConverters may require the generic types to be annotated. For example, NullableConverter requires the underlying type to be DynamicallyAccessedMembers All."; | ||
|
||
internal const string PropertyDescriptorPropertyTypeMessage = "PropertyDescriptor's PropertyType cannot be statically discovered."; | ||
|
||
internal const string EditorRequiresUnreferencedCode = "Editors registered in TypeDescriptor.AddEditorTable may be trimmed."; | ||
|
||
internal const string EventDescriptorRequiresUnreferencedCodeMessage = "The built-in EventDescriptor implementation uses Reflection which requires unreferenced code."; | ||
|
||
internal const string TypeConverterGetPropertiesMessage = "The Type of value cannot be statically discovered. The public parameterless constructor or the 'Default' static field may be trimmed from the Attribute's Type."; | ||
|
||
internal const string BindingListViewFilterMessage = "Members of types used in the filter expression might be trimmed."; | ||
|
||
internal const string SiteNameMessage = "The Type of components in the container cannot be statically discovered to validate the name."; |
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.
Please keep these in A-Z order, because once we have dozens of those searching for these outside an IDE may become a royal pain.
Could you please describe the developer flow what commands you ran, what were the results before this change and with the change? |
No, actually I do not follow that guide, I just trim my sample application using regular trimming options. And fix all errors. What comment by @vitek-karas gives, is one time setup for infra structure and then warnings would be part of build pipeline. also suppressions would be documented and PRs on improving trimmability would be just chipping from long list of issues. I need to prepare new PR based on Vitek's comments so we have baseline for trimming and then I comeback to this PR. |
@@ -516,6 +518,7 @@ string ICustomTypeDescriptor.GetComponentName() | |||
/// <summary> | |||
/// ICustomTypeDescriptor implementation. | |||
/// </summary> | |||
[RequiresUnreferencedCode(AttributesRequiresUnreferencedCodeMessage)] | |||
TypeConverter ICustomTypeDescriptor.GetConverter() | |||
{ | |||
return GetConverter(); |
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 isn't the GetConverter
annotated with RUC?
Typically when we added annotations in runtime we went bottom to top - started with the places which reported warnings and added annotations as necessary. In this case the problematic calls is TypeDescriptor.GetConverter
- which is in GetConverter
, so I would expect that to be annotated first - and from there all of these should be annotated as well.
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 see - the interface definition also annotates these with RUC - got it.
You'll need to annotate GetConverter at some point, but I understand how you got here now - sorry.
@@ -61,6 +62,8 @@ internal AxSourcingSite(IComponent component, Ole32.IOleClientSite clientSite, s | |||
public string? Name | |||
{ | |||
get => _name; | |||
|
|||
[RequiresUnreferencedCode(TrimmingConstants.SiteNameMessage)] |
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 think this needs some comment to explain what's dangerous about setting the property.
I assume that the name is later used in some reflection stuff, but in that case the .ctor should also be annotated as it can also set the name.
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.
From what I see from code, I can add supression saying that Name is used only for naming purposes. Even in the designer I think that names used for CodeDom mostly, and never accessed via reflections.
Okay I found places where reflection used
winforms/src/System.Windows.Forms.Design/src/System/ComponentModel/Design/ComponentDesigner.cs
Lines 546 to 559 in 6e448a3
[Obsolete("This method has been deprecated. Use InitializeNewComponent instead. https://go.microsoft.com/fwlink/?linkid=14202")] | |
public virtual void OnSetComponentDefaults() | |
{ | |
ISite site = Component?.Site; | |
if (site is not null) | |
{ | |
IComponent component = Component; | |
PropertyDescriptor pd = TypeDescriptor.GetDefaultProperty(component); | |
if (pd is not null && pd.PropertyType.Equals(typeof(string))) | |
{ | |
string current = (string)pd.GetValue(component); | |
if (current is null || current.Length == 0) | |
{ | |
pd.SetValue(component, site.Name); |
CodeDom
Lines 188 to 199 in 6e448a3
string compName = site.Name; | |
if (!(site.GetService(typeof(CodeTypeDeclaration)) is CodeTypeDeclaration typeDecl) || compName is null) | |
{ | |
return; | |
} | |
foreach (CodeTypeMember member in typeDecl.Members) | |
{ | |
if (member is CodeMemberField field && field.Name.Equals(compName)) | |
{ | |
typeDecl.Members.Remove(field); |
and
Lines 104 to 111 in 6e448a3
string name = site.Name; | |
if (name is null) | |
{ | |
return MemberAttributes.Private; | |
} | |
FieldInfo field = TypeDescriptor.GetReflectionType(baseType).GetField(name, BindingFlags.NonPublic | BindingFlags.Public | BindingFlags.Instance | BindingFlags.Static); |
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.
And one more I miss to post due to slippery pressing Enter
winforms/src/System.Windows.Forms.Design/src/System/Windows/Forms/Design/ToolStripItemDesigner.cs
Lines 711 to 720 in 6e448a3
defaultValues["Text"] = site.Name; | |
IComponent component = Component; | |
PropertyDescriptor pd = TypeDescriptor.GetProperties(ToolStripItem)["Text"]; | |
if (pd != null && pd.PropertyType.Equals(typeof(string))) | |
{ | |
string current = (string)pd.GetValue(component); | |
if (current is null || current.Length == 0) | |
{ | |
pd.SetValue(component, site.Name); |
I'm curious is designers only partially open sourced, or these locations can be rewritten without need for reflection.
@@ -57,51 +58,71 @@ Type IReflect.UnderlyingSystemType | |||
} | |||
|
|||
// Methods | |||
[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicFields | DynamicallyAccessedMemberTypes.NonPublicFields)] | |||
FieldInfo IReflect.GetField(string name, BindingFlags bindingAttr) | |||
{ | |||
return typeIReflectImplementation.GetField(name, bindingAttr); |
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.
You will need to annotate the fields with All
probably as well for this to work correctly.
Honestly this class is weird - it will end up calling the GetFields on itself - basically typeof(HtmlToClrEventProxy).GetFields
. I assume it's because it's really only used for the InvokeMember
, but in that case I would expect everything else to throw, since the way it is right now it doesn't make much sense to me.
@@ -12,6 +12,7 @@ namespace System.Windows.Forms | |||
/// <summary> | |||
/// Identifies a band or column in the dataGridView. | |||
/// </summary> | |||
[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicParameterlessConstructor)] |
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.
Nit: This is the only type in its hierarchy which technically needs the attribute. The attribute is effectively inherited and so you don't need to add it to a derived type again, even if that type "uses" it. But there's no harm in having it on the derived types either.
If the effort to make winforms trimmable is serious (as in the repo maintainers want this), then I would probably start with some infra similar to what runtime has:
An example of this in the runtime repo is this target: https://github.com/dotnet/runtime/blob/6c7d9359c271212416865c446ad0167cbdcd17e1/src/libraries/oob.proj#L75 This provides infra which has couple of advantages:
|
Mostly make Clone methods works by applying PublicParameterlessConstructor and annotate EditType
Our build agents are currently undergoing some maintenance, so we'll need to re-run the build once the agents are back online. |
/azp run |
Hello @RussKie! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
Azure Pipelines successfully started running 2 pipeline(s). |
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.
Happy to oblige
Mostly make Clone methods works by applying PublicParameterlessConstructor and annotate EditType
Microsoft Reviewers: Open in CodeFlow