Skip to content

Conversation

@steveharter
Copy link
Contributor

Fixes #110247

cc @jgh07

We will likely port to v9.x once verified in v10 since this was new functionality in v9.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 2 out of 2 changed files in this pull request and generated no suggestions.

Comments skipped due to low confidence (1)

src/libraries/System.Reflection.Emit/tests/PersistedAssemblyBuilder/AssemblySaveTypeBuilderTests.cs:244

  • The constructor call should use Type.EmptyTypes for clarity and to avoid potential issues with different runtime environments.
il.Emit(OpCodes.Newobj, declaringType.GetConstructor([]));

Copy link
Contributor

@buyaa-n buyaa-n left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

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

@steveharter steveharter merged commit 36fef72 into dotnet:main Dec 19, 2024
81 of 83 checks passed
@steveharter steveharter deleted the Issue_110247 branch December 19, 2024 14:59
@steveharter
Copy link
Contributor Author

/backport to release/9.0-staging

@github-actions
Copy link
Contributor

Started backporting to release/9.0-staging: https://github.com/dotnet/runtime/actions/runs/12415232737

@ericstj
Copy link
Member

ericstj commented Jan 9, 2025

@jkotas @steveharter - do you think we need to add more test cases to cover other permutations of generic usage? Methods / properties / events / parameters, nested generics, etc.

@jkotas
Copy link
Member

jkotas commented Jan 9, 2025

It is a lot of potentially interesting permutations. The chances of more broken corner-cases being found by manual test authoring are low. I think that the best way to find more broken corner-cases is roundtripping of the existing test asserts through ref.emit and validating that the roundtripped tests still pass. Buya has done some of it, but it was not completed and automated.

@buyaa-n
Copy link
Contributor

buyaa-n commented Jan 13, 2025

do you think we need to add more test cases to cover other permutations of generic usage? Methods / properties / events / parameters, nested generics, etc.

I added as much test as I can that covers many permutations of generic usage and nested generics for methods and its parameter. Agree with Jan that to find more corner cases round tripping tool would have been helpful. As mentioned the tool was used locally, not added into the repo and now that project is gone with my old laptop. Someone could rewrite it using the .NET framework tool.

We don't have tests that calling generic properties / events in IL, because by my understanding properties / events are not called directly in IL, at least not with ref emit APIs, instead their get/set/add/remove methods are called/used in IL, and those are covered by methods tests.

@github-actions github-actions bot locked and limited conversation to collaborators Feb 13, 2025
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.

PersistedAssemblyBuilder generates invalid IL when loading fields with generic type parameters

4 participants