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

Enable more ILLink test cases with native AOT #110166

Merged
merged 3 commits into from
Dec 3, 2024

Conversation

MichalStrehovsky
Copy link
Member

When I was working on the DAMT.All*, I noticed we don't run many Reflection tests, this is progress towards that.

Also fixes rare CreateInstance overloads - these were never worth the effort to implement honestly, but more code sharing made implementing them a breeze.

Contributes to #82447.

Cc @dotnet/ilc-contrib

When I was working on the `DAMT.All*`, I noticed we don't run many Reflection tests, this is progress towards that.

Also fixes rare `CreateInstance` overloads - these were never worth the effort to implement honestly, but more code sharing made implementing them a breeze.

Contributes to dotnet#82447.
Copy link
Contributor

Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas
See info in area-owners.md if you want to be subscribed.

@MichalStrehovsky
Copy link
Member Author

@sbomer could you have a look please?

Copy link
Member

@sbomer sbomer left a comment

Choose a reason for hiding this comment

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

LGTM aside from my question, thanks!

{
// It's not wrong to have a reference to non-existing type - the code may well expect to get an exception in this case
// Note that we did find the assembly, so it's not a ILLink config problem, it's either intentional, or wrong versions of assemblies
// but ILLink can't know that. In case a user tries to create an array using System.Activator we should simply ignore it, the user
Copy link
Member

Choose a reason for hiding this comment

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

For ILLink this logic has a check for array types - why do we check that for ILLink but not ILC?

Copy link
Member Author

@MichalStrehovsky MichalStrehovsky Dec 3, 2024

Choose a reason for hiding this comment

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

That part traces back to dotnet/linker#1566.

The original logic was actually:

							var typeRef = _context.TypeNameResolver.ResolveTypeName (resolvedAssembly, typeNameStringValue.Contents);
							var resolvedType = typeRef?.Resolve ();
							if (resolvedType == null || typeRef is ArrayType) {

So this compensates for the Cecil quirk where a TypeReference for an array type will "resolve" into System.Array for reasons that I never understood.

What is there today is semantically different, it was lost in some of the refactors:

			if (!_reflectionMarker.TryResolveTypeNameAndMark (resolvedAssembly, typeName, _diagnosticContext, out TypeReference? foundType)
				|| foundType.IsTypeOf (WellKnownType.System_Array)) {

This will let arrays through and instead treats System.Array as nothing.

In the end, I don't think this matters. If we were to delete the array special handling, I don't think it would have any observable effect.

…Suites.cs

Co-authored-by: kasperk81 <83082615+kasperk81@users.noreply.github.com>
@MichalStrehovsky
Copy link
Member Author

/ba-g timeouts in legs that don't exercise these code paths

@MichalStrehovsky MichalStrehovsky merged commit c5213de into dotnet:main Dec 3, 2024
108 of 112 checks passed
@MichalStrehovsky MichalStrehovsky deleted the progress82447 branch December 3, 2024 12:38
eduardo-vp pushed a commit to eduardo-vp/runtime that referenced this pull request Dec 5, 2024
When I was working on the `DAMT.All*`, I noticed we don't run many Reflection tests, this is progress towards that.

Also fixes rare `CreateInstance` overloads - these were never worth the effort to implement honestly, but more code sharing made implementing them a breeze.

Contributes to dotnet#82447.
mikelle-rogers pushed a commit to mikelle-rogers/runtime that referenced this pull request Dec 10, 2024
When I was working on the `DAMT.All*`, I noticed we don't run many Reflection tests, this is progress towards that.

Also fixes rare `CreateInstance` overloads - these were never worth the effort to implement honestly, but more code sharing made implementing them a breeze.

Contributes to dotnet#82447.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants