Conversions from PSMethod to Delegate #5287

powercode · 2017-10-31T21:30:47Z

Underpinnings to make calling of Extension methods /Linq
easier from PowerShell.

Enables the following that previously had to be done via
reflection.

class M {
    static [int] DoubleStrLen([string] $value) { return 2 * $value.Length }
    
    static [long] AggregateString([string[]] $values, [func[string, int]] $selector) {
        [long] $res = 0
        foreach($s in $values){
            $res += $selector.Invoke($s)
        }
        return $res
    }        
}

[M]::AggregateString((gci).Name, [M]::DoubleStrLen)

Each PSMethod is created as with a unique type for the combinations of
method signatures in the MethodInfos it represents.

PSMethod where T is a MethodGroup<>, potentially recursive in the
last template argument.

This way, we can determine by just looking at the type of a PSMethod
if there exists a conversion from the PSMethod to a delegate.

Underpinnings to make calling of Extension methods /Linq easier from PowerShell. Enables the following that previously had to be done via reflection. class M { [int] Twice([int] $value) { return 2 * $value } [int] DoubleSum([int[]] $values) { return [Linq.Enumerable]::Sum($values, [M]::Twice) } } Each PSMethod is created as with a unique type for the combinations of method signatures in the MethodInfos it represents. PSMethod<T> where T is a MethodGroup<>, potentially recursive in the last template argument. This way, we can determine by just looking at the type of a PSMethod if there exists a conversion from the PSMethod to a delegate.

powercode · 2017-10-31T21:33:53Z

test/powershell/Language/Parser/Conversions.Tests.ps1

                ## To prevent unraveling, Command needs to be `{, [List[int]]@(1,2)}`, but then the test case title
                ## would become `, [List[int]]@(1,2)`, which is more confusing than `$result = [List[int]]@(1,2)`.
                ## This is why the current form of `$result = [List[int]]@(1,2)` is used intentionally here.



VSCode formats this.
Is it OK to keep this formatting?
Or is there a setting that aligns one line hashtables?

In comments usualy we use ' not `.
Also we should put the comment before $testCases1 and start it with one #

iSazonov · 2017-11-01T05:32:29Z

test/powershell/Language/Parser/Conversions.Tests.ps1

-                @{ Command = {$result = [Collections.Generic.List[int]]@(1,2)};     CollectionType = 'List`1'; ElementType = "Int32";  Elements = @(1,2) }
-                @{ Command = {$result = [Collections.Generic.List[int]]"4"};        CollectionType = 'List`1'; ElementType = "Int32";  Elements = @(4) }
-                @{ Command = {$result = [Collections.Generic.List[int]]@("4","5")}; CollectionType = 'List`1'; ElementType = "Int32";  Elements = @(4,5) }
+                @{ Command = {$result = [Collections.Generic.List[int]]@(1)}; CollectionType = 'List`1'; ElementType = "Int32"; Elements = @(1) }


Please keep the previous formatting pattern - it is more readable.

I find git add -p very useful to add just my changes and not the formatting changes.

iSazonov · 2017-11-01T05:36:59Z

test/powershell/Language/Parser/Conversions.Tests.ps1

                @{ Command = {$result = [Collections.Generic.List[System.IO.FileInfo]]@('TestFile')};
-                    CollectionType = 'List`1'; ElementType = "FileInfo";  Elements = @('TestFile') }
+                    CollectionType = 'List`1'; ElementType = "FileInfo"; Elements = @('TestFile')
+                }


We should avoid formatting changes.
Please revert such formatting changes in the file not related to the PR.

iSazonov · 2017-11-01T05:37:28Z

test/powershell/Language/Parser/Conversions.Tests.ps1

+
+    }
+
+


Please remove extra line. Below too.

iSazonov · 2017-11-01T05:38:03Z

test/powershell/Language/Parser/Conversions.Tests.ps1

