-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Conversation
…essConstructor This allows private constructors on the Types to be trimmed. Fix dotnet#50353
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."); |
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 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.
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'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
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."); |
?
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.
Got it - it's not about "parameterless" but about "public"
Makes sense.
This allows private constructors on the Types to be trimmed.
Fix #50353