Further improve PSMethod to Delegate conversion #6851

daxian-dbw · 2018-05-10T19:59:10Z

PR Summary

Fix partially #5618
Refactor code to make it easier to maintain and a little faster. Changes are as follows:

Support finding a matching signature with variance. But make PowerShell prefer exact match over match with variance.

class Foo {
    static [string] GetA([System.IO.FileSystemInfo] $fsinfo, [object] $o)
    {
        return "Varaince Match: GetA-filesysteminfo-object-string"
    }

    static [string] GetB([System.IO.FileSystemInfo] $fsinfo, [object] $o)
    {
        return "Variance Match: GetB-filesysteminfo-object-string"
    }
    static [object] GetB([System.IO.FileInfo] $finfo, [string] $s)
    {
        return "Exact Match: GetB-fileinfo-string-object"
    }
}

[Func[[System.IO.FileInfo], [string], [object]]] $f = [Foo]::GetA
$f.Invoke([System.IO.FileInfo]::new("abc"), "string")
>  Varaince Match: GetA-filesysteminfo-object-string

$f = [Foo]::GetB
$f.Invoke([System.IO.FileInfo]::new("abc"), "string")
>  Exact Match: GetB-fileinfo-string-object

The metadata signatures in PSMethod<..> are generated based on the array of method overloads in MethodCacheEntry.MethodInformationStructures, in the exact same order. So in LanguagePrimitive.ConvertViaParseMethod, when we try to figure out if there is a match using the metadata signatures in PSMethod<..>, we can get the index of the matching signature, and the same index should locate the matching metod in MethodCacheEntry.MethodInformationStructures. Therefore, we don't need to compare signatures again in the actual conversion method, and instead, we can just leverage the index we found when figuring out conversion in ConvertViaParseMethod.
- This gets rid of the reflection call GetMethod("Invoke") and the subsequent signature comparisons in the final conversion method.
- Also, when comparing signatures using PSMethod<..> in ConvertViaParseMethod, we can just use the generaic argument types of each Func<..> metadata type, instead of calling GetMethod("Invoke") and then GetParameters(). This makes the code for comparing signatures simpler (the type SignatureComparator).
- Move MatchesPSMethodProjectedType from PSMemberInfo.cs to the type SignatureComparator in LanguagePrimitives.cs, as it's closely related to the signature comparison. Also, renamed it to ProjectedTypeMatchesTargetType.
- These changes make PSMethod-to-Delegate conversion a little faster, but no big improvement, as the true bottleneck probably is in delegate creation(?). Actually, the performance of this conversion is not critical at all at this moment because this feature should rarely be used in any hot script path. So this exercise is mainly for fun. If we support extension methods one day, the conversion performance may be more interesting (LINQ extension method calls with predicate delegate). See the measurements on my machine below (both powershell fresh started with -nopro on a warm machine, measure.ps1.txt is attached):

### both built with netcoreapp2.1, and crossgen'ed

####### Before change ######
PS C:\> C:\arena\tmp\measure.ps1
10000 loops per iteration, 10 iterations
Maximum time: 139
Minimum time: 113
Average time: 117.3
PS C:\> C:\arena\tmp\measure.ps1
10000 loops per iteration, 10 iterations
Maximum time: 124
Minimum time: 114
Average time: 117.5
PS C:\>
PS C:\> C:\arena\tmp\measure.ps1
10000 loops per iteration, 10 iterations
Maximum time: 240
Minimum time: 111
Average time: 125.5

###### After Change ######
PS C:\> C:\arena\tmp\measure.ps1
10000 loops per iteration, 10 iterations
Maximum time: 129
Minimum time: 99
Average time: 102.3
PS C:\> C:\arena\tmp\measure.ps1
10000 loops per iteration, 10 iterations
Maximum time: 133
Minimum time: 98
Average time: 105.5
PS C:\> C:\arena\tmp\measure.ps1
10000 loops per iteration, 10 iterations
Maximum time: 104
Minimum time: 98
Average time: 99.4

Remove PSEnum<T>. We can directly use enum types when constructing the metadata type Func<..>.
Remove the code that generate metadata signatures for generic methods (call MakeGenericMethod with fake types like GenericType0, GenericType1). This is because:
- We don't support convert generic method to delegate today, so may be better not spending time on preparing the metadata signature types for those methods.
- When the day comes that we need to support it, it's better to use generic argument types directly to construct the Func<..> metadata types. I left comments in GetMethodGroupType method in PSMemberInfo.cs to explain why that approach is better.

