Fix TypeName.GetReflectionType() to work when the TypeName instance represents a generic type definition within a GenericTypeName
#24985
Conversation
… generic type can be found in cache
There was a problem hiding this comment.
Copilot reviewed 1 out of 2 changed files in this pull request and generated no comments.
Files not reviewed (1)
- test/powershell/Language/Parser/Parsing.Tests.ps1: Language not supported
Comments suppressed due to low confidence (1)
src/System.Management.Automation/engine/parser/SymbolResolver.cs:717
- Ensure that the new behavior of setting the cached type for 'genericTypeName.TypeName' when 'GetReflectionType()' returns null is covered by tests.
((ISupportsTypeCaching)genericDefinition).CachedType = foundType.GetGenericTypeDefinition();
|
/azp run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
daxian-dbw
added
the
CL-General
microsoft-github-policy-service
bot
added
the
Review - Needed
|
@MartinGC94 Can you please also review this PR? Thank you! |
microsoft-github-policy-service
bot
removed
the
Review - Needed
src/System.Management.Automation/engine/parser/SymbolResolver.cs
Outdated
Show resolved
Hide resolved
|
It fixes my real world scenario and the fix itself seems reasonable. Thanks for the quick fix :) |
microsoft-github-policy-service
bot
added
the
Review - Needed
daxian-dbw
changed the title
GenericTypeName.TypeName as needed when the generic type is found in cacheTypeName.GetReflectionType() to work when the TypeName instance represents a generic type definition within a GenericTypeName
microsoft-github-policy-service
bot
removed
the
Review - Needed
| if (genericArgumentCount > 0 && !_name.Contains('`')) | ||
| { | ||
| _genericArgumentCount = genericArgumentCount; | ||
| } |
There was a problem hiding this comment.
Usually, we expect a constructor to throw an exception if the argument is unexpected. But here we perform a correction of the argument. It's amazing. I'm afraid this may provoke accidental misuse of this constructor in the future. At the very least, it would be good to add a comment with examples of the scenarios we address and explaining why this correction is needed in this place.
Alternatively, we could perform a check at the call location and pass the correct arguments.
It could be even better to use a template like private static TypeName TypeName.CreateAlternativeGenericTypeName() with a comprehensive description.
There was a problem hiding this comment.
This "try alternate name" thing happens in multiple places, which is common when dealing with generic type resolution. Comments were added in the GetReflectionType() to explain why we only fall back to alternate name even if genericArgumentCount is set.
As for the scenario for using this constructor, the XML comment calls it out and it's also easy to understand by looking at where it's used. So, I don't think more comments are needed.
I can add a check to throw when genericArgumentCount is a negative value.
iSazonov
added
CL-General
github-project-automation
bot
moved this from PR Todo
to Done
in PowerShell Team Reviews/Investigations
|
📣 Hey @daxian-dbw, how did we do? We would love to hear your feedback with the link below! 🗣️ 🔗 https://aka.ms/PSRepoFeedback |
4 participants
PR Summary
Fix #24982
When a generic type expression is parsed for the second time, the generic type can be found in cache, and hence the symbol resolver won't go through the code path to resolve
genericTypeName.TypeName, which represents the generic type definition. Therefore, unlike the first-time parsing, the cached type forgenericTypeName.TypeNamewon't be set during the symbol resolving process for the second parsing.However, the type resolution in
TypeName.GetReflectionType()doesn't work for a generic type definition in most cases, for example, when the generic type expression is[System.Collections.Generic.List[string]],genericTypeName.TypeNamewill be aTypeNameinstance with the full nameSystem.Collections.Generic.Listwhich will fail to be resolved. That's why the second time you parse the same generic type expression,genericTypeName.TypeName.GetReflectionType()will return null.Why
genericTypeName.TypeNamecan be resolved in the first-time resolution ofgenericTypeName, but doesn't work when directly callinggenericTypeName.TypeName.GetReflectionType()? That's because for the former, withinVisitGenericTypeName(...), it uses aTypeResolutionStateinstance as the resolution context, which is constructed with the information from thegenericTypeName(see code here). But when directly callinggenericTypeName.TypeName.GetReflectionType(), it doesn't have that information and thus cannot use the same/equivalentTypeResolutionStateinstance as the context for type resolution.The fix is to pass the generic argument count to
TypeNamewhen constructing aGenericTypeNameinstance, so that theTypeNameinstance can try resolving with an alternate name (i.e.typename`<n>) when the original name fails to resolve.The changes also fix another issue:
Before: assembly-qualified name
[System.Collections.Generic.List[string], System.Private.CoreLib]cannot be resolved.After:
[System.Collections.Generic.List[string], System.Private.CoreLib]can be resolved as expected.PR Checklist
.h,.cpp,.cs,.ps1and.psm1files have the correct copyright headerWIP:or[ WIP ]to the beginning of the title (theWIPbot will keep its status check atPendingwhile the prefix is present) and remove the prefix when the PR is ready.- [ ] Issue filed: