Add ScriptBlock substitution overload to -replace operator #6029

IISResetMe · Jan 25, 2018

PR Summary

This PR adds rudimentary lambda support to the -replace operator:

PS C:\> "ID 0000123" -replace "\b0+", {return " " * $args[0].Value.Length}
ID     123

The current -replace operator implementation has the following syntax:

<value> -replace <pattern>[, <substitution>]

This (functionally) maps directly onto:

Regex.Replace(<value>, <pattern>, <substitution>)

Regex.Replace() already has an overload that supports a MatchEvaluator delegate, which in turn allows us to pass a scriptblock instead of a string value as the substitution pattern.

Since only wiring is required, we can accomplish this with:

if `<substitution>` argument is a ScriptBlock
    convert `<substitution>` to a `MatchEvaluator` instance
    pass MatchEvaluator to Regex.Replace() instead of "`<substitution>`"

Update:

To support arbitrary delegates (like existing MatchEvaluators and PSMethods),

if `<substitution>` argument is not a string
    attempt to convert `<substitution>` to a `MatchEvaluator` instance
    pass MatchEvaluator to Regex.Replace() instead of "`<substitution>`"

powercode · Jan 25, 2018

powercode reviewed Jan 25, 2018

View changes

powercode · Jan 25, 2018

Sign in to view

@@ -815,6 +816,10 @@ internal static object ReplaceOperator(ExecutionContext context, IScriptExtent e
                    pattern = rList[0];
                    if (rList.Count > 1)
                    {
+                        if(rList[1] is ScriptBlock)


Shouldn't this also support a MatchEvaluator delegate? Do that we can use a method from a PowerShell or .NET class.
That is, in addition to the scriptblock.

Edit:
I mean something that is convertible to a MatchEvaluator.
Should work with

class R { static [string] Replace([Match] $match) { return "X" } } "abc" -replace 'b', [R]::Replace

Good call!
Wondering whether it should accept/evaluate System.Delegate, System.Func<string> and System.Func<Match, string> as well...

Nevermind, cast will fail.

@powercode Passing a PSMethodInfo object would make sense as a use case, but I'm not sure how to safely construct a scriptblock or delegate that dispatches it.

Any ideas?

powercode · Jan 26, 2018

I'd think it would be better to check if you have something that can be converted to a MatchEvaluator.
PSMethods can now be converted to matching delegates.

This works:

class R { static [string] RR([Text.RegularExpressions.Match] $m) {return "Foo"}}
[Regex]::replace("bar", 'a', [R]::RR)

It would be nice if

"bar" -replace "a", [R]::RR

worked too.

powercode · Jan 26, 2018

powercode reviewed Jan 26, 2018

View changes

powercode · Jan 26, 2018

Sign in to view

@@ -816,7 +816,11 @@ internal static object ReplaceOperator(ExecutionContext context, IScriptExtent e
                    pattern = rList[0];
                    if (rList.Count > 1)
                    {
-                        if(rList[1] is ScriptBlock)
+                        if (rList[1] is MatchEvaluator)


Maybe have a fast path for the string replacement (likely the most common case), and then, if it isn't a string check for
scriptblock, MatchEvaluator, and PSMethod?
If it is a PSMethod, try to convert it to a MatchEvaluator.

How about a single branch like:

if(!(rList[1]] is string)) Attempt conversion to MatchEvaluator

powercode · Jan 26, 2018

Sign in to view

+            $res = "ID 0000123" -replace "\b0+", $matchEvaluator
+            $res | Should Be "ID     123"
+        }
+    }


I would like a test like

class R { static [string] ReplaceWithShell([Text.RegularExpressions.Match] $m) {return "Shell"}} "PowerPoint" -replace "Point", [R]::ReplaceWithShell | Should be "PowerShell"

I'd exclude substitutionMethod too.

LaurentDardenne · Jan 26, 2018

What happens if the delegate code raises an exception ?
Is the Match delegate parameter named $args under Powershell ?

IISResetMe · Jan 26, 2018

@LaurentDardenne Replace() will throw a RuntimeException if the delegate throws during substitution

If you pass a scriptblock with no explicit params, the Match parameter argument populates the $args automatic variable. User could also explicitly name it if desired, no functional difference

LaurentDardenne · Jan 26, 2018

Thank you.

iSazonov · Jan 26, 2018

iSazonov reviewed Jan 26, 2018

View changes

iSazonov · Jan 26, 2018

Sign in to view

@@ -0,0 +1,67 @@
+Describe "Split Operator" -Tags CI {


Thanks @iSazonov, fixed in commit aa54fe56a9efb2c0c49b551599ec193d67c47e36

powercode · Jan 26, 2018

powercode approved these changes Jan 26, 2018

View changes

powercode · Jan 26, 2018

powercode reviewed Jan 26, 2018

View changes

powercode · Jan 26, 2018

Sign in to view

+                        if (!(rList[1] is string))
+                        {
+                            useMatchEvaluator = LanguagePrimitives.TryConvertTo(rList[1], out matchEvaluator);
+                        }
                        replacement = PSObject.ToStringParser(context, rList[1]);


Should this line be run if we use a matchEvaluator?

TryConvertTo() will do a simple cast and return early if the input object and out param are the same type, no need to complicate the ReplaceOperator() implementation for this (edge-)case

It is too tricky code. Please store rList[1]); in variable as string and below use it in a help method (see my next comment).

iSazonov · Jan 26, 2018

iSazonov reviewed Jan 26, 2018

View changes

iSazonov · Jan 26, 2018

Sign in to view

+                        if (!(rList[1] is string))
+                        {
+                            useMatchEvaluator = LanguagePrimitives.TryConvertTo(rList[1], out matchEvaluator);
+                        }
                        replacement = PSObject.ToStringParser(context, rList[1]);


It is too tricky code. Please store rList[1]); in variable as string and below use it in a help method (see my next comment).