+        $f = [Func[int, int16, int]] [M]::Add
+        $f.Invoke(2, 6) | Should be 8
+    }
+    It "can call all overloads of M::Foo" {


Please add new line.

iSazonov · 2017-11-01T05:38:32Z

test/powershell/Language/Parser/Conversions.Tests.ps1

+        [Func[TimeSpan, TimeSpan, TimeSpan, TimeSpan, TimeSpan]] $f24 = [M]::Foo
+        $f24.Invoke($timeSpan, [Timespan]::Zero, [Timespan]::Zero, [Timespan]::Zero) | Should BE $timeSpan
+    }
+


Please remove extra line.

iSazonov · 2017-11-01T06:02:32Z

src/System.Management.Automation/engine/MshMemberInfo.cs

+    internal class MethodGroup<T1, T2, T3, T4, T5, T6, T7, T8, T9, T10, T11, T12, T13, T14, T15, T16> : MethodGroup { }
+    internal class MethodGroup<T1, T2, T3, T4, T5, T6, T7, T8, T9, T10, T11, T12, T13, T14, T15, T16, T17, T18, T19, T20, T21, T22, T23, T24, T25, T26, T27, T28, T29, T30, T31, T32> : MethodGroup { }
+
+    class Unit


Private class?

private is not allowed as a modifier. It is implicit.

iSazonov · 2017-11-01T06:07:27Z

src/System.Management.Automation/engine/MshMemberInfo.cs

+            return MoveNext(_t, _current);
+        }
+
+        bool MoveNext(Type type, int index)


Public? Private?

Is there a good reason for this private method? It's only called in one place.

iSazonov · 2017-11-01T06:08:02Z

src/System.Management.Automation/engine/MshMemberInfo.cs

+            return true;
+        }
+
+


Please remove extra line.

iSazonov · 2017-11-01T06:08:39Z

src/System.Management.Automation/engine/MshMemberInfo.cs

+        {
+            return new PSMethod<T>(name, adapter, baseObject, adapterData, isSpecial, isHidden);
+        }
+


Please remove extra line.

iSazonov · 2017-11-01T06:08:45Z

src/System.Management.Automation/engine/MshMemberInfo.cs

+
+    }
+
+


Please remove extra line.

lzybkr

Looks good - just a couple of small complaints.

This must have been a lot of fun to implement - nice work.

lzybkr · 2017-11-01T16:26:50Z

src/System.Management.Automation/engine/LanguagePrimitives.cs

+                            : candidate.CreateDelegate(resultType, psMethod.instance);
+                    }
+                }
+                var msg = $"No matching overload found from {psMethod} to {resultType}";


Strings that appear in error messages need to be in a resource for localization.

lzybkr · 2017-11-01T16:30:47Z

src/System.Management.Automation/engine/LanguagePrimitives.cs

+                return _returnType == typeof(void) ?  returnType == typeof(Unit) : _returnType.IsAssignableFrom(returnType);
+            }
+
+            private bool ParameterTypeMatches(ParameterInfo[] arguments)


ParameterTypesMatches?

lzybkr · 2017-11-01T16:43:53Z

src/System.Management.Automation/engine/MshMemberInfo.cs

+            return Create(name, dotNetInstanceAdapter, baseObject, method, false, false);
+        }
+
+        internal static PSMethod Create(string name, DotNetAdapter dotNetInstanceAdapter, object baseObject, DotNetAdapter.MethodCacheEntry method, bool isSpecial, bool isHidden)


I'm considering imposing a limit on line length :) - working on a laptop this week and GitHub isn't letting me scroll to see the rest of this line.

4k 27" monitor may have desensitized me to the issue :)

lzybkr · 2017-11-01T16:49:12Z

src/System.Management.Automation/engine/MshMemberInfo.cs

+                case 63: return typeof(MethodGroup<,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,>).MakeGenericType(types[0], types[1], types[2], types[3], types[4], types[5], types[6], types[7], types[8], types[9], types[10], types[11], types[12], types[13], types[14], types[15], types[16], types[17], types[18], types[19], types[20], types[21], types[22], types[23], types[24], types[25], types[26], types[27], types[28], types[29], types[30], CreateMethodGroup(types, 31, 32));
+
+            }
+            throw Assert.Unreachable;


