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

Fix type name parsing in NativeAOT #79963

Merged
merged 1 commit into from
Dec 25, 2022

Conversation

kant2002
Copy link
Contributor

Simplified type name parsing was breaking if full name or assembly name has underscode ('_') in it. That breaks referencing SQLitePCL.Batteries_V2, SQLitePCLRaw.batteries_v2 type inside Microsoft.Data.Sqlite

Fixes dotnet/efcore#29725

Simplified type name parsing was breaking if full name or assembly name has underscode ('_') in it. That breaks referencing `SQLitePCL.Batteries_V2, SQLitePCLRaw.batteries_v2` type inside `Microsoft.Data.Sqlite`

Fixes dotnet/efcore#29725
@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Dec 25, 2022
@ghost
Copy link

ghost commented Dec 25, 2022

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

Issue Details

Simplified type name parsing was breaking if full name or assembly name has underscode ('_') in it. That breaks referencing SQLitePCL.Batteries_V2, SQLitePCLRaw.batteries_v2 type inside Microsoft.Data.Sqlite

Fixes dotnet/efcore#29725

Author: kant2002
Assignees: -
Labels:

area-NativeAOT-coreclr

Milestone: -

@@ -23,7 +23,7 @@ public static bool ResolveType(string name, ModuleDesc callingModule, TypeSystem
StringBuilder typeName = new StringBuilder();
StringBuilder typeNamespace = new StringBuilder();
string containingTypeName = null;
while (i < name.Length && (char.IsLetterOrDigit(name[i]) || name[i] == '.' || name[i] == '`' || name[i] == '+'))
while (i < name.Length && (char.IsLetterOrDigit(name[i]) || name[i] == '.' || name[i] == '_' || name[i] == '`' || name[i] == '+'))
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure what the style conventions are in the compiler, but you could simplify this with a pattern if that's allowed?

Suggested change
while (i < name.Length && (char.IsLetterOrDigit(name[i]) || name[i] == '.' || name[i] == '_' || name[i] == '`' || name[i] == '+'))
while (i < name.Length && (char.IsLetterOrDigit(name[i]) || name[i] is '.' or '_' or '`' or '+'))

Copy link
Member

Choose a reason for hiding this comment

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

No need to polish this; this file will be nuked from orbit with #72833. This parser is kind of garbage. It's from a time when this being a heuristic was enough, as the comment above says. That time has passed.

Copy link
Member

@MichalStrehovsky MichalStrehovsky left a comment

Choose a reason for hiding this comment

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

Thank you!

@@ -23,7 +23,7 @@ public static bool ResolveType(string name, ModuleDesc callingModule, TypeSystem
StringBuilder typeName = new StringBuilder();
StringBuilder typeNamespace = new StringBuilder();
string containingTypeName = null;
while (i < name.Length && (char.IsLetterOrDigit(name[i]) || name[i] == '.' || name[i] == '`' || name[i] == '+'))
while (i < name.Length && (char.IsLetterOrDigit(name[i]) || name[i] == '.' || name[i] == '_' || name[i] == '`' || name[i] == '+'))
Copy link
Member

Choose a reason for hiding this comment

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

No need to polish this; this file will be nuked from orbit with #72833. This parser is kind of garbage. It's from a time when this being a heuristic was enough, as the comment above says. That time has passed.

@MichalStrehovsky MichalStrehovsky merged commit bd67092 into dotnet:main Dec 25, 2022
@kant2002 kant2002 deleted the kant/fix-sqlite-trimming branch December 27, 2022 10:48
@ghost ghost locked as resolved and limited conversation to collaborators Jan 26, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-NativeAOT-coreclr community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

NativeAOT/trimming compatibility for Microsoft.Data.Sqlite
3 participants