Fix double execution of `Target` expressions in null conditionals by SeeminglyScience · Pull Request #24765 · PowerShell/PowerShell

SeeminglyScience · 2025-01-12T22:48:31Z

PR Summary

Null conditional expressions (both index and member access) currently evaluate the "target" expression twice. This change saves the "target" result to a temporary variable to compare against.

This change is similar to #12667 which was done to fix null coalesce expressions.

Disassembly

As compiler changes can be challenging to review, I've included some disassembly of compiled scriptblocks before and after the changes. The module uses reflection to get the delegate compiled internally for the end block, and retrieves the associated LINQ expression tree. That expression tree is then interpreted into representative C#

These are manually formatted and with some of the irrelevant parts removed, full disassembly can be viewed here.

Member Expressions

Before Changes ^{_{(click to expand)}}

$i = 0
$null = ($i++)?.ToString()

// $i = 0
locals.Item009 = 0;

// $null = ($i++)?.ToString()
LanguagePrimitives.IsNull(
    // This is an Expression.Block, not a delegate.
    {
        object @object = locals.Item009;
        locals.Item009 = Fake.Dynamic<Func<CallSite, object, object>>(PSVariableAssignmentBinder.Get())(
            Fake.Dynamic<Func<CallSite, object, object>>(PSUnaryOperationBinder.Get(ExpressionType.Increment))(
                @object));

        return (@object == null) ? 0 : @object;
    })
    ? null
    : Fake.Dynamic<Func<CallSite, object, object>>(
        PSInvokeMemberBinder.Get(
            "ToString",
            classScope: null,
            new CallInfo(0),
            static: false,
            nonEnumerating: false,
            constraints: null))
        (
            // This is an Expression.Block, not a delegate.
            {
                object @object = locals.Item009;
                locals.Item009 = Fake.Dynamic<Func<CallSite, object, object>>(PSVariableAssignmentBinder.Get())(
                    Fake.Dynamic<Func<CallSite, object, object>>(PSUnaryOperationBinder.Get(ExpressionType.Increment))(
                        @object));

                return (@object == null) ? 0 : @object;
            });

After - target not strongly typed ^{_{(click to expand)}}

$i = 0
$null = ($i++)?.ToString()

// $i = 0
locals.Item009 = 0;

// $null = ($i++)?.ToString()
object object1 =
    // This is an Expression.Block, not a delegate.
    {
        object object2 = locals.Item009;
        locals.Item009 = Fake.Dynamic<Func<CallSite, object, object>>(PSVariableAssignmentBinder.Get())(
            Fake.Dynamic<Func<CallSite, object, object>>(PSUnaryOperationBinder.Get(ExpressionType.Increment))(
                object2));

        return (object2 == null) ? 0 : object2;
    };

LanguagePrimitives.IsNull(object1)
    ? null
    : Fake.Dynamic<Func<CallSite, object, object>>(
        PSInvokeMemberBinder.Get(
            "ToString",
            classScope: null,
            new CallInfo(0),
            static: false,
            nonEnumerating: false,
            constraints: null))(
                object1);

After - target strongly typed as valuetype ^{_{(click to expand)}}

```powershell [int] $i = 0 $null = ($i++)?.ToString() ```

// [int] $i = 0
locals.Item009 = 0;
// $null = ($i++)?.ToString()
Fake.Dynamic<Func<CallSite, int, object>>(
    PSInvokeMemberBinder.Get(
        "ToString",
        classScope: null,
        new CallInfo(0),
        static: false,
        nonEnumerating: false,
        constraints: null))(
            // This is an Expression.Block, not a delegate.
            {
                int @int = locals.Item009;
                locals.Item009 = Fake.Dynamic<Func<CallSite, object, int>>(
                    PSConvertBinder.Get(typeof(int)))(
                        Fake.Dynamic<Func<CallSite, int, object>>(PSUnaryOperationBinder.Get(ExpressionType.Increment))(
                            @int));

                return @int;
            });

After - target strongly typed as ref type ^{_{(click to expand)}}

[string] $i = ''
$null = ($i += 'x')?.Length

// [string] $i = ''
locals.Item009 = "";

// $null = ($i += 'x')?.Length
string @string = locals.Item009 = Fake.Dynamic<Func<CallSite, object, string>>(PSConvertBinder.Get(typeof(string)))(
    Fake.Dynamic<Func<CallSite, string, string, object>>(
        PSBinaryOperationBinder.Get(ExpressionType.Add, ignoreCase: true,scalarCompare: false))(
            locals.Item009,
            "x"));

return LanguagePrimitives.IsNull(@string)
    ? null
    : Fake.Dynamic<Func<CallSite, string, object>>(
        PSGetMemberBinder.Get("Length", type: null, static: false, nonEnumerating: false))(
            @string);

Index Expressions

Before Changes ^{_{(click to expand)}}

$i = 0
$null = ($i++)?[0]

// $i = 0
locals.Item009 = 0;

