-
Notifications
You must be signed in to change notification settings - Fork 7.1k
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
Fix variable type inference precedence #18691
Fix variable type inference precedence #18691
Conversation
Doesn't this approach of reversing the order break this pattern of coding common to ps1 scripts? $script = '[CmdletBinding()]
param (
# PS1 Script Parameter
[Parameter()]
[hashtable]
$ParameterName
)
function Verb-Noun {
[CmdletBinding()]
param (
# PS1 Function Parameter
[Parameter()]
[string]
$ParameterName
)
}
$ParameterName.'
TabExpansion2 -inputScript $script -cursorColumn $script.Length | select -expandproperty CompletionMatches
|
So, apparently VisitAssignmentStatement returns whatever is on the left: https://github.com/PowerShell/PowerShell/blob/master/src/System.Management.Automation/engine/parser/TypeInferenceVisitor.cs#L977 @SeeminglyScience What's your thoughts on this? How do I fix this properly?
|
Not this exact scenario because parameters are handled before variable assignments, so it will see the hashtable parameter before the other function parameter. But you are right that the approach isn't perfect. If you define a variable, and then a function like this:
then it will see the function parameter instead of the script variable. I will need to look into that. |
Mmmm tricky! I'm inclined to think that it should continue to infer the type on the RHS, but only of the closest assignment. But then if you do [string] $thing = 'something'
$thing = 10
$thing.<tab> then you'll get Though even that's not necessarily perfect because then you'd miss declarations in a foreach loop. BUT you could argue that you should have the declaration outside of the loop, and it's fine to miss it in this case. Sorry I got kind of ramble-y there, but does that make sense? Not in a "just go do that" kind of way, I'm still thinking over exactly what should be done, just kind of talking it out. |
…ix type inference for variables that have the same name as unrelated parameters
I found a simple solution, just add a special case for AssignmentStatements inside ParenExpressions. On a related note, do we have an official name for this: |
This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days. |
So |
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) |
I've reverted the changes related to the assignmentstatementast type inference. I will (probably) look into fixing it in the future but it doesn't belong in this PR. |
@@ -1367,6 +1367,24 @@ Describe "Type inference Tests" -tags "CI" { | |||
$res.Name -join ' ' | Should -Be "System.IO.FileInfo System.IO.DirectoryInfo" | |||
} | |||
|
|||
It 'Infers closest variable type' { | |||
$res = [AstTypeInference]::InferTypeOf( { [string]$TestVar = "";[hashtable]$TestVar = @{};$TestVar }.Ast) |
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.
should there be some sort of order checking 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.
No. There shouldn't even be multiple inferred types but because assignments are included when inferring the output from a scriptblock I need that Select -Last 1
statement.
I tried fixing the issue earlier but it's tricky because sometimes assignments actually do output something.
@@ -2490,7 +2485,8 @@ public static bool AstAssignsToSameVariable(this VariableExpressionAst variableA | |||
if (parameterAst != null) | |||
{ | |||
return variableAstVariablePath.IsUnscopedVariable && | |||
parameterAst.Name.VariablePath.UnqualifiedPath.Equals(variableAstVariablePath.UnqualifiedPath, StringComparison.OrdinalIgnoreCase); | |||
parameterAst.Name.VariablePath.UnqualifiedPath.Equals(variableAstVariablePath.UnqualifiedPath, StringComparison.OrdinalIgnoreCase) && | |||
parameterAst.Parent.Parent.Extent.EndOffset > variableAst.Extent.StartOffset; |
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.
@MartinGC94 This change came from the commit c91e29f, which was reverted by 1289c4e. However, this change was left behind. Did you mean to revert this change as well?
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.
No, that was a fix for this: #18691 (comment) I was just lazy and put 2 changes in 1 commit which was clearly a bad idea 😄
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
🎉 Handy links: |
PR Summary
Fixes #18689
So instead of using the first variable assignment before the cursor to infer the type, it instead takes the last variable assignment before the cursor.
Removed a condition that could never be true because the loop before it always ensures that the parent property is null.
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).