Skip to content
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

Jump to bottom

Further improve PSMethod to Delegate conversion #6851

Merged
merged 6 commits into from
May 15, 2018
Merged

Further improve PSMethod to Delegate conversion #6851

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
86afcda
[Feature] Refactor and improve PSMethod to Delegate conversion
daxian-dbw May 7, 2018
a23368a
[Feature] Add tests
daxian-dbw May 9, 2018
47041a0
[Feature] Add more comment
daxian-dbw May 10, 2018
29eff8d
[Feature] Add minor optimization
daxian-dbw May 10, 2018
8e6a0b9
[Feature] Address Rob's comments
daxian-dbw May 12, 2018
8f0a359
[Feature] Add a test for fail-to-convert code path
daxian-dbw May 14, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
252 changes: 175 additions & 77 deletions src/System.Management.Automation/engine/LanguagePrimitives.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
using System.Text;
using System.Management.Automation.Internal;
using System.Management.Automation.Runspaces;
using System.Diagnostics.CodeAnalysis; // for fxcop
using System.Diagnostics.CodeAnalysis;
using Dbg = System.Management.Automation.Diagnostics;
using MethodCacheEntry = System.Management.Automation.DotNetAdapter.MethodCacheEntry;

Expand Down Expand Up @@ -3202,49 +3202,6 @@ private static PSConverter<bool> CreateNumericToBoolConverter(Type fromType)
valueToConvert.ToString(), resultType.ToString(), exception.Message);
}

private static Delegate ConvertPSMethodInfoToDelegate(object valueToConvert,
Type resultType,
bool recurse,
PSObject originalValueToConvert,
IFormatProvider formatProvider,
TypeTable backupTable)
{
// We can only possibly convert PSMethod instance of the type PSMethod<T>.
// Such a PSMethod essentially represents a set of .NET method overloads.
var psMethod = (PSMethod)valueToConvert;

try
{
var methods = (MethodCacheEntry)psMethod.adapterData;
var isStatic = psMethod.instance is Type;
var targetMethodInfo = resultType.GetMethod("Invoke");
var comparator = new DelegateArgsComparator(targetMethodInfo);

foreach (var methodInformation in methods.methodInformationStructures)
{
var candidate = (MethodInfo)methodInformation.method;
if (comparator.SignatureMatches(candidate.ReturnType, candidate.GetParameters()))
{
return isStatic ? candidate.CreateDelegate(resultType)
: candidate.CreateDelegate(resultType, psMethod.instance);
}
}
}
catch (Exception e)
{
typeConversion.WriteLine("PSMethod to Delegate exception: \"{0}\".", e.Message);
throw new PSInvalidCastException("InvalidCastExceptionPSMethodToDelegate", e,
ExtendedTypeSystem.InvalidCastExceptionWithInnerException,
valueToConvert.ToString(), resultType.ToString(), e.Message);
}

var msg = String.Format(ExtendedTypeSystem.PSMethodToDelegateNoMatchingOverLoad, psMethod, resultType);
typeConversion.WriteLine($"PSMethod to Delegate exception: \"{msg}\".");
throw new PSInvalidCastException("InvalidCastExceptionPSMethodToDelegate", null,
ExtendedTypeSystem.InvalidCastExceptionWithInnerException,
valueToConvert.ToString(), resultType.ToString(), msg);
}

private static object ConvertToNullable(object valueToConvert,
Type resultType,
bool recursion,
Expand Down Expand Up @@ -3486,6 +3443,65 @@ private static PSConverter<bool> CreateNumericToBoolConverter(Type fromType)
return ConvertStringToEnum(sbResult.ToString(), resultType, recursion, originalValueToConvert, formatProvider, backupTable);
}

