Skip to content

Commit 0039807

Browse files
PaulHiginPaulHigin
authored
Fix using variable for nested foreach parallel calls (#14548)
1 parent 8e3c3e0 commit 0039807

4 files changed

Lines changed: 231 additions & 12 deletions

File tree

‎src/System.Management.Automation/engine/InternalCommands.cs‎

Lines changed: 17 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -381,6 +381,17 @@ public void Dispose()
381381
private Exception _taskCollectionException;
382382
private string _currentLocationPath;
383383

384+
// List of Foreach-Object command names and aliases.
385+
// TODO: Look into using SessionState.Internal.GetAliasTable() to find all user created aliases.
386+
// But update Alias command logic to maintain reverse table that lists all aliases mapping
387+
// to a single command definition, for performance.
388+
private static string[] forEachNames = new string[]
389+
{
390+
"ForEach-Object",
391+
"foreach",
392+
"%"
393+
};
394+
384395
private void InitParallelParameterSet()
385396
{
386397
// The following common parameters are not (yet) supported in this parameter set.
@@ -407,12 +418,12 @@ private void InitParallelParameterSet()
407418
{
408419
}
409420

410-
bool allowUsingExpression = this.Context.SessionState.LanguageMode != PSLanguageMode.NoLanguage;
411-
_usingValuesMap = ScriptBlockToPowerShellConverter.GetUsingValuesAsDictionary(
412-
Parallel,
413-
allowUsingExpression,
414-
this.Context,
415-
null);
421+
var allowUsingExpression = this.Context.SessionState.LanguageMode != PSLanguageMode.NoLanguage;
422+
_usingValuesMap = ScriptBlockToPowerShellConverter.GetUsingValuesForEachParallel(
423+
scriptBlock: Parallel,
424+
isTrustedInput: allowUsingExpression,
425+
context: this.Context,
426+
foreachNames: forEachNames);
416427

417428
// Validate using values map, which is a map of '$using:' variables referenced in the script.
418429
// Script block variables are not allowed since their behavior is undefined outside the runspace

‎src/System.Management.Automation/engine/remoting/commands/PSRemotingCmdlet.cs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2440,7 +2440,7 @@ private static List<VariableExpressionAst> GetUsingVariables(ScriptBlock localSc
24402440
throw new ArgumentNullException(nameof(localScriptBlock), "Caller needs to make sure the parameter value is not null");
24412441
}
24422442

2443-
var allUsingExprs = UsingExpressionAstSearcher.FindAllUsingExpressionExceptForWorkflow(localScriptBlock.Ast);
2443+
var allUsingExprs = UsingExpressionAstSearcher.FindAllUsingExpressions(localScriptBlock.Ast);
24442444
return allUsingExprs.Select(usingExpr => UsingExpressionAst.ExtractUsingVariable((UsingExpressionAst)usingExpr)).ToList();
24452445
}
24462446

‎src/System.Management.Automation/engine/runtime/ScriptBlockToPowerShell.cs‎

Lines changed: 152 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -174,11 +174,14 @@ internal static void ThrowError(ScriptBlockToPowerShellNotSupportedException ex,
174174

175175
internal class UsingExpressionAstSearcher : AstSearcher
176176
{
177-
internal static IEnumerable<Ast> FindAllUsingExpressionExceptForWorkflow(Ast ast)
177+
internal static IEnumerable<Ast> FindAllUsingExpressions(Ast ast)
178178
{
179179
Diagnostics.Assert(ast != null, "caller to verify arguments");
180180

181-
var searcher = new UsingExpressionAstSearcher(astParam => astParam is UsingExpressionAst, stopOnFirst: false, searchNestedScriptBlocks: true);
181+
var searcher = new UsingExpressionAstSearcher(
182+
callback: astParam => astParam is UsingExpressionAst,
183+
stopOnFirst: false,
184+
searchNestedScriptBlocks: true);
182185
ast.InternalVisit(searcher);
183186
return searcher.Results;
184187
}
@@ -313,6 +316,145 @@ internal static PowerShell Convert(ScriptBlockAst body,
313316
}
314317
}
315318

319+
/// <summary>
320+
/// Get using values as dictionary for the Foreach-Object parallel cmdlet.
321+
/// Ignore any using expressions that are associated with inner nested Foreach-Object parallel calls,
322+
/// since they are only effective in the nested call scope and not the current outer scope.
323+
/// </summary>
324+
/// <param name = "scriptBlock">Scriptblock to search.</param>
325+
/// <param name = "isTrustedInput">True when input is trusted.</param>
326+
/// <param name = "context">Execution context.</param>
327+
/// <param name = "foreachNames">List of foreach command names and aliases.</param>
328+
/// <returns>Dictionary of using variable map.</returns>
329+
internal static Dictionary<string, object> GetUsingValuesForEachParallel(
330+
ScriptBlock scriptBlock,
331+
bool isTrustedInput,
332+
ExecutionContext context,
333+
string[] foreachNames)
334+
{
335+
// Using variables for Foreach-Object -Parallel use are restricted to be within the
336+
// Foreach-Object -Parallel call scope. This will filter the using variable map to variables
337+
// only within the current (outer) Foreach-Object -Parallel call scope.
338+
var usingAsts = UsingExpressionAstSearcher.FindAllUsingExpressions(scriptBlock.Ast).ToList();
339+
UsingExpressionAst usingAst = null;
340+
var usingValueMap = new Dictionary<string, object>(usingAsts.Count);
341+
Version oldStrictVersion = null;
342+
try
343+
{
344+
if (context != null)
345+
{
346+
oldStrictVersion = context.EngineSessionState.CurrentScope.StrictModeVersion;
347+
context.EngineSessionState.CurrentScope.StrictModeVersion = PSVersionInfo.PSVersion;
348+
}
349+
350+
for (int i = 0; i < usingAsts.Count; ++i)
351+
{
352+
usingAst = (UsingExpressionAst)usingAsts[i];
353+
if (IsInForeachParallelCallingScope(usingAst, foreachNames))
354+
{
355+
var value = Compiler.GetExpressionValue(usingAst.SubExpression, isTrustedInput, context);
356+
string usingAstKey = PsUtils.GetUsingExpressionKey(usingAst);
357+
usingValueMap.TryAdd(usingAstKey, value);
358+
}
359+
}
360+
}
361+
catch (RuntimeException rte)
362+
{
363+
if (rte.ErrorRecord.FullyQualifiedErrorId.Equals("VariableIsUndefined", StringComparison.Ordinal))
364+
{
365+
throw InterpreterError.NewInterpreterException(
366+
targetObject: null,
367+
exceptionType: typeof(RuntimeException),
368+
errorPosition: usingAst.Extent,
369+
resourceIdAndErrorId: "UsingVariableIsUndefined",
370+
resourceString: AutomationExceptions.UsingVariableIsUndefined,
371+
args: rte.ErrorRecord.TargetObject);
372+
}
373+
}
374+
finally
375+
{
376+
if (context != null)
377+
{
378+
context.EngineSessionState.CurrentScope.StrictModeVersion = oldStrictVersion;
379+
}
380+
}
381+
382+
return usingValueMap;
383+
}
384+
385+
/// <summary>
386+
/// Walks the using Ast to verify it is used within a foreach-object -parallel command
387+
/// and parameter set scope, and not from within a nested foreach-object -parallel call.
388+
/// </summary>
389+
/// <param name="usingAst">Using Ast to check.</param>
390+
/// <param name-"foreachNames">List of foreach-object command names.</param>
391+
/// <returns>True if using expression is in current call scope.</returns>
392+
private static bool IsInForeachParallelCallingScope(
393+
UsingExpressionAst usingAst,
394+
string[] foreachNames)
395+
{
396+
/*
397+
Example:
398+
$Test1 = "Hello"
399+
1 | ForEach-Object -Parallel {
400+
$using:Test1
401+
$Test2 = "Goodbye"
402+
1 | ForEach-Object -Parallel {
403+
$using:Test1 # Invalid using scope
404+
$using:Test2 # Valid using scope
405+
}
406+
}
407+
*/
408+
Diagnostics.Assert(usingAst != null, "usingAst argument cannot be null.");
409+
410+
// Search up the parent Ast chain for 'Foreach-Object -Parallel' commands.
411+
Ast currentParent = usingAst.Parent;
412+
int foreachNestedCount = 0;
413+
while (currentParent != null)
414+
{
415+
// Look for Foreach-Object outer commands
416+
if (currentParent is CommandAst commandAst)
417+
{
418+
foreach (var commandElement in commandAst.CommandElements)
419+
{
420+
if (commandElement is StringConstantExpressionAst commandName)
421+
{
422+
bool found = false;
423+
foreach (var foreachName in foreachNames)
424+
{
425+
if (commandName.Value.Equals(foreachName, StringComparison.OrdinalIgnoreCase))
426+
{
427+
found = true;
428+
break;
429+
}
430+
}
431+
432+
if (found)
433+
{
434+
// Verify this is foreach-object with parallel parameter set.
435+
var bindingResult = StaticParameterBinder.BindCommand(commandAst);
436+
if (bindingResult.BoundParameters.ContainsKey("Parallel"))
437+
{
438+
foreachNestedCount++;
439+
break;
440+
}
441+
}
442+
}
443+
}
444+
}
445+
446+
if (foreachNestedCount > 1)
447+
{
448+
// This using expression Ast is outside the original calling scope.
449+
return false;
450+
}
451+
452+
currentParent = currentParent.Parent;
453+
}
454+
455+
return foreachNestedCount == 1;
456+
}
457+
316458
/// <summary>
317459
/// Get using values in the dictionary form.
318460
/// </summary>
@@ -343,11 +485,16 @@ internal static object[] GetUsingValuesAsArray(ScriptBlock scriptBlock, bool isT
343485
/// A tuple of the dictionary-form and the array-form using values.
344486
/// If the array-form using value is null, then there are UsingExpressions used in different scopes.
345487
/// </returns>
346-
private static Tuple<Dictionary<string, object>, object[]> GetUsingValues(Ast body, bool isTrustedInput, ExecutionContext context, Dictionary<string, object> variables, bool filterNonUsingVariables)
488+
private static Tuple<Dictionary<string, object>, object[]> GetUsingValues(
489+
Ast body,
490+
bool isTrustedInput,
491+
ExecutionContext context,
492+
Dictionary<string, object> variables,
493+
bool filterNonUsingVariables)
347494
{
348495
Diagnostics.Assert(context != null || variables != null, "can't retrieve variables with no context and no variables");
349496

350-
var usingAsts = UsingExpressionAstSearcher.FindAllUsingExpressionExceptForWorkflow(body).ToList();
497+
var usingAsts = UsingExpressionAstSearcher.FindAllUsingExpressions(body).ToList();
351498
var usingValueArray = new object[usingAsts.Count];
352499
var usingValueMap = new Dictionary<string, object>(usingAsts.Count);
353500
HashSet<string> usingVarNames = (variables != null && filterNonUsingVariables) ? new HashSet<string>() : null;
@@ -456,7 +603,7 @@ private static Tuple<Dictionary<string, object>, object[]> GetUsingValues(Ast bo
456603
/// Check if the given UsingExpression is in a different scope from the previous UsingExpression that we analyzed.
457604
/// </summary>
458605
/// <remarks>
459-
/// Note that the value of <paramref name="usingExpr"/> is retrieved by calling 'UsingExpressionAstSearcher.FindAllUsingExpressionExceptForWorkflow'.
606+
/// Note that the value of <paramref name="usingExpr"/> is retrieved by calling 'UsingExpressionAstSearcher.FindAllUsingExpressions'.
460607
/// So <paramref name="usingExpr"/> is guaranteed not inside a workflow.
461608
/// </remarks>
462609
/// <param name="usingExpr">The UsingExpression to analyze.</param>

‎test/powershell/Modules/Microsoft.PowerShell.Utility/Foreach-Object-Parallel.Tests.ps1‎

Lines changed: 61 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,67 @@ Describe 'ForEach-Object -Parallel Basic Tests' -Tags 'CI' {
2626
$result[1] | Should -BeExactly $varArray[1]
2727
}
2828

29+
It 'Verifies in scope using variables in nested calls' {
30+
31+
$Test = "Test1"
32+
$results = 1..2 | ForEach-Object -Parallel {
33+
$using:Test
34+
$Test = "Test2"
35+
1..2 | ForEach-Object -Parallel {
36+
$using:Test
37+
$Test = "Test3"
38+
1..2 | ForEach-Object -Parallel {
39+
$using:Test
40+
}
41+
}
42+
}
43+
$results.Count | Should -BeExactly 14
44+
$groups = $results | Group-Object -AsHashTable
45+
$groups['Test1'].Count | Should -BeExactly 2
46+
$groups['Test2'].Count | Should -BeExactly 4
47+
$groups['Test3'].Count | Should -BeExactly 8
48+
}
49+
50+
It 'Verifies in scope using variables with different names in nested calls' {
51+
$Test1 = "TestA"
52+
$results = 1..2 | ForEach-Object -parallel {
53+
$using:Test1
54+
$Test2 = "TestB"
55+
1..2 | ForEach-Object -parallel {
56+
$using:Test2
57+
}
58+
}
59+
$results.Count | Should -BeExactly 6
60+
$groups = $results | Group-Object -AsHashTable
61+
$groups['TestA'].Count | Should -BeExactly 2
62+
$groups['TestB'].Count | Should -BeExactly 4
63+
}
64+
65+
It 'Verifies using variable in nested scriptblock' {
66+
67+
$test = 'testC'
68+
$results = 1..2 | ForEach-Object -parallel {
69+
& { $using:test }
70+
}
71+
$results.Count | Should -BeExactly 2
72+
$groups = $results | Group-Object -AsHashTable
73+
$groups['TestC'].Count | Should -BeExactly 2
74+
}
75+
76+
It 'Verifies expected error for out of scope using variable in nested calls' {
77+
78+
$Test = "TestZ"
79+
1..1 | ForEach-Object -Parallel {
80+
$using:Test
81+
# Variable '$Test' is not defined in this scope.
82+
1..1 | ForEach-Object -Parallel {
83+
$using:Test
84+
}
85+
} -ErrorVariable usingErrors 2>$null
86+
87+
$usingErrors[0].FullyQualifiedErrorId | Should -BeExactly 'UsingVariableIsUndefined,Microsoft.PowerShell.Commands.ForEachObjectCommand'
88+
}
89+
2990
It 'Verifies terminating error streaming' {
3091

3192
$result = 1..1 | ForEach-Object -Parallel { throw 'Terminating Error!'; "Hello" } 2>&1

0 commit comments

Comments
 (0)