Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix variable type inference precedence #18691

Merged
merged 5 commits into from
Jun 12, 2023
Merged

Fix variable type inference precedence #18691

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
e6f31b0
Fix variable type inference precedence
MartinGC94 Nov 30, 2022
989134d
Change assignment vistor to return emptyPSTypeNameArray
MartinGC94 Nov 30, 2022
c91e29f
Fix typeinference for assignmentstatements inside ParenExpressions. F…
MartinGC94 Dec 2, 2022
6ba92b5
Merge branch 'master' into FixVarTypeInferencePrecedence
MartinGC94 Jan 18, 2023
1289c4e
Revert AssignmentStatement changes
MartinGC94 Jan 18, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 4 additions & 8 deletions src/System.Management.Automation/engine/parser/TypeInferenceVisitor.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2019,11 +2019,6 @@ private void InferTypeFrom(VariableExpressionAst variableExpressionAst, List<PST
parent = parent.Parent;
}

if (parent.Parent is FunctionDefinitionAst)
{
parent = parent.Parent;
}

int startOffset = variableExpressionAst.Extent.StartOffset;
var targetAsts = (List<Ast>)AstSearcher.FindAll(
parent,
Expand Down Expand Up @@ -2063,9 +2058,9 @@ private void InferTypeFrom(VariableExpressionAst variableExpressionAst, List<PST

// If any of the assignments lhs use a type constraint, then we use that.
// Otherwise, we use the rhs of the "nearest" assignment
foreach (var assignAst in assignAsts)
for (int i = assignAsts.Length - 1; i >= 0; i--)
{
if (assignAst.Left is ConvertExpressionAst lhsConvert)
if (assignAsts[i].Left is ConvertExpressionAst lhsConvert)
{
inferredTypes.Add(new PSTypeName(lhsConvert.Type.TypeName));
return;
Expand Down Expand Up @@ -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;
Copy link
Member

@daxian-dbw daxian-dbw Jun 5, 2023 •
edited
Loading

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?

Copy link
Contributor Author

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 😄

}

if (ast is ForEachStatementAst foreachAst)
Expand Down
18 changes: 18 additions & 0 deletions test/powershell/engine/Api/TypeInference.Tests.ps1
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Member

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?

Copy link
Contributor Author

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.

$res.Name | Select-Object -Last 1 | Should -Be "System.Collections.Hashtable"
}

It 'Infers closest variable type and ignores unrelated param blocks' {
$res = [AstTypeInference]::InferTypeOf( {
[hashtable]$ParameterName=@{}
function Verb-Noun {
param
(
[string]$ParameterName
)
}
$ParameterName }.Ast)
$res.Name | Should -Be "System.Collections.Hashtable"
}

It 'Infers type of $null after variable assignment' {
$res = [AstTypeInference]::InferTypeOf( { $null = "Hello";$null }.Ast)
$res.Count | Should -Be 0
Expand Down