From 2527829f823adc7a95ad8065c13c13cfb2f91145 Mon Sep 17 00:00:00 2001 From: Patrick Meinecke Date: Sat, 11 Jan 2025 11:59:47 -0500 Subject: [PATCH 1/2] Fix double execution of null conditional targets Fixes #24716 --- .../engine/parser/Compiler.cs | 142 ++++++++++++++---- .../Operators/NullConditional.Tests.ps1 | 20 +++ 2 files changed, 131 insertions(+), 31 deletions(-) diff --git a/src/System.Management.Automation/engine/parser/Compiler.cs b/src/System.Management.Automation/engine/parser/Compiler.cs index 30528f99588..cb41ace995b 100644 --- a/src/System.Management.Automation/engine/parser/Compiler.cs +++ b/src/System.Management.Automation/engine/parser/Compiler.cs @@ -6324,11 +6324,35 @@ public object VisitMemberExpression(MemberExpressionAst memberExpressionAst) // 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)); - - return memberExpressionAst.NullConditional ? GetNullConditionalWrappedExpression(target, memberAccessExpr) : memberAccessExpr; + return MaybeWrapInNullConditionalExpression( + memberExpressionAst.NullConditional, + target, + (this, memberNameAst, memberExpressionAst, _memberFunctionType), + static (target, state) => + { + ( + Compiler compiler, + StringConstantExpressionAst memberNameAst, + MemberExpressionAst memberExpressionAst, + TypeDefinitionAst memberFunctionType + ) = state; + + return memberNameAst != null + ? DynamicExpression.Dynamic( + PSGetMemberBinder.Get( + memberNameAst.Value, + memberFunctionType, + memberExpressionAst.Static), + typeof(object), + target) + : DynamicExpression.Dynamic( + PSGetDynamicMemberBinder.Get( + memberFunctionType, + memberExpressionAst.Static), + typeof(object), + target, + compiler.Compile(memberExpressionAst.Member)); + }); } internal static PSMethodInvocationConstraints GetInvokeMemberConstraints(InvokeMemberExpressionAst invokeMemberExpressionAst) @@ -6403,9 +6427,15 @@ 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)); - - return nullConditional ? GetNullConditionalWrappedExpression(target, dynamicExprFromBinder) : dynamicExprFromBinder; + return MaybeWrapInNullConditionalExpression( + nullConditional, + target, + (binder, args), + static (target, state) => + { + (CallSiteBinder binder, IEnumerable args) = state; + return DynamicExpression.Dynamic(binder, typeof(object), args.Prepend(target)); + }); } private static Expression InvokeBaseCtorMethod(PSMethodInvocationConstraints constraints, Expression target, IEnumerable args) @@ -6424,10 +6454,30 @@ 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); + + return MaybeWrapInNullConditionalExpression( + nullConditional, + target, + (binder, args, memberNameExpr), + static (target, state) => + { + ( + PSInvokeDynamicMemberBinder binder, + IEnumerable args, + Expression memberNameExpr + ) = state; - return nullConditional ? GetNullConditionalWrappedExpression(target, dynamicExprFromBinder) : dynamicExprFromBinder; + return DynamicExpression.Dynamic( + binder, + typeof(object), + args.Prepend(memberNameExpr).Prepend(target)); + }); } public object VisitInvokeMemberExpression(InvokeMemberExpressionAst invokeMemberExpressionAst) @@ -6611,26 +6661,56 @@ 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 - ? DynamicExpression.Dynamic( - PSGetIndexBinder.Get(arrayLiteral.Elements.Count, constraints), - typeof(object), - arrayLiteral.Elements.Select(CompileExpressionOperand).Prepend(targetExpr)) - : DynamicExpression.Dynamic( - PSGetIndexBinder.Get(argCount: 1, constraints), - typeof(object), - targetExpr, - CompileExpressionOperand(index)); - - return indexExpressionAst.NullConditional ? GetNullConditionalWrappedExpression(targetExpr, indexingExpr) : indexingExpr; - } - - private static Expression GetNullConditionalWrappedExpression(Expression targetExpr, Expression memberAccessExpression) - { - return Expression.Condition( - Expression.Call(CachedReflectionInfo.LanguagePrimitives_IsNull, targetExpr.Cast(typeof(object))), - ExpressionCache.NullConstant, - memberAccessExpression); + return MaybeWrapInNullConditionalExpression( + indexExpressionAst.NullConditional, + targetExpr, + (this, arrayLiteral, index, constraints), + static (targetExpr, state) => + { + ( + Compiler compiler, + ArrayLiteralAst arrayLiteral, + ExpressionAst index, + PSMethodInvocationConstraints constraints + ) = state; + + return arrayLiteral is { Elements.Count: > 1 } + ? DynamicExpression.Dynamic( + PSGetIndexBinder.Get(arrayLiteral.Elements.Count, constraints), + typeof(object), + arrayLiteral.Elements.Select(compiler.CompileExpressionOperand).Prepend(targetExpr)) + : DynamicExpression.Dynamic( + PSGetIndexBinder.Get(argCount: 1, constraints), + typeof(object), + targetExpr, + compiler.CompileExpressionOperand(index)); + }); + } + + private static Expression MaybeWrapInNullConditionalExpression( + bool isNullConditional, + Expression targetExpr, + TState state, + Func factory) + { + if (!isNullConditional || targetExpr.Type.IsValueType) + { + return factory(targetExpr, state); + } + + ParameterExpression temp = Expression.Parameter(targetExpr.Type); + return Expression.Block( + typeof(object), + [temp], + [ + Expression.Assign(temp, targetExpr), + Expression.Condition( + Expression.Call( + CachedReflectionInfo.LanguagePrimitives_IsNull, + temp.Cast(typeof(object))), + ExpressionCache.NullConstant, + factory(temp, state).Cast(typeof(object))), + ]); } public object VisitAttributedExpression(AttributedExpressionAst attributedExpressionAst) diff --git a/test/powershell/Language/Operators/NullConditional.Tests.ps1 b/test/powershell/Language/Operators/NullConditional.Tests.ps1 index 237b4d8c987..22e0c7ee390 100644 --- a/test/powershell/Language/Operators/NullConditional.Tests.ps1 +++ b/test/powershell/Language/Operators/NullConditional.Tests.ps1 @@ -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' { @@ -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 + } } } From 4559c51a4b886ca53914530c243265c8b2be29a7 Mon Sep 17 00:00:00 2001 From: Patrick Meinecke Date: Thu, 27 Mar 2025 13:00:53 -0400 Subject: [PATCH 2/2] Address complexity feedback Now uses a temp variable created in the caller instead of a delegate. Thanks to @daxian-dbw for the pattern recommendation! --- .../engine/parser/Compiler.cs | 173 ++++++++---------- 1 file changed, 75 insertions(+), 98 deletions(-) diff --git a/src/System.Management.Automation/engine/parser/Compiler.cs b/src/System.Management.Automation/engine/parser/Compiler.cs index cb41ace995b..485d11dbe73 100644 --- a/src/System.Management.Automation/engine/parser/Compiler.cs +++ b/src/System.Management.Automation/engine/parser/Compiler.cs @@ -6321,38 +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; - return MaybeWrapInNullConditionalExpression( - memberExpressionAst.NullConditional, - target, - (this, memberNameAst, memberExpressionAst, _memberFunctionType), - static (target, state) => - { - ( - Compiler compiler, - StringConstantExpressionAst memberNameAst, - MemberExpressionAst memberExpressionAst, - TypeDefinitionAst memberFunctionType - ) = state; - - return memberNameAst != null - ? DynamicExpression.Dynamic( - PSGetMemberBinder.Get( - memberNameAst.Value, - memberFunctionType, - memberExpressionAst.Static), - typeof(object), - target) - : DynamicExpression.Dynamic( - PSGetDynamicMemberBinder.Get( - memberFunctionType, - memberExpressionAst.Static), - typeof(object), - target, - compiler.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, nullConditionalTemp, memberExpr) + : memberExpr; } internal static PSMethodInvocationConstraints GetInvokeMemberConstraints(InvokeMemberExpressionAst invokeMemberExpressionAst) @@ -6427,15 +6421,16 @@ internal Expression InvokeMember( ? (CallSiteBinder)PSCreateInstanceBinder.Get(callInfo, constraints, publicTypeOnly: true) : PSInvokeMemberBinder.Get(name, callInfo, @static, propertySet, constraints, classScope); - return MaybeWrapInNullConditionalExpression( - nullConditional, - target, - (binder, args), - static (target, state) => - { - (CallSiteBinder binder, IEnumerable args) = state; - return DynamicExpression.Dynamic(binder, typeof(object), args.Prepend(target)); - }); + 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, nullConditionalTemp, memberInvocation) + : memberInvocation; } private static Expression InvokeBaseCtorMethod(PSMethodInvocationConstraints constraints, Expression target, IEnumerable args) @@ -6461,23 +6456,16 @@ internal Expression InvokeDynamicMember( propertySet, constraints); - return MaybeWrapInNullConditionalExpression( - nullConditional, - target, - (binder, args, memberNameExpr), - static (target, state) => - { - ( - PSInvokeDynamicMemberBinder binder, - IEnumerable args, - Expression memberNameExpr - ) = state; + ParameterExpression nullConditionalTemp = null; + var targetToUse = nullConditional + ? nullConditionalTemp = Expression.Parameter(target.Type) + : target; - return DynamicExpression.Dynamic( - binder, - typeof(object), - args.Prepend(memberNameExpr).Prepend(target)); - }); + var memberInvocation = DynamicExpression.Dynamic(binder, typeof(object), args.Prepend(targetToUse)); + + return nullConditional + ? GetNullConditionalWrappedExpression(target, nullConditionalTemp, memberInvocation) + : memberInvocation; } public object VisitInvokeMemberExpression(InvokeMemberExpressionAst invokeMemberExpressionAst) @@ -6651,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] @@ -6661,55 +6654,39 @@ 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. - return MaybeWrapInNullConditionalExpression( - indexExpressionAst.NullConditional, - targetExpr, - (this, arrayLiteral, index, constraints), - static (targetExpr, state) => - { - ( - Compiler compiler, - ArrayLiteralAst arrayLiteral, - ExpressionAst index, - PSMethodInvocationConstraints constraints - ) = state; - - return arrayLiteral is { Elements.Count: > 1 } - ? DynamicExpression.Dynamic( - PSGetIndexBinder.Get(arrayLiteral.Elements.Count, constraints), - typeof(object), - arrayLiteral.Elements.Select(compiler.CompileExpressionOperand).Prepend(targetExpr)) - : DynamicExpression.Dynamic( - PSGetIndexBinder.Get(argCount: 1, constraints), - typeof(object), - targetExpr, - compiler.CompileExpressionOperand(index)); - }); - } - - private static Expression MaybeWrapInNullConditionalExpression( - bool isNullConditional, - Expression targetExpr, - TState state, - Func factory) - { - if (!isNullConditional || targetExpr.Type.IsValueType) - { - return factory(targetExpr, state); - } - - ParameterExpression temp = Expression.Parameter(targetExpr.Type); + var indexExpr = arrayLiteral is { Elements.Count: > 1 } + ? DynamicExpression.Dynamic( + PSGetIndexBinder.Get(arrayLiteral.Elements.Count, constraints), + typeof(object), + arrayLiteral.Elements.Select(CompileExpressionOperand).Prepend(targetToUse)) + : DynamicExpression.Dynamic( + PSGetIndexBinder.Get(argCount: 1, constraints), + typeof(object), + targetToUse, + CompileExpressionOperand(index)); + + return indexExpressionAst.NullConditional + ? GetNullConditionalWrappedExpression(targetExpr, nullConditionalTemp, indexExpr) + : indexExpr; + } + + private static Expression GetNullConditionalWrappedExpression( + Expression originalTarget, + ParameterExpression tempVar, + Expression ifNotNullExpr) + { + return Expression.Block( typeof(object), - [temp], + [tempVar], [ - Expression.Assign(temp, targetExpr), + Expression.Assign(tempVar, originalTarget), Expression.Condition( Expression.Call( CachedReflectionInfo.LanguagePrimitives_IsNull, - temp.Cast(typeof(object))), + tempVar.Cast(typeof(object))), ExpressionCache.NullConstant, - factory(temp, state).Cast(typeof(object))), + ifNotNullExpr), ]); }