Do not wrap return result to `PSObject` when converting ScriptBlock to delegate #10619

daxian-dbw · 2019-09-24T23:06:11Z

PR Summary

Avoid wrapping return result to PSObject when converting ScriptBlock to delegate.
When a ScriptBlock is converted to a delegate type, it's supposed to be used in C# context, and wrapping the result to PSObject brings unneeded troubles:

When the value can be converted to the delegate return type, the PSObject essentially gets unwrapped, so we basically creates PSObject in vain.
When the delegate return type is object, the PSObject is returned, making it hard to work with the true return value in C# code.

This is a breaking change for the delegate types with the object return type:

Before this change, the returned object will always be an PSObject instance.
After this change, the returned object is the underlying object, which could still be an PSObject if that's what the script actually returns.

But I think this would fall in the Bucket 3 Grey Area.
With this change, when setting AddToHistoryHandler to { param($line) $line -notmatch '^\s+|ToSecureString|AsPlainText' }, the time to load history entries from the history file reduces about 100 ms on my dev machine (from ~1750 to ~1650 ) for 213226 history records in the file.

PR Context

PR Checklist

…delegate

lzybkr

This could be a nightmare to track down if it breaks any real world code. It's not the sort of thing that is easy to search for.

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

daxian-dbw · 2019-09-25T00:13:39Z

It's fine to reject this PR if we consider it too risky.
I submit the PR only because the change is so simple.

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

iSazonov · 2019-09-25T03:36:03Z

it's supposed to be used in C# context,

Can a (enhanced) telemetry help to discover where (projects) and how it is used?
Our PR template mentions only PowerShell Editor Services, Completions, PSScriptAnalyzer, EditorSyntax. What other projects could be impacted by this change? These should be projects already ported to .Net Core and PowerShell Core. Have MSFT such projects internally to test the change and weight a impact?
I guess that there are not many projects that we could break. In addition, it will be a new major SDK version, which suggests the admissibility of such breaking changes - developers will in any case test new version.

daxian-dbw · 2019-09-25T23:34:53Z

The only case I can think of that will be affected is that the target delegate type has the return type object, and the user is casting the return value to PSObject directly, like var psobj = (PSObject) MyHandler().
I think for someone using it this way, he/she might actually assume the script block to write out PSObject instead of knowing that the output is always wrapped into PSObject.

iSazonov · 2019-09-26T06:38:08Z

Looking the code

PowerShell/src/System.Management.Automation/engine/lang/scriptblock.cs

Lines 777 to 789 in 6965ba7

    
           Expression call = Expression.Call( 
        
               Expression.Constant(this), 
        
               CachedReflectionInfo.ScriptBlock_InvokeAsDelegateHelper, 
        
               dollarUnderExpr, 
        
               dollarThisExpr, 
        
               Expression.NewArrayInit(typeof(object), parameterExprs.Select(p => p.Cast(typeof(object))))); 
        
           if (returnsSomething) 
        
           { 
        
               call = DynamicExpression.Dynamic( 
        
                   PSConvertBinder.Get(invokeMethod.ReturnType), 
        
                   invokeMethod.ReturnType, 
        
                   call); 
        
           }

What does the code return still unwrapped object or already PSObject? (I mean PSConvertBinder)

daxian-dbw · 2019-09-26T17:27:07Z

@iSazonov Can you please rephrase your question? I'm not sure I understand it.

iSazonov · 2019-09-26T17:32:55Z

The code I referenced calls a code you modified in the PR. I guess that PSConvertBinder.Get(invokeMethod.ReturnType) wrap results in PSObject too. If so your change is not a breaking change.

daxian-dbw · 2019-09-26T22:46:31Z

PSConvertBinder doesn't wrap results in PSObject. It does the right thing :)

adityapatwardhan · 2019-12-10T18:00:25Z

@daxian-dbw Is this ready?

daxian-dbw · 2019-12-10T18:32:51Z

No, this is not ready. I don't think this should be in rc.1. Nobody really reviews it and it might be rejected by the committee.

iSazonov · 2019-12-10T18:40:09Z