iSazonov · Jan 26, 2018

Sign in to view

+                    // If a MatchEvaluator has been assigned, we
+                    // use the corresponding Regex.Replace() overload
+                    return rr.Replace(lvalString, matchEvaluator);
+                }
                return rr.Replace(lvalString, replacement);


The code (L860-L867, L876-L883) should be in a help method.

iSazonov · Jan 26, 2018

Sign in to view

+            $res = "Get-Process" -replace "Get", "Stop"
+            $res | Should Be "Stop-Process"
+
+            $res = "image.gif" -replace "\.gif$",".jpg"


Why we need the test? Looks as dup line 4. Do it test another code path?

First test is the first and simplest example from the about_Comparison_Operators help file, this one is the first example that shows it's actually a regex operator and not just static string replacement.

iSazonov · Jan 26, 2018

Sign in to view

+
+        It "Replace operator can take 2 arguments, a mandatory pattern, and an optional substitution" {
+            $res = "PowerPoint" -replace "Point","Shell"
+            $res | Should Be "PowerShell"


Please use BeExactly for strings. Below too.

iSazonov · Jan 26, 2018

Sign in to view

+            $res | Should Be "domain.example"
+
+            $res = "domain.example" -replace ".*\.(\w+)$","`$1"
+            $res | Should Be "example"


We could use one Should with $res = "domain.example" -replace ".*\.(\w+)$","$0 - $1"

iSazonov · Jan 26, 2018

Sign in to view

+        }
+
+        It "Replace operator can take a ScriptBlock in place of a substitution string" {
+            $res = "ID 0000123" -replace "\b0+", {return " " * $args[0].Value.Length}


Spaces are not visible - it looks bad. Could you please use another char? Below too.

Updated all three cases

iSazonov · Jan 27, 2018

iSazonov reviewed Jan 27, 2018

View changes

iSazonov · Jan 27, 2018

Sign in to view

                }

                return resultList.ToArray();
            }
        }

+        /// <summary>
+        /// ReplaceOperator implementation...


Really we don't need public comment for private method but if we use it please put one final dot in the comment and comments below. Also it is useful to mention that the method is needed to detect a matchEvaluator or use string regex (please reword :) ).

iSazonov · Jan 27, 2018

Sign in to view

