TypeBuilder::CreateTypeInfo() should not throw COMException

[marek-safar]

When TypeBuilder encounters by-ref field it throws COMException. This seems to be the only reference to COMException and the behaviour is not consistent with other error handling for different type members.

runtime/src/libraries/System.Reflection.Emit/tests/TypeBuilder/TypeBuilderDefineField.cs

Lines 107 to 113 in cfb1335

	public void DefineField_ByRefFieldType_ThrowsCOMExceptionOnCreation()
	{
	TypeBuilder type = Helpers.DynamicType(TypeAttributes.Public);
	type.DefineField("Name", typeof(int).MakeByRefType(), FieldAttributes.Public);
	Assert.Throws<COMException>(() => type.CreateTypeInfo());
	}

It should be replaced with ArgumentException as it's done for all other type members. Here is an example with events

runtime/src/libraries/System.Reflection.Emit/tests/TypeBuilder/TypeBuilderDefineEvent.cs

Lines 109 to 115 in cfb1335

	[Fact]
	public void DefineEvent_ByRefEventType_ThrowsArgumentException()
	{
	TypeBuilder type = Helpers.DynamicType(TypeAttributes.Class | TypeAttributes.Public);
	AssertExtensions.Throws<ArgumentException>(null, () => type.DefineEvent("Name", EventAttributes.None, typeof(int).MakeByRefType()));
	}

[AaronRobinsonMSFT]

The error here is VLDTR_E_SIG_BYREFINFIELD (HRESULT 0x801312e4) and is originating in a deep call stack.

coreclr.dll!ThrowHR(HRESULT hr) Line 965
	at D:\runtime\src\coreclr\src\utilcode\ex.cpp(965)
coreclr.dll!IfFailThrow(HRESULT hr) Line 1312
	at D:\runtime\src\coreclr\src\inc\ex.h(1312)
coreclr.dll!MethodTableBuilder::InitializeFieldDescs(FieldDesc * pFieldDescList, const LayoutRawFieldInfo * pLayoutRawFieldInfos, MethodTableBuilder::bmtInternalInfo * bmtInternal, const MethodTableBuilder::bmtGenericsInfo * bmtGenerics, MethodTableBuilder::bmtMetaDataInfo * bmtMetaData, MethodTableBuilder::bmtEnumFieldInfo * bmtEnumFields, MethodTableBuilder::bmtErrorInfo * bmtError, MethodTable * * * pByValueClassCache, MethodTableBuilder::bmtMethAndFieldDescs * bmtMFDescs, MethodTableBuilder::bmtFieldPlacement * bmtFP, unsigned int * totalDeclaredSize) Line 3775
	at D:\runtime\src\coreclr\src\vm\methodtablebuilder.cpp(3775)
coreclr.dll!MethodTableBuilder::BuildMethodTableThrowing(LoaderAllocator * pAllocator, Module * pLoaderModule, Module * pModule, unsigned int cl, BuildingInterfaceInfo_t * pBuildingInterfaceList, const LayoutRawFieldInfo * pLayoutRawFieldInfos, MethodTable * pParentMethodTable, const MethodTableBuilder::bmtGenericsInfo * bmtGenericsInfo, SigPointer parentInst, unsigned short cBuildingInterfaceList) Line 1705
	at D:\runtime\src\coreclr\src\vm\methodtablebuilder.cpp(1705)
coreclr.dll!ClassLoader::CreateTypeHandleForTypeDefThrowing(Module * pModule, unsigned int cl, Instantiation inst, AllocMemTracker * pamTracker) Line 12232
	at D:\runtime\src\coreclr\src\vm\methodtablebuilder.cpp(12232)
coreclr.dll!ClassLoader::CreateTypeHandleForTypeKey(TypeKey * pKey, AllocMemTracker * pamTracker) Line 3197
	at D:\runtime\src\coreclr\src\vm\clsload.cpp(3197)
coreclr.dll!ClassLoader::DoIncrementalLoad(TypeKey * pTypeKey, TypeHandle typeHnd, ClassLoadLevel currentLevel) Line 3125
	at D:\runtime\src\coreclr\src\vm\clsload.cpp(3125)
coreclr.dll!ClassLoader::LoadTypeHandleForTypeKey_Body(TypeKey * pTypeKey, TypeHandle typeHnd, ClassLoadLevel targetLevel) Line 3894
	at D:\runtime\src\coreclr\src\vm\clsload.cpp(3894)