private class PSMethodToDelegateConverter
{
// Index of the matching overload method.
private readonly int _matchIndex;
// Size of the cache. It's rare to have more than 10 overloads for a method.
private const int CacheSize = 10;
private static readonly PSMethodToDelegateConverter[] s_converterCache = new PSMethodToDelegateConverter[CacheSize];

private PSMethodToDelegateConverter(int matchIndex)
{
_matchIndex = matchIndex;
}

internal static PSMethodToDelegateConverter GetConverter(int matchIndex)
{
if (matchIndex >= CacheSize) { return new PSMethodToDelegateConverter(matchIndex); }

var result = s_converterCache[matchIndex];
if (result == null)
{
// If the cache entry is null, generate a new instance for the cache slot.
var converter = new PSMethodToDelegateConverter(matchIndex);
Threading.Interlocked.CompareExchange(ref s_converterCache[matchIndex], converter, null);
Copy link
Collaborator

Choose a reason for hiding this comment

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

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".

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

result = s_converterCache[matchIndex];
}

return result;
}

internal Delegate Convert(object valueToConvert,
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Type resultType,
bool recursion,
PSObject originalValueToConvert,
IFormatProvider formatProvider,
TypeTable backupTable)
{
// We can only possibly convert PSMethod instance of the type PSMethod<T>.
// Such a PSMethod essentially represents a set of .NET method overloads.
var psMethod = (PSMethod)valueToConvert;

try
{
var methods = (MethodCacheEntry)psMethod.adapterData;
var isStatic = psMethod.instance is Type;

var candidate = (MethodInfo)methods.methodInformationStructures[_matchIndex].method;
return isStatic ? candidate.CreateDelegate(resultType)
: candidate.CreateDelegate(resultType, psMethod.instance);
}
catch (Exception e)
{
typeConversion.WriteLine("PSMethod to Delegate exception: \"{0}\".", e.Message);
throw new PSInvalidCastException("InvalidCastExceptionPSMethodToDelegate", e,
ExtendedTypeSystem.InvalidCastExceptionWithInnerException,
valueToConvert.ToString(), resultType.ToString(), e.Message);
}
}
}

private class ConvertViaParseMethod
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

{
// TODO - use an ETS wrapper that generates a dynamic method
Expand Down Expand Up @@ -4772,66 +4788,148 @@ internal static object ThrowInvalidConversionException(object valueToConvert, Ty
return CacheConversion<object>(fromType, toType, LanguagePrimitives.ConvertIntegerToEnum, ConversionRank.Language);
}

if (fromType.IsSubclassOf(typeof(PSMethod)) && toType.IsSubclassOf(typeof(Delegate)))
if (fromType.IsSubclassOf(typeof(PSMethod)) && toType.IsSubclassOf(typeof(Delegate)) && !toType.IsAbstract)
{
var mi = toType.GetMethod("Invoke");

var comparator = new DelegateArgsComparator(mi);
var targetMethod = toType.GetMethod("Invoke");
var comparator = new SignatureComparator(targetMethod);
var signatureEnumerator = new PSMethodSignatureEnumerator(fromType);
int index = -1, matchedIndex = -1;

while (signatureEnumerator.MoveNext())
{
var candidate = signatureEnumerator.Current.GetMethod("Invoke");
if (comparator.SignatureMatches(candidate.ReturnType, candidate.GetParameters()))
index++;
var signatureType = signatureEnumerator.Current;
// Skip the non-bindable signatures
if (signatureType == typeof(Func<PSNonBindableType>)) { continue; }

Type[] argumentTypes = signatureType.GenericTypeArguments;
if (comparator.ProjectedSignatureMatchesTarget(argumentTypes, out bool signaturesMatchExactly))
{
return CacheConversion<Delegate>(fromType, toType, LanguagePrimitives.ConvertPSMethodInfoToDelegate, ConversionRank.Language);
if (signaturesMatchExactly)
{
// We prefer the signature that exactly matches the target delegate.
matchedIndex = index;
break;
}

// If there is no exact match, then we use the first compatible signature we found.
if (matchedIndex == -1) { matchedIndex = index; }
}
}

if (matchedIndex > -1)
{
// We got the index of the matching method signature based on the PSMethod<..> type.
// Signatures in PSMethod<..> type were constructed based on the array of method overloads,
// in the exact order. So we can use this index directly to locate the matching overload in
// the converter, without having to compare the signature again.
var converter = PSMethodToDelegateConverter.GetConverter(matchedIndex);
return CacheConversion<Delegate>(fromType, toType, converter.Convert, ConversionRank.Language);
}
}

return null;
}

