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

Proper type name parser for native AOT compiler #83657

Merged
merged 10 commits into from
Mar 30, 2023

Conversation

jkotas
Copy link
Member

@jkotas jkotas commented Mar 20, 2023

Fixes #72833

@ghost
Copy link

ghost commented Mar 20, 2023

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

Issue Details

Fixes #72833

Depends on #83484

Author: jkotas
Assignees: -
Labels:

area-NativeAOT-coreclr

Milestone: -

@jkotas jkotas added the NO-REVIEW Experimental/testing PR, do NOT review it label Mar 20, 2023
@jkotas jkotas removed the NO-REVIEW Experimental/testing PR, do NOT review it label Mar 25, 2023
@jkotas jkotas marked this pull request as ready for review March 25, 2023 03:53
@jkotas
Copy link
Member Author

jkotas commented Mar 25, 2023

Ready for review

@jkotas
Copy link
Member Author

jkotas commented Mar 25, 2023

/azp run runtime-extra-platforms

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@jkotas jkotas requested a review from vitek-karas March 27, 2023 16:12
@vitek-karas
Copy link
Member

I pushed a change which fixes some of the test failures. There's one test failure remaining:

Basically the equivalent of:

static void Get([DAM] string b) => Type.GetType(b);

Get("TypeName,Invalid/Assembly/Name");
Get("TypeName,NonExistingAssembly");

Both cases above produce IL2105 with these change in NativeAOT, but they don't produce warnings in illink. This is because illink will only produce IL2105 if there's no assembly name. If there is assembly name, but it fails to resolve for whatever reason, illink will simply treat it non-existent type and assume the return value is null.

Given the current codebase in NativeAOT this is not a simple fix, because the information about the presence of assembly name is hidden inside the typename parser, and is not exposed in any way, such that we can use it to suppress producing the warning. I didn't spend much time on this though.
Also not sure it's worth the trouble to fix this - maybe we will simply live with the inconsistency.

@jkotas
Copy link
Member Author

jkotas commented Mar 28, 2023

the information about the presence of assembly name is hidden inside the typename parser, and is not exposed in any way, such that we can use it to suppress producing the warning. I didn't spend much time on this though.

That's not hard to fix. I will take care of it. Thank you!

@jkotas
Copy link
Member Author

jkotas commented Mar 29, 2023

/azp run runtime-extra-platforms

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@jkotas
Copy link
Member Author

jkotas commented Mar 29, 2023

/azp run runtime-extra-platforms

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@jkotas
Copy link
Member Author

jkotas commented Mar 30, 2023

/azp run runtime-extra-platforms

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@jkotas
Copy link
Member Author

jkotas commented Mar 30, 2023

/azp run runtime-extra-platforms

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@jkotas
Copy link
Member Author

jkotas commented Mar 30, 2023

/azp run runtime-extra-platforms

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@jkotas
Copy link
Member Author

jkotas commented Mar 30, 2023

NativeAOT failure is #84112

@jkotas jkotas merged commit c104939 into dotnet:main Mar 30, 2023
@jkotas jkotas deleted the typename-parser-nativeaot branch March 30, 2023 16:16
@ghost ghost locked as resolved and limited conversation to collaborators Apr 29, 2023
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.

Better reflection type name parser for the AOT compiler
3 participants