E5E0 Fix double execution of `Target` expressions in null conditionals by SeeminglyScience · Pull Request #24765 · PowerShell/PowerShell · GitHub
[go: up one dir, main page]
Skip to content

Fix double execution of Target expressions in null conditionals #24765

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

Open
SeeminglyScience wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Fix double execution of Target expressions in null conditionals #24765

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2527829
Fix double execution of null conditional targets
SeeminglyScience Jan 11, 2025
f6eb92d
Merge branch 'master' into SeeminglyScience/issue24716
SeeminglyScience Mar 27, 2025
4559c51
Address complexity feedback
SeeminglyScience Mar 27, 2025
2e5d76f
Merge branch 'master' into SeeminglyScience/issue24716
SeeminglyScience Mar 31, 2025
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
97 changes: 77 additions & 20 deletions src/System.Management.Automation/engine/parser/Compiler.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6321,14 +6321,32 @@ public object VisitMemberExpression(MemberExpressionAst memberExpressionAst)
}

var target = CompileExpressionOperand(memberExpressionAst.Expression);
ParameterExpression nullConditionalTemp = null;
var targetToUse = memberExpressionAst.NullConditional
? nullConditionalTemp = Expression.Parameter(target.Type)
: target;

// If the ?. operator is used for null conditional check, add the null conditional expression.
var memberNameAst = memberExpressionAst.Member as StringConstantExpressionAst;
Expression memberAccessExpr = memberNameAst != null
? DynamicExpression.Dynamic(PSGetMemberBinder.Get(memberNameAst.Value, _memberFunctionType, memberExpressionAst.Static), typeof(object), target)
: DynamicExpression.Dynamic(PSGetDynamicMemberBinder.Get(_memberFunctionType, memberExpressionAst.Static), typeof(object), target, Compile(memberExpressionAst.Member));
var memberExpr = memberNameAst != null
? DynamicExpression.Dynamic(
PSGetMemberBinder.Get(
memberNameAst.Value,
_memberFunctionType,
memberExpressionAst.Static),
typeof(object),
targetToUse)
: DynamicExpression.Dynamic(
PSGetDynamicMemberBinder.Get(
_memberFunctionType,
memberExpressionAst.Static),
typeof(object),
targetToUse,
Compile(memberExpressionAst.Member));

return memberExpressionAst.NullConditional ? GetNullConditionalWrappedExpression(target, memberAccessExpr) : memberAccessExpr;
return memberExpressionAst.NullConditional
? GetNullConditionalWrappedExpression(target, nullConditionalTemp, memberExpr)
: memberExpr;
}

internal static PSMethodInvocationConstraints GetInvokeMemberConstraints(InvokeMemberExpressionAst invokeMemberExpressionAst)
Expand Down Expand Up @@ -6403,9 +6421,16 @@ internal Expression InvokeMember(
? (CallSiteBinder)PSCreateInstanceBinder.Get(callInfo, constraints, publicTypeOnly: true)
: PSInvokeMemberBinder.Get(name, callInfo, @static, propertySet, constraints, classScope);

var dynamicExprFromBinder = DynamicExpression.Dynamic(binder, typeof(object), args.Prepend(target));
ParameterExpression nullConditionalTemp = null;
var targetToUse = nullConditional
? nullConditionalTemp = Expression.Parameter(target.Type)
: target;

return nullConditional ? GetNullConditionalWrappedExpression(target, dynamicExprFromBinder) : dynamicExprFromBinder;
var memberInvocation = DynamicExpression.Dynamic(binder, typeof(object), args.Prepend(targetToUse));

return nullConditional
? GetNullConditionalWrappedExpression(target, nullConditionalTemp, memberInvocation)
: memberInvocation;
}

private static Expression InvokeBaseCtorMethod(PSMethodInvocationConstraints constraints, Expression target, IEnumerable<Expression> args)
Expand All @@ -6424,10 +6449,23 @@ internal Expression InvokeDynamicMember(
bool propertySet,
bool nullConditional = false)
{
var binder = PSInvokeDynamicMemberBinder.Get(new CallInfo(args.Count()), _memberFunctionType, @static, propertySet, constraints);
var dynamicExprFromBinder = DynamicExpression.Dynamic(binder, typeof(object), args.Prepend(memberNameExpr).Prepend(target));
var binder = PSInvokeDynamicMemberBinder.Get(
new CallInfo(args.Count()),
_memberFunctionType,
@static,
propertySet,
constraints);

ParameterExpression nullConditionalTemp = null;
var targetToUse = nullConditional
? nullConditionalTemp = Expression.Parameter(target.Type)
: target;

var memberInvocation = DynamicExpression.Dynamic(binder, typeof(object), args.Prepend(targetToUse));

return nullConditional ? GetNullConditionalWrappedExpression(target, dynamicExprFromBinder) : dynamicExprFromBinder;
return nullConditional
? GetNullConditionalWrappedExpression(target, nullConditionalTemp, memberInvocation)
: memberInvocation;
}

