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.

Sign up for GitHub

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

Jump to bottom

Strip the ILLinkTrim.xml file from the System.ComponentModel.TypeConverter assembly #37402

Merged
merged 5 commits into from
Jun 5, 2020
Merged

Strip the ILLinkTrim.xml file from the System.ComponentModel.TypeConverter assembly #37402

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
1dc0187
Strip the ILLinkTrim.xml file from the System.ComponentModel.TypeConv…
layomia Jun 3, 2020
4702782
Address review feedback
layomia Jun 4, 2020
667969e
Merge remote-tracking branch 'upstream/master' into trim_xml_typeconv…
layomia Jun 4, 2020
e4a268b
Update DefaultConstructor -> PublicParameterlessConstructor
layomia Jun 4, 2020
6643238
Fix ComObjectType property annotation
layomia Jun 5, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/coreclr/src/System.Private.CoreLib/src/Internal/Runtime/InteropServices/ComActivator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
using System;
using System.Collections.Generic;
using System.Diagnostics;
using System.Diagnostics.CodeAnalysis;
using System.IO;
using System.Reflection;
using System.Runtime.InteropServices;
Expand Down Expand Up @@ -700,6 +701,7 @@ internal sealed class LicenseInteropProxy
// LicenseContext
private readonly MethodInfo _setSavedLicenseKey;

[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicMethods | DynamicallyAccessedMemberTypes.PublicParameterlessConstructor)]
private readonly Type _licInfoHelper;
private readonly MethodInfo _licInfoHelperContains;

Expand Down
0 ...entModel.TypeConverter/src/ILLinkTrim.xml → ...Converter/src/ILLinkTrim_LibraryBuild.xml
View file
Open in desktop
File renamed without changes.
3 changes: 1 addition & 2 deletions ...libraries/System.ComponentModel.TypeConverter/src/System/ComponentModel/TypeDescriptor.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2319,8 +2319,7 @@ public static void Refresh(Assembly assembly)
[EditorBrowsable(EditorBrowsableState.Advanced)]
public static Type ComObjectType
{
[PreserveDependency(".ctor", "System.ComponentModel.TypeDescriptor/TypeDescriptorComObject")]
[PreserveDependency(".ctor", "System.ComponentModel.TypeDescriptor/ComNativeDescriptorProxy")] // TODO: https://github.com/mono/linker/issues/801
[return: DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicParameterlessConstructor)]
Copy link
Member

Choose a reason for hiding this comment

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

(nit) - this may be a preference thing, but IMO it feels more natural for this attribute to be applied to the property:

[EditorBrowsable(EditorBrowsableState.Advanced)]
[DynamicallyAccessedMembersAttribute(DynamicallyAccessedMemberTypes.PublicParameterlessConstructor)]
public static Type ComObjectType
{
    get => typeof(TypeDescriptorComObject);
}

Copy link
Member

Choose a reason for hiding this comment

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

I'm actually not sure linker will be able to handle this - the attribute on a property works for implicit getters/setters, but for those with explicit bodies it might not be reliable (since we need to detect the backing field if there's one to annotate it as well, and that detection is tricky/impossible for random properties).

Copy link
Member

Choose a reason for hiding this comment

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

Note that if it doesn't work linker will not warn - TODO - I created dotnet/linker#1248 to get that fixed.

Copy link
Member

Choose a reason for hiding this comment

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

I'm actually not sure linker will be able to handle this - the attribute on a property works for implicit getters/setters, but for those with explicit bodies it might not be reliable (since we need to detect the backing field if there's one to annotate it as well, and that detection is tricky/impossible for random properties).

The behavior is that if the annotation is placed on the property, the annotation is propagated to the return value of the getter and the parameter of the setter. If this is an auto property, the annotation is also propagated to the backing field. Otherwise, this won't propagate to the backing field, even if it looks "obvious". One will get a warning and needs to annotate the backing field manually.

Copy link
Member

Choose a reason for hiding this comment

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

What about this case where there is no backing field? Does the TypeDescriptorComObject type get annotated with [DynamicallyAccessedMembersAttribute(DynamicallyAccessedMemberTypes.PublicParameterlessConstructor)] if the attribute is applied to the property?

Copy link
Member

Choose a reason for hiding this comment

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

What about this case where there is no backing field?

This part applies always: "The behavior is that if the annotation is placed on the property, the annotation is propagated to the return value of the getter and the parameter of the setter"

Does the TypeDescriptorComObject type get annotated with

Yes, because it's a value that was returned from a method that has the return parameter annotated (because the annotation was propagated from the property).

get => typeof(TypeDescriptorComObject);
}

Expand Down
7 changes: 5 additions & 2 deletions ...ibraries/System.ObjectModel/src/System/ComponentModel/TypeDescriptionProviderAttribute.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

using System.Diagnostics.CodeAnalysis;

namespace System.ComponentModel
{
[AttributeUsage(AttributeTargets.Class, Inherited = true)]
Expand All @@ -10,7 +12,7 @@ public sealed class TypeDescriptionProviderAttribute : Attribute
/// <summary>
/// Creates a new TypeDescriptionProviderAttribute object.
/// </summary>
public TypeDescriptionProviderAttribute(string typeName)
public TypeDescriptionProviderAttribute([DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicParameterlessConstructor)] string typeName)
{
if (typeName == null)
{
Expand All @@ -23,7 +25,7 @@ public TypeDescriptionProviderAttribute(string typeName)
/// <summary>
/// Creates a new TypeDescriptionProviderAttribute object.
/// </summary>
public TypeDescriptionProviderAttribute(Type type)
public TypeDescriptionProviderAttribute([DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicParameterlessConstructor)] Type type)
{
if (type == null)
{
Expand All @@ -37,6 +39,7 @@ public TypeDescriptionProviderAttribute(Type type)
/// The TypeName property returns the assembly qualified type name
/// for the type description provider.
/// </summary>
[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicParameterlessConstructor)]
public string TypeName { get; }
}
}
3 changes: 3 additions & 0 deletions src/libraries/System.Private.CoreLib/src/System/ComponentModel/DefaultValueAttribute.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

using System.Globalization;
using System.Reflection;
using System.Runtime.CompilerServices;
using System.Threading;

namespace System.ComponentModel
Expand All @@ -27,6 +28,8 @@ public class DefaultValueAttribute : Attribute
/// class, converting the specified value to the specified type, and using the U.S. English
/// culture as the translation context.
/// </summary>
// TODO: https://github.com/mono/linker/issues/943
[PreserveDependency("ConvertFromInvariantString", "System.ComponentModel.TypeConverter", "System.ComponentModel.TypeConverter")]
layomia marked this conversation as resolved.
Show resolved Hide resolved
public DefaultValueAttribute(Type type, string? value)
{
// The null check and try/catch here are because attributes should never throw exceptions.
Expand Down