Skip to content

Rework passing array literal to native commands #5301

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
daxian-dbw merged 2 commits into from
Nov 7, 2017
Merged

Rework passing array literal to native commands #5301

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b58df7d
Rework passing array literal to native commands
lzybkr Oct 27, 2017
619f8ae
Address review feedback
lzybkr Nov 2, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 3 additions & 19 deletions src/System.Management.Automation/engine/CommandParameter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@ private class Argument
internal Ast ast;
internal object value;
internal bool splatted;
internal bool arrayIsSingleArgumentForNativeCommand;
}

private Parameter _parameter;
Expand Down Expand Up @@ -113,16 +112,6 @@ internal bool ArgumentSplatted
get { return _argument != null ? _argument.splatted : false; }
}

/// <summary>
/// If an argument was specified and it was an array literal with no spaces around the
/// commas, the value should be passed a single argument with it's commas if the command is
/// a native command.
/// </summary>
internal bool ArrayIsSingleArgumentForNativeCommand
{
get { return _argument != null ? _argument.arrayIsSingleArgumentForNativeCommand : false; }
}

/// <summary>
/// Set the argument value and ast.
/// </summary>
Expand Down Expand Up @@ -175,12 +164,10 @@ internal static CommandParameterInternal CreateParameter(
/// <param name="value">The argument value.</param>
/// <param name="ast">The ast of the argument value in the script.</param>
/// <param name="splatted">True if the argument value is to be splatted, false otherwise.</param>
/// <param name="arrayIsSingleArgumentForNativeCommand">If the command is native, pass the string with commas instead of multiple arguments</param>
internal static CommandParameterInternal CreateArgument(
object value,
Ast ast = null,
bool splatted = false,
bool arrayIsSingleArgumentForNativeCommand = false)
bool splatted = false)
{
return new CommandParameterInternal
{
Expand All @@ -189,7 +176,6 @@ internal static CommandParameterInternal CreateArgument(
value = value,
ast = ast,
splatted = splatted,
arrayIsSingleArgumentForNativeCommand = arrayIsSingleArgumentForNativeCommand
}
};
}
Expand All @@ -210,20 +196,18 @@ internal static CommandParameterInternal CreateArgument(
/// <param name="argumentAst">The ast of the argument value in the script.</param>
/// <param name="value">The argument value.</param>
/// <param name="spaceAfterParameter">Used in native commands to correctly handle -foo:bar vs. -foo: bar</param>
/// <param name="arrayIsSingleArgumentForNativeCommand">If the command is native, pass the string with commas instead of multiple arguments</param>
internal static CommandParameterInternal CreateParameterWithArgument(
Ast parameterAst,
string parameterName,
string parameterText,
Ast argumentAst,
object value,
bool spaceAfterParameter,
bool arrayIsSingleArgumentForNativeCommand = false)
bool spaceAfterParameter)
{
return new CommandParameterInternal
{
_parameter = new Parameter { ast = parameterAst, parameterName = parameterName, parameterText = parameterText },
_argument = new Argument { ast = argumentAst, value = value, arrayIsSingleArgumentForNativeCommand = arrayIsSingleArgumentForNativeCommand },
_argument = new Argument { ast = argumentAst, value = value },
_spaceAfterParameter = spaceAfterParameter
};
}
Expand Down
2 changes: 1 addition & 1 deletion src/System.Management.Automation/engine/MinishellParameterBinderController.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -214,7 +214,7 @@ internal Collection<CommandParameterInternal> BindParameters(Collection<CommandP
parameters[i] = CommandParameterInternal.CreateParameterWithArgument(
parameter.ArgumentAst, EncodedCommandParameter, "-" + EncodedCommandParameter,
parameter.ArgumentAst, encodedScript,
spaceAfterParameter: true, arrayIsSingleArgumentForNativeCommand: false);
spaceAfterParameter: true);
}
}
}
Expand Down
79 changes: 53 additions & 26 deletions src/System.Management.Automation/engine/NativeCommandParameterBinder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,28 +105,27 @@ internal void BindParameters(Collection<CommandParameterInternal> parameters)

