Fix completion of variables with an attribute in the assignment #25016
Conversation
iSazonov
added
the
CL-General
src/System.Management.Automation/engine/CommandCompletion/CompletionCompleters.cs
Show resolved
Hide resolved
| { | ||
| Type lastAssignedType = assignmentStatementAst.Right is CommandExpressionAst commandExpression | ||
| ? GetInferredVarTypeFromAst(commandExpression.Expression) | ||
| : null; |
There was a problem hiding this comment.
- We can use only CommandExpressionAst for type inference? What other types can be in assignmentStatementAst.Right? I see GetInferredVarTypeFromAst() can process another types.
- null - no useful default we can use?
There was a problem hiding this comment.
All the types handled by GetInferredVarTypeFromAst() are wrapped in a CommandExpressionAst. Other possible types include if/else statements, loops, etc. while GetInferredVarTypeFromAst() could get updated to handle this, it's out of scope for this PR.
It's also worth keeping in mind that the original goal of this Ast finder was also just to do a quick and dirty Ast analysis without slowing down variable completion too much. The type inference is only used for the tooltip so I intentionally left out stuff like type inference from commands as that could potentially take several seconds (if not minutes) if many modules need to be imported.
As for useful defaults: I can't think of any. I mean I guess I could add [System.Object] for the rest, but that's not a particularly useful tooltip.
There was a problem hiding this comment.
Other possible types include if/else statements, loops, etc. while
GetInferredVarTypeFromAst()could get updated to handle this, it's out of scope for this PR.
If it makes sense to improve in future, I'd prefer to use case expression pattern with a todo comment for null default about types we can process in future.
There was a problem hiding this comment.
"If it makes sense" is subjective. If it was up to me alone I'd leave it exactly like it is now as it was never meant to be a comprehensive type analysis.
|
/azp run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
|
📣 Hey @MartinGC94, how did we do? We would love to hear your feedback with the link below! 🗣️ 🔗 https://aka.ms/PSRepoFeedback |
2 participants
PR Summary
Fixes completion of variables that have been assigned with an attribute so they correctly show up in scenarios like:
This also fixes the tooltip for variables with multiple type constraints so they show up with the applicable type constraint so
[string] [int] $Var1 = 2correctly shows up as a string due to string being the first type constraint. Before this they wouldn't even be completed.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).