From a012afbe18c28fa49288d408d059148e730a3a60 Mon Sep 17 00:00:00 2001 From: Paul Higinbotham Date: Fri, 3 Dec 2021 16:07:24 -0800 Subject: [PATCH 1/7] First try at fix --- .../engine/hostifaces/PSTask.cs | 1 + .../engine/runtime/ScriptBlockToPowerShell.cs | 2 +- .../Foreach-Object-Parallel.Tests.ps1 | 97 ++++++++++++++++++- 3 files changed, 98 insertions(+), 2 deletions(-) diff --git a/src/System.Management.Automation/engine/hostifaces/PSTask.cs b/src/System.Management.Automation/engine/hostifaces/PSTask.cs index 6e08733cc2b..55da4061363 100644 --- a/src/System.Management.Automation/engine/hostifaces/PSTask.cs +++ b/src/System.Management.Automation/engine/hostifaces/PSTask.cs @@ -192,6 +192,7 @@ internal sealed class PSJobTask : PSTaskBase /// Dollar underbar variable value for script block. /// Current working directory. /// Job object associated with task. + /// public PSJobTask( ScriptBlock scriptBlock, Dictionary usingValuesMap, diff --git a/src/System.Management.Automation/engine/runtime/ScriptBlockToPowerShell.cs b/src/System.Management.Automation/engine/runtime/ScriptBlockToPowerShell.cs index fc1dba4aed8..edfb41f1916 100644 --- a/src/System.Management.Automation/engine/runtime/ScriptBlockToPowerShell.cs +++ b/src/System.Management.Automation/engine/runtime/ScriptBlockToPowerShell.cs @@ -452,7 +452,7 @@ private static bool IsInForeachParallelCallingScope( currentParent = currentParent.Parent; } - return foreachNestedCount == 1; + return foreachNestedCount <= 1; } /// diff --git a/test/powershell/Modules/Microsoft.PowerShell.Utility/Foreach-Object-Parallel.Tests.ps1 b/test/powershell/Modules/Microsoft.PowerShell.Utility/Foreach-Object-Parallel.Tests.ps1 index 994d71601a7..d36a1740267 100644 --- a/test/powershell/Modules/Microsoft.PowerShell.Utility/Foreach-Object-Parallel.Tests.ps1 +++ b/test/powershell/Modules/Microsoft.PowerShell.Utility/Foreach-Object-Parallel.Tests.ps1 @@ -87,6 +87,101 @@ Describe 'ForEach-Object -Parallel Basic Tests' -Tags 'CI' { $usingErrors[0].FullyQualifiedErrorId | Should -BeExactly 'UsingVariableIsUndefined,Microsoft.PowerShell.Commands.ForEachObjectCommand' } + It 'Verifies using variables with passed-in script block object' { + + $var = "Hello" + $varArray = "Hello","There" + $sBlock = [scriptblock]::Create('$using:var; $using:varArray[1]') + $result = 1..1 | ForEach-Object -Parallel $sBlock + $result[0] | Should -BeExactly $var + $result[1] | Should -BeExactly $varArray[1] + } + + It 'Verifies in scope using same-name variables in nested calls for passed-in script block objects' { + + $Test = "Test1" + $sBlock = [scriptblock]::Create(@' + $using:Test1 + $Test = "Test2" + $sBlock2 = [scriptblock]::Create('$using:Test') + 1..2 | ForEach-Object -Parallel $sBlock2 +'@) + $results = 1..2 | ForEach-Object -Parallel $sBlock + $results.Count | Should -BeExactly 6 + $groups = $results | Group-Object -AsHashTable + $groups['Test1'].Count | Should -BeExactly 2 + $groups['Test2'].Count | Should -BeExactly 4 + } + + It 'Verifies in scope using same-name variables in nested calls for mixed script block objects' { + + $Test = "Test1" + $sBlock = [scriptblock]::Create(@' + $using:Test1 + $Test = "Test2" + 1..2 | ForEach-Object -Parallel { $using:Test } +'@) + $results = 1..2 | ForEach-Object -Parallel $sBlock + $results.Count | Should -BeExactly 6 + $groups = $results | Group-Object -AsHashTable + $groups['Test1'].Count | Should -BeExactly 2 + $groups['Test2'].Count | Should -BeExactly 4 + } + + It 'Verifies in scope using different-name variables in nested calls for passed-in script block objects' { + + $Test1 = "Test1" + $sBlock = [scriptblock]::Create(@' + $using:Test1 + $Test2 = "Test2" + $sBlock2 = [scriptblock]::Create('$using:Test2') + 1..2 | ForEach-Object -Parallel $sBlock2 +'@) + $results = 1..2 | ForEach-Object -Parallel $sBlock + $results.Count | Should -BeExactly 6 + $groups = $results | Group-Object -AsHashTable + $groups['Test1'].Count | Should -BeExactly 2 + $groups['Test2'].Count | Should -BeExactly 4 + } + + It 'Verifies in scope using different-name variables in nested calls for mixed script block objects' { + + $Test1 = "Test1" + $sBlock = [scriptblock]::Create(@' + $using:Test1 + $Test2 = "Test2" + 1..2 | ForEach-Object -Parallel { $using:Test2 } +'@) + $results = 1..2 | ForEach-Object -Parallel $sBlock + $results.Count | Should -BeExactly 6 + $groups = $results | Group-Object -AsHashTable + $groups['Test1'].Count | Should -BeExactly 2 + $groups['Test2'].Count | Should -BeExactly 4 + } + + It 'Verifies expected error for out of scope using variable in nested calls for passed-in script block objects' { + + $Test = "TestZ" + $sBlock = [scriptblock]::Create(@' + $using:Test + $sBlock2 = [scriptblock]::Create('$using:Test') + 1..1 | ForEach-Object -Parallel $sBlock2 +'@) + 1..1 | ForEach-Object -Parallel $SBlock -ErrorVariable usingErrors 2>$null + $usingErrors[0].FullyQualifiedErrorId | Should -BeExactly 'UsingVariableIsUndefined,Microsoft.PowerShell.Commands.ForEachObjectCommand' + } + + It 'Verifies expected error for out of scope using variable in nested calls for mixed script block objects' { + + $Test = "TestZ" + $sBlock = [scriptblock]::Create(@' + $using:Test + 1..1 | ForEach-Object -Parallel { $using:Test } +'@) + 1..1 | ForEach-Object -Parallel $SBlock -ErrorVariable usingErrors 2>$null + $usingErrors[0].FullyQualifiedErrorId | Should -BeExactly 'UsingVariableIsUndefined,Microsoft.PowerShell.Commands.ForEachObjectCommand' + } + It 'Verifies terminating error streaming' { $result = 1..1 | ForEach-Object -Parallel { throw 'Terminating Error!'; "Hello" } 2>&1 @@ -150,7 +245,7 @@ Describe 'ForEach-Object -Parallel Basic Tests' -Tags 'CI' { $pr[0].StatusDescription | Should -Be Running $ps.Dispose() } - + It 'Verifies information data streaming' { $actualInformation = 1..1 | ForEach-Object -Parallel { Write-Information "Information!" } 6>&1 From 7e0d73159d77163d482924c2d42da20b768c828f Mon Sep 17 00:00:00 2001 From: Paul Higinbotham Date: Mon, 6 Dec 2021 15:58:35 -0800 Subject: [PATCH 2/7] Fix scope function to correctly account for provided script block type. --- .../engine/runtime/ScriptBlockToPowerShell.cs | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/src/System.Management.Automation/engine/runtime/ScriptBlockToPowerShell.cs b/src/System.Management.Automation/engine/runtime/ScriptBlockToPowerShell.cs index edfb41f1916..aab16dbd6d8 100644 --- a/src/System.Management.Automation/engine/runtime/ScriptBlockToPowerShell.cs +++ b/src/System.Management.Automation/engine/runtime/ScriptBlockToPowerShell.cs @@ -350,7 +350,10 @@ internal static Dictionary GetUsingValuesForEachParallel( for (int i = 0; i < usingAsts.Count; ++i) { usingAst = (UsingExpressionAst)usingAsts[i]; - if (IsInForeachParallelCallingScope(usingAst, foreachNames)) + if (IsInForeachParallelCallingScope( + usingAst: usingAst, + astIncludesForEachCommand: scriptBlock.Ast.Parent is not null, + foreachNames: foreachNames)) { var value = Compiler.GetExpressionValue(usingAst.SubExpression, isTrustedInput, context); string usingAstKey = PsUtils.GetUsingExpressionKey(usingAst); @@ -387,10 +390,17 @@ internal static Dictionary GetUsingValuesForEachParallel( /// and parameter set scope, and not from within a nested foreach-object -parallel call. /// /// Using Ast to check. + /// + /// True when the provided ast includes a parent containing the originating ForEach-Object command + /// e.g., + /// '1 | ForEach-Object -Parallel { }' - script block ast includes parent with initial 'ForEach-Object' + /// '1 | ForEach-Object -Parallel $scriptBlock' - script block ast *does not* include parent with initial 'ForEach-Object' + /// /// List of foreach-object command names. /// True if using expression is in current call scope. private static bool IsInForeachParallelCallingScope( UsingExpressionAst usingAst, + bool astIncludesForEachCommand, string[] foreachNames) { /* @@ -452,7 +462,8 @@ private static bool IsInForeachParallelCallingScope( currentParent = currentParent.Parent; } - return foreachNestedCount <= 1; + // Expected foreachNestedCount depends on provided script block ast (see above). + return astIncludesForEachCommand ? foreachNestedCount == 1 : foreachNestedCount == 0; } /// From 22d2b87c39fb86534b7114784e3572e26813b0c2 Mon Sep 17 00:00:00 2001 From: Paul Higinbotham Date: Mon, 6 Dec 2021 16:51:11 -0800 Subject: [PATCH 3/7] Fix tests --- .../engine/runtime/ScriptBlockToPowerShell.cs | 5 ++++- .../Foreach-Object-Parallel.Tests.ps1 | 12 ++++++------ 2 files changed, 10 insertions(+), 7 deletions(-) diff --git a/src/System.Management.Automation/engine/runtime/ScriptBlockToPowerShell.cs b/src/System.Management.Automation/engine/runtime/ScriptBlockToPowerShell.cs index aab16dbd6d8..927acf4c3e7 100644 --- a/src/System.Management.Automation/engine/runtime/ScriptBlockToPowerShell.cs +++ b/src/System.Management.Automation/engine/runtime/ScriptBlockToPowerShell.cs @@ -336,6 +336,9 @@ internal static Dictionary GetUsingValuesForEachParallel( // Foreach-Object -Parallel call scope. This will filter the using variable map to variables // only within the current (outer) Foreach-Object -Parallel call scope. var usingAsts = UsingExpressionAstSearcher.FindAllUsingExpressions(scriptBlock.Ast).ToList(); + // If the scriptblock ast parent is null, then it comes from a scriptblock variable and does + // not include the initiating Foreach-Object -Parallel command. + bool astIncludesForEachCommand = scriptBlock.Ast.Parent is not null; UsingExpressionAst usingAst = null; var usingValueMap = new Dictionary(usingAsts.Count); Version oldStrictVersion = null; @@ -352,7 +355,7 @@ internal static Dictionary GetUsingValuesForEachParallel( usingAst = (UsingExpressionAst)usingAsts[i]; if (IsInForeachParallelCallingScope( usingAst: usingAst, - astIncludesForEachCommand: scriptBlock.Ast.Parent is not null, + astIncludesForEachCommand: astIncludesForEachCommand, foreachNames: foreachNames)) { var value = Compiler.GetExpressionValue(usingAst.SubExpression, isTrustedInput, context); diff --git a/test/powershell/Modules/Microsoft.PowerShell.Utility/Foreach-Object-Parallel.Tests.ps1 b/test/powershell/Modules/Microsoft.PowerShell.Utility/Foreach-Object-Parallel.Tests.ps1 index d36a1740267..67bf8ce5d0f 100644 --- a/test/powershell/Modules/Microsoft.PowerShell.Utility/Foreach-Object-Parallel.Tests.ps1 +++ b/test/powershell/Modules/Microsoft.PowerShell.Utility/Foreach-Object-Parallel.Tests.ps1 @@ -99,11 +99,11 @@ Describe 'ForEach-Object -Parallel Basic Tests' -Tags 'CI' { It 'Verifies in scope using same-name variables in nested calls for passed-in script block objects' { - $Test = "Test1" + $Test1 = "Test1" $sBlock = [scriptblock]::Create(@' $using:Test1 - $Test = "Test2" - $sBlock2 = [scriptblock]::Create('$using:Test') + $Test2 = "Test2" + $sBlock2 = [scriptblock]::Create('$using:Test2') 1..2 | ForEach-Object -Parallel $sBlock2 '@) $results = 1..2 | ForEach-Object -Parallel $sBlock @@ -115,11 +115,11 @@ Describe 'ForEach-Object -Parallel Basic Tests' -Tags 'CI' { It 'Verifies in scope using same-name variables in nested calls for mixed script block objects' { - $Test = "Test1" + $Test1 = "Test1" $sBlock = [scriptblock]::Create(@' $using:Test1 - $Test = "Test2" - 1..2 | ForEach-Object -Parallel { $using:Test } + $Test2 = "Test2" + 1..2 | ForEach-Object -Parallel { $using:Test2 } '@) $results = 1..2 | ForEach-Object -Parallel $sBlock $results.Count | Should -BeExactly 6 From afa912047c78ceb5e2b259e365c5672b564a2a52 Mon Sep 17 00:00:00 2001 From: Paul Higinbotham Date: Tue, 7 Dec 2021 10:11:18 -0800 Subject: [PATCH 4/7] Update src/System.Management.Automation/engine/runtime/ScriptBlockToPowerShell.cs Co-authored-by: Ilya --- .../engine/runtime/ScriptBlockToPowerShell.cs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/System.Management.Automation/engine/runtime/ScriptBlockToPowerShell.cs b/src/System.Management.Automation/engine/runtime/ScriptBlockToPowerShell.cs index 927acf4c3e7..4715afba264 100644 --- a/src/System.Management.Automation/engine/runtime/ScriptBlockToPowerShell.cs +++ b/src/System.Management.Automation/engine/runtime/ScriptBlockToPowerShell.cs @@ -336,6 +336,7 @@ internal static Dictionary GetUsingValuesForEachParallel( // Foreach-Object -Parallel call scope. This will filter the using variable map to variables // only within the current (outer) Foreach-Object -Parallel call scope. var usingAsts = UsingExpressionAstSearcher.FindAllUsingExpressions(scriptBlock.Ast).ToList(); + // If the scriptblock ast parent is null, then it comes from a scriptblock variable and does // not include the initiating Foreach-Object -Parallel command. bool astIncludesForEachCommand = scriptBlock.Ast.Parent is not null; From b76969a9c44f376485c7648bb815dce08536444d Mon Sep 17 00:00:00 2001 From: Paul Higinbotham Date: Tue, 7 Dec 2021 10:14:50 -0800 Subject: [PATCH 5/7] PR comments --- src/System.Management.Automation/engine/hostifaces/PSTask.cs | 1 - .../engine/runtime/ScriptBlockToPowerShell.cs | 1 + 2 files changed, 1 insertion(+), 1 deletion(-) diff --git a/src/System.Management.Automation/engine/hostifaces/PSTask.cs b/src/System.Management.Automation/engine/hostifaces/PSTask.cs index 55da4061363..6e08733cc2b 100644 --- a/src/System.Management.Automation/engine/hostifaces/PSTask.cs +++ b/src/System.Management.Automation/engine/hostifaces/PSTask.cs @@ -192,7 +192,6 @@ internal sealed class PSJobTask : PSTaskBase /// Dollar underbar variable value for script block. /// Current working directory. /// Job object associated with task. - /// public PSJobTask( ScriptBlock scriptBlock, Dictionary usingValuesMap, diff --git a/src/System.Management.Automation/engine/runtime/ScriptBlockToPowerShell.cs b/src/System.Management.Automation/engine/runtime/ScriptBlockToPowerShell.cs index 927acf4c3e7..926a9a58a18 100644 --- a/src/System.Management.Automation/engine/runtime/ScriptBlockToPowerShell.cs +++ b/src/System.Management.Automation/engine/runtime/ScriptBlockToPowerShell.cs @@ -336,6 +336,7 @@ internal static Dictionary GetUsingValuesForEachParallel( // Foreach-Object -Parallel call scope. This will filter the using variable map to variables // only within the current (outer) Foreach-Object -Parallel call scope. var usingAsts = UsingExpressionAstSearcher.FindAllUsingExpressions(scriptBlock.Ast).ToList(); + // If the scriptblock ast parent is null, then it comes from a scriptblock variable and does // not include the initiating Foreach-Object -Parallel command. bool astIncludesForEachCommand = scriptBlock.Ast.Parent is not null; From 2ae7a3f677ca16c115ce663e1baeeb8b14dbf22b Mon Sep 17 00:00:00 2001 From: Paul Higinbotham Date: Fri, 10 Dec 2021 12:04:43 -0800 Subject: [PATCH 6/7] Fix using scope check to account for other type of passed in script block object --- .../engine/InternalCommands.cs | 14 +-- .../engine/runtime/ScriptBlockToPowerShell.cs | 105 +++++++++++------- .../Foreach-Object-Parallel.Tests.ps1 | 84 ++++++++++++++ 3 files changed, 151 insertions(+), 52 deletions(-) diff --git a/src/System.Management.Automation/engine/InternalCommands.cs b/src/System.Management.Automation/engine/InternalCommands.cs index d9d54539bac..c642e30e885 100644 --- a/src/System.Management.Automation/engine/InternalCommands.cs +++ b/src/System.Management.Automation/engine/InternalCommands.cs @@ -382,17 +382,6 @@ public void Dispose() private Exception _taskCollectionException; private string _currentLocationPath; - // List of Foreach-Object command names and aliases. - // TODO: Look into using SessionState.Internal.GetAliasTable() to find all user created aliases. - // But update Alias command logic to maintain reverse table that lists all aliases mapping - // to a single command definition, for performance. - private static readonly string[] forEachNames = new string[] - { - "ForEach-Object", - "foreach", - "%" - }; - private void InitParallelParameterSet() { // The following common parameters are not (yet) supported in this parameter set. @@ -423,8 +412,7 @@ private void InitParallelParameterSet() _usingValuesMap = ScriptBlockToPowerShellConverter.GetUsingValuesForEachParallel( scriptBlock: Parallel, isTrustedInput: allowUsingExpression, - context: this.Context, - foreachNames: forEachNames); + context: this.Context); // Validate using values map, which is a map of '$using:' variables referenced in the script. // Script block variables are not allowed since their behavior is undefined outside the runspace diff --git a/src/System.Management.Automation/engine/runtime/ScriptBlockToPowerShell.cs b/src/System.Management.Automation/engine/runtime/ScriptBlockToPowerShell.cs index 926a9a58a18..747eb580728 100644 --- a/src/System.Management.Automation/engine/runtime/ScriptBlockToPowerShell.cs +++ b/src/System.Management.Automation/engine/runtime/ScriptBlockToPowerShell.cs @@ -324,22 +324,17 @@ internal static PowerShell Convert(ScriptBlockAst body, /// Scriptblock to search. /// True when input is trusted. /// Execution context. - /// List of foreach command names and aliases. /// Dictionary of using variable map. internal static Dictionary GetUsingValuesForEachParallel( ScriptBlock scriptBlock, bool isTrustedInput, - ExecutionContext context, - string[] foreachNames) + ExecutionContext context) { // Using variables for Foreach-Object -Parallel use are restricted to be within the // Foreach-Object -Parallel call scope. This will filter the using variable map to variables // only within the current (outer) Foreach-Object -Parallel call scope. var usingAsts = UsingExpressionAstSearcher.FindAllUsingExpressions(scriptBlock.Ast).ToList(); - - // If the scriptblock ast parent is null, then it comes from a scriptblock variable and does - // not include the initiating Foreach-Object -Parallel command. - bool astIncludesForEachCommand = scriptBlock.Ast.Parent is not null; + bool astIncludesForEachCommand = CheckScriptBlockForForeachCommand(scriptBlock); UsingExpressionAst usingAst = null; var usingValueMap = new Dictionary(usingAsts.Count); Version oldStrictVersion = null; @@ -356,8 +351,7 @@ internal static Dictionary GetUsingValuesForEachParallel( usingAst = (UsingExpressionAst)usingAsts[i]; if (IsInForeachParallelCallingScope( usingAst: usingAst, - astIncludesForEachCommand: astIncludesForEachCommand, - foreachNames: foreachNames)) + astIncludesForEachCommand: astIncludesForEachCommand)) { var value = Compiler.GetExpressionValue(usingAst.SubExpression, isTrustedInput, context); string usingAstKey = PsUtils.GetUsingExpressionKey(usingAst); @@ -389,6 +383,65 @@ internal static Dictionary GetUsingValuesForEachParallel( return usingValueMap; } + // List of Foreach-Object command names and aliases. + // TODO: Look into using SessionState.Internal.GetAliasTable() to find all user created aliases. + // But update Alias command logic to maintain reverse table that lists all aliases mapping + // to a single command definition, for performance. + private static readonly string[] forEachNames = new string[] + { + "ForEach-Object", + "foreach", + "%" + }; + + private static bool FindForEachInCommand(CommandAst commandAst) + { + foreach (var commandElement in commandAst.CommandElements) + { + if (commandElement is StringConstantExpressionAst commandName) + { + bool found = false; + foreach (var foreachName in forEachNames) + { + if (commandName.Value.Equals(foreachName, StringComparison.OrdinalIgnoreCase)) + { + found = true; + break; + } + } + + if (found) + { + // Verify this is foreach-object with parallel parameter set. + var bindingResult = StaticParameterBinder.BindCommand(commandAst); + if (bindingResult.BoundParameters.ContainsKey("Parallel")) + { + return true; + } + } + } + } + + return false; + } + + private static bool CheckScriptBlockForForeachCommand(ScriptBlock scriptBlock) + { + var currentAst = scriptBlock.Ast; + while (currentAst is not null) + { + if (currentAst is CommandAst commandAst && + FindForEachInCommand(commandAst)) + { + return true; + } + + currentAst = currentAst.Parent; + } + + return false; + } + /// /// Walks the using Ast to verify it is used within a foreach-object -parallel command /// and parameter set scope, and not from within a nested foreach-object -parallel call. @@ -400,12 +453,10 @@ internal static Dictionary GetUsingValuesForEachParallel( /// '1 | ForEach-Object -Parallel { }' - script block ast includes parent with initial 'ForEach-Object' /// '1 | ForEach-Object -Parallel $scriptBlock' - script block ast *does not* include parent with initial 'ForEach-Object' /// - /// List of foreach-object command names. /// True if using expression is in current call scope. private static bool IsInForeachParallelCallingScope( UsingExpressionAst usingAst, - bool astIncludesForEachCommand, - string[] foreachNames) + bool astIncludesForEachCommand) { /* Example: @@ -427,34 +478,10 @@ private static bool IsInForeachParallelCallingScope( while (currentParent != null) { // Look for Foreach-Object outer commands - if (currentParent is CommandAst commandAst) + if (currentParent is CommandAst commandAst && + FindForEachInCommand(commandAst)) { - foreach (var commandElement in commandAst.CommandElements) - { - if (commandElement is StringConstantExpressionAst commandName) - { - bool found = false; - foreach (var foreachName in foreachNames) - { - if (commandName.Value.Equals(foreachName, StringComparison.OrdinalIgnoreCase)) - { - found = true; - break; - } - } - - if (found) - { - // Verify this is foreach-object with parallel parameter set. - var bindingResult = StaticParameterBinder.BindCommand(commandAst); - if (bindingResult.BoundParameters.ContainsKey("Parallel")) - { - foreachNestedCount++; - break; - } - } - } - } + foreachNestedCount++; } if (foreachNestedCount > 1) diff --git a/test/powershell/Modules/Microsoft.PowerShell.Utility/Foreach-Object-Parallel.Tests.ps1 b/test/powershell/Modules/Microsoft.PowerShell.Utility/Foreach-Object-Parallel.Tests.ps1 index 67bf8ce5d0f..fb7438608d1 100644 --- a/test/powershell/Modules/Microsoft.PowerShell.Utility/Foreach-Object-Parallel.Tests.ps1 +++ b/test/powershell/Modules/Microsoft.PowerShell.Utility/Foreach-Object-Parallel.Tests.ps1 @@ -97,6 +97,16 @@ Describe 'ForEach-Object -Parallel Basic Tests' -Tags 'CI' { $result[1] | Should -BeExactly $varArray[1] } + It 'Verifies using variables with passed-in {} script block object' { + + $var = "Hello" + $varArray = "Hello","There" + $sBlock = { $using:var; $using:varArray[1] } + $result = 1..1 | ForEach-Object -Parallel $sBlock + $result[0] | Should -BeExactly $var + $result[1] | Should -BeExactly $varArray[1] + } + It 'Verifies in scope using same-name variables in nested calls for passed-in script block objects' { $Test1 = "Test1" @@ -113,6 +123,22 @@ Describe 'ForEach-Object -Parallel Basic Tests' -Tags 'CI' { $groups['Test2'].Count | Should -BeExactly 4 } + It 'Verifies in scope using same-name variables in nested calls for passed-in {} script block objects' { + + $Test1 = "Test1" + $sBlock = { + $using:Test1 + $Test2 = "Test2" + $sBlock2 = [scriptblock]::Create('$using:Test2') + 1..2 | ForEach-Object -Parallel $sBlock2 + } + $results = 1..2 | ForEach-Object -Parallel $sBlock + $results.Count | Should -BeExactly 6 + $groups = $results | Group-Object -AsHashTable + $groups['Test1'].Count | Should -BeExactly 2 + $groups['Test2'].Count | Should -BeExactly 4 + } + It 'Verifies in scope using same-name variables in nested calls for mixed script block objects' { $Test1 = "Test1" @@ -128,6 +154,21 @@ Describe 'ForEach-Object -Parallel Basic Tests' -Tags 'CI' { $groups['Test2'].Count | Should -BeExactly 4 } + It 'Verifies in scope using same-name variables in nested calls for mixed script block {} objects' { + + $Test1 = "Test1" + $sBlock = { + $using:Test1 + $Test2 = "Test2" + 1..2 | ForEach-Object -Parallel { $using:Test2 } + } + $results = 1..2 | ForEach-Object -Parallel $sBlock + $results.Count | Should -BeExactly 6 + $groups = $results | Group-Object -AsHashTable + $groups['Test1'].Count | Should -BeExactly 2 + $groups['Test2'].Count | Should -BeExactly 4 + } + It 'Verifies in scope using different-name variables in nested calls for passed-in script block objects' { $Test1 = "Test1" @@ -144,6 +185,22 @@ Describe 'ForEach-Object -Parallel Basic Tests' -Tags 'CI' { $groups['Test2'].Count | Should -BeExactly 4 } + It 'Verifies in scope using different-name variables in nested calls for passed-in script {} block objects' { + + $Test1 = "Test1" + $sBlock = { + $using:Test1 + $Test2 = "Test2" + $sBlock2 = [scriptblock]::Create('$using:Test2') + 1..2 | ForEach-Object -Parallel $sBlock2 + } + $results = 1..2 | ForEach-Object -Parallel $sBlock + $results.Count | Should -BeExactly 6 + $groups = $results | Group-Object -AsHashTable + $groups['Test1'].Count | Should -BeExactly 2 + $groups['Test2'].Count | Should -BeExactly 4 + } + It 'Verifies in scope using different-name variables in nested calls for mixed script block objects' { $Test1 = "Test1" @@ -159,6 +216,21 @@ Describe 'ForEach-Object -Parallel Basic Tests' -Tags 'CI' { $groups['Test2'].Count | Should -BeExactly 4 } + It 'Verifies in scope using different-name variables in nested calls for mixed script block {} objects' { + + $Test1 = "Test1" + $sBlock = { + $using:Test1 + $Test2 = "Test2" + 1..2 | ForEach-Object -Parallel { $using:Test2 } + } + $results = 1..2 | ForEach-Object -Parallel $sBlock + $results.Count | Should -BeExactly 6 + $groups = $results | Group-Object -AsHashTable + $groups['Test1'].Count | Should -BeExactly 2 + $groups['Test2'].Count | Should -BeExactly 4 + } + It 'Verifies expected error for out of scope using variable in nested calls for passed-in script block objects' { $Test = "TestZ" @@ -171,6 +243,18 @@ Describe 'ForEach-Object -Parallel Basic Tests' -Tags 'CI' { $usingErrors[0].FullyQualifiedErrorId | Should -BeExactly 'UsingVariableIsUndefined,Microsoft.PowerShell.Commands.ForEachObjectCommand' } + It 'Verifies expected error for out of scope using variable in nested calls for passed-in {} script block objects' { + + $Test = "TestZ" + $sBlock = [scriptblock]::Create(@' + $using:Test + $sBlock2 = { $using:Test } + 1..1 | ForEach-Object -Parallel $sBlock2 +'@) + 1..1 | ForEach-Object -Parallel $SBlock -ErrorVariable usingErrors 2>$null + $usingErrors[0].FullyQualifiedErrorId | Should -BeExactly 'UsingVariableIsUndefined,Microsoft.PowerShell.Commands.ForEachObjectCommand' + } + It 'Verifies expected error for out of scope using variable in nested calls for mixed script block objects' { $Test = "TestZ" From 8ca40d8fe357d5ff1db156222f0524c05bcfb85d Mon Sep 17 00:00:00 2001 From: Paul Higinbotham Date: Fri, 10 Dec 2021 15:02:15 -0800 Subject: [PATCH 7/7] Simplify using in scope logic per PR comments --- cgmanifest.json | 2 +- .../engine/runtime/ScriptBlockToPowerShell.cs | 81 ++++++------------- 2 files changed, 26 insertions(+), 57 deletions(-) diff --git a/cgmanifest.json b/cgmanifest.json index 30a342907d8..96137367dd2 100644 --- a/cgmanifest.json +++ b/cgmanifest.json @@ -685,7 +685,7 @@ "Type": "nuget", "Nuget": { "Name": "StyleCop.Analyzers.Unstable", - "Version": "1.2.0.354" + "Version": "1.2.0.376" } }, "DevelopmentDependency": true diff --git a/src/System.Management.Automation/engine/runtime/ScriptBlockToPowerShell.cs b/src/System.Management.Automation/engine/runtime/ScriptBlockToPowerShell.cs index 747eb580728..efa604eb21e 100644 --- a/src/System.Management.Automation/engine/runtime/ScriptBlockToPowerShell.cs +++ b/src/System.Management.Automation/engine/runtime/ScriptBlockToPowerShell.cs @@ -334,7 +334,6 @@ internal static Dictionary GetUsingValuesForEachParallel( // Foreach-Object -Parallel call scope. This will filter the using variable map to variables // only within the current (outer) Foreach-Object -Parallel call scope. var usingAsts = UsingExpressionAstSearcher.FindAllUsingExpressions(scriptBlock.Ast).ToList(); - bool astIncludesForEachCommand = CheckScriptBlockForForeachCommand(scriptBlock); UsingExpressionAst usingAst = null; var usingValueMap = new Dictionary(usingAsts.Count); Version oldStrictVersion = null; @@ -349,9 +348,7 @@ internal static Dictionary GetUsingValuesForEachParallel( for (int i = 0; i < usingAsts.Count; ++i) { usingAst = (UsingExpressionAst)usingAsts[i]; - if (IsInForeachParallelCallingScope( - usingAst: usingAst, - astIncludesForEachCommand: astIncludesForEachCommand)) + if (IsInForeachParallelCallingScope(scriptBlock.Ast, usingAst)) { var value = Compiler.GetExpressionValue(usingAst.SubExpression, isTrustedInput, context); string usingAstKey = PsUtils.GetUsingExpressionKey(usingAst); @@ -396,28 +393,28 @@ internal static Dictionary GetUsingValuesForEachParallel( private static bool FindForEachInCommand(CommandAst commandAst) { - foreach (var commandElement in commandAst.CommandElements) + // Command name is always the first element in the CommandAst. + // e.g., 'foreach -parallel {}' + var commandNameElement = (commandAst.CommandElements.Count > 0) ? commandAst.CommandElements[0] : null; + if (commandNameElement is StringConstantExpressionAst commandName) { - if (commandElement is StringConstantExpressionAst commandName) + bool found = false; + foreach (var foreachName in forEachNames) { - bool found = false; - foreach (var foreachName in forEachNames) + if (commandName.Value.Equals(foreachName, StringComparison.OrdinalIgnoreCase)) { - if (commandName.Value.Equals(foreachName, StringComparison.OrdinalIgnoreCase)) - { - found = true; - break; - } + found = true; + break; } + } - if (found) + if (found) + { + // Verify this is foreach-object with parallel parameter set. + var bindingResult = StaticParameterBinder.BindCommand(commandAst); + if (bindingResult.BoundParameters.ContainsKey("Parallel")) { - // Verify this is foreach-object with parallel parameter set. - var bindingResult = StaticParameterBinder.BindCommand(commandAst); - if (bindingResult.BoundParameters.ContainsKey("Parallel")) - { - return true; - } + return true; } } } @@ -425,39 +422,19 @@ private static bool FindForEachInCommand(CommandAst commandAst) return false; } - private static bool CheckScriptBlockForForeachCommand(ScriptBlock scriptBlock) - { - var currentAst = scriptBlock.Ast; - while (currentAst is not null) - { - if (currentAst is CommandAst commandAst && - FindForEachInCommand(commandAst)) - { - return true; - } - - currentAst = currentAst.Parent; - } - - return false; - } - /// /// Walks the using Ast to verify it is used within a foreach-object -parallel command /// and parameter set scope, and not from within a nested foreach-object -parallel call. /// + /// Scriptblock Ast containing this using Ast /// Using Ast to check. - /// - /// True when the provided ast includes a parent containing the originating ForEach-Object command - /// e.g., - /// '1 | ForEach-Object -Parallel { }' - script block ast includes parent with initial 'ForEach-Object' - /// '1 | ForEach-Object -Parallel $scriptBlock' - script block ast *does not* include parent with initial 'ForEach-Object' - /// /// True if using expression is in current call scope. private static bool IsInForeachParallelCallingScope( - UsingExpressionAst usingAst, - bool astIncludesForEachCommand) + Ast scriptblockAst, + UsingExpressionAst usingAst) { + Diagnostics.Assert(usingAst != null, "usingAst argument cannot be null."); + /* Example: $Test1 = "Hello" @@ -470,31 +447,23 @@ private static bool IsInForeachParallelCallingScope( } } */ - Diagnostics.Assert(usingAst != null, "usingAst argument cannot be null."); // Search up the parent Ast chain for 'Foreach-Object -Parallel' commands. Ast currentParent = usingAst.Parent; - int foreachNestedCount = 0; - while (currentParent != null) + while (currentParent != scriptblockAst) { // Look for Foreach-Object outer commands if (currentParent is CommandAst commandAst && FindForEachInCommand(commandAst)) { - foreachNestedCount++; - } - - if (foreachNestedCount > 1) - { - // This using expression Ast is outside the original calling scope. + // Using Ast is outside the invoking foreach scope. return false; } currentParent = currentParent.Parent; } - // Expected foreachNestedCount depends on provided script block ast (see above). - return astIncludesForEachCommand ? foreachNestedCount == 1 : foreachNestedCount == 0; + return true; } ///