nhibernate-core icon indicating copy to clipboard operation
nhibernate-core copied to clipboard

MemberwiseClone should be virtual in dotnet 9 preview 3

Open crodriguesbr opened this issue 9 months ago • 3 comments

I have begun testing my system with dotnet 9 Preview 3 and received a message that the method MemberwiseClone should be virtual. I have just changed the version from 8 to 9 and turned to receive the error when the system started. I have looked for the changes between DotNet 8 and 9 and found a different signature from that method, MemberwiseClone. It has changed to protected internal and turned to be considered as a virtual declaration.

In dotnet 8 (release/8.0) - Object.CoreCLR.cs

// Returns a new object instance that is a memberwise copy of this
// object.  This is always a shallow copy of the instance. The method is protected
// so that other object may only call this method on themselves.  It is intended to
// support the ICloneable interface.
[Intrinsic]
protected unsafe object MemberwiseClone()
{
    object clone = RuntimeHelpers.AllocateUninitializedClone(this);

    // copy contents of "this" to the clone

    nuint byteCount = RuntimeHelpers.GetRawObjectDataSize(clone);
    ref byte src = ref this.GetRawData();
    ref byte dst = ref clone.GetRawData();

    if (RuntimeHelpers.GetMethodTable(clone)->ContainsGCPointers)
        Buffer.BulkMoveWithWriteBarrier(ref dst, ref src, byteCount);
    else
        Buffer.Memmove(ref dst, ref src, byteCount);

    return clone;
}

In dotnet 9 preview 3 (release/9.0-preview3) - Object.CoreCLR.cs

// Returns a new object instance that is a memberwise copy of this
// object.  This is always a shallow copy of the instance. The method is protected
// so that other object may only call this method on themselves.  It is intended to
// support the ICloneable interface.
[Intrinsic]
protected internal unsafe object MemberwiseClone()
{
    object clone = this;
    RuntimeHelpers.AllocateUninitializedClone(ObjectHandleOnStack.Create(ref clone));
    Debug.Assert(clone != this);

    // copy contents of "this" to the clone

    nuint byteCount = RuntimeHelpers.GetRawObjectDataSize(clone);
    ref byte src = ref this.GetRawData();
    ref byte dst = ref clone.GetRawData();

    if (RuntimeHelpers.GetMethodTable(clone)->ContainsGCPointers)
        Buffer.BulkMoveWithWriteBarrier(ref dst, ref src, byteCount);
    else
        SpanHelpers.Memmove(ref dst, ref src, byteCount);

    return clone;
}

I have changed the NHibernate code to consider the MemberwiseClone in a method ShouldBeProxiable at class DefaultDynamicProxyMethodCheckerExtensions. It works, but I wonder if it is the best approach.

public static bool ShouldBeProxiable(this MethodInfo method)
{
	// to use only for real methods (no getter/setter)
	return (method.DeclaringType != typeof (MarshalByRefObject)) &&
	       !IsFinalizeMethod(method) &&
	       (!(method.DeclaringType == typeof (object) && "MemberwiseClone".Equals(method.Name))) &&
		   (!(method.DeclaringType == typeof(object) && "GetType".Equals(method.Name))) &&
		   (!(method.DeclaringType == typeof (object) && "obj_address".Equals(method.Name))) && // Mono-specific method
	       !IsDisposeMethod(method) &&
				 (method.IsPublic || method.IsAssembly || method.IsFamilyOrAssembly);
}

I know DotNet 9 is in the preview stage yet, but I need to test my system using the latest version of DotNet. Another way would be to define the validation Proxy as false, but I wouldn't like to do that. If my approach is correct, I could send a pull request. If someone has another idea, please let me know.

crodriguesbr avatar Apr 29 '24 18:04 crodriguesbr

It seems to me as the way to go, but I have to check why is it on the dynamic proxy. The default proxy factory is the static one, and the dynamic one is a legacy.

fredericDelaporte avatar Apr 30 '24 20:04 fredericDelaporte

I think we should consider any methods from System.Object as not required to be proxiable.

UPD: rewording for clarity

hazzik avatar May 01 '24 00:05 hazzik

I have tested the version, and it works fine. If it's not too radical, remove all methods from the System.Object, I think it prevents future problems.

crodriguesbr avatar May 02 '24 19:05 crodriguesbr