// $null = ($i++)?[0]
LanguagePrimitives.IsNull(
    // This is an Expression.Block, not a delegate.
    {
        object @object = locals.Item009;
        locals.Item009 = Fake.Dynamic<Func<CallSite, object, object>>(PSVariableAssignmentBinder.Get())(
            Fake.Dynamic<Func<CallSite, object, object>>(PSUnaryOperationBinder.Get(ExpressionType.Increment))(
                @object));

        return (@object == null) ? 0 : @object;
    })
    ? null
    : Fake.Dynamic<Func<CallSite, object, int, object>>(
        PSGetIndexBinder.Get(
            argCount: 1,
            constraints: null,
            allowSlicing: true))
        (
            // This is an Expression.Block, not a delegate.
            {
                object @object = locals.Item009;
                locals.Item009 = Fake.Dynamic<Func<CallSite, object, object>>(PSVariableAssignmentBinder.Get())(
                    Fake.Dynamic<Func<CallSite, object, object>>(PSUnaryOperationBinder.Get(ExpressionType.Increment))(
                        @object));

                return (@object == null) ? 0 : @object;
            },
            0);

After - target not strongly typed ^{_{(click to expand)}}

$i = 0
$null = ($i++)?[0]

// $i = 0
locals.Item009 = 0;

// $null = ($i++)?[0]
object object1 =
    // This is an Expression.Block, not a delegate.
    {
        object object2 = locals.Item009;
        locals.Item009 = Fake.Dynamic<Func<CallSite, object, object>>(PSVariableAssignmentBinder.Get())(
            Fake.Dynamic<Func<CallSite, object, object>>(
                PSUnaryOperationBinder.Get(ExpressionType.Increment))
            (object2));

        return (object2 == null) ? 0 : object2;
    };

LanguagePrimitives.IsNull(object1)
    ? null
    : Fake.Dynamic<Func<CallSite, object, int, object>>(
        PSGetIndexBinder.Get(
            argCount: 1,
            constraints: null,
            allowSlicing: true))
        (
            object1,
            0);

After - target strongly typed as valuetype ^{_{(click to expand)}}

[int] $i = 0
$null = ($i++)?[0]

// [int] $i = 0
locals.Item009 = 0;

// $null = ($i++)?[0]
Fake.Dynamic<Func<CallSite, int, int, object>>(
    PSGetIndexBinder.Get(argCount: 1, constraints: null, allowSlicing: true))
    (
        // This is an Expression.Block, not a delegate.
        {
           
10BC0
 int @int = locals.Item009;
            locals.Item009 = Fake.Dynamic<Func<CallSite, object, int>>(PSConvertBinder.Get(typeof(int)))(Fake.Dynamic<Func<CallSite, int, object>>(PSUnaryOperationBinder.Get(ExpressionType.Increment))(@int));

            return @int;
        },
        0);

After - target strongly typed as ref type ^{_{(click to expand)}}

[string] $i = ''
$null = ($i += 'x')?[0]

locals.Item009 = "";
string @string = locals.Item009 = Fake.Dynamic<Func<CallSite, object, string>>(
    PSConvertBinder.Get(typeof(string)))
    (
        Fake.Dynamic<Func<CallSite, string, string, object>>(
            PSBinaryOperationBinder.Get(
                ExpressionType.Add,
                ignoreCase: true,
                scalarCompare: false))
            (
                locals.Item009,
                "x"));

LanguagePrimitives.IsNull(@string)
    ? null
    : Fake.Dynamic<Func<CallSite, string, int, object>>(
        PSGetIndexBinder.Get(
            argCount: 1,
            constraints: null,
            allowSlicing: true))
        (
            @string,
            0);

PR Context

PR Checklist

Fixes #24716

SeeminglyScience · 2025-01-15T20:17:22Z

/azp run

azure-pipelines · 2025-01-15T20:17:47Z

Azure Pipelines successfully started running 5 pipeline(s).

daxian-dbw

@SeeminglyScience Thanks for fixing this! Sorry for the delay in review.

daxian-dbw · 2025-03-13T18:07:04Z

src/System.Management.Automation/engine/parser/Compiler.cs