+        /// ReplaceOperator implementation...
+        /// </summary>
+        /// <param name="context">The execution context in which to evaluate the expression</param>
+        /// <param name="errorPosition">The position to use for error reporting.</param>


We don't use the parameter. Please remove.

iSazonov · Jan 27, 2018

@IISResetMe Thanks for your contribution. Now we are waiting PowerShell Committee conclusion in days.

SteveL-MSFT · Jan 31, 2018

@PowerShell/powershell-committee reviewed this and agree with taking this change, would prefer $_ implemented, but can accept $args[0] since it's automatically there

IISResetMe · Feb 1, 2018

@SteveL-MSFT Cool! I agree that $_ would be nicer, but since we're passing the scriptblock directly to the method call as a delegate I can't really inject anything into the resulting expression .

Only way I can think of is by parsing the user-supplied scriptblock, inspecting the param block (optionally adding param([Match]$_)) and then re-creating it. It feels a bit ugly.

If anyone has suggestions or comments on this approach (or better ideas) I'm all ears.

lzybkr · Feb 1, 2018

@IISResetMe - you could create a closure as the delegate instead, something like this:

MatchEvaluator GetEvaluator(ScriptBlock sb)
{
    return m => sb.DoInvokeReturnAsIs(
        useLocalScope: true, // or false, not sure
        errorHandlingBehavior: ErrorHandlingBehavior.WriteToCurrentErrorPipe,
        dollarUnder: m,
        input: AutomationNull.Value,
        scriptThis: AutomationNull.Value,
        args: Utils.EmptyArray<object>());
}

IISResetMe · Feb 1, 2018

@lzybkr Interesting, I thought DoInvokeReturnAsIs() would execute the scriptblock immediately, no?

IISResetMe · Feb 1, 2018

As expected, the following works as intended:

private static object ReplaceOperatorImpl(ExecutionContext context, string input, Regex regex, object substitute)
{
    MatchEvaluator matchEvaluator = null;
    if (!(substitute is string) )
    {
        if(substitute is ScriptBlock) 
        {
            ScriptBlock substituteScriptBlock = substitute as ScriptBlock;
            ScriptBlockAst substituteAst = substituteScriptBlock.Ast as ScriptBlockAst;
            if(substituteAst.ParamBlock == null && 
                substituteAst.Find(
                    ast => ast is VariableExpressionAst && 
                    (ast as VariableExpressionAst).VariablePath.UserPath == "_", 
                false) != null)
            {
                substitute = ScriptBlock.Create(@"param([System.Text.RegularExpressions.Match]$_)" + substituteScriptBlock.ToString());
            }
        }   
        if(LanguagePrimitives.TryConvertTo(substitute, out matchEvaluator)){
            return regex.Replace(input, matchEvaluator);
        }
    }

    string replacement = PSObject.ToStringParser(context, substitute);
    return regex.Replace(input, replacement);
}

... but it doesn't look or feel very clean :-\

IISResetMe · Feb 1, 2018

@lzybkr Had a closer look at your suggestion, makes sense, but when I try to compile it I get:

.\PowerShell\src\System.Management.Automation\engine\lang\parserutils.cs(909,25): 
  error CS1662: Cannot convert lambda expression to intended delegate type because some of the return types in the block are not implicitly convertible to the delegate return type

lzybkr · Feb 1, 2018

Oh, right, I guess call LanguagePrimitives.ConvertTo<string> on the return value.

iSazonov · Feb 16, 2018

Added fix #6164 in description.

iSazonov · Feb 16, 2018

@IISResetMe What is the PR status?

daxian-dbw · Mar 13, 2018

@IISResetMe This PR is a good enhancement to have. But it's been a while since this PR was left unattended, so I updated the changes to address the $_ pending issue. Hope you don't mind.

My changes were:

Squash the existing commits to a single one.
Refactor ReplaceOperatorImpl with switch (pattern matching).
Add copyright string to the new .ps1 file and use $_ for the -replace xx { ... } test case.

@iSazonov @lzybkr @powercode Can you please take another look?

iSazonov · Mar 13, 2018

iSazonov reviewed Mar 13, 2018

View changes

iSazonov · Mar 13, 2018