PR Checklist

rjmholt · 2018-05-11T17:32:55Z

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

@@ -3486,6 +3443,64 @@ private static PSConverter<bool> CreateNumericToBoolConverter(Type fromType)
            return ConvertStringToEnum(sbResult.ToString(), resultType, recursion, originalValueToConvert, formatProvider, backupTable);
        }

+        private class ConvertPSMethodToDelegate


PSMethodDelegateConverter might be a more "object-oriented" name for this class.

Changed to PSMethodToDelegateConverter.

rjmholt · 2018-05-11T17:38:42Z

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

+                return result;
+            }
+
+            internal Delegate Convert(object valueToConvert,


I've only ever seen docs refer to delegate. Is this different? There seems to be a general preference for using the type aliases like delegate and string in code I've read both here and in documentation.

delegate is a keyword for you to declare a delegate type, either named or anonymous. Unlike string, it cannot be used as a type, for example var a = typeof(delegate) results in a compilation error.

Ah I see! I tried to find documentation on it, but Google doesn't do case-sensitivity well

rjmholt · 2018-05-11T17:46:24Z

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

+                if (result == null)
+                {
+                    var converter = new ConvertPSMethodToDelegate(matchIndex);
+                    Threading.Interlocked.CompareExchange(ref s_converterCache[matchIndex], converter, null);


Might be worth adding a comment here explaining what's going on (reading up on Threading.Interlocked.CompareExchange takes a while). Just something like "if the cache entry is null, generate a new one and replace it".

rjmholt · 2018-05-11T17:49:46Z

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

                while (signatureEnumerator.MoveNext())
                {
-                    var candidate = signatureEnumerator.Current.GetMethod("Invoke");
-                    if (comparator.SignatureMatches(candidate.ReturnType, candidate.GetParameters()))
+                    index ++;


I would remove the whitespace between index and ++ here.

rjmholt · 2018-05-11T18:01:13Z

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

+                }
+            }
+        }
+
        private class ConvertViaParseMethod


This class also has a verb-oriented name that should probably be changed.

Rename a bunch of existing names will make the PR harder to review. I would prefer another PR if we want to change the names.

Yes, agreed. Thought it was useful to express here on the basis of my other comment about a class name.

rjmholt · 2018-05-11T18:25:45Z

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

        {
            if (methodInfo.DeclaringType.IsGenericTypeDefinition)
            {
+                // If the method is from a generic type definition, consider it not convertable


convertable -> convertible

rjmholt · 2018-05-11T18:28:17Z

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

+                // However, we don't yet handle generic methods in PSMethod-to-Delegate conversion, so for now, we
+                // don't produce the metadata type that represent the signature of a generic method.
+                //
+                // Say one day we want to support generic method in PSMethod-to-Delegate conversion and need to produce


This comment is amazing -- someone is going to be very grateful it exists one day.

rjmholt · 2018-05-11T18:35:35Z

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

                return typeof(Func<PSNonBindableType>);
            }

-            var res = new Type[parameterInfos.Length + 1];
-            for (int i = 0; i < res.Length - 1; i++)
+            var allTypes = new Type[parameterInfos.Length + 1];


I know you already renamed this, but something like methodTypes seems clearer than allTypes.

rjmholt · 2018-05-11T18:36:43Z

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

-
-    struct PSEnum<T> { }
-
+    class VOID { }


Should we be explicit here with internal?

Yes, good catch. Done.

rjmholt · 2018-05-11T20:03:27Z

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

+
+        [Func[[E], [object]]] $f = $n.GetC
+        $f.Invoke([E]::Week) | Should -BeExactly "Week"
+    }


We should also include the following scenarios in tests to ensure we know current behaviour and detect regressions:

Type constraint applied to an argument

[object]$var = "Hi"; $cls.TestMethod($var)

$cls.TestMethod([object]"Hi")

[Super]$sub = [Sub]::new(); $cls.TestMethodWithSuperAndSubOverloads($sub)

$sub = [Sub]::new(); $cls.TestMethodWithSuperAndSubOverloads([Super]$sub)

[string]$fileName = [System.IO.FileInfo]::new("myFile"); $cls.TestMethodWithFileInfoAndStringOverloads($fileName)

