Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add/update AssemblyBuilder/PersistedAssemblyBuilder tests covering boundary / edge case scenarios #105782

Merged
merged 5 commits into from
Aug 7, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ internal PropertyBuilderImpl(string name, PropertyAttributes attributes, Calling
_name = name;
_attributes = attributes;
_callingConvention = callingConvention;
_propertyType = returnType;
_propertyType = returnType ?? containingType.GetModuleBuilder().GetTypeFromCoreAssembly(CoreTypeId.Void);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Existing AssemblyBuilder adds void type for null return type when populating the signature, now this scenario causing NRE in the SignatureHelper, I could fix it in the signature only, but to avoid NRE's for other case setting the return type to void here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The constructor parameter, Type returnType isn't marked as nullable. If a null value is sneaking through, we should fix that or mark the parameter as nullable.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, allowing null might was not intentional, but making it throw for null now probably will be breaking change. We might also want to move the null handling from SignatureHelper to RuntimePropertyBuilder

public static SignatureHelper GetPropertySigHelper(Module? mod, CallingConventions callingConvention,
Type? returnType, Type[]? requiredReturnTypeCustomModifiers, Type[]? optionalReturnTypeCustomModifiers,
Type[]? parameterTypes, Type[][]? requiredParameterTypeCustomModifiers, Type[][]? optionalParameterTypeCustomModifiers)
{
SignatureHelper sigHelp;
returnType ??= typeof(void);

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, that makes sense. I don't think we should throw either, but I do like to have the nullable indicators correct so we can check these things.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Existing AssemblyBuilder adds void type for null return type when populating the signature,

I assume this behavior exists in previous versions as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, RuntimePropertyBuilder constructor was allowing null, but not handling null which could cause NRE further if SetConstant called for the property, the null has been resolved only for GetPropertySigHelper. I see same behavior in .NET framework.

Now with this PR the null return type will be handled to void type in the RuntimePropertyBuilder constructor, same as in GetPropertySigHelper.

_parameterTypes = parameterTypes;
_containingType = containingType;
_returnTypeRequiredCustomModifiers = returnTypeRequiredCustomModifiers;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
// The .NET Foundation licenses this file to you under the MIT license.

using System.Collections.Generic;
using System.Runtime.InteropServices;
using System.IO;
using Xunit;

namespace System.Reflection.Emit.Tests
Expand All @@ -18,13 +18,13 @@ public static IEnumerable<object[]> DefineEnum_TestData()
yield return new object[] { "a\0b\0c", TypeAttributes.Public, typeof(int) };
yield return new object[] { "Name", TypeAttributes.Public, typeof(uint) };
yield return new object[] { "Name", TypeAttributes.Public, typeof(long) };
yield return new object[] { "Name", TypeAttributes.Public, typeof(char) };
yield return new object[] { "Name", TypeAttributes.Public, typeof(bool) };
yield return new object[] { "Name", TypeAttributes.Public, typeof(ulong) };
yield return new object[] { "N%ame", TypeAttributes.Public, typeof(char) };
yield return new object[] { "N`ame", TypeAttributes.Public, typeof(bool) };
yield return new object[] { "N'ame", TypeAttributes.Public, typeof(ulong) };
yield return new object[] { "Name", TypeAttributes.Public, typeof(float) };
yield return new object[] { "Name", TypeAttributes.Public, typeof(double) };
yield return new object[] { "Name", TypeAttributes.Public, typeof(IntPtr) };
yield return new object[] { "Name", TypeAttributes.Public, typeof(UIntPtr) };
yield return new object[] { "Nam<e>", TypeAttributes.Public, typeof(double) };
yield return new object[] { "Nam~e", TypeAttributes.Public, typeof(IntPtr) };
yield return new object[] { "\rName", TypeAttributes.Public, typeof(UIntPtr) };
yield return new object[] { "Name", TypeAttributes.Public, typeof(Int32Enum) };
}

Expand Down Expand Up @@ -76,6 +76,45 @@ public void DefineEnum(string name, TypeAttributes visibility, Type underlyingTy
Assert.Equal(FieldAttributes.Public | FieldAttributes.SpecialName | FieldAttributes.RTSpecialName, createdUnderlyingField.Attributes);
}

[Theory]
[MemberData(nameof(DefineEnum_TestData))]
public void DefineEnumPersistedAssembly(string name, TypeAttributes visibility, Type underlyingType)
{
PersistedAssemblyBuilder ab = AssemblySaveTools.PopulateAssemblyBuilder(new AssemblyName("MyAssembly"));
ModuleBuilder module = ab.DefineDynamicModule("MyModule");
EnumBuilder enumBuilder = module.DefineEnum(name, visibility, underlyingType);
enumBuilder.CreateType();

Assert.True(enumBuilder.IsEnum);
Assert.Equal(module.Assembly, enumBuilder.Assembly);
Assert.Equal(module, enumBuilder.Module);
Assert.Equal(name, enumBuilder.Name);
Assert.Equal(Helpers.GetFullName(name), enumBuilder.FullName);
Assert.Equal(typeof(Enum), enumBuilder.BaseType);
Assert.Null(enumBuilder.DeclaringType);
Assert.Equal(visibility | TypeAttributes.Sealed, enumBuilder.Attributes);
Assert.Equal("value__", enumBuilder.UnderlyingField.Name);
Assert.Equal(underlyingType, enumBuilder.UnderlyingField.FieldType);
Assert.Equal(FieldAttributes.Public | FieldAttributes.SpecialName, enumBuilder.UnderlyingField.Attributes);

using (var stream = new MemoryStream())
using (MetadataLoadContext mlc = new MetadataLoadContext(new CoreMetadataAssemblyResolver()))
{
ab.Save(stream);
Assembly assemblyFromStream = mlc.LoadFromStream(stream);
Type createdEnum = assemblyFromStream.GetType(name);
if (createdEnum != null) // null when name = "a\0b\0c"
{
Assert.True(createdEnum.IsEnum);
Assert.Equal(Helpers.GetFullName(name), createdEnum.Name);
Assert.Equal(Helpers.GetFullName(name), enumBuilder.FullName);
Assert.Equal(typeof(Enum).FullName, createdEnum.BaseType.FullName);
Assert.Null(createdEnum.DeclaringType);
Assert.Equal(visibility | TypeAttributes.Sealed, createdEnum.Attributes);
}
}
}

[Fact]
[ActiveIssue("https://github.com/dotnet/runtime/issues/2389", TestRuntimes.Mono)]
public void DefineEnum_DynamicUnderlyingType_Works()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
using System.Collections.Generic;
using Xunit;
using System.Diagnostics.CodeAnalysis;
using System.IO;

namespace System.Reflection.Emit.Tests
{
Expand All @@ -12,7 +13,7 @@ public class ModuleBuilderDefineType
{
public static IEnumerable<object[]> TestData()
{
foreach (string name in new string[] { "TestName", "testname", "class", "\uD800\uDC00" })
foreach (string name in new string[] { "TestName", "testname", "class", "\uD800\uDC00", "432" })
{
foreach (TypeAttributes attributes in new TypeAttributes[] { TypeAttributes.NotPublic, TypeAttributes.Interface | TypeAttributes.Abstract, TypeAttributes.Class })
{
Expand Down Expand Up @@ -94,6 +95,153 @@ void Verify(TypeBuilder type, Module module)
}
}

[Theory]
[MemberData(nameof(TestData))]
public void DefineTypePersistedAssembly(string name, TypeAttributes attributes, Type parent, PackingSize packingSize, int typesize, Type[] implementedInterfaces)
{
bool isDefaultImplementedInterfaces = implementedInterfaces?.Length == 0;
bool isDefaultPackingSize = packingSize == PackingSize.Unspecified;
bool isDefaultSize = typesize == 0;
bool isDefaultParent = parent == null;
bool isDefaultAttributes = attributes == TypeAttributes.NotPublic;
Type baseType = attributes.HasFlag(TypeAttributes.Abstract) && parent == null ? null : (parent ?? typeof(object));

void VerifyTypeBuilder(TypeBuilder type, Module module)
{
Assert.Equal(module, type.Module);
Assert.Equal(module.Assembly, type.Assembly);
Assert.Equal(name, type.Name);
Assert.Equal(Helpers.GetFullName(name), type.FullName);
Assert.Equal(attributes, type.Attributes);
Assert.Equal(baseType, type.BaseType);
Assert.Equal(typesize, type.Size);
Assert.Equal(packingSize, type.PackingSize);
Assert.Equal(implementedInterfaces ?? new Type[0], type.GetInterfaces());
}

void VerifyType(Type createdType)
{
Assert.Equal(name, createdType.Name);
Assert.Equal(attributes, createdType.Attributes);
Assert.Equal(Helpers.GetFullName(name), createdType.FullName);
Assert.Equal(attributes, createdType.Attributes);
if (baseType != null)
{
Assert.Equal(baseType.Name, createdType.BaseType.Name);
}
Assert.Equal((implementedInterfaces ?? new Type[0]).Length, createdType.GetInterfaces().Length);
}

PersistedAssemblyBuilder ab;
TypeBuilder typeBuilder;
if (isDefaultImplementedInterfaces)
{
if (isDefaultSize && isDefaultPackingSize)
{
if (isDefaultParent)
{
if (isDefaultAttributes)
{
// Use DefineType(string)
ab = AssemblySaveTools.PopulateAssemblyAndModule(out ModuleBuilder module1);
typeBuilder = module1.DefineType(name);
typeBuilder.CreateType();
VerifyTypeBuilder(typeBuilder, module1);

using (var stream = new MemoryStream())
using (MetadataLoadContext mlc = new MetadataLoadContext(new CoreMetadataAssemblyResolver()))
{
ab.Save(stream);
VerifyType(mlc.LoadFromStream(stream).GetType(name));
}
}
// Use DefineType(string, TypeAttributes)
ab = AssemblySaveTools.PopulateAssemblyAndModule(out ModuleBuilder module2);
typeBuilder = module2.DefineType(name, attributes);
typeBuilder.CreateType();
VerifyTypeBuilder(typeBuilder, module2);

using (var stream = new MemoryStream())
using (MetadataLoadContext mlc = new MetadataLoadContext(new CoreMetadataAssemblyResolver()))
{
ab.Save(stream);
VerifyType(mlc.LoadFromStream(stream).GetType(name));
}
}
// Use DefineType(string, TypeAttributes, Type)
ab = AssemblySaveTools.PopulateAssemblyAndModule(out ModuleBuilder module3);
typeBuilder = module3.DefineType(name, attributes, parent);
typeBuilder.CreateType();
VerifyTypeBuilder(typeBuilder, module3);

using (var stream = new MemoryStream())
using (MetadataLoadContext mlc = new MetadataLoadContext(new CoreMetadataAssemblyResolver()))
{
ab.Save(stream);
VerifyType(mlc.LoadFromStream(stream).GetType(name));
}
}
else if (isDefaultSize)
{
// Use DefineType(string, TypeAttributes, Type, PackingSize)
ab = AssemblySaveTools.PopulateAssemblyAndModule(out ModuleBuilder module4);
typeBuilder = module4.DefineType(name, attributes, parent, packingSize);
typeBuilder.CreateType();
VerifyTypeBuilder(typeBuilder, module4);

using (var stream = new MemoryStream())
using (MetadataLoadContext mlc = new MetadataLoadContext(new CoreMetadataAssemblyResolver()))
{
ab.Save(stream);
VerifyType(mlc.LoadFromStream(stream).GetType(name));
}
}
else if (isDefaultPackingSize)
{
// Use DefineType(string, TypeAttributes, Type, int)
ab = AssemblySaveTools.PopulateAssemblyAndModule(out ModuleBuilder module5);
typeBuilder = module5.DefineType(name, attributes, parent, typesize);
typeBuilder.CreateType();
VerifyTypeBuilder(typeBuilder, module5);

using (var stream = new MemoryStream())
using (MetadataLoadContext mlc = new MetadataLoadContext(new CoreMetadataAssemblyResolver()))
{
ab.Save(stream);
VerifyType(mlc.LoadFromStream(stream).GetType(name));
}
}
// Use DefineType(string, TypeAttributes, Type, PackingSize, int)
ab = AssemblySaveTools.PopulateAssemblyAndModule(out ModuleBuilder module6);
typeBuilder = module6.DefineType(name, attributes, parent, packingSize, typesize);
typeBuilder.CreateType();
VerifyTypeBuilder(typeBuilder, module6);

using (var stream = new MemoryStream())
using (MetadataLoadContext mlc = new MetadataLoadContext(new CoreMetadataAssemblyResolver()))
{
ab.Save(stream);
VerifyType(mlc.LoadFromStream(stream).GetType(name));
}
}
else
{
// Use DefineType(string, TypeAttributes, Type, Type[])
Assert.True(isDefaultSize && isDefaultPackingSize); // Sanity check
ab = AssemblySaveTools.PopulateAssemblyAndModule(out ModuleBuilder module7);
typeBuilder = module7.DefineType(name, attributes, parent, implementedInterfaces);
typeBuilder.CreateType();
VerifyTypeBuilder(typeBuilder, module7);

using (var stream = new MemoryStream())
using (MetadataLoadContext mlc = new MetadataLoadContext(new CoreMetadataAssemblyResolver()))
{
ab.Save(stream);
VerifyType(mlc.LoadFromStream(stream).GetType(name));
}
}
}

[Fact]
public void DefineType_String_TypeAttributes_Type_TypeCreatedInModule()
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,13 @@ internal static PersistedAssemblyBuilder PopulateAssemblyBuilderAndTypeBuilder(o
return ab;
}

internal static PersistedAssemblyBuilder PopulateAssemblyAndModule(out ModuleBuilder moduleBuilder)
{
PersistedAssemblyBuilder ab = PopulateAssemblyBuilder(s_assemblyName, null);
moduleBuilder = ab.DefineDynamicModule("MyModule");
return ab;
}

internal static PersistedAssemblyBuilder PopulateAssemblyBuilder(AssemblyName assemblyName, List<CustomAttributeBuilder>? assemblyAttributes = null) =>
new PersistedAssemblyBuilder(assemblyName, CoreMetadataAssemblyResolver.s_coreAssembly, assemblyAttributes);

Expand Down
Loading
Loading