Skip to content

Commit 771b8d9

Browse files
Copilotkilasuit
andcommitted
Apply PR review feedback: use GetScriptBlock() and HashSet for magic variables in MiscOps.cs
Co-authored-by: kilasuit <6355225+kilasuit@users.noreply.github.com>
1 parent 27f100f commit 771b8d9

1 file changed

Lines changed: 22 additions & 49 deletions

File tree

  • src/System.Management.Automation/engine/runtime/Operations

src/System.Management.Automation/engine/runtime/Operations/MiscOps.cs

Lines changed: 22 additions & 49 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,13 @@ namespace System.Management.Automation
2828
{
2929
internal static class PipelineOps
3030
{
31+
// PowerShell magic/automatic variable names that should not be auto-prefixed with $using:
32+
// when constructing a background job script block for the &! operator.
33+
private static readonly HashSet<string> s_backgroundJobMagicVariables = new HashSet<string>(StringComparer.OrdinalIgnoreCase)
34+
{
35+
"PID", "PSVersionTable", "PSEdition", "PSHOME", "HOST", "TRUE", "FALSE", "NULL"
36+
};
37+
3138
private static CommandProcessorBase AddCommand(PipelineProcessor pipe,
3239
CommandParameterInternal[] commandElements,
3340
CommandBaseAst commandBaseAst,
@@ -564,53 +571,18 @@ cmdExpr.Expression is ScriptBlockExpressionAst sbExpr
564571

565572
if (scriptBlockExpr != null)
566573
{
567-
// The pipeline is already a script block - extract the body without outer braces.
568-
// ScriptBlockExpressionAst.Extent.Text includes the outer '{ }', so we strip them
569-
// to get just the body. Using ScriptBlock.Create("{ content }") would create a
570-
// script that returns a ScriptBlock object rather than executing the body.
571-
var sbExprText = scriptBlockExpr.Extent.Text;
572-
Diagnostics.Assert(
573-
sbExprText.Length >= 2 && sbExprText[0] == '{' && sbExprText[sbExprText.Length - 1] == '}',
574-
"ScriptBlockExpressionAst extent should always start with '{' and end with '}'");
575-
var scriptblockBodyString = sbExprText.Substring(1, sbExprText.Length - 2);
576-
var pipelineOffset = scriptBlockExpr.Extent.StartOffset + 1;
577-
var variables = scriptBlockExpr.FindAll(static x => x is VariableExpressionAst, true);
578-
579-
// Minimize allocations by initializing the stringbuilder to the size of the source string + space for ${using:} * 2
580-
System.Text.StringBuilder updatedScriptblock = new System.Text.StringBuilder(scriptblockBodyString.Length + 18);
581-
int position = 0;
582-
583-
// Prefix variables in the scriptblock with $using:
584-
foreach (var v in variables)
585-
{
586-
var variableName = ((VariableExpressionAst)v).VariablePath.UserPath;
587-
588-
// Skip variables that don't exist
589-
if (funcContext._executionContext.EngineSessionState.GetVariable(variableName) == null)
590-
{
591-
continue;
592-
}
593-
594-
// Skip PowerShell magic variables
595-
if (!Regex.Match(
596-
variableName,
597-
"^(global:){0,1}(PID|PSVersionTable|PSEdition|PSHOME|HOST|TRUE|FALSE|NULL)$",
598-
RegexOptions.IgnoreCase | RegexOptions.CultureInvariant).Success)
599-
{
600-
updatedScriptblock.Append(scriptblockBodyString.AsSpan(position, v.Extent.StartOffset - pipelineOffset - position));
601-
updatedScriptblock.Append("${using:");
602-
updatedScriptblock.Append(CodeGeneration.EscapeVariableName(variableName));
603-
updatedScriptblock.Append('}');
604-
position = v.Extent.EndOffset - pipelineOffset;
605-
}
606-
}
607-
608-
updatedScriptblock.Append(scriptblockBodyString.AsSpan(position));
609-
sb = ScriptBlock.Create(updatedScriptblock.ToString());
574+
// The pipeline is already a script block - use the ScriptBlock from the AST directly.
575+
// Using ScriptBlock.Create("{ content }") would create a script that returns a ScriptBlock
576+
// object rather than executing the body. Getting the ScriptBlock from the ScriptBlockAst
577+
// avoids all text manipulation and correctly executes the body.
578+
// Users are expected to use explicit $using: scoping in their scriptblock to capture
579+
// outer variables (e.g., { $using:testVar } &!).
580+
sb = scriptBlockExpr.ScriptBlock.GetScriptBlock();
610581
}
611582
else
612583
{
613-
// The pipeline is a regular command - wrap it in a script block
584+
// The pipeline is a regular command - wrap it in a script block.
585+
// Auto-inject $using: for variables that exist in the current scope.
614586
var scriptblockBodyString = pipelineAst.Extent.Text;
615587
var pipelineOffset = pipelineAst.Extent.StartOffset;
616588
var variables = pipelineAst.FindAll(static x => x is VariableExpressionAst, true);
@@ -630,11 +602,12 @@ cmdExpr.Expression is ScriptBlockExpressionAst sbExpr
630602
continue;
631603
}
632604

633-
// Skip PowerShell magic variables
634-
if (!Regex.Match(
635-
variableName,
636-
"^(global:){0,1}(PID|PSVersionTable|PSEdition|PSHOME|HOST|TRUE|FALSE|NULL)$",
637-
RegexOptions.IgnoreCase | RegexOptions.CultureInvariant).Success)
605+
// Strip global: prefix if present, then check against magic variable names
606+
var cleanVariableName = variableName.StartsWith("global:", StringComparison.OrdinalIgnoreCase)
607+
? variableName.Substring(7)
608+
: variableName;
609+
610+
if (!s_backgroundJobMagicVariables.Contains(cleanVariableName))
638611
{
639612
updatedScriptblock.Append(scriptblockBodyString.AsSpan(position, v.Extent.StartOffset - pipelineOffset - position));
640613
updatedScriptblock.Append("${using:");

0 commit comments

Comments
 (0)