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

Conversation

buyaa-n
Copy link
Contributor

@buyaa-n buyaa-n commented Aug 1, 2024

It was considered useful to add tests for the following cases:

  • Test for special characters in member and class names. ECMA-335 doesn't provide rules here, unlike C# for example, but we should check for special characters and strings such as "*", "!", "@", "_", "__", "0x42", "\42" (Unicode escaping), and embedded quotes and nulls. We shouldn't have cases where the input is valid, but the encoding of that fails or is misrepresented.
  • Tests for high length counts in member and class names. ECMA-335 doesn't provide rules here, unlike C# for example, but we should verify that long names work as expected.
  • Test for class and member name collisions; adding the same property more than once for example.
  • Validate ECMA-335 "II.24.2.6 #~ stream" cases when it switches from a 2 byte index to a 4 byte index (65536 methods for example). See also Compiler can silently generate a bad image when there's 0xFFFF parameters in the module #94892

Fixes #103086

@@ -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.

Copy link
Member

@steveharter steveharter left a comment

Choose a reason for hiding this comment

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

Some misc questions, but otherwise LGTM

@steveharter steveharter merged commit 8e1904c into dotnet:main Aug 7, 2024
145 of 148 checks passed
@steveharter
Copy link
Member

Buyaa out, merged.

@buyaa-n buyaa-n deleted the add-tests branch August 12, 2024 02:51
@github-actions github-actions bot locked and limited conversation to collaborators Sep 11, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add PersistedAssemblyBuilder tests to cover input and boundary cases
3 participants