Fixed creating generic type with abstract type when it is has a default constractor constrant

[Faolan-Rad]

I think this fixes the bug that I was having

[dotnet-policy-service]

Tagging subscribers to this area: @mangod9
See info in area-owners.md if you want to be subscribed.

[jkotas]

• This change would need tests
• There are number of places where the constraint checks would need to be fixed (e.g.

  runtime/src/coreclr/tools/Common/TypeSystem/Common/TypeSystemConstraintsHelpers.cs

  Lines 39 to 40 in 7dc3669

  	if (!instantiationParam.HasExplicitOrImplicitDefaultConstructor()
  	&& !CheckGenericSpecialConstraint(instantiationParam, GenericConstraints.DefaultConstructorConstraint))

  or

  runtime/src/mono/mono/metadata/verify.c

  Lines 102 to 103 in 7dc3669

  	if ((param_info->flags & GENERIC_PARAMETER_ATTRIBUTE_CONSTRUCTOR_CONSTRAINT) && !m_class_is_valuetype (paramClass) && !mono_class_has_default_constructor (paramClass, TRUE))
  	return FALSE;

  )
• We may need to file a breaking change notice for this change. I am not sure where it is worth fixing given that - the C# constraint checks are generally more restrictive compared to constraint checks done by the runtime.

[Faolan-Rad]

I am trying to think of a use case where someone would want a none installable type when you are expecting one so I don't think it would be a braking change but I get what you mean someone could still be using it in that way I will still go through and do the checks for where you specified and check if there is more that I am missing

[Faolan-Rad]

I don't know how you would make it do the check for runtime/src/coreclr/tools/Common/TypeSystem/Common/TypeSystemConstraintsHelpers.cs
From what I can tell TypeDesc has no way of knowing if it is an abstract class or not

[Faolan-Rad]

@dotnet-policy-service agree

[MichalStrehovsky]

I don't know how you would make it do the check for runtime/src/coreclr/tools/Common/TypeSystem/Common/TypeSystemConstraintsHelpers.cs From what I can tell TypeDesc has no way of knowing if it is an abstract class or not

That one already behaves according to the ECMA-335 spec. It goes to here:

runtime/src/coreclr/tools/Common/TypeSystem/Common/TypeSystemHelpers.cs

Lines 95 to 98 in e1c6717

	public static bool HasExplicitOrImplicitDefaultConstructor(this TypeDesc type)
	{
	return type.IsValueType || type.GetDefaultConstructor() != null;
	}

That goes to here:

runtime/src/coreclr/tools/Common/TypeSystem/Ecma/EcmaType.cs

Lines 386 to 389 in e1c6717

	public override MethodDesc GetDefaultConstructor()
	{
	if (IsAbstract)
	return null;

[fanyang-mono]

The change to mono runtime looks good to me.

[ivdiazsa]

@Faolan-Rad Is this work still planned to be completed?

[Faolan-Rad]

I don't really know what else is needed does it need a unit test to be made for it?

[ivdiazsa]

I don't really know what else is needed does it need a unit test to be made for it?

Yes, a test would be great. Since we're changing behavior we would need to have it covered in our pipelines.

[Faolan-Rad]

Its is up to date now and I believe I added the test correctly that was needed I put it in the reflection tests

[ivdiazsa]

Its is up to date now and I believe I added the test correctly that was needed I put it in the reflection tests

Thank you very much! I think that's the last thing we need for this fix. Can we get a review please? @lambdageek @thaystg @fanyang-mono @MichalStrehovsky

[jkotas]

The new test does not build:

src/tests/reflection/DisallowAbstractConstructors/DisallowAbstractConstructors.cs(43,95): error CS1002: ; expected [/__w/1/s/src/tests/reflection/DisallowAbstractConstructors/DisallowAbstractConstructors.csproj]

[ivdiazsa]

The new test does not build:

src/tests/reflection/DisallowAbstractConstructors/DisallowAbstractConstructors.cs(43,95): error CS1002: ; expected [/__w/1/s/src/tests/reflection/DisallowAbstractConstructors/DisallowAbstractConstructors.csproj]

Seems like a typo of a missing ;. I'll just add it on top of the current changes as it's a very simple fix.

[MichalStrehovsky]

I'd hold off merging this until RC1 is snapped off due to the breaking change potential.

[fanyang-mono]

Mono related change looks good to me!