public object VisitInvokeMemberExpression(InvokeMemberExpressionAst invokeMemberExpressionAst)
Expand Down Expand Up @@ -6601,8 +6639,13 @@ public object VisitIndexExpression(IndexExpressionAst indexExpressionAst)
var index = indexExpressionAst.Index;
var arrayLiteral = (index as ArrayLiteralAst);
var constraints = CombineTypeConstraintForMethodResolution(
GetTypeConstraintForMethodResolution(indexExpressionAst.Target),
GetTypeConstraintForMethodResolution(index));
GetTypeConstraintForMethodResolution(indexExpressionAst.Target),
GetTypeConstraintForMethodResolution(index));

ParameterExpression nullConditionalTemp = null;
var targetToUse = indexExpressionAst.NullConditional
? nullConditionalTemp = Expression.Parameter(targetExpr.Type)
: targetExpr;

// An array literal is either:
// $x[1,2]
Expand All @@ -6611,26 +6654,40 @@ public object VisitIndexExpression(IndexExpressionAst indexExpressionAst)
// In the former case, the user is requesting an array slice. In the latter case, they index expression is likely
// an array (dynamically determined) and they don't want an array slice, they want to use the array as the index
// expression.
Expression indexingExpr = arrayLiteral != null && arrayLiteral.Elements.Count > 1
var indexExpr = arrayLiteral is { Elements.Count: > 1 }
? DynamicExpression.Dynamic(
PSGetIndexBinder.Get(arrayLiteral.Elements.Count, constraints),
typeof(object),
arrayLiteral.Elements.Select(CompileExpressionOperand).Prepend(targetExpr))
arrayLiteral.Elements.Select(CompileExpressionOperand).Prepend(targetToUse))
: DynamicExpression.Dynamic(
PSGetIndexBinder.Get(argCount: 1, constraints),
typeof(object),
targetExpr,
targetToUse,
CompileExpressionOperand(index));

return indexExpressionAst.NullConditional ? GetNullConditionalWrappedExpression(targetExpr, indexingExpr) : indexingExpr;
return indexExpressionAst.NullConditional
? GetNullConditionalWrappedExpression(targetExpr, nullConditionalTemp, indexExpr)
: indexExpr;
}

private static Expression GetNullConditionalWrappedExpression(Expression targetExpr, Expression memberAccessExpression)
private static Expression GetNullConditionalWrappedExpression(
Expression originalTarget,
ParameterExpression tempVar,
Expression ifNotNullExpr)
{
return Expression.Condition(
Expression.Call(CachedReflectionInfo.LanguagePrimitives_IsNull, targetExpr.Cast(typeof(object))),
ExpressionCache.NullConstant,
memberAccessExpression);

return Expression.Block(
typeof(object),
[tempVar],
[
Expression.Assign(tempVar, originalTarget),
Expression.Condition(
Expression.Call(
CachedReflectionInfo.LanguagePrimitives_IsNull,
tempVar.Cast(typeof(object))),
ExpressionCache.NullConstant,
ifNotNullExpr),
]);
}

public object VisitAttributedExpression(AttributedExpressionAst attributedExpressionAst)
Expand Down
20 changes: 20 additions & 0 deletions test/powershell/Language/Operators/NullConditional.Tests.ps1
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -329,6 +329,16 @@ Describe 'NullConditionalMemberAccess' -Tag 'CI' {
It 'Should throw error when method does not exist' {
{ ${psObj}?.nestedMethod?.NonExistent() } | Should -Throw -ErrorId 'MethodNotFound'
}

It 'Should not run the target expression twice' {
$i = 0
$($i++; [PSCustomObject]@{ Prop = $true })?.Prop | Should -Be $true
$i | Should -Be 1

# Strongly typed expression as a valuetype should skip test
([bool]$($i++; $true))?.ToString() | Should -Be 'True'
$i | Should -Be 2
}
}

Context '?[] operator tests' {
Expand Down Expand Up @@ -368,5 +378,15 @@ Describe 'NullConditionalMemberAccess' -Tag 'CI' {
${dateArray}?[1234]?.ToLongDateString() | Should -BeNullOrEmpty
${doesNotExist}?[0]?.MyGetMethod() | Should -BeNullOrEmpty
}

It 'Should not run the target expression twice' {
$i = 0
$($i++; 'value')?[0] | Should -Be 'v'
$i | Should -Be 1

# Strongly typed expression as a valuetype should skip test
([bool]$($i++; $true))?[0] | Should -BeTrue
$i | Should -Be 2
}
}
}
Loading
0