$file = [System.IO.FileInfo]::new("myFile"); $cls.TestMethodWithFileInfoAndStringOverloads([string]$file)

class Cls { [string] TestMethod([string] $str) { return "String" } [string] TestMethod([object] $obj) { return "Object" } [string] TestMethodWithSuperAndSubOverloads([Super] $sup) { return "Super" } [string] TestMethodWithSuperAndSubOverloads([Sub] $sub) { return "Sub" } [string] TestMethodWithFileInfoAndStringOverloads([string] $str) { return "FileString" } [string] TestMethodWithFileInfoAndStringOverloads([System.IO.FileInfo] $fileInfo) { return "FileInfo" } } $cls = [Cls]::new()

Sub-class which matches two or more method signatures, but none exactly

class Animal { } class Cat : Animal { } # If the order of method definition is what affects the method resolution using the current # algorithm, we should have another test with the same methods in a different order on a different # class to demonstrate that class TestClass { [string] TestMethod([object] $obj) { return "Object" } [string] TestMethod([Animal] $animal) { return "Animal" } }

Interfaces

Interface as parameter type in signature, object implements interface

Overload of method for multiple interfaces, object implements both interfaces

Implicit coercions

I can't remember if we allow coercions in methods, but if so we must test how coercion works when
there are multiple overloads we could coerce to.

Maybe some scenarios where the method binding should fail, so we cover the failure mode code paths

No overload that matches

Generic methods

It looks to me your focus is on testing the overload resolution when calling a method, instead of PSMethod-to-Delegate conversion. This change doesn't touch the overload resolution for method invocation.

Note that, even if we will consider a more sophisticated resolution for this PSMethod-to-Delegate conversion feature, we probably won't be able to directly use the existing overload resolution code, because, for overload resolution of method invocation, powershell considers converting the argument to the target type, however, for this feature we cannot convert any source type (parameter or return) to the target type.

I do think the fail code path should be tested too.

powercode · 2018-05-14T15:55:55Z

Nice!
Will look closer tonight!

daxian-dbw · 2018-05-14T17:50:52Z

@rjmholt Thanks for your review!
@powercode Please take your time, thanks!

powercode · 2018-05-14T21:27:05Z

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

+                // In this way, '$metadataType.ContainsGenericParameters' returns 'True', indicating it represents a generic method.
+                // And also, given a generic argument type from `$metadataType.GetGenericArguments()`, it's easy to tell if it's a
+                // generic parameter (for example, 'T') based on the property 'IsGenericParameter'.
+                // Moreover, it's also easy to get constraints of the generic parameter, via 'GetGenericParameterConstraints()'


Much cleaner!

We may run into problems with representing methods containing Span<*> parameters.
Span cannot be a generic parameter. That is why I had all the convoluted PSGenericType stuff IIRC.

And since span is getting more and more usage, it would be nice if we have a story around it.
Sample below:

class A{ public static void Foo(Span<char> x){}} delegate void SpanAFunc(Span<char> a);

[SpanAFunc] $x = [A]::Foo;

But it is probably the right thing to special case Span anyway, since it is an exceptional type.

Very good point. I will add a placeholder type PSSpan<> to represent a Span<> type.

In fact, it's not just types like Span<char> or ReadOnlySpan<int>, but a general problem for all ref-like types, such as public ref struct Foo { private Span<int> _pointer }. I don't have a generic solution for the ref-lik type at this moment, but at least we should handle Span<T> and ReadOnlySpan<T>. I have updated #5618 to track this work.

Interestingly, as typeof(Func<,>).MakeGenericType(new Type[] { typeof(Span<int>), typeof(int) }) in C# (7.2 language) throws TypeLoadException as expected, [Func`2].MakeGenericType(@([System.Span[int]], [int])) runs successfully in powershell 6.1.0-preview.2 ~~and the build from latest master~~. Scratch that, it does fail in master branch now.

Good news is that we catch TypeLoadException when constructing the metadata type Func<>, so at least a method with ref-like type parameter or return type won't crash PowerShell even if MakeGenericType fails -- it will only mark the method as NonBindable and thus fails any conversion.

I will merge this PR as is, and submit another PR later to address the ref-like types (or at least the Span<T> and ReadOnlySpan<T>).

powercode

Nice cleanup

powercode · 2018-05-14T21:58:42Z

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

