-
Notifications
You must be signed in to change notification settings - Fork 7.6k
Improve type inference of hashtable keys #17907
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
Improve type inference of hashtable keys #17907
Conversation
This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days. |
properties.Add(new PSMemberNameAndType(name, pstypeName, value)); | ||
if (!foundAnyTypes) | ||
{ | ||
properties.Add(new PSMemberNameAndType(name, new PSTypeName("System.Object"))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If PSTypeName("System.Object") is immutable makes sense allocate this again and again?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How would you avoid that? Use a field?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First question - is it a hot path or no? If yes we could use a field with lazy initialization.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hard to say. It's just a fallback for when PowerShell can't infer the type in the value expression, or when the user uses a literal $null
as the value so in theory it should rarely be used. The only realistic scenario where I think it would be hot is if the user is analyzing a hashtable that uses a bunch of commands/types that are unavailable because the module hasn't been installed on the computer. In that case it would be run for each key/value pair in the hashtable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with one minor comment.
src/System.Management.Automation/engine/parser/TypeInferenceVisitor.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Co-authored-by: Ilya <darpa@yandex.ru>
Co-authored-by: Ilya <darpa@yandex.ru>
af65381
to
990b0ff
Compare
This PR has Quantification details
Why proper sizing of changes matters
Optimal pull request sizes drive a better predictable PR flow as they strike a
What can I do to optimize my changes
How to interpret the change counts in git diff output
Was this comment helpful? 👍 :ok_hand: :thumbsdown: (Email) |
PR Summary
Updates the hashtable visitor so all members are inferred and not just "pure" expressions.
Also updates type inference for the IndexExpression to support synthetic types.
This enables tab completion scenarios like:
$Hashtable=@{Key1=ls};$Hashtable.Key1.<Tab>
to properly infer the type from the command.PR Context
PR Checklist
.h
,.cpp
,.cs
,.ps1
and.psm1
files have the correct copyright headerWIP:
or[ WIP ]
to the beginning of the title (theWIP
bot will keep its status check atPending
while the prefix is present) and remove the prefix when the PR is ready.(which runs in a different PS Host).