if (argValue != AutomationNull.Value && argValue != UnboundParameter.Value)
{
// ArrayIsSingleArgumentForNativeCommand is true when a comma is used in the
// command line, e.g.
// ArrayLiteralAst is used to reconstruct the correct argument, e.g.
// windbg -k com:port=\\devbox\pipe\debug,pipe,resets=0,reconnect
// The parser produced an array of strings but marked the parameter so we
// can properly reconstruct the correct command line.
bool usedQuotes = false;
var pAst = parameter.ArgumentAst;
if (pAst != null)
ArrayLiteralAst arrayLiteralAst = null;
switch (parameter?.ArgumentAst)
{
if (pAst is StringConstantExpressionAst sce)
{
usedQuotes = sce.StringConstantType != StringConstantType.BareWord;
}
else if (pAst is ExpandableStringExpressionAst ese)
{
usedQuotes = ese.StringConstantType != StringConstantType.BareWord;
}
case StringConstantExpressionAst sce:
usedQuotes = sce.StringConstantType != StringConstantType.BareWord;
break;
case ExpandableStringExpressionAst ese:
usedQuotes = ese.StringConstantType != StringConstantType.BareWord;
break;
case ArrayLiteralAst ala:
arrayLiteralAst = ala;
break;
}

appendOneNativeArgument(Context, argValue,
parameter.ArrayIsSingleArgumentForNativeCommand ? ',' : ' ',
sawVerbatimArgumentMarker,
usedQuotes);
arrayLiteralAst, sawVerbatimArgumentMarker, usedQuotes);
}
}
}
Expand Down Expand Up @@ -158,14 +157,19 @@ internal String Arguments
/// </summary>
/// <param name="context">Execution context instance</param>
/// <param name="obj">The object to append</param>
/// <param name="separator">A space or comma used when obj is enumerable</param>
/// <param name="argArrayAst">If the argument was an array literal, the Ast, otherwise null</param>
/// <param name="sawVerbatimArgumentMarker">true if the argument occurs after --%</param>
/// <param name="usedQuotes">True if the argument was a quoted string (single or double)</param>
private void appendOneNativeArgument(ExecutionContext context, object obj, char separator, bool sawVerbatimArgumentMarker, bool usedQuotes)
private void appendOneNativeArgument(ExecutionContext context, object obj, ArrayLiteralAst argArrayAst, bool sawVerbatimArgumentMarker, bool usedQuotes)
{
IEnumerator list = LanguagePrimitives.GetEnumerator(obj);
bool needSeparator = false;

Diagnostics.Assert(argArrayAst == null
|| obj is object[] && ((object[])obj).Length == argArrayAst.Elements.Count,
"array argument and ArrayLiteralAst differ in number of elements");

int currentElement = -1;
string separator = "";
Copy link
Collaborator

@iSazonov iSazonov Nov 2, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we use String.Empty?

Copy link
Contributor Author

@lzybkr lzybkr Nov 2, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No reason to imo.

do
{
string arg;
Expand All @@ -180,18 +184,17 @@ private void appendOneNativeArgument(ExecutionContext context, object obj, char
break;
}
arg = PSObject.ToStringParser(context, ParserOps.Current(null, list));

currentElement += 1;
if (currentElement != 0)
{
separator = GetEnumerableArgSeparator(argArrayAst, currentElement);
}
}

if (!String.IsNullOrEmpty(arg))
{
if (needSeparator)
{
_arguments.Append(separator);
}
else
{
needSeparator = true;
}
_arguments.Append(separator);

if (sawVerbatimArgumentMarker)
{
Expand Down Expand Up @@ -349,6 +352,30 @@ internal static bool NeedQuotes(string stringToCheck)
return needQuotes;
}

static private string GetEnumerableArgSeparator(ArrayLiteralAst arrayLiteralAst, int index)
{
if (arrayLiteralAst == null) return " ";

// index points to the *next* element, so we're looking for space between
// it and the previous element.

var next = arrayLiteralAst.Elements[index];
var prev = arrayLiteralAst.Elements[index - 1];

var arrayExtent = arrayLiteralAst.Extent;
var afterPrev = prev.Extent.EndOffset;
var beforeNext = next.Extent.StartOffset - 1;

if (afterPrev == beforeNext) return ",";

var arrayText = arrayExtent.Text;
afterPrev -= arrayExtent.StartOffset;
beforeNext -= arrayExtent.StartOffset;

if (arrayText[afterPrev] == ',') return ", ";
if (arrayText[beforeNext] == ',') return " ,";
return " , ";
}

/// <summary>
/// The native command to bind to
Expand Down
33 changes: 2 additions & 31 deletions src/System.Management.Automation/engine/parser/Compiler.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3443,13 +3443,11 @@ public object VisitCommand(CommandAst commandAst)
splatted = variableExpression.Splatted;
}

bool arrayIsSingleArgumentForNativeCommand = ArgumentIsNotReallyArrayIfCommandIsNative(element);
elementExprs[i] =
Expression.Call(CachedReflectionInfo.CommandParameterInternal_CreateArgument,
Expression.Convert(GetCommandArgumentExpression(element), typeof(object)),
Expression.Constant(element),
ExpressionCache.Constant(splatted),
ExpressionCache.Constant(arrayIsSingleArgumentForNativeCommand));
ExpressionCache.Constant(splatted));
}
}