I think it is right direction. Delegates look good for future optimizations and this cleanup once can simplify our lives.

daxian-dbw · 2019-12-12T00:00:49Z

@PowerShell/powershell-committee reviewed this PR and believes the possible breaking case described in #10619 (comment) is unlikely and it falls into the Bucket 3: Unlikely Grey Area. Therefore, this PR is approved.

iSazonov · 2019-12-12T04:52:41Z

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

+                    object resultArray = result.ToArray();
+                    return wrapToPSObject ? LanguagePrimitives.AsPSObjectOrNull(resultArray) : resultArray;


Is the converting to immutable array mandatory?

@iSazonov Sorry for the super long delay to respond to your comment.
The result.ToArray() is existing code, and it would be a breaking change to end users to change that because users may expect an object array as it's always be.

If the PR breaks an C# application in any case and developers have to fix their code we could weight removing result.ToArray() too. If we think about enumeration the replacing List<object> with Array does seem not break this. We could add test for the default case.

@daxian-dbw Could you please share your thoughts about my last comment?

Note that this method is also used by DoInvokeReturnAsIs. The current change keeps the existing behavior of that API, but removing result.ToArray() definitely breaks it.
Also, there is no obvious benefit by removing result.ToArray() except for a bit less allocation that may not be noticeable, so I don't think a breaking change can be justified.

daxian-dbw · 2020-03-26T18:41:37Z

@iSazonov I just reviewed this PR and think it's ready for official review. Can you please take a look when you have time? Thanks!

iSazonov · 2020-03-27T04:38:38Z

@SeeminglyScience Perhaps you are interesting to review the PR.

daxian-dbw · 2020-03-27T05:08:23Z

@rjmholt please also review this PR. Thanks!

SeeminglyScience

LGTM

rjmholt

This seems reasonable. I take the point about real world code depending on this, but it's also something that makes more sense this way and is probably not very widely used, so seems less dangerous than we might think

daxian-dbw · 2020-03-30T16:26:14Z

@iSazonov I think this is ready to merge. Thanks!

iSazonov · 2020-03-30T16:46:56Z

Should we have a doc issue?

daxian-dbw · 2020-03-30T18:10:24Z

Good point. I don't think we have a doc about the ScriptBlock-casting-to-delegate behavior, but I could be wrong.

ghost · 2020-04-23T18:03:50Z

🎉v7.1.0-preview.2 has been released which incorporates this pull request.:tada:

Handy links:

Release Notes

Do not wrap return result to PSObject when converting ScriptBlock to …

89132bf

…delegate

daxian-dbw requested review from lzybkr and SteveL-MSFT September 24, 2019 23:06

Add test

32a63fc

daxian-dbw added the Breaking-Change breaking change that may affect users label Sep 24, 2019

lzybkr reviewed Sep 25, 2019

View reviewed changes

PoshChan reviewed Sep 25, 2019

View reviewed changes

src/System.Management.Automation/engine/lang/scriptblock.cs Show resolved Hide resolved

PoshChan reviewed Sep 25, 2019

View reviewed changes

src/System.Management.Automation/engine/lang/scriptblock.cs Show resolved Hide resolved

PoshChan reviewed Sep 25, 2019

View reviewed changes

src/System.Management.Automation/engine/lang/scriptblock.cs Show resolved Hide resolved

fix test

6965ba7

TravisEz13 assigned anmenaga Oct 24, 2019

TravisEz13 added this to the rc.1-consider milestone Dec 3, 2019

daxian-dbw added the Review - Committee The PR/Issue needs a review from the PowerShell Committee label Dec 10, 2019

SteveL-MSFT modified the milestones: rc.1-consider, 7.1-Consider Dec 10, 2019

daxian-dbw added Committee-Reviewed PS-Committee has reviewed this and made a decision and removed Review - Committee The PR/Issue needs a review from the PowerShell Committee labels Dec 12, 2019

iSazonov reviewed Dec 12, 2019

View reviewed changes

daxian-dbw requested a review from rjmholt March 27, 2020 05:07

SeeminglyScience approved these changes Mar 27, 2020

View reviewed changes