You should have a real error or at least NotSupported exception.
And FYI - I've seen real code with > 300 parameters - code from a company I'd like to name but won't. :)

Can this be a problem? I don't really like having these arbitrary constants as limitations.
Is that 300 generic type parameters?

I took some of the limitations from the choises already made for Func/Action, but 63 was just a bit higher than the 58, which was the hightest number of overloads any type in the assemblies loaded by Windows Powershell used.

I am very open to suggestions here.

Oh, not type parameters, just normal. But I think what you have is fine, just need a better error.

lzybkr · 2017-11-01T16:50:57Z

src/System.Management.Automation/engine/MshMemberInfo.cs

+                case 15: return typeof(PSGenericType15);
+                case 16: return typeof(PSGenericType16);
+            }
+            throw Assert.Unreachable;


Real error here - and must resist urge to think of template metaprogramming to solve this problem.

yeah - templates != generics. Or do you have some tricks up your sleave that I don't know about?

Hardcoded up to a limit, and dynamic type generation after that?

Yeah, but hard coded is fine. MutableTuple has an interesting approach if you want to explore it - basically you have:
PSGenericType
PSGenericType<T1, T2>
PSGenericType<T1, T2, T3, T4>
PSGenericType<T1, T2, T3, T4, T5, T6, T7, T8>
etc.

And if your list is too long, you encode the last parameter as another PSGenericType<...>.
If you don't have a power of 2, you use placeholder types like you've done with Unit.

I actually already do that to an extent with MethodGroup. Maybe I could rewrite it to be more general.

lzybkr · 2017-11-01T16:53:55Z

test/powershell/Language/Parser/Conversions.Tests.ps1

-                @{ Command = {$result = [Collections.Generic.List[int]]@(1,2)};     CollectionType = 'List`1'; ElementType = "Int32";  Elements = @(1,2) }
-                @{ Command = {$result = [Collections.Generic.List[int]]"4"};        CollectionType = 'List`1'; ElementType = "Int32";  Elements = @(4) }
-                @{ Command = {$result = [Collections.Generic.List[int]]@("4","5")}; CollectionType = 'List`1'; ElementType = "Int32";  Elements = @(4,5) }
+                @{ Command = {$result = [Collections.Generic.List[int]]@(1)}; CollectionType = 'List`1'; ElementType = "Int32"; Elements = @(1) }


I find git add -p very useful to add just my changes and not the formatting changes.

…eric method Refactoring to move generation and test of method signature to the same place.

powercode · 2017-11-02T18:34:14Z

I would still like a discussion about what's the right thing to do methods with more than 63 overloads and or more than 16 generic parameters.

These are for sure edge cases, but it would be nice to at least handle them gracefully

lzybkr · 2017-11-02T20:35:31Z

@powercode - if you want full generality, as long as MethodGroup is an internal type, you can have a special case where, if the last type parameter is MethodGroup, then you have essentially a linked list, and the type parameters to that last MethodGroup is the continuation of the list.

To be concrete, say we only support 1 or 2 methods in MethodGroup, but you have 3 overloads. You would encode that as:

MethodGroup[
    Func[int, int],
    MethodGroup[
        Func[string, string],
        Func[long, long]]]

Using this trick, you could use a single generic type to represent all generic type parameters, the length of the list would encode T0, T1, etc.

PSGenericType[unit] # T0
PSGenericType[PSGenericType[unit]] # T1
PSGenericType[PSGenericType[PSGenericType[unit]]] # T2
# etc.

powercode · 2017-11-02T21:00:25Z

@lzybkr I kinda already to that:

case 7: return typeof(MethodGroup<,,,>).MakeGenericType(types[0], types[1], types[2], CreateMethodGroup(types, 3, 4));

It is recursive in the last parameter.

