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 1 commit
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
Prev Previous commit
Next Next commit
Fix typeinference for assignmentstatements inside ParenExpressions. F… 
…ix type inference for variables that have the same name as unrelated parameters
  • Loading branch information
@MartinGC94
MartinGC94 committed Dec 2, 2022
commit c91e29f27ef323a52429ae59661692001ddd8348
14 changes: 13 additions & 1 deletion 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 @@ -609,6 +609,17 @@ object ICustomAstVisitor.VisitScriptBlockExpression(ScriptBlockExpressionAst scr

object ICustomAstVisitor.VisitParenExpression(ParenExpressionAst parenExpressionAst)
{
// AssignmentStatements normally return nothing, but when used inside a ParenExpression it returns the left side of the assignment
// For VariableExpressions the right side is used for simplicity, but it doesn't take variable type constraints into consideration
if (parenExpressionAst.Pipeline is AssignmentStatementAst assignment)
{
if (assignment.Left is VariableExpressionAst)
{
return assignment.Right.Accept(this);
}

return assignment.Left.Accept(this);
}
return parenExpressionAst.Pipeline.Accept(this);
}

Expand Down Expand Up @@ -2468,7 +2479,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
30 changes: 30 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 @@ -34,6 +34,11 @@ Describe "Type inference Tests" -tags "CI" {
return $script:inferTypeOf4.Invoke($null, @($ast, $powerShell, $runtimePermissions))
}
}

class TypeConstrainedTestClass
{
[string] $Property1
}
}

It "Infers type from integer" {
Expand Down Expand Up @@ -1359,6 +1364,31 @@ Describe "Type inference Tests" -tags "CI" {
$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 | 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 output from AssignmentStatement inside ParenExpression' {
$res = [AstTypeInference]::InferTypeOf( { ($TestVar = "Hello") }.Ast)
$res.Name | Should -Be "System.String"
}

It 'Infers output from left side of AssignmentStatement inside ParenExpression when assigning to property' {
$res = [AstTypeInference]::InferTypeOf( {
$Object = [TypeConstrainedTestClass]@{Property1="Test"}
($Object.Property1 = 2) }.Ast)
$res.Name | Should -Be "System.String"
}
}

Describe "AstTypeInference tests" -Tags CI {
Expand Down