TypeName parsing API #100094
Merged
TypeName parsing API #100094
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- allow ignoring errors (return null) - assembly name parsing
- report errorIndex in the ex message - fix Full Framework build - fix nested types support - implement NativeAOT part
… methods, move "allowFullyQualifiedName" to Options bag
…me and they are consistent with Sytem.Type APIs
… in test project and cover with tests, fix edge case bugs
…e API is complete
jkotas
reviewed
Apr 24, 2024
src/libraries/Common/src/System/Reflection/Metadata/TypeNameParserHelpers.cs
Outdated
Show resolved
Hide resolved
jkotas
reviewed
Apr 24, 2024
| { | ||
| Debug.Assert(genericArgs.Length > 0); | ||
|
|
||
| ValueStringBuilder result = new(stackalloc char[128]); |
There was a problem hiding this comment.
This can be made more efficient by creating one top level ValueStringBuilder instead of creating ValueStringBuilder at each level of the generic type recursion.
There was a problem hiding this comment.
You are right. But after I've addressed #100094 (comment) FullName is very unlikely to be used for generic types. Since it should be rare I am not going to implement it for now.
jkotas
reviewed
Apr 24, 2024
src/libraries/Common/src/System/Reflection/Metadata/AssemblyNameInfo.cs
Outdated
Show resolved
Hide resolved
marek-safar
reviewed
Apr 24, 2024
src/libraries/Common/src/System/Reflection/AssemblyNameParser.cs
Outdated
Show resolved
Hide resolved
- don't throw TypeLoadException in the parser, let loader do that - add Debug.Assert(typeName.IsSimple) for clarity - always include invalid character index in the error message - remove resources added to CoreLib (they are never going to be used) - use [DoesNotReturn] and implement proper throw helper pattern - remove outdated comment about name validation - AssemblyNameInfo.Flags should return the exact value provided in ctor - don't allocate FullName until it's really needed - refactor Make and Resovle methods into one method - fix the test: specify namespace to make sure the type loader loads System.Int32 and later throws TypeLoadException for byref to byref
Co-authored-by: Jan Kotas <jkotas@microsoft.com>
This was referenced Apr 24, 2024
Open
|
/backport to release/9.0-preview4 |
|
Started backporting to release/9.0-preview4: https://github.com/dotnet/runtime/actions/runs/8823414508 |
|
@jeffhandley backporting to release/9.0-preview4 failed, the patch most likely resulted in conflicts: $ git am --3way --ignore-whitespace --keep-non-patch changes.patch
Applying: TypeNameParser first ref and configurable input validation
Applying: assembly name parsing
Applying: initial generic type info parsing
Applying: decorator parsing
Applying: Handle single dimensional array indexing
Applying: implement TypeName.GetType
Applying: nested types support
Applying: support ignore case
Applying: integrate with System.Private.CoreLib:
Using index info to reconstruct a base tree...
M src/coreclr/System.Private.CoreLib/System.Private.CoreLib.csproj
M src/libraries/System.Private.CoreLib/src/System.Private.CoreLib.Shared.projitems
Falling back to patching base and 3-way merge...
Removing src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/TypeNameParser.cs
Auto-merging src/libraries/System.Private.CoreLib/src/System.Private.CoreLib.Shared.projitems
Auto-merging src/coreclr/System.Private.CoreLib/System.Private.CoreLib.csproj
Applying: integrate with System.Private.CoreLib:
Using index info to reconstruct a base tree...
M src/coreclr/System.Private.CoreLib/System.Private.CoreLib.csproj
M src/libraries/System.Private.CoreLib/src/System.Private.CoreLib.Shared.projitems
Falling back to patching base and 3-way merge...
Auto-merging src/libraries/System.Private.CoreLib/src/System.Private.CoreLib.Shared.projitems
Removing src/coreclr/System.Private.CoreLib/src/System/Reflection/TypeNameResolver.cs
Auto-merging src/coreclr/System.Private.CoreLib/System.Private.CoreLib.csproj
Applying: integrate with System.Private.CoreLib for Mono and clr tools, improve perf
Applying: build fix 1/n
Applying: make TypeNameParser internal, extend TypeName with Parse and TryParse methods, move "allowFullyQualifiedName" to Options bag
Applying: introduce TotalComplexity
Applying: introduce FullName, so we have Name, FullName and AssemblyQualifiedName and they are consistent with Sytem.Type APIs
Applying: back tick error handling
Applying: move helper methods to a standalone helper type, include it as a link in test project and cover with tests, fix edge case bugs
Applying: increase test coverage, improve edge case handling
Applying: sample SerializationBinder that uses the new APIs
Applying: cover more serialization binder scenarios with the tests to ensure the API is complete
Applying: strict mode parsing: type names
Applying: strict mode parsing: assembly names
Applying: fix the build and apply some design changes
Applying: add escaping support
Applying: fix the last failing tests, increase test coverage, fix the perf, fix the build
Applying: apply changes based on the 1st API Design Review
Applying: apply changes based on the final API Design Review
Applying: solve the TODOs
Using index info to reconstruct a base tree...
M src/libraries/System.Private.CoreLib/src/Resources/Strings.resx
Falling back to patching base and 3-way merge...
Auto-merging src/libraries/System.Private.CoreLib/src/Resources/Strings.resx
CONFLICT (content): Merge conflict in src/libraries/System.Private.CoreLib/src/Resources/Strings.resx
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0028 solve the TODOs
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Error: The process '/usr/bin/git' failed with exit code 128Please backport manually! |
|
@jeffhandley an error occurred while backporting to release/9.0-preview4, please check the run log for details! Error: git am failed, most likely due to a merge conflict. |
This was referenced Apr 25, 2024
|
We're holding off on the backport for now. We may decide to proceed with it on Thursday though. |
This was referenced Apr 25, 2024
|
@adamsitnik, as @TIHan mentioned, we have widespread test failures which seem to happen after this PR. Could you please take a quick look? CC @jeffhandley. |
matouskozak
pushed a commit
to matouskozak/runtime
that referenced
this pull request
Apr 30, 2024
Co-authored-by: Jan Kotas <jkotas@microsoft.com>
|
Likely caused improvements: |
michaelgsharp
pushed a commit
to michaelgsharp/runtime
that referenced
this pull request
May 9, 2024
Co-authored-by: Jan Kotas <jkotas@microsoft.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
fixes #97566
fixes #100867