Sign in to view

+            $res = "ID 0000123" -replace "\b0+", $matchEvaluator
+            $res | Should Be "ID     123"
+        }
+    }


I'd exclude substitutionMethod too.

iSazonov · Mar 13, 2018

Sign in to view

+                case ScriptBlock sb:
+                    MatchEvaluator me = match => {
+                        var result = sb.DoInvokeReturnAsIs(
+                            useLocalScope: false, /* Use local scope to be consistent with 'ForEach/Where-Object {}' and 'collection.ForEach{}/Where{}' */


I don't understand the comment here - we use false.

Good catch. It's a typo, should be "use current scope ..."

iSazonov · Mar 13, 2018

src/System.Management.Automation/engine/lang/parserutils.cs

+                    return regex.Replace(input, matchEvaluator);
+
+                default:
+                    string replacement = PSObject.ToStringParser(context, substitute);


I don't see tests for PSObject.ToStringParser - why we use this?

It's used in the original code, to convert an object to a string:

if (rList.Count > 1) { replacement = PSObject.ToStringParser(context, rList[1]); }

What PowerShell magic it give us? If it is useful I'd prefer to see related tests.

It converts an object to a string in a powershell way and is used extensively in powershell. You can see the code yourself for more information.

It's an internal method at https://github.com/PowerShell/PowerShell/blob/master/src/System.Management.Automation/engine/MshObject.cs#L1206

@daxian-dbw Thanks! I thought that using the method add some magic in the PR but no - it is only to get string.

iSazonov · Mar 13, 2018

Sign in to view

@@ -40,7 +43,7 @@ Describe "Replace Operator" -Tags CI {
        }

        It "Replace operator can take a ScriptBlock in place of a substitution string" {
-            $res = "ID ABC123" -replace "\b[A-C]+", {return "X" * $args[0].Value.Length}


I'd keep the $args test too.

$args won't work. When used in the form of something -replace xxx, { ... }, the "Match" object is passed to $_ of the script block, just like ForEach/Where-Object {...} and collection.ForEach{...}/Where{...}.

powercode · Mar 13, 2018

@daxian-dbw LGTM!

IISResetMe · Mar 13, 2018

Thanks @daxian-dbw! Couldn't get $_ to work inside the MatchEvaluator and then life got in the way :)

Looks good!

daxian-dbw · Mar 13, 2018

@iSazonov Your comments have been addressed. Can you please take another look?

lzybkr · Mar 13, 2018

lzybkr approved these changes Mar 13, 2018

View changes

daxian-dbw · Mar 14, 2018

daxian-dbw dismissed iSazonov’s stale review Mar 14, 2018

The typo 'local scope' in the comment has been corrected to 'current scope'. Feel free to leave other comments.

daxian-dbw · Mar 14, 2018

@iSazonov Given that your pending comment is not blocking (questions about PSObject.ToStringParser), I will merge this PR. Feel free to continue the discussion in the comment thread.

IISResetMe requested review from BrucePay, daxian-dbw and lzybkr as code owners Jan 25, 2018

iSazonov requested review from lzybkr and removed request for lzybkr Jan 26, 2018

daxian-dbw added the Review - Committee label Jan 27, 2018

SteveL-MSFT added Committee-Reviewed and removed Review - Committee labels Jan 31, 2018

daxian-dbw self-assigned this Feb 1, 2018

lzybkr referenced this pull request Feb 16, 2018
Closed
Enhancement: support a script block as the replacement operand of the -replace operator to enable dynamic substitutions #6164

Pass the 'Match' object to $_ for the substitute script block in '-re…
…place' operation (#6029)

Loading status checks…

02107f6

daxian-dbw added the Documentation Needed label Mar 13, 2018

daxian-dbw dismissed iSazonov’s stale review Mar 14, 2018

mklement0 referenced this pull request May 9, 2018
Closed
Dollar Sign Gets 'Eaten' by -replace When More Than One Dollar Sign Adjacent In String #6842

mklement0 referenced this pull request Jun 2, 2018
Closed
Combining the -replace operator with a lookbehind assertion slowed down by a factor of 30+ between v6.0.2 and v6.1.0-preview.2 #6976

joeyaiello referenced this pull request Oct 15, 2018
Closed
Docs needed for 'Add ScriptBlock substitution overload to -replace operator' #3046

joeyaiello removed Documentation Needed labels Oct 15, 2018

Jun	JUL	Aug
	17
2018	2019	2020

PowerShell/PowerShell

Join GitHub today

Add ScriptBlock substitution overload to -replace operator #6029

Conversation

IISResetMe commented Jan 25, 2018 • edited by iSazonov

PR Summary

IISResetMe requested review from BrucePay, daxian-dbw and lzybkr as code owners Jan 25, 2018

powercode reviewed Jan 25, 2018 View changes

This comment has been minimized.

powercode Jan 25, 2018 • edited

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

powercode commented Jan 26, 2018 • edited

powercode reviewed Jan 26, 2018 View changes

This comment has been minimized.

This comment has been minimized.

IISResetMe Jan 26, 2018 • edited

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

LaurentDardenne commented Jan 26, 2018

This comment has been minimized.

IISResetMe commented Jan 26, 2018

This comment has been minimized.

LaurentDardenne commented Jan 26, 2018

iSazonov requested review from lzybkr and removed request for lzybkr Jan 26, 2018

iSazonov reviewed Jan 26, 2018 View changes

This comment has been minimized.

This comment has been minimized.

powercode approved these changes Jan 26, 2018 View changes

powercode reviewed Jan 26, 2018 View changes

This comment has been minimized.

This comment has been minimized.

IISResetMe Jan 26, 2018 • edited

This comment has been minimized.

iSazonov reviewed Jan 26, 2018 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

daxian-dbw added the Review - Committee label Jan 27, 2018

iSazonov reviewed Jan 27, 2018 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

iSazonov commented Jan 27, 2018

This comment has been minimized.

SteveL-MSFT commented Jan 31, 2018

SteveL-MSFT added Committee-Reviewed and removed Review - Committee labels Jan 31, 2018

This comment has been minimized.

IISResetMe commented Feb 1, 2018 • edited

This comment has been minimized.

lzybkr commented Feb 1, 2018

This comment has been minimized.

IISResetMe commented Feb 1, 2018 • edited

This comment has been minimized.

IISResetMe commented Feb 1, 2018 • edited

This comment has been minimized.

IISResetMe commented Feb 1, 2018

This comment has been minimized.

lzybkr commented Feb 1, 2018

daxian-dbw self-assigned this Feb 1, 2018

lzybkr referenced this pull request Feb 16, 2018

This comment has been minimized.

iSazonov commented Feb 16, 2018

This comment has been minimized.

iSazonov commented Feb 16, 2018

This comment has been minimized.

daxian-dbw commented Mar 13, 2018 • edited

iSazonov reviewed Mar 13, 2018 View changes

This comment has been minimized.

This comment has been minimized.

IISResetMe commented Jan 25, 2018 •

edited by iSazonov

powercode reviewed Jan 25, 2018

View changes

powercode Jan 25, 2018 •

edited

powercode commented Jan 26, 2018 •

edited

powercode reviewed Jan 26, 2018

View changes

IISResetMe Jan 26, 2018 •

edited

iSazonov reviewed Jan 26, 2018

View changes

powercode approved these changes Jan 26, 2018

View changes

powercode reviewed Jan 26, 2018

View changes

IISResetMe Jan 26, 2018 •

edited

iSazonov reviewed Jan 26, 2018

View changes

iSazonov reviewed Jan 27, 2018

View changes

IISResetMe commented Feb 1, 2018 •

edited

IISResetMe commented Feb 1, 2018 •

edited

IISResetMe commented Feb 1, 2018 •

edited

daxian-dbw commented Mar 13, 2018 •

edited

iSazonov reviewed Mar 13, 2018

View changes

iSazonov Mar 14, 2018 •

edited

daxian-dbw Mar 14, 2018 •

edited

lzybkr approved these changes Mar 13, 2018

View changes

daxian-dbw merged commit `7721042` into PowerShell:master Mar 14, 2018
4 checks passed