Return empty string in InternalAssemblyBuilder.Location

[pedrobsaila]

Return empty string in AssemblyBuilder.Location

Issue #51924

[jkotas]

Could you please also add a simple test? src\libraries\System.Reflection.Emit\tests\AssemblyBuilderTests.cs can be a good place to add it.

[pedrobsaila]

Hi @jkotas , I tried to add a test inside AssemblyBuilderTests, but I have not succeeded yet in getting Location = string.Empty. I found that the test uses assembly System.Private.CoreLib.dll inside .dotnet folder which is downloaded with dotnet-install.ps, instead of the one inside artifacts directory (containing my new code). Do you know how to force using the new version of System.Private.CoreLib.dll ?

[jkotas]

the test uses assembly System.Private.CoreLib.dll inside .dotnet folder

That is not expected. How are you running the tests ? https://github.com/dotnet/runtime/blob/main/docs/workflow/testing/libraries/testing.md has the instructions for running the tests.

[pedrobsaila]

I use this link dotnet/sdk#7419 (comment) to debug tests inside VS

[vitek-karas]

Would it be possible to rewrite the test to not use private reflection?
The test is valid regardless of how the builder is implemented, but now it would fail if we changed the implementation (to rename/remove the internal property).
I think it should be possible to call something like AssemblyLoadContext.Default.Assemblies and find the one just defined and validate it there.

[pedrobsaila]

thanks for the clue

[vitek-karas]

Suggested change

	AssemblyBuilder assembly = Helpers.DynamicAssembly();
	AssemblyBuilder assembly = Helpers.DynamicAssembly(nameof(DefineDynamicAssembly_InternalAssemblyLocationIsEmpty));

Otherwise the assembly will get a default name TestAssembly and the test might succeed by accident if some other test uses the same assembly name. (Unlikely as the test would still likely do the right validation, but still)

[eerhardt]

Suggested change

	.FirstOrDefault(_ => _.FullName == assembly.FullName);
	.FirstOrDefault(a => a.FullName == assembly.FullName);

Don't use _ for a variable name if you are actually going to use the variable. _ is used when you want to ignore the variable/return value.

[vitek-karas]

@lambdageek Could you please advice about the behavior expected in Mono? It seems that defining a dynamic assembly doesn't add that assembly to the AssemblyLoadContext.Assemblies collection.

On top of that I expect the Mono's version to throw from the Location property - but that should be easily fixable as part of this change.

[lambdageek]

@lambdageek Could you please advice about the behavior expected in Mono?

It seems that defining a dynamic assembly doesn't add that assembly to the AssemblyLoadContext.Assemblies collection.

Seems like a bug in Mono's AssemblyLoadContext.GetLoadContext - a mono AssemblyBuilder doesn't derive from RuntimeAssembly.

runtime/src/mono/System.Private.CoreLib/src/System/Runtime/Loader/AssemblyLoadContext.Mono.cs

Lines 78 to 88 in 6acb72b

	RuntimeAssembly? rtAsm = assembly as RuntimeAssembly;
	// We only support looking up load context for runtime assemblies.
	if (rtAsm != null)
	{
	RuntimeAssembly runtimeAssembly = rtAsm;
	IntPtr ptrAssemblyLoadContext = GetLoadContextForAssembly(runtimeAssembly);
	loadContextForAssembly = GetAssemblyLoadContext(ptrAssemblyLoadContext);
	}
	return loadContextForAssembly;

On top of that I expect the Mono's version to throw from the Location property - but that should be easily fixable as part of this change.

I think it probably throws from here:

runtime/src/libraries/System.Private.CoreLib/src/System/Reflection/Assembly.cs

Line 92 in 6acb72b

public virtual string Location => throw NotImplemented.ByDesign;

So we'd need a new override in

runtime/src/mono/System.Private.CoreLib/src/System/Reflection/Emit/AssemblyBuilder.Mono.cs

Line 174 in 6acb72b

public sealed partial class AssemblyBuilder : Assembly

[pedrobsaila]

I use now AppDomain.CurrentDomain.GetAssemblies instead of AssemblyLoadContext.Assemblies in my test , it seemed to return the test assembly on my machine for coreclr and mono.

Location should be updated to return string.Empty in src/libraries/System.Private.CoreLib/src/System/Reflection/Emit/AssemblyBuilder.cs because it's the partial class of
/src/mono/System.Private.CoreLib/src/System/Reflection/Emit/AssemblyBuilder.Mono.cs

I'm not so sure of what I did, if there's something to adjust or drop, I would be happy to update my PR.

[vitek-karas]

Thanks a lot for the fix! LGTM