Resolve ILLink warnings in System.ComponentModel.TypeConverter (Round 2) #49467
Conversation
eerhardt
added
the
linkable-framework
|
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. |
src/libraries/System.ComponentModel.TypeConverter/src/System/ComponentModel/TypeDescriptor.cs
Outdated
Show resolved
Hide resolved
| public string DesignerBaseTypeName { get; } | ||
|
|
||
| /// <summary> | ||
| /// Gets the name of the designer type associated with this designer attribute. | ||
| /// </summary> | ||
| [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicParameterlessConstructor)] |
There was a problem hiding this comment.
I wonder if we want this actually. Are the designers ever used at runtime?
The problem is that this will basically force include the designer types and some code from them into apps - but they will never be used (since they're designers, those should not run at runtime).
Also - the only method which accesses this TypeDescriptor.CreateDesigner is already marked with RUC - and I think we should not allow calling it on a trimmed app.
Technically the annotation is correct, I'm questioning if it's the right thing to do for size.
There was a problem hiding this comment.
the only method which accesses this TypeDescriptor.CreateDesigner is already marked with RUC - and I think we should not allow calling it on a trimmed app.
Just because a method is marked with RUC doesn't mean it can't be called in a trimmed app.
Maybe if this becomes a size issue for WinForms, they can strip out DesignerAttribute instances by default - similar to how we strip out some attributes in Blazor WASM that aren't needed at runtime.
There was a problem hiding this comment.
It's a good point that a better size solution is to strip out the attribute as a whole.
There was a problem hiding this comment.
These attributes can be and are used at runtime by PropertyGrid control, and can be used by custom designers (e.g. a report viewer)
| public ComNativeDescriptorProxy() | ||
| { | ||
| Assembly assembly = Assembly.Load("System.Windows.Forms"); | ||
| Type realComNativeDescriptor = assembly.GetType("System.Windows.Forms.ComponentModel.Com2Interop.ComNativeDescriptor", throwOnError: true); | ||
| Type realComNativeDescriptor = Type.GetType("System.Windows.Forms.ComponentModel.Com2Interop.ComNativeDescriptor, System.Windows.Forms", throwOnError: true); |
There was a problem hiding this comment.
FYI - @AaronRobinsonMSFT. I changed this pattern given the discussion at dotnet/linker#1884 (comment).
Let me know if you have feedback on this change. And how I can test it to make sure I didn't break the scenario.
There was a problem hiding this comment.
This looks fine to me. Adding @JeremyKuhne, @RussKie, and @merriemcgaw for WinForms impact.
There was a problem hiding this comment.
Should be ok? I've never fully walked through this though. :(
…it is available) by using Type.GetType.
eerhardt
force-pushed
the
TypeConverterILLinkWarningsRound2
branch
from
addd814 to
0562916
Compare
src/libraries/System.ComponentModel.Primitives/ref/System.ComponentModel.Primitives.cs
Show resolved
Hide resolved
...s/System.ComponentModel.TypeConverter/src/System/ComponentModel/ReflectPropertyDescriptor.cs
Outdated
Show resolved
Hide resolved
...ibraries/System.ComponentModel.TypeConverter/src/System/ComponentModel/PropertyDescriptor.cs
Outdated
Show resolved
Hide resolved
...s/System.ComponentModel.TypeConverter/src/System/ComponentModel/ReflectPropertyDescriptor.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.ComponentModel.TypeConverter/src/System/ComponentModel/TypeDescriptor.cs
Outdated
Show resolved
Hide resolved
| @@ -485,7 +485,7 @@ internal PropertyDescriptorCollection GetProperties() | |||
| /// then a global Type.GetType is performed. | |||
| /// </summary> | |||
| [UnconditionalSuppressMessage("ReflectionAnalysis", "IL2026:RequiresUnreferencedCode", | |||
| Justification = "typeName is annotated with DynamicallyAccessedMembers, which will preserve the type.")] | |||
| Justification = "Calling _type.Assembly.GetType on a non-assembly qualified type will still work. See https://github.com/mono/linker/issues/1895")] | |||
There was a problem hiding this comment.
This is not 100% true, but I think it's probably good enough for now. Once we solver the linker issue referred here, we'll see if this needs some tweaking or not.
|
runtime failures are #49569 |
ghost
locked as resolved and limited conversation to collaborators
Contributes to #45623