powercode · 2017-11-03T12:08:39Z

I just learned that I need to have a representation of Span too. That special class cannot be used as a generic argument. https://github.com/dotnet/corefxlab/blob/master/docs/specs/span.md

powercode · 2017-11-03T12:30:11Z

@lzybkr I will explore the PSGenericType[unit] route in the weekend. I liked the idea better than the hardcoded types. Or maybe a combination, that if more than n parameters, use the template

Handling more cases and having a Func<PSNonBindable> fallback for the cases that I don't handle (Mostly Methods for open generic types and Generic Methods with type constraints.

Correct spelling

Correct formatting

Fix formatting

lzybkr · 2017-11-09T15:45:41Z

src/System.Management.Automation/engine/MshMemberInfo.cs

+
+    internal struct PSMethodSignatureEnumerator : IEnumerator<Type>
+    {
+        private int _current;


_current is not the backing field for Current, you should rename _current to avoid any confusion.

lzybkr

I just had one naming nit that should be addressed.

renaming _current -> _currentIndex

iSazonov

LGTM with 3 minor comment.

iSazonov · 2017-11-14T04:12:10Z

src/System.Management.Automation/resources/ExtendedTypeSystem.resx

+  <data name="PSMethodToDelegateNoMatchingOverLoad" xml:space="preserve">
+    <value>No matching overload found from {0} to {1}</value>
+  </data>
+</root>


Please add new line at EOF.

iSazonov · 2017-11-14T04:14:53Z

test/powershell/Language/Parser/Conversions.Tests.ps1

+        [Func[timespan, timespan, timespan, timespan, timespan, timespan, timespan, timespan, timespan]] $f72 = [M]::Foo
+        $f72.Invoke($timeSpan, [Timespan]::Zero, [Timespan]::Zero, [Timespan]::Zero, [Timespan]::Zero, [Timespan]::Zero, [Timespan]::Zero, [Timespan]::Zero) | Should BE $timeSpan
+    }
+


Please remove extra line.

iSazonov · 2017-11-14T04:15:10Z

src/System.Management.Automation/engine/LanguagePrimitives.cs

                valueToConvert.ToString(), resultType.ToString(), exception.Message);
        }

+


Please remove extra line.

daxian-dbw

Sorry for this late review. I will address the comments myself and also start a feature test run.

daxian-dbw · 2017-12-01T19:32:13Z

src/System.Management.Automation/engine/LanguagePrimitives.cs

+            {
+                var mi = toType.GetMethod("Invoke");
+
+                var isAction = mi.ReturnType == typeof(void);


The isAction variable is not used anywhere.

daxian-dbw · 2017-12-01T19:34:08Z

src/System.Management.Automation/engine/MshMemberInfo.cs

+                    return typeof(Func<PSNonBindableType>);
+                }
+            }
+            if (methodInfo.DeclaringType.IsGenericTypeDefinition)


This check should be moved to the top. If the methodInfo is a generic method from a generic type definition, we will be wasting time calling ReplaceGenericTypeArgumentsWithMarkerTypes.

daxian-dbw · 2017-12-01T19:36:23Z

src/System.Management.Automation/engine/MshMemberInfo.cs

+            var returnType = methodInfo.ReturnType == typeof(void) ? typeof(Unit) : GetPSMethodTypeProjection(methodInfo.ReturnType);
+            res[parameterInfos.Length] = returnType;
+
+            if (res.Length > 17)


This check should be moved to before the 'for' loop: if (parameterInfos.Length > 16)

daxian-dbw · 2017-12-01T19:37:10Z

src/System.Management.Automation/engine/MshMemberInfo.cs

+                res[i] = GetPSMethodTypeProjection(parameterType,
+                    (parameterInfo.Attributes | ParameterAttributes.Out) == ParameterAttributes.Out);
+            }
+            var returnType = methodInfo.ReturnType == typeof(void) ? typeof(Unit) : GetPSMethodTypeProjection(methodInfo.ReturnType);


