Skip to content

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

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.

Sign up for GitHub

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

Merged
merged 5 commits into from
Aug 7, 2024
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
dcb8193
Add AssemblyBuilder tests covering boundary / edge case scenarios
buyaa-n Aug 1, 2024
55a9650
Make returnType parameter nullable, add test conditions for browser
buyaa-n Aug 1, 2024
d96b3ca
Merge branch 'main' of github.com:dotnet/runtime into add-tests
buyaa-n Aug 1, 2024
52fd746
Update returnType nullability on Mono
buyaa-n Aug 1, 2024
26f0bf1
Disable some tests on Mono
buyaa-n Aug 1, 2024
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
4 changes: 2 additions & 2 deletions src/coreclr/System.Private.CoreLib/src/System/Reflection/Emit/RuntimePropertyBuilder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ internal RuntimePropertyBuilder(
RuntimeModuleBuilder mod, // the module containing this PropertyBuilder
string name, // property name
PropertyAttributes attr, // property attribute such as DefaultProperty, Bindable, DisplayBind, etc
Type returnType, // return type of the property.
Type? returnType, // return type of the property.
int prToken, // the metadata token for this property
RuntimeTypeBuilder containingType) // the containing type
{
Expand All @@ -40,7 +40,7 @@ internal RuntimePropertyBuilder(
m_name = name;
m_moduleBuilder = mod;
m_attributes = attr;
m_returnType = returnType;
m_returnType = returnType ?? typeof(void);
m_tkProperty = prToken;
m_containingType = containingType;
}
Expand Down
2 changes: 1 addition & 1 deletion src/coreclr/System.Private.CoreLib/src/System/Reflection/Emit/RuntimeTypeBuilder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1388,7 +1388,7 @@ protected override FieldBuilder DefineUninitializedDataCore(string name, int siz
#region Define Properties and Events

protected override PropertyBuilder DefinePropertyCore(string name, PropertyAttributes attributes, CallingConventions callingConvention,
Type returnType, Type[]? returnTypeRequiredCustomModifiers, Type[]? returnTypeOptionalCustomModifiers,
Type? returnType, Type[]? returnTypeRequiredCustomModifiers, Type[]? returnTypeOptionalCustomModifiers,
Type[]? parameterTypes, Type[][]? parameterTypeRequiredCustomModifiers, Type[][]? parameterTypeOptionalCustomModifiers)
{
lock (SyncRoot)
Expand Down
10 changes: 5 additions & 5 deletions src/libraries/System.Private.CoreLib/src/System/Reflection/Emit/TypeBuilder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -225,22 +225,22 @@ protected abstract MethodBuilder DefinePInvokeMethodCore(string name, string dll
Type[]? parameterTypes, Type[][]? parameterTypeRequiredCustomModifiers, Type[][]? parameterTypeOptionalCustomModifiers,
CallingConvention nativeCallConv, CharSet nativeCharSet);

public PropertyBuilder DefineProperty(string name, PropertyAttributes attributes, Type returnType, Type[]? parameterTypes)
public PropertyBuilder DefineProperty(string name, PropertyAttributes attributes, Type? returnType, Type[]? parameterTypes)
=> DefineProperty(name, attributes, returnType, null, null, parameterTypes, null, null);

public PropertyBuilder DefineProperty(string name, PropertyAttributes attributes,
CallingConventions callingConvention, Type returnType, Type[]? parameterTypes)
CallingConventions callingConvention, Type? returnType, Type[]? parameterTypes)
=> DefineProperty(name, attributes, callingConvention, returnType, null, null, parameterTypes, null, null);

public PropertyBuilder DefineProperty(string name, PropertyAttributes attributes,
Type returnType, Type[]? returnTypeRequiredCustomModifiers, Type[]? returnTypeOptionalCustomModifiers,
Type? returnType, Type[]? returnTypeRequiredCustomModifiers, Type[]? returnTypeOptionalCustomModifiers,
Type[]? parameterTypes, Type[][]? parameterTypeRequiredCustomModifiers, Type[][]? parameterTypeOptionalCustomModifiers)
=> DefineProperty(name, attributes, default,
returnType, returnTypeRequiredCustomModifiers, returnTypeOptionalCustomModifiers,
parameterTypes, parameterTypeRequiredCustomModifiers, parameterTypeOptionalCustomModifiers);

public PropertyBuilder DefineProperty(string name, PropertyAttributes attributes, CallingConventions callingConvention,
Type returnType, Type[]? returnTypeRequiredCustomModifiers, Type[]? returnTypeOptionalCustomModifiers,
Type? returnType, Type[]? returnTypeRequiredCustomModifiers, Type[]? returnTypeOptionalCustomModifiers,
Type[]? parameterTypes, Type[][]? parameterTypeRequiredCustomModifiers, Type[][]? parameterTypeOptionalCustomModifiers)
{
ArgumentException.ThrowIfNullOrEmpty(name);
Expand All @@ -251,7 +251,7 @@ public PropertyBuilder DefineProperty(string name, PropertyAttributes attributes
}

protected abstract PropertyBuilder DefinePropertyCore(string name, PropertyAttributes attributes, CallingConventions callingConvention,
Type returnType, Type[]? returnTypeRequiredCustomModifiers, Type[]? returnTypeOptionalCustomModifiers,
Type? returnType, Type[]? returnTypeRequiredCustomModifiers, Type[]? returnTypeOptionalCustomModifiers,
Type[]? parameterTypes, Type[][]? parameterTypeRequiredCustomModifiers, Type[][]? parameterTypeOptionalCustomModifiers);

public ConstructorBuilder DefineTypeInitializer()
Expand Down
10 changes: 5 additions & 5 deletions src/libraries/System.Reflection.Emit/ref/System.Reflection.Emit.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -608,11 +608,11 @@ public void DefineMethodOverride(System.Reflection.MethodInfo methodInfoBody, Sy
public System.Reflection.Emit.MethodBuilder DefinePInvokeMethod(string name, string dllName, string entryName, System.Reflection.MethodAttributes attributes, System.Reflection.CallingConventions callingConvention, System.Type? returnType, System.Type[]? returnTypeRequiredCustomModifiers, System.Type[]? returnTypeOptionalCustomModifiers, System.Type[]? parameterTypes, System.Type[][]? parameterTypeRequiredCustomModifiers, System.Type[][]? parameterTypeOptionalCustomModifiers, System.Runtime.InteropServices.CallingConvention nativeCallConv, System.Runtime.InteropServices.CharSet nativeCharSet) { throw null; }
[System.Diagnostics.CodeAnalysis.RequiresUnreferencedCodeAttribute("P/Invoke marshalling may dynamically access members that could be trimmed.")]
protected abstract System.Reflection.Emit.MethodBuilder DefinePInvokeMethodCore(string name, string dllName, string entryName, System.Reflection.MethodAttributes attributes, System.Reflection.CallingConventions callingConvention, System.Type? returnType, System.Type[]? returnTypeRequiredCustomModifiers, System.Type[]? returnTypeOptionalCustomModifiers, System.Type[]? parameterTypes, System.Type[][]? parameterTypeRequiredCustomModifiers, System.Type[][]? parameterTypeOptionalCustomModifiers, System.Runtime.InteropServices.CallingConvention nativeCallConv, System.Runtime.InteropServices.CharSet nativeCharSet);
public System.Reflection.Emit.PropertyBuilder DefineProperty(string name, System.Reflection.PropertyAttributes attributes, System.Reflection.CallingConventions callingConvention, System.Type returnType, System.Type[]? parameterTypes) { throw null; }
public System.Reflection.Emit.PropertyBuilder DefineProperty(string name, System.Reflection.PropertyAttributes attributes, System.Reflection.CallingConventions callingConvention, System.Type returnType, System.Type[]? returnTypeRequiredCustomModifiers, System.Type[]? returnTypeOptionalCustomModifiers, System.Type[]? parameterTypes, System.Type[][]? parameterTypeRequiredCustomModifiers, System.Type[][]? parameterTypeOptionalCustomModifiers) { throw null; }
public System.Reflection.Emit.PropertyBuilder DefineProperty(string name, System.Reflection.PropertyAttributes attributes, System.Type returnType, System.Type[]? parameterTypes) { throw null; }
public System.Reflection.Emit.PropertyBuilder DefineProperty(string name, System.Reflection.PropertyAttributes attributes, System.Type returnType, System.Type[]? returnTypeRequiredCustomModifiers, System.Type[]? returnTypeOptionalCustomModifiers, System.Type[]? parameterTypes, System.Type[][]? parameterTypeRequiredCustomModifiers, System.Type[][]? parameterTypeOptionalCustomModifiers) { throw null; }
protected abstract System.Reflection.Emit.PropertyBuilder DefinePropertyCore(string name, System.Reflection.PropertyAttributes attributes, System.Reflection.CallingConventions callingConvention, System.Type returnType, System.Type[]? returnTypeRequiredCustomModifiers, System.Type[]? returnTypeOptionalCustomModifiers, System.Type[]? parameterTypes, System.Type[][]? parameterTypeRequiredCustomModifiers, System.Type[][]? parameterTypeOptionalCustomModifiers);
public System.Reflection.Emit.PropertyBuilder DefineProperty(string name, System.Reflection.PropertyAttributes attributes, System.Reflection.CallingConventions callingConvention, System.Type? returnType, System.Type[]? parameterTypes) { throw null; }
public System.Reflection.Emit.PropertyBuilder DefineProperty(string name, System.Reflection.PropertyAttributes attributes, System.Reflection.CallingConventions callingConvention, System.Type? returnType, System.Type[]? returnTypeRequiredCustomModifiers, System.Type[]? returnTypeOptionalCustomModifiers, System.Type[]? parameterTypes, System.Type[][]? parameterTypeRequiredCustomModifiers, System.Type[][]? parameterTypeOptionalCustomModifiers) { throw null; }
public System.Reflection.Emit.PropertyBuilder DefineProperty(string name, System.Reflection.PropertyAttributes attributes, System.Type? returnType, System.Type[]? parameterTypes) { throw null; }
public System.Reflection.Emit.PropertyBuilder DefineProperty(string name, System.Reflection.PropertyAttributes attributes, System.Type? returnType, System.Type[]? returnTypeRequiredCustomModifiers, System.Type[]? returnTypeOptionalCustomModifiers, System.Type[]? parameterTypes, System.Type[][]? parameterTypeRequiredCustomModifiers, System.Type[][]? parameterTypeOptionalCustomModifiers) { throw null; }
protected abstract System.Reflection.Emit.PropertyBuilder DefinePropertyCore(string name, System.Reflection.PropertyAttributes attributes, System.Reflection.CallingConventions callingConvention, System.Type? returnType, System.Type[]? returnTypeRequiredCustomModifiers, System.Type[]? returnTypeOptionalCustomModifiers, System.Type[]? parameterTypes, System.Type[][]? parameterTypeRequiredCustomModifiers, System.Type[][]? parameterTypeOptionalCustomModifiers);
public System.Reflection.Emit.ConstructorBuilder DefineTypeInitializer() { throw null; }
protected abstract System.Reflection.Emit.ConstructorBuilder DefineTypeInitializerCore();
public System.Reflection.Emit.FieldBuilder DefineUninitializedData(string name, int size, System.Reflection.FieldAttributes attributes) { throw null; }
Expand Down
4 changes: 2 additions & 2 deletions src/libraries/System.Reflection.Emit/src/System/Reflection/Emit/PropertyBuilderImpl.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,14 +26,14 @@ internal sealed class PropertyBuilderImpl : PropertyBuilder
internal List<CustomAttributeWrapper>? _customAttributes;
internal object? _defaultValue = DBNull.Value;

internal PropertyBuilderImpl(string name, PropertyAttributes attributes, CallingConventions callingConvention, Type returnType, Type[]? returnTypeRequiredCustomModifiers, Type[]? returnTypeOptionalCustomModifiers, Type[]? parameterTypes, Type[][]? parameterTypeRequiredCustomModifiers, Type[][]? parameterTypeOptionalCustomModifiers, TypeBuilderImpl containingType)
internal PropertyBuilderImpl(string name, PropertyAttributes attributes, CallingConventions callingConvention, Type? returnType, Type[]? returnTypeRequiredCustomModifiers, Type[]? returnTypeOptionalCustomModifiers, Type[]? parameterTypes, Type[][]? parameterTypeRequiredCustomModifiers, Type[][]? parameterTypeOptionalCustomModifiers, TypeBuilderImpl containingType)
{
ArgumentException.ThrowIfNullOrEmpty(name);

_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

runtime/src/coreclr/System.Private.CoreLib/src/System/Reflection/Emit/SignatureHelper.cs

Lines 144 to 150 in 9fe8c95

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
Contributor

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
2 changes: 1 addition & 1 deletion src/libraries/System.Reflection.Emit/src/System/Reflection/Emit/TypeBuilderImpl.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -408,7 +408,7 @@ protected override MethodBuilder DefinePInvokeMethodCore(string name, string dll
}

protected override PropertyBuilder DefinePropertyCore(string name, PropertyAttributes attributes, CallingConventions callingConvention,
Type returnType, Type[]? returnTypeRequiredCustomModifiers, Type[]? returnTypeOptionalCustomModifiers, Type[]? parameterTypes,
Type? returnType, Type[]? returnTypeRequiredCustomModifiers, Type[]? returnTypeOptionalCustomModifiers, Type[]? parameterTypes,
Type[][]? parameterTypeRequiredCustomModifiers, Type[][]? parameterTypeOptionalCustomModifiers)
{
PropertyBuilderImpl property = new PropertyBuilderImpl(name, attributes, callingConvention, returnType, returnTypeRequiredCustomModifiers,
Expand Down
53 changes: 46 additions & 7 deletions src/libraries/System.Reflection.Emit/tests/ModuleBuilder/ModuleBuilderDefineEnum.cs
View file
Open in desktop
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);
}

[ConditionalTheory(typeof(PlatformDetection), nameof(PlatformDetection.IsNotBrowser))]
[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
Loading