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.

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

Annotate CreateInstanceForAnotherGenericParameter as PublicParameterlessConstructor #50390

Merged
merged 2 commits into from
Mar 30, 2021

Conversation

eerhardt
Copy link
Member

This allows private constructors on the Types to be trimmed.

Fix #50353

…essConstructor

This allows private constructors on the Types to be trimmed.

Fix dotnet#50353
Comment on lines 216 to 217
Debug.Assert(type.GetConstructor(Type.EmptyTypes) != null,
$"CreateInstanceForAnotherGenericParameter requires {nameof(type)} to have a public parameterless constructor so it can be annotated for trimming without preserving private constructors.");
Copy link
Member

Choose a reason for hiding this comment

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

I don't think the desire to put some annotation on the method is enough to make it an assumption. I assume you checked that the runtime will actually call the parameterless .ctor (since we're not passing in any .ctor args here) - I think that would be a much more solid assumption to make - which then leads to the better annotation.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure I understand your comment. I changed all callers of this method to ensure they have a public parameterless constructor.

This assert is to catch the cases where someone calls CreateInstanceForAnotherGenericParameter passing in a Type with an internal or private default constructor. That way they get an assert telling them this isn't supported.

Would it be clearer if I added

Suggested change
Debug.Assert(type.GetConstructor(Type.EmptyTypes) != null,
$"CreateInstanceForAnotherGenericParameter requires {nameof(type)} to have a public parameterless constructor so it can be annotated for trimming without preserving private constructors.");
Debug.Assert(type.GetConstructor(Type.EmptyTypes) is ConstructorInfo c && c.IsPublic,
$"CreateInstanceForAnotherGenericParameter requires {nameof(type)} to have a public parameterless constructor so it can be annotated for trimming without preserving private constructors.");

?

Copy link
Member

Choose a reason for hiding this comment

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

Got it - it's not about "parameterless" but about "public"
Makes sense.

@jkotas jkotas merged commit ec0bb49 into dotnet:main Mar 30, 2021
@eerhardt eerhardt deleted the Fix50353 branch March 30, 2021 14:05
@ghost ghost locked as resolved and limited conversation to collaborators Apr 29, 2021
@karelz karelz added this to the 6.0.0 milestone May 20, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Lack of DynamicallyAccessedMemberTypes for any default constructor forces keeping unused types
4 participants