From a2c2a7698ced12eff09f2ea6fa472d0b52db1d10 Mon Sep 17 00:00:00 2001 From: Dongbo Wang Date: Fri, 5 Aug 2022 14:15:48 -0700 Subject: [PATCH 1/3] Do not preserve temporary results when no need to do so --- .../engine/parser/Compiler.cs | 27 +++++++++--------- .../Scripting/Scripting.Followup.Tests.ps1 | 28 +++++++++++++++++++ 2 files changed, 41 insertions(+), 14 deletions(-) diff --git a/src/System.Management.Automation/engine/parser/Compiler.cs b/src/System.Management.Automation/engine/parser/Compiler.cs index cdf053dcb2e..563a7c7631c 100644 --- a/src/System.Management.Automation/engine/parser/Compiler.cs +++ b/src/System.Management.Automation/engine/parser/Compiler.cs @@ -2278,27 +2278,26 @@ private Expression CaptureAstResults( switch (context) { case CaptureAstContext.AssignmentWithResultPreservation: - case CaptureAstContext.AssignmentWithoutResultPreservation: result = Expression.Call(CachedReflectionInfo.PipelineOps_PipelineResult, resultList); - // Clear the temporary pipe in case of exception, if we are not required to preserve the results - if (context == CaptureAstContext.AssignmentWithoutResultPreservation) - { - var catchExprs = new List - { - Expression.Call(CachedReflectionInfo.PipelineOps_ClearPipe, resultList), - Expression.Rethrow(), - Expression.Constant(null, typeof(object)) - }; - - catches.Add(Expression.Catch(typeof(RuntimeException), Expression.Block(typeof(object), catchExprs))); - } - // PipelineResult might get skipped in some circumstances due to a FlowControlException thrown out, in which case // we write to the oldPipe. This can happen in cases like: // $(1;2;return 3) finallyExprs.Add(Expression.Call(CachedReflectionInfo.PipelineOps_FlushPipe, oldPipe, resultList)); break; + case CaptureAstContext.AssignmentWithoutResultPreservation: + result = Expression.Call(CachedReflectionInfo.PipelineOps_PipelineResult, resultList); + + // Clear the temporary pipe in case of exception, if we are not required to preserve the results + var catchExprs = new List + { + Expression.Call(CachedReflectionInfo.PipelineOps_ClearPipe, resultList), + Expression.Rethrow(), + Expression.Constant(null, typeof(object)) + }; + + catches.Add(Expression.Catch(typeof(RuntimeException), Expression.Block(typeof(object), catchExprs))); + break; case CaptureAstContext.Condition: result = DynamicExpression.Dynamic(PSPipelineResultToBoolBinder.Get(), typeof(bool), resultList); break; diff --git a/test/powershell/Language/Scripting/Scripting.Followup.Tests.ps1 b/test/powershell/Language/Scripting/Scripting.Followup.Tests.ps1 index 933c53de6de..7491d6e8205 100644 --- a/test/powershell/Language/Scripting/Scripting.Followup.Tests.ps1 +++ b/test/powershell/Language/Scripting/Scripting.Followup.Tests.ps1 @@ -68,4 +68,32 @@ Describe "Scripting.Followup.Tests" -Tags "CI" { $result = [ordered]@{ key = 1 } $result | Should -BeOfType 'System.Collections.Specialized.OrderedDictionary' } + + It "Don't preserve result when no need to do so in case of flow-control exception" { + function TestFunc1([switch]$p) { + ## No need to preserve and flush the results from the IF statement to the outer + ## pipeline, because the results are supposed to be assigned to a variable. + if ($p) { + $null = if ($true) { "one"; return "two" } + } else { + $a = foreach ($a in 1) { "one"; return; } + } + } + + function TestFunc2([switch]$p) { + ## The results from the sub-expression and IF statement need to be preserved and + ## flushed to the outer pipeline. + if ($p) { + $("1";return "2") + } else { + if ($true) { "one"; return "two" } + } + } + + TestFunc1 | Should -Be $null + TestFunc1 -p | Should -Be $null + + TestFunc2 | Should -Be @("one", "two") + TestFunc2 -p | Should -Be @("1", "2") + } } From bf82856cb4a83ce6eef92f4d78b7aa732c8c4aeb Mon Sep 17 00:00:00 2001 From: Dongbo Wang Date: Fri, 5 Aug 2022 14:25:06 -0700 Subject: [PATCH 2/3] Update test --- .../Scripting/Scripting.Followup.Tests.ps1 | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-) diff --git a/test/powershell/Language/Scripting/Scripting.Followup.Tests.ps1 b/test/powershell/Language/Scripting/Scripting.Followup.Tests.ps1 index 7491d6e8205..0d4e8245e2c 100644 --- a/test/powershell/Language/Scripting/Scripting.Followup.Tests.ps1 +++ b/test/powershell/Language/Scripting/Scripting.Followup.Tests.ps1 @@ -80,20 +80,14 @@ Describe "Scripting.Followup.Tests" -Tags "CI" { } } - function TestFunc2([switch]$p) { - ## The results from the sub-expression and IF statement need to be preserved and - ## flushed to the outer pipeline. - if ($p) { - $("1";return "2") - } else { - if ($true) { "one"; return "two" } - } + function TestFunc2 { + ## The results from the sub-expression need to be preserved and flushed to the outer pipeline. + $("1";return "2") } TestFunc1 | Should -Be $null TestFunc1 -p | Should -Be $null - TestFunc2 | Should -Be @("one", "two") - TestFunc2 -p | Should -Be @("1", "2") + TestFunc2 | Should -Be @("1", "2") } } From 46fd8eb63cd128a5735a35b8548ea9cb3583af6a Mon Sep 17 00:00:00 2001 From: Dongbo Wang Date: Mon, 8 Aug 2022 10:24:07 -0700 Subject: [PATCH 3/3] Update src/System.Management.Automation/engine/parser/Compiler.cs Co-authored-by: Patrick Meinecke --- src/System.Management.Automation/engine/parser/Compiler.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/System.Management.Automation/engine/parser/Compiler.cs b/src/System.Management.Automation/engine/parser/Compiler.cs index 563a7c7631c..cf7527069db 100644 --- a/src/System.Management.Automation/engine/parser/Compiler.cs +++ b/src/System.Management.Automation/engine/parser/Compiler.cs @@ -2280,7 +2280,7 @@ private Expression CaptureAstResults( case CaptureAstContext.AssignmentWithResultPreservation: result = Expression.Call(CachedReflectionInfo.PipelineOps_PipelineResult, resultList); - // PipelineResult might get skipped in some circumstances due to a FlowControlException thrown out, in which case + // PipelineResult might get skipped in some circumstances due to an early return or a FlowControlException thrown out, in which case // we write to the oldPipe. This can happen in cases like: // $(1;2;return 3) finallyExprs.Add(Expression.Call(CachedReflectionInfo.PipelineOps_FlushPipe, oldPipe, resultList));