daxian-dbw assigned iSazonov and unassigned anmenaga Mar 27, 2020

iSazonov modified the milestones: 7.1-Consider, 7.1.0-preview.2 Mar 27, 2020

rjmholt approved these changes Mar 27, 2020

View reviewed changes

iSazonov approved these changes Mar 30, 2020

View reviewed changes

iSazonov merged commit c0c17de into PowerShell:master Mar 30, 2020

daxian-dbw deleted the sbToDelegate branch March 30, 2020 18:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do not wrap return result to `PSObject` when converting ScriptBlock to delegate #10619

Do not wrap return result to `PSObject` when converting ScriptBlock to delegate #10619

daxian-dbw commented Sep 24, 2019 •

edited

lzybkr left a comment

daxian-dbw commented Sep 25, 2019

iSazonov commented Sep 25, 2019

daxian-dbw commented Sep 25, 2019

iSazonov commented Sep 26, 2019 •

edited

daxian-dbw commented Sep 26, 2019

iSazonov commented Sep 26, 2019 •

edited

daxian-dbw commented Sep 26, 2019

adityapatwardhan commented Dec 10, 2019

daxian-dbw commented Dec 10, 2019

iSazonov commented Dec 10, 2019

daxian-dbw commented Dec 12, 2019

iSazonov Dec 12, 2019

daxian-dbw Mar 26, 2020

iSazonov Mar 27, 2020 •

edited

iSazonov Mar 30, 2020

daxian-dbw Mar 30, 2020

daxian-dbw commented Mar 26, 2020

iSazonov commented Mar 27, 2020

daxian-dbw commented Mar 27, 2020

SeeminglyScience left a comment

rjmholt left a comment

daxian-dbw commented Mar 30, 2020

iSazonov commented Mar 30, 2020

daxian-dbw commented Mar 30, 2020

ghost commented Apr 23, 2020

		object resultArray = result.ToArray();
		return wrapToPSObject ? LanguagePrimitives.AsPSObjectOrNull(resultArray) : resultArray;

Do not wrap return result to PSObject when converting ScriptBlock to delegate #10619

Do not wrap return result to PSObject when converting ScriptBlock to delegate #10619

Conversation

daxian-dbw commented Sep 24, 2019 • edited

PR Summary

PR Context

PR Checklist

lzybkr left a comment

Choose a reason for hiding this comment

daxian-dbw commented Sep 25, 2019

iSazonov commented Sep 25, 2019

daxian-dbw commented Sep 25, 2019

iSazonov commented Sep 26, 2019 • edited

daxian-dbw commented Sep 26, 2019

iSazonov commented Sep 26, 2019 • edited

daxian-dbw commented Sep 26, 2019

adityapatwardhan commented Dec 10, 2019

daxian-dbw commented Dec 10, 2019

iSazonov commented Dec 10, 2019

daxian-dbw commented Dec 12, 2019

iSazonov Dec 12, 2019

Choose a reason for hiding this comment

daxian-dbw Mar 26, 2020

Choose a reason for hiding this comment

iSazonov Mar 27, 2020 • edited

Choose a reason for hiding this comment

iSazonov Mar 30, 2020

Choose a reason for hiding this comment

daxian-dbw Mar 30, 2020

Choose a reason for hiding this comment

daxian-dbw commented Mar 26, 2020

iSazonov commented Mar 27, 2020

daxian-dbw commented Mar 27, 2020

SeeminglyScience left a comment

Choose a reason for hiding this comment

rjmholt left a comment

Choose a reason for hiding this comment

daxian-dbw commented Mar 30, 2020

iSazonov commented Mar 30, 2020

daxian-dbw commented Mar 30, 2020

ghost commented Apr 23, 2020

Do not wrap return result to `PSObject` when converting ScriptBlock to delegate #10619

Do not wrap return result to `PSObject` when converting ScriptBlock to delegate #10619

daxian-dbw commented Sep 24, 2019 •

edited

iSazonov commented Sep 26, 2019 •

edited

iSazonov commented Sep 26, 2019 •

edited

iSazonov Mar 27, 2020 •

edited