Relax validtion of MakeGeneric* on struct and unmanaged constraint#1888
Relax validtion of MakeGeneric* on struct and unmanaged constraint#1888vitek-karas merged 3 commits intodotnet:mainfrom
Conversation
`struct` and `unmanaged` constraint also imply `new()` constraint, but any type allowable for `struct` or `unmanaged` constraints always has public parameterless constructor. Or rather the type can be constructed without parameters (the .ctor itself is actually not accessible through reflection). Since MakeGenericType and MakeGenericMethod both enforce that only types compatible with `struct` or `unmanaged` constraints can be passed as type arguments to such generic parameters, there's no need for linker to validate this as well. At runtime it will always work - and on top of that, there's nothing for linker to mark - struct and unmanaged don't have a parameterless .ctor in IL - it's just that at runtime they behave as if they do. So enforcing the `new()` constraint in `MakeGenericMethod` or `MakeGenericType` makes sense only when it's just `new()` constraint, not if it's implied from `struct` or `unmanaged` constraints.
|
/cc @eerhardt |
|
I was thinking a little more about this tonight. I wonder if we could change Roslyn to only put a This would solve all the cases in |
|
Oh this is annoying. Roslyn is considering adding support for parameterless constructors in C# 10 so this fix might potentially make us miss constructors. I would expect this to print "Hello" but I don't know if there's anything that would prevent us from stripping the constructor and make this go with the implicit instance of Foo instead. typeof(Factory<>).MakeGenericType(typeof(Foo)).GetMethod("MakeNew").Invoke(null, Array.Empty<object>());
class Factory<T> where T: struct
{
public static T MakeNew() => new T();
}
struct Foo
{
public Foo() => Console.WriteLine("Hello");
}Here's ILDASM |
|
The C# team just discussed it in the LDM: https://github.com/dotnet/csharplang/blob/master/meetings/2021/LDM-2021-03-10.md#parameterless-struct-constructors There's mentions of private parameterless constructors so that might mess with things. The meeting notes are kind of hand-wavy "This may be a case where we have to simply hold our noses and compromise on letting the runtime exception happen.". Maybe we'll just have to hold our noses too. I haven't sorted all of the implications of this in my head yet. |
|
What we could do is that we would extend the analysis of It's linker work though and not sure how messy that would be on the consumption side of things. |
|
We could also just simply mark all parameterless .ctors on structs. Currently this won't do anything on C# generated code, if C# adds the feature we would start marking those .ctors. It's not ideal from size perspective, but it's probably not terrible either. as @eerhardt pointed out earlier - there are way too many places in the framework itself where We could obviously special case |
|
Discussed this with @MichalStrehovsky and given the C# development we're not comfortable with just allowing all So the new proposed behavior of linker is:
|
… a special case. Nullable<> doesn't make use of the implied new() constraint, so there's no reason for linker to enforce it. It's by far the most common case where this is problematic - that's why the special handling.
| foreach (var genericParameter in typeValue.TypeRepresented.GenericParameters) { | ||
| if (_context.Annotations.FlowAnnotations.GetGenericParameterAnnotation (genericParameter) != DynamicallyAccessedMemberTypes.None || | ||
| genericParameter.HasDefaultConstructorConstraint) { | ||
| (genericParameter.HasDefaultConstructorConstraint && !typeValue.TypeRepresented.IsTypeOf ("System", "Nullable"))) { |
There was a problem hiding this comment.
I don't remember - does Cecil also try to unmangle C# or should this rather be
| (genericParameter.HasDefaultConstructorConstraint && !typeValue.TypeRepresented.IsTypeOf ("System", "Nullable"))) { | |
| (genericParameter.HasDefaultConstructorConstraint && !typeValue.TypeRepresented.IsTypeOf ("System", "Nullable`1"))) { |
There was a problem hiding this comment.
You're right - I wrote a test for it, but then forgot to call the test :-(
All fixed now.
…otnet/linker#1888) `struct` and `unmanaged` constraint also imply `new()` constraint, but any type allowable for `struct` or `unmanaged` constraints always has public parameterless constructor. Or rather the type can be constructed without parameters (the .ctor itself is actually not accessible through reflection). Since MakeGenericType and MakeGenericMethod both enforce that only types compatible with `struct` or `unmanaged` constraints can be passed as type arguments to such generic parameters, there's no need for linker to validate this as well. At runtime it will always work - and on top of that, there's nothing for linker to mark - struct and unmanaged don't have a parameterless .ctor in IL - it's just that at runtime they behave as if they do. So enforcing the `new()` constraint in `MakeGenericMethod` or `MakeGenericType` makes sense only when it's just `new()` constraint, not if it's implied from `struct` or `unmanaged` constraints. * Change the behavior to not relax but instead single out Nullable<> as a special case. Nullable<> doesn't make use of the implied new() constraint, so there's no reason for linker to enforce it. It's by far the most common case where this is problematic - that's why the special handling. Commit migrated from dotnet/linker@47c00c1
structandunmanagedconstraint also implynew()constraint, but any type allowable forstructorunmanagedconstraints always has public parameterless constructor. Or rather the type can be constructed without parameters (the.ctoritself is actually not accessible through reflection).Since
MakeGenericTypeandMakeGenericMethodboth enforce that only types compatible withstructorunmanagedconstraints can be passed as type arguments to such generic parameters, there's no need for linker to validate this as well. At runtime it will always work - and on top of that, there's nothing for linker to mark - struct and unmanaged don't have a parameterless.ctorin IL - it's just that at runtime they behave as if they do.So enforcing the
new()constraint inMakeGenericMethodorMakeGenericTypemakes sense only when it's justnew()constraint, not if it's implied fromstructorunmanagedconstraints.Fixes #1887