From 43faebd241800ffd31b1cc0cee7ab0005dbb4c1b Mon Sep 17 00:00:00 2001 From: Robert Holt Date: Thu, 14 May 2020 14:50:38 -0700 Subject: [PATCH 1/4] Ensure null-coalescing LHS is evaluated only once --- .../engine/parser/Compiler.cs | 23 +++++++++-------- .../Operators/NullConditional.Tests.ps1 | 25 +++++++++++++++++++ 2 files changed, 37 insertions(+), 11 deletions(-) diff --git a/src/System.Management.Automation/engine/parser/Compiler.cs b/src/System.Management.Automation/engine/parser/Compiler.cs index ac40050d4ab..2adcad95ceb 100644 --- a/src/System.Management.Automation/engine/parser/Compiler.cs +++ b/src/System.Management.Automation/engine/parser/Compiler.cs @@ -982,19 +982,20 @@ private static Expression Coalesce(Expression left, Expression right) { return left; } - else if (leftType == typeof(AutomationNull)) - { - return right; - } else { - Expression lhs = left.Cast(typeof(object)); - Expression rhs = right.Cast(typeof(object)); - - return Expression.Condition( - Expression.Call(CachedReflectionInfo.LanguagePrimitives_IsNull, lhs), - rhs, - lhs); + ParameterExpression lhsStoreVar = Expression.Variable(typeof(object)); + return Expression.Block( + typeof(object), + new ParameterExpression[] { lhsStoreVar }, + new Expression[] + { + Expression.Assign(lhsStoreVar, left.Cast(typeof(object))), + Expression.Condition( + Expression.Call(CachedReflectionInfo.LanguagePrimitives_IsNull, lhsStoreVar), + right.Cast(typeof(object)), + lhsStoreVar), + }); } } diff --git a/test/powershell/Language/Operators/NullConditional.Tests.ps1 b/test/powershell/Language/Operators/NullConditional.Tests.ps1 index b24bb87d352..0deb9d56589 100644 --- a/test/powershell/Language/Operators/NullConditional.Tests.ps1 +++ b/test/powershell/Language/Operators/NullConditional.Tests.ps1 @@ -103,7 +103,22 @@ Describe 'NullCoalesceOperations' -Tags 'CI' { } Context 'Null coalesce operator ??' { + BeforeAll { + function Set-TestStateWithStringResult { + 'Test' + [void]$testState.Value++ + } + + function Set-TestStateWithNullResult { + [void]$testState.Value++ + } + } + BeforeEach { + $testState = [pscustomobject]@{ + Value = 0 + } + $x = $null } @@ -172,6 +187,16 @@ Describe 'NullCoalesceOperations' -Tags 'CI' { It 'Lhs is $?' { {$???$false} | Should -BeTrue } + + It 'Should only evaluate LHS once when it is NOT null' { + (Set-TestStateWithStringResult) ?? 'Nothing' | Should -BeExactly 'Test' + $testState.Value | Should -Be 1 + } + + It 'Should only evaluate LHS once when it IS null' { + (Set-TestStateWithNullResult) ?? 'Nothing' | Should -BeExactly 'Nothing' + $testState.Value | Should -Be 1 + } } Context 'Null Coalesce ?? operator precedence' { From 823517d46ba9eb2681ec28850d93f3facf2b2949 Mon Sep 17 00:00:00 2001 From: Robert Holt Date: Thu, 14 May 2020 14:58:03 -0700 Subject: [PATCH 2/4] Optimise test state creation --- test/powershell/Language/Operators/NullConditional.Tests.ps1 | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/test/powershell/Language/Operators/NullConditional.Tests.ps1 b/test/powershell/Language/Operators/NullConditional.Tests.ps1 index 0deb9d56589..497d2baa303 100644 --- a/test/powershell/Language/Operators/NullConditional.Tests.ps1 +++ b/test/powershell/Language/Operators/NullConditional.Tests.ps1 @@ -112,13 +112,14 @@ Describe 'NullCoalesceOperations' -Tags 'CI' { function Set-TestStateWithNullResult { [void]$testState.Value++ } - } - BeforeEach { $testState = [pscustomobject]@{ Value = 0 } + } + BeforeEach { + $testState.Value = 0 $x = $null } From fddb646c8863c355be3befb7d1858318080c640f Mon Sep 17 00:00:00 2001 From: Robert Holt Date: Thu, 14 May 2020 15:11:30 -0700 Subject: [PATCH 3/4] Pander to CodeFactor's brainless opinions --- .../engine/parser/Compiler.cs | 21 +++++++++++-------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/src/System.Management.Automation/engine/parser/Compiler.cs b/src/System.Management.Automation/engine/parser/Compiler.cs index 2adcad95ceb..776691a68f0 100644 --- a/src/System.Management.Automation/engine/parser/Compiler.cs +++ b/src/System.Management.Automation/engine/parser/Compiler.cs @@ -985,17 +985,20 @@ private static Expression Coalesce(Expression left, Expression right) else { ParameterExpression lhsStoreVar = Expression.Variable(typeof(object)); + var blockParameters = new ParameterExpression[] { lhsStoreVar }; + var blockStatements = new Expression[] + { + Expression.Assign(lhsStoreVar, left.Cast(typeof(object))), + Expression.Condition( + Expression.Call(CachedReflectionInfo.LanguagePrimitives_IsNull, lhsStoreVar), + right.Cast(typeof(object)), + lhsStoreVar), + }; + return Expression.Block( typeof(object), - new ParameterExpression[] { lhsStoreVar }, - new Expression[] - { - Expression.Assign(lhsStoreVar, left.Cast(typeof(object))), - Expression.Condition( - Expression.Call(CachedReflectionInfo.LanguagePrimitives_IsNull, lhsStoreVar), - right.Cast(typeof(object)), - lhsStoreVar), - }); + blockParameters, + blockStatements); } } From 7170f35a9981bce804470d3231229224b21eba21 Mon Sep 17 00:00:00 2001 From: Robert Holt Date: Fri, 15 May 2020 09:50:03 -0700 Subject: [PATCH 4/4] Address @iSazonov's feedback --- .../Operators/NullConditional.Tests.ps1 | 26 +++++-------------- 1 file changed, 6 insertions(+), 20 deletions(-) diff --git a/test/powershell/Language/Operators/NullConditional.Tests.ps1 b/test/powershell/Language/Operators/NullConditional.Tests.ps1 index 497d2baa303..5c2c65e4ad9 100644 --- a/test/powershell/Language/Operators/NullConditional.Tests.ps1 +++ b/test/powershell/Language/Operators/NullConditional.Tests.ps1 @@ -103,23 +103,7 @@ Describe 'NullCoalesceOperations' -Tags 'CI' { } Context 'Null coalesce operator ??' { - BeforeAll { - function Set-TestStateWithStringResult { - 'Test' - [void]$testState.Value++ - } - - function Set-TestStateWithNullResult { - [void]$testState.Value++ - } - - $testState = [pscustomobject]@{ - Value = 0 - } - } - BeforeEach { - $testState.Value = 0 $x = $null } @@ -189,13 +173,15 @@ Describe 'NullCoalesceOperations' -Tags 'CI' { {$???$false} | Should -BeTrue } - It 'Should only evaluate LHS once when it is NOT null' { - (Set-TestStateWithStringResult) ?? 'Nothing' | Should -BeExactly 'Test' + It 'Should only evaluate LHS once when it IS null' { + $testState = [pscustomobject]@{ Value = 0 } + (& { [void]$testState.Value++ }) ?? 'Nothing' | Should -BeExactly 'Nothing' $testState.Value | Should -Be 1 } - It 'Should only evaluate LHS once when it IS null' { - (Set-TestStateWithNullResult) ?? 'Nothing' | Should -BeExactly 'Nothing' + It 'Should only evaluate LHS once when it is NOT null' { + $testState = [pscustomobject]@{ Value = 0 } + (& { 'Test'; [void]$testState.Value++ }) ?? 'Nothing' | Should -BeExactly 'Test' $testState.Value | Should -Be 1 } }