GetPSMethodTypeProjection(typeof(void)) will return typeof(Unit), so no need to use a conditional operator here.

daxian-dbw · 2017-12-01T19:37:31Z

src/System.Management.Automation/engine/MshMemberInfo.cs

+        {
+            if (type == typeof(void))
+            {
+                type = typeof(Unit);


I think you mean return typeof(Unit).

daxian-dbw · 2017-12-01T21:12:42Z

A few further improvement points

DelegateArgsComparator.ParameterTypesMatches doesn't consider the Variance in delegate parameter types. A method signature int CallMe(object input) should be able to be converted to Func<string, int>.
PSMethod.MatchesPSMethodProjectedType doesn't handle the case where both targetType and projectedSourceType are the same ByRef type or Pointer type.
I think the 'Unit' type name should be changed to 'VOID'. Use type Unit to represent void is not intuitive and I misread it to uint when first looking at the code.
I don't get the point of having ReplaceGenericTypeArgumentsWithMarkerTypes. After making a generic method using our synthetic reference/value types, the generated MethodInfo signature won't match any real delegates except in one condition -- the original generic method doesn't use the generic type argument for any of its parameters or return type -- I think it's rare for this condition to be met for a generic method.

Two questions about the design

It's eye-opening for me to see how you can track the signature information using the generic type argument of PSMethod<> 😮 But why do you choose the current design over adding a List<Type> field/property to PSMethod to keep all the Func<...> delegate types?
With a List<Type> field/property, you don't need the algorithm to create MethodGroup<...> or to enumerate the Func<...> type from the MethodGroup<...> type, and we won't need to make that many generic types, and the code would be easier to understand too.
I agree that we need pseudo types to represent ByRef type (ref/out), Pointer type void type and even TypeReference, but why do we need PSEnum<> to wrap enum types?

lzybkr · 2017-12-01T21:39:15Z

@daxian-dbw - The use of PSMethod<> allows for faster conversions.

If you have a conversion from PSMethod to Action, Action<int>, Action<int,int>, etc., then it requires a more expensive check to determine the conversion validity.

If it is type based, then there is no conversion from PSMethod to Action<int>, but there is for PSMethod<MethodGroup<Action<int>>> to Action<int>.

Put another way, the FigureConversion logic essentially relies on types. If you look closely, you'll see that value dependent conversions do exist, but the design only accommodates them because I couldn't see a better way to support that rarely used scenario.

daxian-dbw · 2017-12-02T00:51:19Z

@lzybkr That makes perfect sense now. I forgot about the conversion cache.

Without PSMethod<> type, the cache is useless for this kind of conversion -- we will end up doing the validity check on every PSMethod -> Delegate conversion.
With the PSMethod<> type, no duplicate validity check is needed. For invalid
PSMethod<> -> Delegate conversions, the conversion cache will have a ConvertNoConversion for the FromType, ToType tuples, powershell won't need to do the check again.

Can you please also shed some light on the PSEnum<> question?

daxian-dbw · 2017-12-02T01:01:56Z

The failed tests in macOS build are known test failures. It's tracked by #5590.

lzybkr · 2017-12-02T01:17:19Z

@powercode will have to address PSEnum<> - he just took my basic idea and worked out all the details - and I do mean all, I might have skipped Pointer because you can't really use it from PowerShell.

daxian-dbw · 2017-12-02T02:38:40Z

@powercode I will merge this PR, but let's continue the discussion if you have time 😄

powercode · 2017-12-30T16:37:39Z

Sorry all for my horrible response time. I have to revisit the code and look at it. Some of the special cases, like pointer, never shows up from PowerShell, but I ran into them by enumerating all methods in all loaded assemblies and tried to make sure I could create PSMethod<>s for them.

iSazonov · 2018-01-03T15:11:02Z

Should we document the feature?

powercode requested review from BrucePay, daxian-dbw and lzybkr as code owners October 31, 2017 21:30

powercode commented Oct 31, 2017

View reviewed changes

iSazonov reviewed Nov 1, 2017

View reviewed changes

lzybkr suggested changes Nov 1, 2017

View reviewed changes

Fixing case where there is a value type constraint on a type of a gen…

a4981bc

…eric method Refactoring to move generation and test of method signature to the same place.

lzybkr changed the title ~~Convertions from PSMethod to Func/Action~~ Conversions from PSMethod to Func/Action Nov 2, 2017

powercode changed the title ~~Conversions from PSMethod to Func/Action~~ Conversions from PSMethod to Delegate Nov 2, 2017

Addressing review comments

5d8dcbb

Fixing refactoring mistake

23a76ea

Staffan Gustafsson added 2 commits November 2, 2017 22:47

Don't impose artificial limit on number of overloads

49321db

Fixing encoding of pointer types

0b1df59

Staffan Gustafsson and others added 4 commits November 7, 2017 21:52

Robustness improvements to MethodInfo=>MethodGroup projection.

6e6e70d

Handling more cases and having a Func<PSNonBindable> fallback for the cases that I don't handle (Mostly Methods for open generic types and Generic Methods with type constraints.

Update CoreAdapter.cs

229f82b

Correct spelling

Update CoreAdapter.cs

f8afa47

Correct formatting

Update MshMemberInfo.cs

db60454

Fix formatting

lzybkr reviewed Nov 9, 2017

View reviewed changes

lzybkr suggested changes Nov 9, 2017

View reviewed changes

lzybkr self-assigned this Nov 9, 2017

Addressing review comments

639000d

renaming _current -> _currentIndex

lzybkr approved these changes Nov 13, 2017

View reviewed changes

lzybkr assigned daxian-dbw and unassigned lzybkr Nov 13, 2017

iSazonov approved these changes Nov 14, 2017

View reviewed changes

[Feature] Address Dongbo's review comments

8d205c0

daxian-dbw reviewed Dec 1, 2017

View reviewed changes

[Feature] Address Ilya's comments

969b694

daxian-dbw merged commit a9c6292 into PowerShell:master Dec 2, 2017

daxian-dbw mentioned this pull request Dec 4, 2017

A few improvement/discussion points of the PSMethod -> Delegate conversion work #5618

Closed

6 tasks

TravisEz13 added this to the 6.1.0-MQ milestone Dec 5, 2017

TravisEz13 mentioned this pull request Dec 5, 2017

Merge changes from master to 6.0.0-rc.2 #5626

Merged

powercode deleted the psmethod-func branch January 26, 2018 19:02

powercode mentioned this pull request Mar 8, 2018

Add conversion from PSMethodInfo to delegate/Func<> #2864

Closed

daxian-dbw mentioned this pull request Apr 5, 2018

Improve PSMethod-to-Delegate conversion #6570

Merged

11 tasks

joeyaiello added the Documentation Needed in this repo Documentation is needed in this repo label Sep 11, 2018

joeyaiello mentioned this pull request Oct 15, 2018

Docs needed for 'Conversions from PSMethod to Delegate' MicrosoftDocs/PowerShell-Docs#3068

Closed

joeyaiello removed Documentation Needed in this repo Documentation is needed in this repo labels Oct 15, 2018

		valueToConvert.ToString(), resultType.ToString(), exception.Message);
		}

Conversions from PSMethod to Delegate #5287

Conversions from PSMethod to Delegate #5287

Uh oh!

Conversation

powercode commented Oct 31, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lzybkr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

powercode commented Nov 2, 2017

Uh oh!

lzybkr commented Nov 2, 2017

Uh oh!

powercode commented Nov 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

powercode commented Nov 3, 2017

Uh oh!

powercode commented Nov 3, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lzybkr left a comment

Choose a reason for hiding this comment

powercode commented Nov 2, 2017 •

edited

Loading

iSazonov Nov 14, 2017 •

edited

Loading

daxian-dbw commented Dec 1, 2017 •

edited

Loading