+                // In this way, '$metadataType.ContainsGenericParameters' returns 'True', indicating it represents a generic method.
+                // And also, given a generic argument type from `$metadataType.GetGenericArguments()`, it's easy to tell if it's a
+                // generic parameter (for example, 'T') based on the property 'IsGenericParameter'.
+                // Moreover, it's also easy to get constraints of the generic parameter, via 'GetGenericParameterConstraints()'


We may run into problems with representing methods containing Span<*> parameters.
Span cannot be a generic parameter. That is why I had all the convoluted PSGenericType stuff IIRC.

And since span is getting more and more usage, it would be nice if we have a story around it.
Sample below:

class A{ public static void Foo(Span<char> x){}} delegate void SpanAFunc(Span<char> a);

[SpanAFunc] $x = [A]::Foo;

powercode · 2018-05-14T21:59:37Z

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

+                // In this way, '$metadataType.ContainsGenericParameters' returns 'True', indicating it represents a generic method.
+                // And also, given a generic argument type from `$metadataType.GetGenericArguments()`, it's easy to tell if it's a
+                // generic parameter (for example, 'T') based on the property 'IsGenericParameter'.
+                // Moreover, it's also easy to get constraints of the generic parameter, via 'GetGenericParameterConstraints()'


But it is probably the right thing to special case Span anyway, since it is an exceptional type.

daxian-dbw · 2018-05-15T07:10:27Z

The reminding CodeFactor issues are from the existing code, not changes from this PR.

daxian-dbw added 4 commits May 6, 2018 23:19

[Feature] Refactor and improve PSMethod to Delegate conversion

86afcda

[Feature] Add tests

a23368a

[Feature] Add more comment

47041a0

[Feature] Add minor optimization

29eff8d

daxian-dbw requested review from powercode, BrucePay and rjmholt May 10, 2018 19:59

rjmholt suggested changes May 11, 2018

View reviewed changes

[Feature] Address Rob's comments

8e6a0b9

rjmholt approved these changes May 13, 2018

View reviewed changes

[Feature] Add a test for fail-to-convert code path

8f0a359

powercode reviewed May 14, 2018

View reviewed changes

powercode approved these changes May 14, 2018

View reviewed changes

daxian-dbw mentioned this pull request May 15, 2018

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

Closed

6 tasks

daxian-dbw merged commit aa0af5e into PowerShell:master May 15, 2018

daxian-dbw deleted the psmethod branch May 15, 2018 18:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Further improve PSMethod to Delegate conversion #6851

Further improve PSMethod to Delegate conversion #6851

daxian-dbw commented May 10, 2018 •

edited

rjmholt May 11, 2018

daxian-dbw May 11, 2018

rjmholt May 11, 2018

daxian-dbw May 11, 2018

rjmholt May 13, 2018

rjmholt May 11, 2018

daxian-dbw May 11, 2018

rjmholt May 11, 2018

daxian-dbw May 11, 2018

rjmholt May 11, 2018

daxian-dbw May 11, 2018

rjmholt May 13, 2018

rjmholt May 11, 2018

daxian-dbw May 11, 2018

rjmholt May 11, 2018

rjmholt May 11, 2018

daxian-dbw May 11, 2018

rjmholt May 11, 2018

daxian-dbw May 11, 2018

rjmholt May 11, 2018 •

edited

daxian-dbw May 12, 2018 •

edited

powercode commented May 14, 2018

daxian-dbw commented May 14, 2018

powercode May 14, 2018

powercode May 14, 2018

powercode May 14, 2018

daxian-dbw May 15, 2018

daxian-dbw May 15, 2018 •

edited

daxian-dbw May 15, 2018

powercode left a comment

powercode May 14, 2018

powercode May 14, 2018

daxian-dbw commented May 15, 2018

Further improve PSMethod to Delegate conversion #6851

Further improve PSMethod to Delegate conversion #6851

Conversation

daxian-dbw commented May 10, 2018 • edited

PR Summary

PR Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rjmholt May 11, 2018 • edited

Choose a reason for hiding this comment

daxian-dbw May 12, 2018 • edited

Choose a reason for hiding this comment

powercode commented May 14, 2018

daxian-dbw commented May 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daxian-dbw May 15, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

powercode left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daxian-dbw commented May 15, 2018

daxian-dbw commented May 10, 2018 •

edited

rjmholt May 11, 2018 •

edited

daxian-dbw May 12, 2018 •

edited

daxian-dbw May 15, 2018 •

edited