- Notifications
You must be signed in to change notification settings - Fork8.1k
Fix completion that relies on pseudobinding in script blocks#25122
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Fix completion that relies on pseudobinding in script blocks#25122
Conversation
| { | ||
| try | ||
| { | ||
| commandName=GetSafeValueVisitor.GetSafeValue(commandAst.CommandElements[0],null,GetSafeValueVisitor.SafeValueContext.ModuleAnalysis)asstring; |
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.
Could you please add a code comment to document the scenario, maybe with examples of command names we expect here?
| stringcommandName=commandAst.GetCommandName(); | ||
| if(commandNameisnull) | ||
| { | ||
| // GetCommandName only works if the name is a string constant. GetSafeValueVistor can evaluate some safe dynamic expressions |
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.
GetCommandName? Comment for the method says about scripts, modules, aliases. So I'd like to understand what could be going on here.
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.
GetCommandName is the method that was run on line 280. It failed to get the name because it only handles constants but there's also thisGetSafeValueVisitor that can handle a bit more dynamic expressions but still be safe. When this visitor sees a scriptblock expression, it tries to create a new scriptblock using the text but that fails when there's any parse errors in the script text. This is why:& {(New-Guid).<Tab> fails, but& {(New-Guid).N<Tab> does not, because the N removes the parser error from the missing name after the member access token.
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.
Thanks! I see GetSafeValue() process StringConstantExpressionAst too so we could not call commandAst.GetCommandName(). Perhaps it was added as fast way vs visitor.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
650189e intoPowerShell:masterUh oh!
There was an error while loading.Please reload this page.
microsoft-github-policy-servicebot commentedMar 5, 2025 • edited by unfurl-linksbot
Loading Uh oh!
There was an error while loading.Please reload this page.
edited by unfurl-linksbot
Uh oh!
There was an error while loading.Please reload this page.
📣 Hey@MartinGC94, how did we do? We would love to hear your feedback with the link below! 🗣️ 🔗https://aka.ms/PSRepoFeedback |
PR Summary
Fixes an issue in the pseudobinding code where a parse exception would prevent the pseudobinding from working when attempting to parse an incomplete script. This would prevent this:
& {(New-Guid).<Tab>from working.It would fail because the script text has errors (due to incomplete input) and the safe value visitor would try to create a scriptblock based on the incomplete text.
PR Context
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:
(which runs in a different PS Host).