Expand Down Expand Up @@ -3519,31 +3517,6 @@ public object VisitCommandExpression(CommandExpressionAst commandExpressionAst)
return CallAddPipe(expr, s_getCurrentPipe);
}

private bool ArgumentIsNotReallyArrayIfCommandIsNative(Ast arg)
{
var arrayLiteralAst = arg as ArrayLiteralAst;
if (arrayLiteralAst == null)
{
return false;
}

Diagnostics.Assert(arrayLiteralAst.Elements.Count > 1, "Single dimension array arguments are surrounded with parens if the value is an argument");
var previousElement = arrayLiteralAst.Elements[0];
for (int index = 1; index < arrayLiteralAst.Elements.Count; index++)
{
var element = arrayLiteralAst.Elements[index];
// EndOffset is 1 past the end which puts it on the comma, so if +1 is not the next element,
// there is whitespace between elements
if (previousElement.Extent.EndOffset + 1 != element.Extent.StartOffset)
{
return false;
}
previousElement = element;
}

return true;
}

public object VisitCommandParameter(CommandParameterAst commandParameterAst)
{
var arg = commandParameterAst.Argument;
Expand All @@ -3552,15 +3525,13 @@ public object VisitCommandParameter(CommandParameterAst commandParameterAst)
{
bool spaceAfterParameter = (errorPos.EndLineNumber != arg.Extent.StartLineNumber ||
errorPos.EndColumnNumber != arg.Extent.StartColumnNumber);
bool arrayIsSingleArgumentForNativeCommand = ArgumentIsNotReallyArrayIfCommandIsNative(arg);
return Expression.Call(CachedReflectionInfo.CommandParameterInternal_CreateParameterWithArgument,
Expression.Constant(commandParameterAst),
Expression.Constant(commandParameterAst.ParameterName),
Expression.Constant(errorPos.Text),
Expression.Constant(arg),
Expression.Convert(GetCommandArgumentExpression(arg), typeof(object)),
ExpressionCache.Constant(spaceAfterParameter),
ExpressionCache.Constant(arrayIsSingleArgumentForNativeCommand));
ExpressionCache.Constant(spaceAfterParameter));
}

return Expression.Call(CachedReflectionInfo.CommandParameterInternal_CreateParameter,
Expand Down
18 changes: 18 additions & 0 deletions test/powershell/Language/Scripting/NativeExecution/NativeCommandArguments.Tests.ps1
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,4 +43,22 @@ Describe "Native Command Arguments" -tags "CI" {
$lines[$i] | Should Be "Arg $i is <$($expected[$i])>"
}
}

It "Should handle PowerShell arrays with or without spaces correctly: <arguments>" -TestCases @(
@{arguments = "1,2"; expected = @("1,2")}
@{arguments = "1,2,3"; expected = @("1,2,3")}
@{arguments = "1, 2"; expected = "1,", "2"}
@{arguments = "1 ,2"; expected = "1", ",2"}
@{arguments = "1 , 2"; expected = "1", ",", "2"}
@{arguments = "1, 2,3"; expected = "1,", "2,3"}
@{arguments = "1 ,2,3"; expected = "1", ",2,3"}
@{arguments = "1 , 2,3"; expected = "1", ",", "2,3"}
) {
param($arguments, $expected)
$lines = @(Invoke-Expression "testexe -echoargs $arguments")
$lines.Count | Should Be $expected.Count
for ($i = 0; $i -lt $expected.Count; $i++) {
$lines[$i] | Should Be "Arg $i is <$($expected[$i])>"
}
}
}