struct DelegateArgsComparator
private struct SignatureComparator
{
private readonly ParameterInfo[] _targetParametersInfos;
private readonly Type _returnType;

public DelegateArgsComparator(MethodInfo targetMethodInfo)
enum TypeMatchingContext
{
_returnType = targetMethodInfo.ReturnType;
_targetParametersInfos = targetMethodInfo.GetParameters();
ReturnType,
ParameterType,
OutParameterType
}

public bool SignatureMatches(Type returnType, ParameterInfo[] arguments)
private readonly ParameterInfo[] targetParameters;
private readonly Type targetReturnType;

internal SignatureComparator(MethodInfo targetMethodInfo)
{
return ReturnTypeMatches(returnType) && ParameterTypesMatches(arguments);
targetReturnType = targetMethodInfo.ReturnType;
targetParameters = targetMethodInfo.GetParameters();
}

private bool ReturnTypeMatches(Type returnType)
{
return PSMethod.MatchesPSMethodProjectedType(_returnType, returnType, testAssignment: true);
/// <summary>
/// Check if a projected signature matches the target method.
/// </summary>
/// <param name="argumentTypes">
/// The type arguments from the metadata type 'Func[..]' that represents the projected signature.
/// It contains the return type as the last item in the array.
/// </param>
/// <param name="signaturesMatchExactly">
/// Set by this method to indicate if it's an exact match.
/// </param>
internal bool ProjectedSignatureMatchesTarget(Type[] argumentTypes, out bool signaturesMatchExactly)
{
signaturesMatchExactly = false;
int length = argumentTypes.Length;
if (length != targetParameters.Length + 1) { return false; }
Copy link
Collaborator

Choose a reason for hiding this comment

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

Might be worth a comment explaining the + 1 here. Or doc-comments explaining the fields and the arguments to this method which explain (I'm guessing) that argumentTypes includes the return type and targetParameters does not.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done


bool typesMatchExactly, allTypesMatchExactly;
Type sourceReturnType = argumentTypes[length - 1];

if (ProjectedTypeMatchesTargetType(sourceReturnType, targetReturnType, TypeMatchingContext.ReturnType, out typesMatchExactly))
{
allTypesMatchExactly = typesMatchExactly;
for (int i = 0; i < targetParameters.Length; i++)
{
var targetParam = targetParameters[i];
var sourceType = argumentTypes[i];
var matchContext = targetParam.IsOut ? TypeMatchingContext.OutParameterType : TypeMatchingContext.ParameterType;

if (!ProjectedTypeMatchesTargetType(sourceType, targetParam.ParameterType, matchContext, out typesMatchExactly))
{
return false;
}
allTypesMatchExactly &= typesMatchExactly;
}

signaturesMatchExactly = allTypesMatchExactly;
return true;
}

return false;
}

private bool ParameterTypesMatches(ParameterInfo[] arguments)
private static bool ProjectedTypeMatchesTargetType(Type sourceType, Type targetType, TypeMatchingContext matchContext, out bool matchExactly)
{
var argsCount = _targetParametersInfos.Length;
// void is encoded as typeof(VOID) in the PSMethod<MethodGroup<>> as the last parameter
if (arguments.Length != argsCount)
matchExactly = false;
if (targetType.IsByRef || targetType.IsPointer)
{
if (!sourceType.IsGenericType) { return false; }

var sourceTypeDef = sourceType.GetGenericTypeDefinition();
bool isOutParameter = matchContext == TypeMatchingContext.OutParameterType;

if (targetType.IsByRef && sourceTypeDef == (isOutParameter ? typeof(PSOutParameter<>) : typeof(PSReference<>)) ||
targetType.IsPointer && sourceTypeDef == typeof(PSPointer<>))
{
// For ref/out parameter types and pointer types, the element types need to match exactly.
if (targetType.GetElementType() == sourceType.GenericTypeArguments[0])
{
matchExactly = true;
return true;
}
}
return false;
}
for (int i = 0; i < arguments.Length; i++)

if (targetType == sourceType ||
targetType == typeof(void) && sourceType == typeof(VOID) ||
targetType == typeof(TypedReference) && sourceType == typeof(PSTypedReference))
{
var arg = arguments[i];
var argType = arg.ParameterType;
var targetParamType = _targetParametersInfos[i].ParameterType;
var isOut = (arg.Attributes | ParameterAttributes.Out) == ParameterAttributes.Out;
if (!PSMethod.MatchesPSMethodProjectedType(targetParamType, argType, isOut: isOut))
{
return false;
}
matchExactly = true;
return true;
}
return true;

if (targetType == typeof(void) || targetType == typeof(TypedReference))
{
return false;
}

return matchContext == TypeMatchingContext.ReturnType
? targetType.IsAssignableFrom(sourceType)
: sourceType.IsAssignableFrom(targetType);
}
}

Expand Down