-
Notifications
You must be signed in to change notification settings - Fork 933
Fix attempt of static proxies to call base method for abstract classes #1884
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.
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
Changes from all commits
73cc71c
20fbd03
118a61f
67a9ffc
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,7 @@ | ||
using System; | ||
using System.Collections.Generic; | ||
using System.Linq; | ||
using System.Linq.Expressions; | ||
using System.Reflection; | ||
using System.Reflection.Emit; | ||
using System.Runtime.Serialization; | ||
|
@@ -366,19 +367,20 @@ private static void ImplementCallMethodOnImplementation( | |
} | ||
|
||
private static void EmitCallBaseIfLazyInitializerIsNull( | ||
ILGenerator IL, MethodInfo method, FieldInfo lazyInitializerField, System.Type parentType) | ||
ILGenerator IL, | ||
MethodInfo method, | ||
FieldInfo lazyInitializerField, | ||
System.Type parentType) | ||
{ | ||
/* | ||
<if (method.DeclaringType.IsAssignableFrom(parentType)) | ||
{> | ||
if (this.__lazyInitializer == null) | ||
return base.<method>(args..) | ||
<if (method.IsAbstract) | ||
{> | ||
return default; | ||
<} else {> | ||
return base.<method>(args..); | ||
<}> | ||
*/ | ||
if (!method.DeclaringType.IsAssignableFrom(parentType)) | ||
// The proxy does not derive from a type implementing the method, do not attempt | ||
// calling its base. In such case, the lazy initializer is never null. | ||
return; | ||
|
||
// When deriving from the entity class, the entity class constructor may trigger | ||
// virtual calls accessing the proxy state before its own constructor has a chance | ||
|
@@ -393,9 +395,76 @@ private static void EmitCallBaseIfLazyInitializerIsNull( | |
IL.Emit(OpCodes.Ldnull); | ||
IL.Emit(OpCodes.Bne_Un, skipBaseCall); | ||
|
||
IL.Emit(OpCodes.Ldarg_0); | ||
EmitCallMethod(IL, OpCodes.Call, method); | ||
IL.Emit(OpCodes.Ret); | ||
if (method.DeclaringType.IsInterface && | ||
method.DeclaringType.IsAssignableFrom(parentType)) | ||
{ | ||
var interfaceMap = parentType.GetInterfaceMap(method.DeclaringType); | ||
var methodIndex = Array.IndexOf(interfaceMap.InterfaceMethods, method); | ||
method = interfaceMap.TargetMethods[methodIndex]; | ||
} | ||
|
||
if (method.IsAbstract) | ||
{ | ||
fredericDelaporte marked this conversation as resolved.
Show resolved
Hide resolved
|
||
/* | ||
* return default(<ReturnType>); | ||
*/ | ||
|
||
if (!method.ReturnType.IsValueType) | ||
{ | ||
IL.Emit(OpCodes.Ldnull); | ||
This comment was marked as resolved.
Sorry, something went wrong. |
||
} | ||
else if (method.ReturnType != typeof(void)) | ||
{ | ||
var local = IL.DeclareLocal(method.ReturnType); | ||
IL.Emit(OpCodes.Ldloca, local); | ||
IL.Emit(OpCodes.Initobj, method.ReturnType); | ||
IL.Emit(OpCodes.Ldloc, local); | ||
} | ||
|
||
IL.Emit(OpCodes.Ret); | ||
} | ||
else if (method.IsPrivate) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is an explicit interface method. Instead we can emit no-op (same as for the abstract method). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was not considering to do the effort of calling a base explicit implementation, since that is already not easily doable in C# code, and previous releases were not supporting it anyway. So no-op would have suited me. But now that it is done and tested, why not keeping it. It is still better. |
||
{ | ||
/* | ||
* var mi = (MethodInfo)MethodBase.GetMethodFromHandle(<method>, <parentType>); | ||
* var delegate = (<delegateType>)mi.CreateDelegate(typeof(<delegateType>), this); | ||
* delegate.Invoke(args...); | ||
*/ | ||
|
||
var parameters = method.GetParameters(); | ||
|
||
var delegateType = Expression.GetDelegateType( | ||
parameters.Select(p => p.ParameterType).Concat(new[] {method.ReturnType}).ToArray()); | ||
|
||
var invokeDelegate = delegateType.GetMethod("Invoke"); | ||
|
||
IL.Emit(OpCodes.Ldtoken, method); | ||
IL.Emit(OpCodes.Ldtoken, parentType); | ||
IL.Emit(OpCodes.Call, ReflectionCache.MethodBaseMethods.GetMethodFromHandleWithDeclaringType); | ||
IL.Emit(OpCodes.Castclass, typeof(MethodInfo)); | ||
IL.Emit(OpCodes.Ldtoken, delegateType); | ||
IL.Emit(OpCodes.Call, ReflectionCache.TypeMethods.GetTypeFromHandle); | ||
IL.Emit(OpCodes.Ldarg_0); | ||
IL.Emit( | ||
OpCodes.Callvirt, | ||
typeof(MethodInfo).GetMethod( | ||
nameof(MethodInfo.CreateDelegate), | ||
new[] {typeof(System.Type), typeof(object)})); | ||
IL.Emit(OpCodes.Castclass, delegateType); | ||
|
||
EmitCallMethod(IL, OpCodes.Callvirt, invokeDelegate); | ||
IL.Emit(OpCodes.Ret); | ||
} | ||
else | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. With the current tests this branch is not needed at all. If the I'm considering to make a breaking change and throw exception if An alternative to an exception would be to set |
||
{ | ||
/* | ||
* base.<method>(args...); | ||
*/ | ||
|
||
IL.Emit(OpCodes.Ldarg_0); | ||
EmitCallMethod(IL, OpCodes.Call, method); | ||
IL.Emit(OpCodes.Ret); | ||
} | ||
|
||
IL.MarkLabel(skipBaseCall); | ||
} | ||
|
Uh oh!
There was an error while loading. Please reload this page.