-
Notifications
You must be signed in to change notification settings - Fork 934
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
Fix attempt of static proxies to call base method for abstract classes #1884
Conversation
@fredericDelaporte what if we move |
This comment has been minimized.
This comment has been minimized.
{ | ||
if (!method.ReturnType.IsValueType) | ||
{ | ||
IL.Emit(OpCodes.Ldnull); |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There is no such a requirement. In fact this is how field initializers are done. First class initializes its fields, then it calls a base constructor: For this C#: public class A { }
public class B { int X = -1; public B() { X = 2; } } Following IL isgenerated: .class public auto ansi beforefieldinit A
extends [mscorlib]System.Object
{
// Methods
.method public hidebysig specialname rtspecialname
instance void .ctor () cil managed
{
// Method begins at RVA 0x2050
// Code size 8 (0x8)
.maxstack 8
IL_0000: ldarg.0
IL_0001: call instance void [mscorlib]System.Object::.ctor()
IL_0006: nop
IL_0007: ret
} // end of method A::.ctor
} // end of class A
.class public auto ansi beforefieldinit B
extends [mscorlib]System.Object
{
// Fields
.field private int32 X
// Methods
.method public hidebysig specialname rtspecialname
instance void .ctor () cil managed
{
// Method begins at RVA 0x2059
// Code size 23 (0x17)
.maxstack 8
IL_0000: ldarg.0
IL_0001: ldc.i4.m1
IL_0002: stfld int32 B::X
IL_0007: ldarg.0
IL_0008: call instance void [mscorlib]System.Object::.ctor()
IL_000d: nop
IL_000e: nop
IL_000f: ldarg.0
IL_0010: ldc.i4.2
IL_0011: stfld int32 B::X
IL_0016: ret
} // end of method B::.ctor
} // end of class B
This is exactly what I do here. IL_000A-IL_000D just stores the value on a stack to another variable and do short jump to an instruction following the jump, which is completely unnecessary. sharplab shows following: IL_0000: ldloca.s 0
IL_0002: initobj !!T
IL_0008: ldloc.0
IL_0009: ret |
Only thing in the PR which is actual are the flags. The code we generate is valid (except the one fixed by this PR). However, even if we fix the flags and make a program "valid" it will still have the issues you have demonstrated before: the code will behave correctly only is the a variable holding the proxy is declared as an interface. And, there could be some potential issues with hidden traps. |
Ok, so it should be possible to initialize the lazy initializer before calling the base constructor. But then it will be a sizeable possible breaking change: this will cause proxies of classes initializing some of their properties in their constructor to force their initialization on construction, defeating the proxy purpose. Of course, we can consider it as to be expected, since the class access its state during construction. Having it still lazy and actually ignoring those calls may defeat the constructor intended behavior. But for classes doing some initialization useful for new entities but unneeded for loaded ones, like initializing collections, this would cause them to be no more actually lazy. |
I am no more convinced of this, because it looks to me what we actually generate for proxies with #1881 is indeed more like this: not only it will add an explicit implementation, but also an implicit one. That is somewhat duplicated code, but for generated code, should we care? And in such case, there are no issues. We just need to add a test for ascertaining it. |
I've thought about this after I wrote my comment. Yes, it will trigger the initialization if a property is accessed in the constructor, and we do not want this (as per what you've just said). It's fairly common to have following code in the entities: public class A
{
ICollection<B> _children = new List<B>();
public virtual IEnumerable<B> Children => _children;
} Or public class A
{
public virtual IEnumerable<B> Children { get; private set; } = new List<B>();
} These two examples would behave as intended, however the following code would not: public class A
{
public virtual IEnumerable<B> Children { get; private set; }
public A()
{
Children = new List<B>();
}
} But the later code is having a compilation warning "Access of virtual members in a constructor". |
I have implemented this on intention. Exactly because of the case you're describing in "Now I consider pursuing..." It is just easier to implement two methods explicit and implicit in case of interfaces than pick and choose what one kind method to implement. The case with ILazyCollectionInitializer will still generate semi-valid code which will behave correctly only when the instance is declared as the interface (it cannot be declared as proxy per se). |
Still thinking about all that, it seems to me the static proxy lacks tests for #1389, |
I have added some tests for the explicit interface case, since the change here is also enough to have it no more failing validation. I have also added checks for constructor calls in other cases, since this PR is about having those calls not failing (in the abstract base class case, but better check the more usual case too). I do not think I will add tests for |
Actually, I've applied your last commit onto master and it is even worse than NullReferenceException. Explicit interface fails with |
@@ -114,7 +158,7 @@ public void VerifyProxyForClassWithInternalInterface() | |||
} | |||
|
|||
[Test] | |||
public void VerifyProxyForClassWithAdditionalInterface() | |||
public void VerifyProxyOnInterface() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did you mixed up names? The name was correct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not really. This case tests a proxy: IPublic
, while the other case tests proxy: TestClass
, with TestClass
being also TestClass: IPublic
.
The names were not allowing to do the distinction. ProxyOnInterface
seems to me well more explicit about the fact we test a proxy based on an interface, not a proxy based on a class which also happens to implement some interface.
This comment has been minimized.
This comment has been minimized.
@@ -158,7 +158,7 @@ public void VerifyProxyForClassWithInternalInterface() | |||
} | |||
|
|||
[Test] | |||
public void VerifyProxyOnInterface() | |||
public void VerifyProxyForClassWithAdditionalInterface() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test generates a proxy class solely based on the interfaces. It is not actually a proxy of the class. Going to that name is going back to the confusion. Even worse combined with the other renaming.
We now have:
VerifyProxyForClassWithInterface
: does test aclass proxy: PublicInterfaceTestClass, INHibernateProxy, IPublic
.VerifyProxyForClassWithAdditionalInterface
: does actually test aclass proxy: INHibernateProxy, IPublic
, without any direct relationship toPublicInterfaceTestClass
.Additional
is far from telling this.
The fact that obtaining this requires to add the interface in the interface list of the proxy factory is a (bad) implementation detail. It seems way more important to me to describe the kind of proxy we test, rather than the way we obtain such proxy.
This comment has been minimized.
This comment has been minimized.
3860bd7
to
c1ae5d4
Compare
Previously it was failing PEVerify similarly to nhibernate#1728 but for abstract classes (this is possible in case of a polymorphic entities). Unlike interfaces the abstract base class can go into a situation when lazy initializer is not yet available, eg. code in a constructor.
And check access into constructor, and check proxy on type with interfaces. To be squashed
c1ae5d4
to
20fbd03
Compare
This comment has been minimized.
This comment has been minimized.
|
||
IL.Emit(OpCodes.Ret); | ||
} | ||
else |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The 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 if
condition replace with if(true)
all tests still pass.
I'm considering to make a breaking change and throw exception if __lazyInitializer
is null. Because the code in the constructor will not execute properly in any way.
An alternative to an exception would be to set __lazyInitializer
before calling base constructor. This way the code will execute properly, but the proxy would be initialized.
I finally understood what you have meant, @fredericDelaporte and how the comment differs to the committed tests. What you describe in a comment as a "weird" class in fact could be fairly common code. However, ReSharper complains on such a code with "virtual call in constructor" warning. Because public class A
{
public virtual int X { get; set; }
public A()
{
X = -1;
}
}
var a = new AProxy();
// a.HibernateLazyInitializer.SetImplementation(new A());
Assert.That(a.X, Is.Equal(-1)); This will pass, because the implementation will have a real constructor. So far, we have following options for a solution:
I would prefer either 1 or 5. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
For me, neither 1 or 5 are good. You are absolutely right when you write
Yes, but should we really care? I think no. We are building a proxy for a case who normally does not care at all of the initial state of the instance, because this state is to be replaced later by another one, on initialization. So I think we should just focus on getting the proxy successfully instantiated, without caring much about its base state, which the proxy does not need to care of. So what does currently this PR, or eventually option 3, seems to me as the way to go. Option 1 or 5 would be breaking changes without any benefit. It would just restrict what the user can do, without bringing him any benefit in my opinion. What he does in the constructor is still done, and done properly, within the delegated state. The fact it is not done in the base state is in most cases irrelevant, since the proxy behavior depends on the delegated state, not on the base one. I think that what does currently this PR is still better than option 3, because with option 3 the possible breaking change I have just edited out should be brought back in. |
With option 3 user can see a NullReferenceException if they try access to the state. |
Yes, that is why I consider what does currently this PR is still better. |
My stance on the subject is valid for the lazy entity proxy case. But for the lazy property case, that is a bit less well. There is no delegated state for this other kind of proxy, it does care about its base state. It is not ideal. In some special cases, especially for non-mapped properties explicitly implementing an interface, this could result in hard to diagnose behavior for the user. Throwing an explicit exception there would be better for those cases. But it would be worst for other cases, in my opinion more usual, where the user only cares for what does the constructor for new entities, not for loaded ones. So I think keeping this PR current state is the right, pragmatic decision to take. Maybe just rename |
|
||
IL.Emit(OpCodes.Ret); | ||
} | ||
else if (method.IsPrivate) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The 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 comment
The 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.
|
||
IL.Emit(OpCodes.Ret); | ||
} | ||
else if (method.IsPrivate) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The 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.
Previously it was failing PEVerify similarly to #1728 but for abstract classes (this is possible in case of a polymorphic entities). Unlike interfaces the abstract base class can go into a situation when lazy initializer is not yet available, eg. code in a constructor.