-                : DynamicExpression.Dynamic(PSGetDynamicMemberBinder.Get(_memberFunctionType, memberExpressionAst.Static), typeof(object), target, Compile(memberExpressionAst.Member));
-
-            return memberExpressionAst.NullConditional ? GetNullConditionalWrappedExpression(target, memberAccessExpr) : memberAccessExpr;
+            return MaybeWrapInNullConditionalExpression(


The way you abstract the code with MaybeWrapInNullConditionalExpression feels complex and not very readable.
Take this particular call site as an example, how about abstract it in the way shown with the code below? With this approach, we only need to make minor changes to the existing code

add var targetToUse = section

replace target with targetToUse when constructing the member-access expression

change the call to GetNullConditionalWrappedExpression slightly

var target = CompileExpressionOperand(memberExpressionAst.Expression); // **** Use a temp variable to represent the target for null-conditional **** var targetToUse = memberExpressionAst.NullConditional ? Expression.Parameter(target.Type) : target; // If the ?. operator is used for null conditional check, add the null conditional expression. // **** Generate the member-access expression with `targetToUse` **** var memberNameAst = memberExpressionAst.Member as StringConstantExpressionAst; Expression memberAccessExpr = 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((ParameterExpression)targetToUse, target, memberAccessExpr) : memberAccessExpr;

And change GetNullConditionalWrappedExpression to the following:

private static Expression GetNullConditionalWrappedExpression(ParameterExpression temp, Expression targetExpr, Expression memberAccessExpression) { return Expression.Block( typeof(object), [temp], [ Expression.Assign(temp, targetExpr), Expression.Condition( Expression.Call( CachedReflectionInfo.LanguagePrimitives_IsNull, temp.Cast(typeof(object))), ExpressionCache.NullConstant, memberAccessExpression), ]); }

Definitely agree on the complexity 💯

That suggestion might work! I'll give it a go when I get a minute. Thanks Dongbo!

SeeminglyScience · 2025-03-27T17:03:56Z

/azp run

azure-pipelines · 2025-03-27T17:04:09Z

Azure Pipelines successfully started running 1 pipeline(s).

SeeminglyScience · 2025-03-27T17:16:47Z

~~trying to force tests to rerun~~ had to pull in changes from master

SeeminglyScience · 2025-03-27T17:18:04Z

/azp run

azure-pipelines · 2025-03-27T17:18:16Z

Azure Pipelines successfully started running 1 pipeline(s).

@daxian-dbw

Now uses a temp variable created in the caller instead of a delegate. Thanks to @daxian-dbw for the pattern recommendation!

daxian-dbw · 2025-04-21T21:18:14Z

@SeeminglyScience Some tests are failing in CI, can you please take a look?

microsoft-github-policy-service · 2025-04-28T21:35:33Z

This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days.
Maintainer, please provide feedback and/or mark it as Waiting on Author

Fix double execution of null conditional targets

2527829

Fixes #24716

SeeminglyScience requested a review from daxian-dbw as a code owner January 12, 2025 22:48

microsoft-github-policy-service bot added the Review - Needed The PR is being reviewed label Jan 23, 2025

daxian-dbw reviewed Mar 13, 2025

View reviewed changes

daxian-dbw added this to PowerShell Team Reviews/Investigations Mar 14, 2025

daxian-dbw moved this to PR In Progress in PowerShell Team Reviews/Investigations Mar 14, 2025

daxian-dbw self-assigned this Mar 14, 2025

microsoft-github-policy-service bot removed the Review - Needed The PR is being reviewed label Mar 14, 2025

microsoft-github-policy-service bot added the Review - Needed The PR is being reviewed label Mar 26, 2025

SeeminglyScience closed this Mar 27, 2025

github-project-automation bot moved this from PR In Progress to Done in PowerShell Team Reviews/Investigations Mar 27, 2025

SeeminglyScience reopened this Mar 27, 2025

Merge branch 'master' into SeeminglyScience/issue24716

f6eb92d

daxian-dbw moved this from Done to PR In Progress in PowerShell Team Reviews/Investigations Mar 31, 2025

SeeminglyScience and others added 2 commits March 31, 2025 14:54

Address complexity feedback

4559c51

Now uses a temp variable created in the caller instead of a delegate. Thanks to @daxian-dbw for the pattern recommendation!

Merge branch 'master' into SeeminglyScience/issue24716

2e5d76f

iSazonov added the CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log label Apr 1, 2025

microsoft-github-policy-service bot removed the Review - Needed The PR is being reviewed label Apr 21, 2025

microsoft-github-policy-service bot added the Review - Needed The PR is being reviewed label Apr 28, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix double execution of `Target` expressions in null conditionals#24765

Fix double execution of `Target` expressions in null conditionals#24765
SeeminglyScience wants to merge 4 commits intomasterfrom
SeeminglyScience/issue24716

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

PR Summary

Disassembly

Member Expressions

Before Changes (click to expand)

After - target not strongly typed (click to expand)

After - target strongly typed as valuetype (click to expand)

After - target strongly typed as ref type (click to expand)

Index Expressions

Before Changes (click to expand)

After - target not strongly typed (click to expand)

After - target strongly typed as valuetype (click to expand)

After - target strongly typed as ref type (click to expand)

PR Context

PR Checklist

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh! 10BC0

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Before Changes ^{_{(click to expand)}}

After - target not strongly typed ^{_{(click to expand)}}

After - target strongly typed as valuetype ^{_{(click to expand)}}

After - target strongly typed as ref type ^{_{(click to expand)}}

Before Changes ^{_{(click to expand)}}

After - target not strongly typed ^{_{(click to expand)}}

After - target strongly typed as valuetype ^{_{(click to expand)}}

After - target strongly typed as ref type ^{_{(click to expand)}}