Fix Enum field type bug found when underlying type is set from assembly loaded with MLC

[buyaa-n]

EnumBuilder.UnderlyingSystemType, currently returns GetEnumUnderlyingType() which returns _underlyingField.FieldType underneath. Because Type.Equals checks UnderlyingSystemType for equality when the EnumBuilder underlying type is set from MLC Core assembly it is causing issue for enum fields defined, for example:

Type intType = mlc.CoreAssembly.GetType("System.Int32");
EnumBuilder enumBuilder = mb.DefineEnum("TestEnum", TypeAttributes.Public, intType);
FieldBuilder field = enumBuilder.DefineLiteral("Default", 0);

In this case the field type should be TestEnum, but it is evaluated to be equal to System.Int32 when writing the field signature on Save. I think we should not return underlying field type for EnumBuilder.UnderlyingSystemType, should probably return the Enum itself instead (runtime enums returns the enum type itself). Though the EnumBuilder.GetEnumUnderlyingType() method should keep returning the underlying field type.

Related to #105903

[dotnet-policy-service]

Tagging subscribers to this area: @dotnet/area-system-reflection-emit
See info in area-owners.md if you want to be subscribed.

[jkotas]

Should this

runtime/src/libraries/System.Reflection.Emit/src/System/Reflection/Emit/TypeBuilderImpl.cs

Lines 623 to 635 in f9c0846

	if (IsEnum)
	{
	if (_enumUnderlyingType == null)
	{
	throw new InvalidOperationException(SR.InvalidOperation_NoUnderlyingTypeOnEnum);
	}
	return _enumUnderlyingType;
	}
	else
	{
	return this;
	}

be changed to just return this as well?

[jkotas]

How did you find this issue?

[steveharter]

In this case the field type should be TestEnum, but it is evaluated to be equal to System.Int32 when writing the field signature on Save

Does .NET Framework have this issue?

Because Type.Equals checks UnderlyingSystemType for equality

Is an alternative to override Equals(Type) instead of changing UnderlyingSystemType? The "correct" way to check for the underlying type on an enum is GetEnumUnderlyingType() so I'm OK with returning "this" from EnumBuilder.UnderlyingSystemType.

[jkotas]

Is an alternative to override Equals(Type)

I would stay away from that. The problem with overriding Type.Equals is that it only works in one direction, it does not handle the case where this is the foreign Type and the argument is your Type.

[buyaa-n]

Should this

runtime/src/libraries/System.Reflection.Emit/src/System/Reflection/Emit/TypeBuilderImpl.cs

Lines 623 to 635 in f9c0846

	if (IsEnum)
	{
	if (_enumUnderlyingType == null)
	{
	throw new InvalidOperationException(SR.InvalidOperation_NoUnderlyingTypeOnEnum);
	}
	return _enumUnderlyingType;
	}
	else
	{
	return this;
	}

be changed to just return this as well?

Good point, I think we also need to add GetEnumUnderlyingType() override that returns this _enumUnderlyingType

[buyaa-n]

How did you find this issue?

Found with #105903

[buyaa-n]

Does .NET Framework have this issue?

.NET Framework and existing runtime EnumBuilder doesn't have this issue as there is no Core assembly logic, the Type equality check happens when trying to determine the core type from core assembly.

runtime/src/libraries/System.Reflection.Emit/src/System/Reflection/Emit/ModuleBuilderImpl.cs

Line 108 in bfd964a

if (_coreTypes[i] == type)

Is an alternative to override Equals(Type)

I would stay away from that. The problem with overriding Type.Equals is that it only works in one direction, it does not handle the case where this is the foreign Type and the argument is your Type.

I tried that, and exactly as @jkotas said it was not solving the issue completely

[buyaa-n]

/backport to release/9.0

[github-actions]

Started backporting to release/9.0: https://github.com/dotnet/runtime/actions/runs/10411954784