Skip to content

Commit

Permalink
Address PR feedback and revert a downlevel change
Browse files Browse the repository at this point in the history
  • Loading branch information
stephentoub committed Jul 21, 2024
1 parent ee0a277 commit 2c35bb6
Show file tree
Hide file tree
Showing 4 changed files with 59 additions and 53 deletions.
83 changes: 43 additions & 40 deletions src/coreclr/System.Private.CoreLib/src/System/Delegate.CoreCLR.cs
Original file line number Diff line number Diff line change
Expand Up @@ -156,56 +156,59 @@ public override int GetHashCode()

protected virtual MethodInfo GetMethodImpl()
{
if (_methodBase is null or not MethodInfo)
if (_methodBase is MethodInfo methodInfo)
{
IRuntimeMethodInfo method = FindMethodHandle();
RuntimeType? declaringType = RuntimeMethodHandle.GetDeclaringType(method);
// need a proper declaring type instance method on a generic type
if (declaringType.IsGenericType)
return methodInfo;
}

IRuntimeMethodInfo method = FindMethodHandle();
RuntimeType? declaringType = RuntimeMethodHandle.GetDeclaringType(method);
// need a proper declaring type instance method on a generic type
if (declaringType.IsGenericType)
{
bool isStatic = (RuntimeMethodHandle.GetAttributes(method) & MethodAttributes.Static) != (MethodAttributes)0;
if (!isStatic)
{
bool isStatic = (RuntimeMethodHandle.GetAttributes(method) & MethodAttributes.Static) != (MethodAttributes)0;
if (!isStatic)
if (_methodPtrAux == IntPtr.Zero)
{
if (_methodPtrAux == IntPtr.Zero)
// The target may be of a derived type that doesn't have visibility onto the
// target method. We don't want to call RuntimeType.GetMethodBase below with that
// or reflection can end up generating a MethodInfo where the ReflectedType cannot
// see the MethodInfo itself and that breaks an important invariant. But the
// target type could include important generic type information we need in order
// to work out what the exact instantiation of the method's declaring type is. So
// we'll walk up the inheritance chain (which will yield exactly instantiated
// types at each step) until we find the declaring type. Since the declaring type
// we get from the method is probably shared and those in the hierarchy we're
// walking won't be we compare using the generic type definition forms instead.
Type? currentType = _target!.GetType();
Type targetType = declaringType.GetGenericTypeDefinition();
while (currentType != null)
{
// The target may be of a derived type that doesn't have visibility onto the
// target method. We don't want to call RuntimeType.GetMethodBase below with that
// or reflection can end up generating a MethodInfo where the ReflectedType cannot
// see the MethodInfo itself and that breaks an important invariant. But the
// target type could include important generic type information we need in order
// to work out what the exact instantiation of the method's declaring type is. So
// we'll walk up the inheritance chain (which will yield exactly instantiated
// types at each step) until we find the declaring type. Since the declaring type
// we get from the method is probably shared and those in the hierarchy we're
// walking won't be we compare using the generic type definition forms instead.
Type? currentType = _target!.GetType();
Type targetType = declaringType.GetGenericTypeDefinition();
while (currentType != null)
if (currentType.IsGenericType &&
currentType.GetGenericTypeDefinition() == targetType)
{
if (currentType.IsGenericType &&
currentType.GetGenericTypeDefinition() == targetType)
{
declaringType = currentType as RuntimeType;
break;
}
currentType = currentType.BaseType;
declaringType = currentType as RuntimeType;
break;
}

// RCWs don't need to be "strongly-typed" in which case we don't find a base type
// that matches the declaring type of the method. This is fine because interop needs
// to work with exact methods anyway so declaringType is never shared at this point.
Debug.Assert(currentType != null || _target.GetType().IsCOMObject, "The class hierarchy should declare the method");
}
else
{
// it's an open one, need to fetch the first arg of the instantiation
MethodInfo invoke = this.GetType().GetMethod("Invoke")!;
declaringType = (RuntimeType)invoke.GetParametersAsSpan()[0].ParameterType;
currentType = currentType.BaseType;
}

// RCWs don't need to be "strongly-typed" in which case we don't find a base type
// that matches the declaring type of the method. This is fine because interop needs
// to work with exact methods anyway so declaringType is never shared at this point.
Debug.Assert(currentType != null || _target.GetType().IsCOMObject, "The class hierarchy should declare the method");
}
else
{
// it's an open one, need to fetch the first arg of the instantiation
MethodInfo invoke = this.GetType().GetMethod("Invoke")!;
declaringType = (RuntimeType)invoke.GetParametersAsSpan()[0].ParameterType;
}
}
_methodBase = (MethodInfo)RuntimeType.GetMethodBase(declaringType, method)!;
}

_methodBase = (MethodInfo)RuntimeType.GetMethodBase(declaringType, method)!;
return (MethodInfo)_methodBase;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -515,20 +515,23 @@ protected override MethodInfo GetMethodImpl()
{
// we handle unmanaged function pointers here because the generic ones (used for WinRT) would otherwise
// be treated as open delegates by the base implementation, resulting in failure to get the MethodInfo
if (_methodBase is null or not MethodInfo)
if (_methodBase is MethodInfo methodInfo)
{
IRuntimeMethodInfo method = FindMethodHandle();
RuntimeType declaringType = RuntimeMethodHandle.GetDeclaringType(method);
return methodInfo;
}

// need a proper declaring type instance method on a generic type
if (declaringType.IsGenericType)
{
// we are returning the 'Invoke' method of this delegate so use this.GetType() for the exact type
RuntimeType reflectedType = (RuntimeType)GetType();
declaringType = reflectedType;
}
_methodBase = (MethodInfo)RuntimeType.GetMethodBase(declaringType, method)!;
IRuntimeMethodInfo method = FindMethodHandle();
RuntimeType declaringType = RuntimeMethodHandle.GetDeclaringType(method);

// need a proper declaring type instance method on a generic type
if (declaringType.IsGenericType)
{
// we are returning the 'Invoke' method of this delegate so use this.GetType() for the exact type
RuntimeType reflectedType = (RuntimeType)GetType();
declaringType = reflectedType;
}

_methodBase = (MethodInfo)RuntimeType.GetMethodBase(declaringType, method)!;
return (MethodInfo)_methodBase;
}

Expand Down
2 changes: 1 addition & 1 deletion src/libraries/Common/src/System/IO/PathInternal.Windows.cs
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ internal static bool EndsWithPeriodOrSpace(string? path)
if (string.IsNullOrEmpty(path))
return false;

char c = path[^1];
char c = path[path.Length - 1];
return c == ' ' || c == '.';
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ internal static string GetGenericTypeFullName(ReadOnlySpan<char> fullTypeName, R
result.Append(']');
result.Append(',');
}
result[^1] = ']'; // replace ',' with ']'
result[result.Length - 1] = ']'; // replace ',' with ']'

return result.ToString();
}
Expand Down

0 comments on commit 2c35bb6

Please sign in to comment.