coreclr.dll!ClassLoader::LoadTypeHandleForTypeKey(TypeKey * pTypeKey, TypeHandle typeHnd, ClassLoadLevel targetLevel, const InstantiationContext * pInstContext) Line 3613
	at D:\runtime\src\coreclr\src\vm\clsload.cpp(3613)
coreclr.dll!COMDynamicWrite::TermCreateClass(QCall::ModuleHandle pModule, int tk, QCall::ObjectHandleOnStack retType) Line 477
	at D:\runtime\src\coreclr\src\vm\comdynamic.cpp(477)

At the point of the throw, the exception is of type HRException, however it is converted to an EEMessageException which is classified as kCOMException. This exception then gets propagated out to managed as a COMException. I do not know the best way to fix this because I am sure it is going to be a breaking change for someone. Whether that breaking changes matters to anyone is a good question.

coreclr.dll!CLRException::GetThrowableFromException(Exception * pException) Line 787
	at D:\runtime\src\coreclr\src\vm\clrex.cpp(787)
coreclr.dll!UnwindAndContinueRethrowHelperInsideCatch(Frame * pEntryFrame, Exception * pException) Line 8112
	at D:\runtime\src\coreclr\src\vm\excep.cpp(8112)
[snip]
coreclr.dll!COMDynamicWrite::TermCreateClass(QCall::ModuleHandle pModule, int tk, QCall::ObjectHandleOnStack retType) Line 477
	at D:\runtime\src\coreclr\src\vm\comdynamic.cpp(477)

@davidwrighton or @jkotas Any opinions or suggestions on how to change this exception type? I guess we could catch prior to the QCall exist and convert to a better managed type.

[jkotas]

We have plans to enabled byref fields in .NET 6. If that happens, there will be no exception thrown for this repro that will take of this specific problem.

As for the more general problem of mapping HRESULTs to right exception types inside CoreCLR, I am not sure the best way to fix it either.

[marek-safar]

Why not add suggested ArgumentException check to DefineField (that's what it'd done for everything else) ?

[jkotas]

The check for ByRefs in GetTypeTokenWorkerNoLock is not right. I think we should remove it. It is too aggresive. For example, it will incorrectly fire for:

ILGenerator ilGenerator = ...
ilGenerator.Emit(OpCodes.Ldtoken,  typeof(int).MakeByRefType());

that is valid IL.

Reflection.Emit should not really be in the business of validating the emitted IL.

[steveharter]

The check for ByRefs in GetTypeTokenWorkerNoLock is not right

This issue can be re-purposed somewhat then to fix or remove the byref check and add tests.

[ghost]

Tagging subscribers to 'size-reduction': @eerhardt, @SamMonoRT, @marek-safar, @tannergooding, @CoffeeFlux
See info in area-owners.md if you want to be subscribed.

Issue Details

---

When TypeBuilder encounters by-ref field it throws COMException. This seems to be the only reference to COMException and the behaviour is not consistent with other error handling for different type members.

runtime/src/libraries/System.Reflection.Emit/tests/TypeBuilder/TypeBuilderDefineField.cs

Lines 107 to 113 in cfb1335

	public void DefineField_ByRefFieldType_ThrowsCOMExceptionOnCreation()
	{
	TypeBuilder type = Helpers.DynamicType(TypeAttributes.Public);
	type.DefineField("Name", typeof(int).MakeByRefType(), FieldAttributes.Public);
	Assert.Throws<COMException>(() => type.CreateTypeInfo());
	}

It should be replaced with ArgumentException as it's done for all other type members. Here is an example with events

runtime/src/libraries/System.Reflection.Emit/tests/TypeBuilder/TypeBuilderDefineEvent.cs

Lines 109 to 115 in cfb1335

	[Fact]
	public void DefineEvent_ByRefEventType_ThrowsArgumentException()
	{
	TypeBuilder type = Helpers.DynamicType(TypeAttributes.Class | TypeAttributes.Public);
	AssertExtensions.Throws<ArgumentException>(null, () => type.DefineEvent("Name", EventAttributes.None, typeof(int).MakeByRefType()));
	}

Author:	marek-safar
Assignees:	-
Labels:	area-System.Reflection.Emit, bug, size-reduction
Milestone:	6.0.0

[krwq]

moving to 7.0 since this is non-essential for 6.0