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

[release/9.0] Fix Enum field type bug found when underlying type is set from assembly loaded with MLC #106513

Merged
merged 2 commits into from
Aug 16, 2024

Conversation

github-actions[bot]
Copy link
Contributor

@github-actions github-actions bot commented Aug 15, 2024

Backport of #106375 to release/9.0

/cc @buyaa-n

Customer Impact

  • Customer reported
  • Found internally

This is found while fixing a customer reported bug. When MLC used for setting the core assembly and a type loaded from that assembly used as underlying type when creating EnumBuilder, defining a field for that EnumBuilder would create a field with wrong type (The field type will be generated with underlying type instead of the EnumBuilder type). Plus this PR removes the validation that causing issue for setting constants for such field.

Root cause:

The EnumBuilder.UnderlyingSystemType, currently returns GetEnumUnderlyingType() which returns _underlyingField.FieldType underneath. Because Type.Equals checks UnderlyingSystemType for equality when the EnumBuilder underlying type is set from MLC Core assembly it is causing issue for enum fields defined, for example:

Type intType = mlc.CoreAssembly.GetType("System.Int32");
EnumBuilder enumBuilder = mb.DefineEnum("TestEnum", TypeAttributes.Public, intType);
FieldBuilder field = enumBuilder.DefineLiteral("Default", 0);

In this case the field type should be TestEnum, but it is evaluated to be equal to System.Int32 when writing the field signature on Save. We should not return underlying field type for EnumBuilder.UnderlyingSystemType, it should return the Enum itself instead (runtime enums returns the enum type itself). Though the EnumBuilder.GetEnumUnderlyingType() method should keep returning the underlying field type. Related to #105903

Regression

  • Yes
  • No

It is a bug in a new PersistedAssemblyBuilder added in .NET 9

Testing

A unit test added that reproes the issue

Risk

Low, the fix is straight forward and clean, will not cause a regression

Copy link
Contributor

Tagging subscribers to this area: @dotnet/area-system-reflection-emit
See info in area-owners.md if you want to be subscribed.

@carlossanlop
Copy link
Member

We have a very short window for merging RC1 backports. Please send the email to Tactics requesting approval ASAP.

@buyaa-n buyaa-n added the Servicing-consider Issue for next servicing release review label Aug 16, 2024
@buyaa-n
Copy link
Contributor

buyaa-n commented Aug 16, 2024

We have a very short window for merging RC1 backports. Please send the email to Tactics requesting approval ASAP.

@carlossanlop this is targeting to RC2, do we still need Tactics approval?

@carlossanlop
Copy link
Member

Ah my bad, I didn't notice the target branch.

RC2 (release/9.0) PRs need M2 approval. @artl93 can you please take a look?

Copy link
Member

@artl93 artl93 left a comment

Choose a reason for hiding this comment

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

Approved for me.

@carlossanlop carlossanlop added Servicing-approved Approved for servicing release api-approved API was approved in API review, it can be implemented and removed Servicing-consider Issue for next servicing release review api-approved API was approved in API review, it can be implemented labels Aug 16, 2024
@carlossanlop carlossanlop merged commit e64da60 into release/9.0 Aug 16, 2024
88 of 93 checks passed
@carlossanlop carlossanlop deleted the backport/pr-106375-to-release/9.0 branch August 16, 2024 18:50
@github-actions github-actions bot locked and limited conversation to collaborators Sep 16, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants