From b6ae0cb29670a7974386f867edd26773d64c4656 Mon Sep 17 00:00:00 2001 From: David Bond Date: Sun, 4 Oct 2026 13:50:36 +0100 Subject: [PATCH 1/3] refactor: cut complexity in ClonerToExprGenerator and clear remaining Codacy findings - ClonerToExprGenerator: split GenerateProcessMethod, the 1D/2D/N-D array cloners and the index odometer into small methods; add braces everywhere; drop the dead assignments and commented-out code. Behaviour is unchanged (same results as the original on every test, including the new struct-array case). - ShallowClonerGenerator: braces - Tests: scoped suppressions with reasons for deliberate public fields (S1104, S2357) and a deliberately throwing Equals (S3877); add a 2D struct-array CopyTo test - .codacy.yml: exclude the vendored Imported/ FastDeepCloner comparison code - CLAUDE.md: split the compound boundaries bullet Co-Authored-By: Claude Sonnet 5.5 --- .codacy.yml | 5 + CLAUDE.md | 5 +- DeepCloner.Tests/ArraysSpec.cs | 3 +- DeepCloner.Tests/ConstructorsSpec.cs | 3 +- DeepCloner.Tests/CopyToObjectSpec.cs | 28 ++ DeepCloner.Tests/InheritanceSpec.cs | 3 +- DeepCloner.Tests/Objects/DoableStruct1.cs | 3 +- DeepCloner.Tests/SimpleObjectSpec.cs | 3 +- DeepCloner/Helpers/ClonerToExprGenerator.cs | 342 ++++++++++++------- DeepCloner/Helpers/ShallowClonerGenerator.cs | 4 + 10 files changed, 265 insertions(+), 134 deletions(-) create mode 100644 .codacy.yml diff --git a/.codacy.yml b/.codacy.yml new file mode 100644 index 0000000..3adcbb1 --- /dev/null +++ b/.codacy.yml @@ -0,0 +1,5 @@ +--- +exclude_paths: + # Vendored third-party comparison code (the FastDeepCloner library), kept verbatim so the tests + # can compare behaviour against it. It is not ours to refactor. + - "DeepCloner.Tests/Imported/**" diff --git a/CLAUDE.md b/CLAUDE.md index 8d247da..d8793c4 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -16,8 +16,9 @@ licensing and community files are governed by the open source PanoramicData.Nuge ## Scope and boundaries -- Do not weaken `TreatWarningsAsErrors`, delete or skip tests to make a build pass, or bypass - CI/CD checks. +- Do not weaken `TreatWarningsAsErrors`. +- Do not delete or skip tests to make a build pass. +- Do not bypass CI/CD checks. - Do not commit secrets, credentials, or API tokens. - Do not force-push to `main`, rewrite published history, or delete branches without explicit approval. diff --git a/DeepCloner.Tests/ArraysSpec.cs b/DeepCloner.Tests/ArraysSpec.cs index 16abd2f..d8658b0 100644 --- a/DeepCloner.Tests/ArraysSpec.cs +++ b/DeepCloner.Tests/ArraysSpec.cs @@ -1,4 +1,5 @@ -#nullable disable +#pragma warning disable S1104 // Public fields are deliberate: these fixtures exist to test field cloning +#nullable disable using Xunit; using PanoramicData.DeepCloner; diff --git a/DeepCloner.Tests/ConstructorsSpec.cs b/DeepCloner.Tests/ConstructorsSpec.cs index 2bfdd81..821b013 100644 --- a/DeepCloner.Tests/ConstructorsSpec.cs +++ b/DeepCloner.Tests/ConstructorsSpec.cs @@ -1,4 +1,5 @@ -#nullable disable +#pragma warning disable S3877 // Equals throws deliberately: the test checks that cloning never calls it +#nullable disable using Xunit; using PanoramicData.DeepCloner; diff --git a/DeepCloner.Tests/CopyToObjectSpec.cs b/DeepCloner.Tests/CopyToObjectSpec.cs index afeb512..bc49652 100644 --- a/DeepCloner.Tests/CopyToObjectSpec.cs +++ b/DeepCloner.Tests/CopyToObjectSpec.cs @@ -369,6 +369,34 @@ public void MultiDim_Array_Should_Be_Cloned(bool isDeep) Assert.Equal(1, arrTo.GetValue(0, 0)); } + public readonly record struct StructWithReference(int[] Items); + + [Theory] + [InlineData(false)] + [InlineData(true)] + public void TwoDim_Array_Of_Structs_With_References_Should_Be_Cloned(bool isDeep) + { + var arrFrom = new[,] + { + { new StructWithReference([1]), new StructWithReference([2]) }, + { new StructWithReference([3]), new StructWithReference([4]) } + }; + var arrTo = new StructWithReference[2, 2]; + + if (isDeep) + { + arrFrom.DeepCloneTo(arrTo); + } + else + { + arrFrom.ShallowCloneTo(arrTo); + } + + Assert.Equal([4], arrTo[1, 1].Items); + Assert.Equal([1], arrTo[0, 0].Items); + Assert.Equal([3], arrTo[1, 0].Items); + } + [Theory] [InlineData(false)] [InlineData(true)] diff --git a/DeepCloner.Tests/InheritanceSpec.cs b/DeepCloner.Tests/InheritanceSpec.cs index ccddca2..a3fa17b 100644 --- a/DeepCloner.Tests/InheritanceSpec.cs +++ b/DeepCloner.Tests/InheritanceSpec.cs @@ -1,4 +1,5 @@ -#nullable disable +#pragma warning disable S1104, S2357 // Public fields are deliberate: these fixtures exist to test field cloning +#nullable disable using Xunit; using PanoramicData.DeepCloner; diff --git a/DeepCloner.Tests/Objects/DoableStruct1.cs b/DeepCloner.Tests/Objects/DoableStruct1.cs index 924a5a8..b5d96ee 100644 --- a/DeepCloner.Tests/Objects/DoableStruct1.cs +++ b/DeepCloner.Tests/Objects/DoableStruct1.cs @@ -1,4 +1,5 @@ -namespace PanoramicData.DeepCloner.Test.Objects; +#pragma warning disable S1104 // Public fields are deliberate: these fixtures exist to test field cloning +namespace PanoramicData.DeepCloner.Test.Objects; public struct DoableStruct1 : IDoable, IEquatable { diff --git a/DeepCloner.Tests/SimpleObjectSpec.cs b/DeepCloner.Tests/SimpleObjectSpec.cs index 3e96b67..33c1fe9 100644 --- a/DeepCloner.Tests/SimpleObjectSpec.cs +++ b/DeepCloner.Tests/SimpleObjectSpec.cs @@ -1,4 +1,5 @@ -#nullable disable +#pragma warning disable S1104, S2357 // Public fields are deliberate: these fixtures exist to test field cloning +#nullable disable using Xunit; using PanoramicData.DeepCloner.Test.Objects; diff --git a/DeepCloner/Helpers/ClonerToExprGenerator.cs b/DeepCloner/Helpers/ClonerToExprGenerator.cs index 696e984..c9a3449 100644 --- a/DeepCloner/Helpers/ClonerToExprGenerator.cs +++ b/DeepCloner/Helpers/ClonerToExprGenerator.cs @@ -13,7 +13,10 @@ internal static class ClonerToExprGenerator internal static object GenerateClonerInternal(Type realType, bool isDeepClone) { if (realType.IsValueType()) + { throw new InvalidOperationException("Operation is valid only for reference types"); + } + return GenerateProcessMethod(realType, isDeepClone); } @@ -28,91 +31,100 @@ private static object GenerateProcessMethod(Type type, bool isDeepClone) var expressionList = new List(); - ParameterExpression from = Expression.Parameter(methodType); - var fromLocal = from; + var from = Expression.Parameter(methodType); var to = Expression.Parameter(methodType); - var toLocal = to; var state = Expression.Parameter(typeof(DeepCloneState)); - // if (!type.IsValueType()) + var fromLocal = Expression.Variable(type); + var toLocal = Expression.Variable(type); + + // fromLocal = (T)from + expressionList.Add(Expression.Assign(fromLocal, Expression.Convert(from, type))); + expressionList.Add(Expression.Assign(toLocal, Expression.Convert(to, type))); + + if (isDeepClone) { - fromLocal = Expression.Variable(type); - toLocal = Expression.Variable(type); - // fromLocal = (T)from - expressionList.Add(Expression.Assign(fromLocal, Expression.Convert(from, type))); - expressionList.Add(Expression.Assign(toLocal, Expression.Convert(to, type))); + // added from -> to binding to ensure reference loop handling + // structs cannot loop here + // state.AddKnownRef(from, to) + expressionList.Add(Expression.Call(state, typeof(DeepCloneState).GetMethod("AddKnownRef"), from, to)); + } - if (isDeepClone) - { - // added from -> to binding to ensure reference loop handling - // structs cannot loop here - // state.AddKnownRef(from, to) - expressionList.Add(Expression.Call(state, typeof(DeepCloneState).GetMethod("AddKnownRef"), from, to)); - } + foreach (var fieldInfo in GetAllFields(type)) + { + expressionList.Add(GenerateFieldCopy(fieldInfo, fromLocal, toLocal, state, isDeepClone)); } - List fi = new List(); + expressionList.Add(Expression.Convert(toLocal, methodType)); + + var funcType = typeof(Func<,,,>).MakeGenericType(methodType, methodType, typeof(DeepCloneState), methodType); + + return Expression.Lambda(funcType, Expression.Block(new[] { fromLocal, toLocal }, expressionList), from, to, state).Compile(); + } + + private static List GetAllFields(Type type) + { + var fields = new List(); var tp = type; do { #if !NETCORE // don't do anything with this dark magic! - if (tp == typeof(ContextBoundObject)) break; + if (tp == typeof(ContextBoundObject)) + { + break; + } #else - if (tp.Name == "ContextBoundObject") break; + if (tp.Name == "ContextBoundObject") + { + break; + } #endif - fi.AddRange(tp.GetDeclaredFields()); + fields.AddRange(tp.GetDeclaredFields()); tp = tp.BaseType(); } while (tp != null); - foreach (var fieldInfo in fi) + return fields; + } + + private static Expression GenerateFieldCopy( + FieldInfo fieldInfo, + ParameterExpression fromLocal, + ParameterExpression toLocal, + ParameterExpression state, + bool isDeepClone) + { + if (!isDeepClone || DeepClonerSafeTypes.CanReturnSameObject(fieldInfo.FieldType)) { - if (isDeepClone && !DeepClonerSafeTypes.CanReturnSameObject(fieldInfo.FieldType)) - { - var methodInfo = fieldInfo.FieldType.IsValueType() - ? typeof(DeepClonerGenerator).GetPrivateStaticMethod("CloneStructInternal") - .MakeGenericMethod(fieldInfo.FieldType) - : typeof(DeepClonerGenerator).GetPrivateStaticMethod("CloneClassInternal"); - - var get = Expression.Field(fromLocal, fieldInfo); - - // toLocal.Field = Clone...Internal(fromLocal.Field) - var call = (Expression)Expression.Call(methodInfo, get, state); - if (!fieldInfo.FieldType.IsValueType()) - call = Expression.Convert(call, fieldInfo.FieldType); - - // should handle specially - // todo: think about optimization, but it rare case - if (fieldInfo.IsInitOnly) - { - // var setMethod = fieldInfo.GetType().GetMethod("SetValue", new[] { typeof(object), typeof(object) }); - // expressionList.Add(Expression.Call(Expression.Constant(fieldInfo), setMethod, toLocal, call)); - var setMethod = typeof(DeepClonerExprGenerator).GetPrivateStaticMethod("ForceSetField"); - expressionList.Add(Expression.Call(setMethod, Expression.Constant(fieldInfo), - Expression.Convert(toLocal, typeof(object)), Expression.Convert(call, typeof(object)))); - } - else - { - expressionList.Add(Expression.Assign(Expression.Field(toLocal, fieldInfo), call)); - } - } - else - { - expressionList.Add(Expression.Assign(Expression.Field(toLocal, fieldInfo), Expression.Field(fromLocal, fieldInfo))); - } + return Expression.Assign(Expression.Field(toLocal, fieldInfo), Expression.Field(fromLocal, fieldInfo)); } - expressionList.Add(Expression.Convert(toLocal, methodType)); + var methodInfo = fieldInfo.FieldType.IsValueType() + ? typeof(DeepClonerGenerator).GetPrivateStaticMethod("CloneStructInternal") + .MakeGenericMethod(fieldInfo.FieldType) + : typeof(DeepClonerGenerator).GetPrivateStaticMethod("CloneClassInternal"); - var funcType = typeof(Func<,,,>).MakeGenericType(methodType, methodType, typeof(DeepCloneState), methodType); + var get = Expression.Field(fromLocal, fieldInfo); - var blockParams = new List(); - if (from != fromLocal) blockParams.Add(fromLocal); - if (to != toLocal) blockParams.Add(toLocal); + // toLocal.Field = Clone...Internal(fromLocal.Field) + var call = (Expression)Expression.Call(methodInfo, get, state); + if (!fieldInfo.FieldType.IsValueType()) + { + call = Expression.Convert(call, fieldInfo.FieldType); + } - return Expression.Lambda(funcType, Expression.Block(blockParams, expressionList), from, to, state).Compile(); + // should handle specially + // todo: think about optimization, but it rare case + if (fieldInfo.IsInitOnly) + { + var setMethod = typeof(DeepClonerExprGenerator).GetPrivateStaticMethod("ForceSetField"); + return Expression.Call(setMethod, Expression.Constant(fieldInfo), + Expression.Convert(toLocal, typeof(object)), Expression.Convert(call, typeof(object))); + } + + return Expression.Assign(Expression.Field(toLocal, fieldInfo), call); } private static object GenerateProcessArrayMethod(Type type, bool isDeep) @@ -128,35 +140,48 @@ private static object GenerateProcessArrayMethod(Type type, bool isDeep) if (rank == 1 && type == elementType.MakeArrayType()) { - if (!isDeep) - { - var callS = Expression.Call( - typeof(ClonerToExprGenerator).GetPrivateStaticMethod("ShallowClone1DimArraySafeInternal") - .MakeGenericMethod(elementType), Expression.Convert(from, type), Expression.Convert(to, type)); - return Expression.Lambda(funcType, callS, from, to, state).Compile(); - } - else - { - var methodName = "Clone1DimArrayClassInternal"; - if (DeepClonerSafeTypes.CanReturnSameObject(elementType)) methodName = "Clone1DimArraySafeInternal"; - else if (elementType.IsValueType()) methodName = "Clone1DimArrayStructInternal"; - var methodInfo = typeof(ClonerToExprGenerator).GetPrivateStaticMethod(methodName).MakeGenericMethod(elementType); - var callS = Expression.Call(methodInfo, Expression.Convert(from, type), Expression.Convert(to, type), state); - return Expression.Lambda(funcType, callS, from, to, state).Compile(); - } + return GenerateProcessOneDimArrayMethod(type, elementType, isDeep, funcType, from, to, state); } - else + + // multidim or not zero-based arrays + var methodInfo = rank == 2 && type == elementType.MakeArrayType(2) + ? typeof(ClonerToExprGenerator).GetPrivateStaticMethod("Clone2DimArrayInternal").MakeGenericMethod(elementType) + : typeof(ClonerToExprGenerator).GetPrivateStaticMethod("CloneAbstractArrayInternal"); + + var callS = Expression.Call(methodInfo, Expression.Convert(from, type), Expression.Convert(to, type), state, Expression.Constant(isDeep)); + return Expression.Lambda(funcType, callS, from, to, state).Compile(); + } + + private static object GenerateProcessOneDimArrayMethod( + Type type, + Type elementType, + bool isDeep, + Type funcType, + ParameterExpression from, + ParameterExpression to, + ParameterExpression state) + { + if (!isDeep) { - // multidim or not zero-based arrays - MethodInfo methodInfo; - if (rank == 2 && type == elementType.MakeArrayType(2)) - methodInfo = typeof(ClonerToExprGenerator).GetPrivateStaticMethod("Clone2DimArrayInternal").MakeGenericMethod(elementType); - else - methodInfo = typeof(ClonerToExprGenerator).GetPrivateStaticMethod("CloneAbstractArrayInternal"); - - var callS = Expression.Call(methodInfo, Expression.Convert(from, type), Expression.Convert(to, type), state, Expression.Constant(isDeep)); + var callS = Expression.Call( + typeof(ClonerToExprGenerator).GetPrivateStaticMethod("ShallowClone1DimArraySafeInternal") + .MakeGenericMethod(elementType), Expression.Convert(from, type), Expression.Convert(to, type)); return Expression.Lambda(funcType, callS, from, to, state).Compile(); } + + var methodName = "Clone1DimArrayClassInternal"; + if (DeepClonerSafeTypes.CanReturnSameObject(elementType)) + { + methodName = "Clone1DimArraySafeInternal"; + } + else if (elementType.IsValueType()) + { + methodName = "Clone1DimArrayStructInternal"; + } + + var methodInfo = typeof(ClonerToExprGenerator).GetPrivateStaticMethod(methodName).MakeGenericMethod(elementType); + var callD = Expression.Call(methodInfo, Expression.Convert(from, type), Expression.Convert(to, type), state); + return Expression.Lambda(funcType, callD, from, to, state).Compile(); } // when we can't use code generation, we can use these methods @@ -179,12 +204,18 @@ internal static T[] Clone1DimArraySafeInternal(T[] objFrom, T[] objTo, DeepCl internal static T[] Clone1DimArrayStructInternal(T[] objFrom, T[] objTo, DeepCloneState state) { // not null from called method, but will check it anyway - if (objFrom == null || objTo == null) return null; + if (objFrom == null || objTo == null) + { + return null; + } + var l = Math.Min(objFrom.Length, objTo.Length); state.AddKnownRef(objFrom, objTo); var cloner = DeepClonerGenerator.GetClonerForValueType(); for (var i = 0; i < l; i++) + { objTo[i] = cloner(objTo[i], state); + } return objTo; } @@ -192,11 +223,17 @@ internal static T[] Clone1DimArrayStructInternal(T[] objFrom, T[] objTo, Deep internal static T[] Clone1DimArrayClassInternal(T[] objFrom, T[] objTo, DeepCloneState state) { // not null from called method, but will check it anyway - if (objFrom == null || objTo == null) return null; + if (objFrom == null || objTo == null) + { + return null; + } + var l = Math.Min(objFrom.Length, objTo.Length); state.AddKnownRef(objFrom, objTo); for (var i = 0; i < l; i++) + { objTo[i] = (T)DeepClonerGenerator.CloneClassInternal(objFrom[i], state); + } return objTo; } @@ -204,17 +241,20 @@ internal static T[] Clone1DimArrayClassInternal(T[] objFrom, T[] objTo, DeepC internal static T[,] Clone2DimArrayInternal(T[,] objFrom, T[,] objTo, DeepCloneState state, bool isDeep) { // not null from called method, but will check it anyway - if (objFrom == null || objTo == null) return null; - if (objFrom.GetLowerBound(0) != 0 || objFrom.GetLowerBound(1) != 0 - || objTo.GetLowerBound(0) != 0 || objTo.GetLowerBound(1) != 0) + if (objFrom == null || objTo == null) + { + return null; + } + + if (HasNonZeroLowerBound(objFrom) || HasNonZeroLowerBound(objTo)) + { return (T[,])CloneAbstractArrayInternal(objFrom, objTo, state, isDeep); + } var l1 = Math.Min(objFrom.GetLength(0), objTo.GetLength(0)); var l2 = Math.Min(objFrom.GetLength(1), objTo.GetLength(1)); state.AddKnownRef(objFrom, objTo); - if ((!isDeep || DeepClonerSafeTypes.CanReturnSameObject(typeof(T))) - && objFrom.GetLength(0) == objTo.GetLength(0) - && objFrom.GetLength(1) == objTo.GetLength(1)) + if ((!isDeep || DeepClonerSafeTypes.CanReturnSameObject(typeof(T))) && HaveSameShape(objFrom, objTo)) { Array.Copy(objFrom, objTo, objFrom.Length); return objTo; @@ -222,38 +262,76 @@ internal static T[] Clone1DimArrayClassInternal(T[] objFrom, T[] objTo, DeepC if (!isDeep) { - for (var i = 0; i < l1; i++) - for (var k = 0; k < l2; k++) - objTo[i, k] = objFrom[i, k]; - return objTo; + Copy2DimArray(objFrom, objTo, l1, l2); } - - if (typeof(T).IsValueType()) + else if (typeof(T).IsValueType()) { - var cloner = DeepClonerGenerator.GetClonerForValueType(); - for (var i = 0; i < l1; i++) - for (var k = 0; k < l2; k++) - objTo[i, k] = cloner(objFrom[i, k], state); + Clone2DimArrayStructs(objFrom, objTo, l1, l2, state); } else { - for (var i = 0; i < l1; i++) - for (var k = 0; k < l2; k++) - objTo[i, k] = (T)DeepClonerGenerator.CloneClassInternal(objFrom[i, k], state); + Clone2DimArrayClasses(objFrom, objTo, l1, l2, state); } return objTo; } + private static bool HasNonZeroLowerBound(Array array) + => array.GetLowerBound(0) != 0 || array.GetLowerBound(1) != 0; + + private static bool HaveSameShape(Array first, Array second) + => first.GetLength(0) == second.GetLength(0) && first.GetLength(1) == second.GetLength(1); + + private static void Copy2DimArray(T[,] objFrom, T[,] objTo, int l1, int l2) + { + for (var i = 0; i < l1; i++) + { + for (var k = 0; k < l2; k++) + { + objTo[i, k] = objFrom[i, k]; + } + } + } + + private static void Clone2DimArrayStructs(T[,] objFrom, T[,] objTo, int l1, int l2, DeepCloneState state) + { + var cloner = DeepClonerGenerator.GetClonerForValueType(); + for (var i = 0; i < l1; i++) + { + for (var k = 0; k < l2; k++) + { + objTo[i, k] = cloner(objFrom[i, k], state); + } + } + } + + private static void Clone2DimArrayClasses(T[,] objFrom, T[,] objTo, int l1, int l2, DeepCloneState state) + { + for (var i = 0; i < l1; i++) + { + for (var k = 0; k < l2; k++) + { + objTo[i, k] = (T)DeepClonerGenerator.CloneClassInternal(objFrom[i, k], state); + } + } + } + // rare cases, very slow cloning. currently it's ok internal static Array CloneAbstractArrayInternal(Array objFrom, Array objTo, DeepCloneState state, bool isDeep) { // not null from called method, but will check it anyway - if (objFrom == null || objTo == null) return null; + if (objFrom == null || objTo == null) + { + return null; + } + var rank = objFrom.Rank; if (objTo.Rank != rank) + { throw new InvalidOperationException("Invalid rank of target array"); + } + var lowerBoundsFrom = Enumerable.Range(0, rank).Select(objFrom.GetLowerBound).ToArray(); var lowerBoundsTo = Enumerable.Range(0, rank).Select(objTo.GetLowerBound).ToArray(); var lengths = Enumerable.Range(0, rank).Select(x => Math.Min(objFrom.GetLength(x), objTo.GetLength(x))).ToArray(); @@ -264,30 +342,40 @@ internal static Array CloneAbstractArrayInternal(Array objFrom, Array objTo, Dee // unable to copy any element if (lengths.Any(x => x == 0)) + { return objTo; + } - while (true) + do { - if (isDeep) - objTo.SetValue(DeepClonerGenerator.CloneClassInternal(objFrom.GetValue(idxesFrom), state), idxesTo); - else - objTo.SetValue(objFrom.GetValue(idxesFrom), idxesTo); - var ofs = rank - 1; - while (true) + var value = objFrom.GetValue(idxesFrom); + objTo.SetValue(isDeep ? DeepClonerGenerator.CloneClassInternal(value, state) : value, idxesTo); + } + while (AdvanceIndexes(idxesFrom, idxesTo, lowerBoundsFrom, lowerBoundsTo, lengths)); + + return objTo; + } + + /// + /// Moves both index vectors to the next element, odometer style. Returns false once every element has been visited. + /// + private static bool AdvanceIndexes(int[] idxesFrom, int[] idxesTo, int[] lowerBoundsFrom, int[] lowerBoundsTo, int[] lengths) + { + var ofs = idxesFrom.Length - 1; + while (ofs >= 0) + { + idxesFrom[ofs]++; + idxesTo[ofs]++; + if (idxesFrom[ofs] < lowerBoundsFrom[ofs] + lengths[ofs]) { - idxesFrom[ofs]++; - idxesTo[ofs]++; - if (idxesFrom[ofs] >= lowerBoundsFrom[ofs] + lengths[ofs]) - { - idxesFrom[ofs] = lowerBoundsFrom[ofs]; - idxesTo[ofs] = lowerBoundsTo[ofs]; - ofs--; - if (ofs < 0) return objTo; - } - else - break; + return true; } + + idxesFrom[ofs] = lowerBoundsFrom[ofs]; + idxesTo[ofs] = lowerBoundsTo[ofs]; + ofs--; } - } -} \ No newline at end of file + return false; + } +} diff --git a/DeepCloner/Helpers/ShallowClonerGenerator.cs b/DeepCloner/Helpers/ShallowClonerGenerator.cs index 113844d..dc50b85 100644 --- a/DeepCloner/Helpers/ShallowClonerGenerator.cs +++ b/DeepCloner/Helpers/ShallowClonerGenerator.cs @@ -22,10 +22,14 @@ public static T CloneObject(T obj) } if (ReferenceEquals(obj, null)) + { return (T)(object)null; + } if (DeepClonerSafeTypes.CanReturnSameObject(obj.GetType())) + { return obj; + } return (T)ShallowObjectCloner.CloneObject(obj); } From 50c3235e882c1db85fe5702366f67f49bc22041e Mon Sep 17 00:00:00 2001 From: David Bond Date: Sun, 4 Oct 2026 14:09:31 +0100 Subject: [PATCH 2/3] fix: deep clone reference fields of readonly structs For a readonly (init-only) field the generated cloner boxed the struct local, wrote the cloned value into the box via FieldInfo.SetValue, and discarded the box, so the clone kept the source's reference. DeepClone and DeepCloneTo of any readonly struct (including readonly record structs), alone, nested in a class, or as array elements, were silently shallow. Box once, set the field on the box, unbox back into the struct local. Classes are unaffected. Adds ReadonlyStructSpec (6 of its 8 tests failed before the fix) and strengthens the 2D struct-array CopyTo test. Co-Authored-By: Claude Sonnet 5.5 --- DeepCloner.Tests/CopyToObjectSpec.cs | 1 + DeepCloner.Tests/ReadonlyStructSpec.cs | 122 ++++++++++++++++++ DeepCloner/Helpers/DeepClonerExprGenerator.cs | 32 ++++- 3 files changed, 149 insertions(+), 6 deletions(-) create mode 100644 DeepCloner.Tests/ReadonlyStructSpec.cs diff --git a/DeepCloner.Tests/CopyToObjectSpec.cs b/DeepCloner.Tests/CopyToObjectSpec.cs index bc49652..ff1ad7e 100644 --- a/DeepCloner.Tests/CopyToObjectSpec.cs +++ b/DeepCloner.Tests/CopyToObjectSpec.cs @@ -395,6 +395,7 @@ public void TwoDim_Array_Of_Structs_With_References_Should_Be_Cloned(bool isDeep Assert.Equal([4], arrTo[1, 1].Items); Assert.Equal([1], arrTo[0, 0].Items); Assert.Equal([3], arrTo[1, 0].Items); + Assert.Equal(isDeep, !ReferenceEquals(arrFrom[1, 1].Items, arrTo[1, 1].Items)); } [Theory] diff --git a/DeepCloner.Tests/ReadonlyStructSpec.cs b/DeepCloner.Tests/ReadonlyStructSpec.cs new file mode 100644 index 0000000..c375b39 --- /dev/null +++ b/DeepCloner.Tests/ReadonlyStructSpec.cs @@ -0,0 +1,122 @@ +#nullable disable + +using Xunit; + +namespace PanoramicData.DeepCloner.Test; + +/// +/// A struct whose fields are all readonly (a readonly struct or readonly record struct) must be +/// deep cloned like any other struct: its reference-type fields must not be shared with the source. +/// +public class ReadonlyStructSpec() : BaseTest(true) +{ + public readonly record struct Holder(int[] Items); + + public sealed class Box + { + public int Value { get; set; } + } + + public readonly struct BoxHolder(Box box) + { + public Box Box { get; } = box; + } + + public struct MutableHolder + { + public int[] Items { get; set; } + } + + public sealed class HoldsReadonlyStruct + { + public Holder Holder { get; set; } + } + + [Fact] + public void Readonly_Struct_With_Array_Should_Be_Deep_Cloned() + { + var source = new Holder([1, 2, 3]); + + var clone = source.DeepClone(); + + Assert.Equal([1, 2, 3], clone.Items); + Assert.NotSame(source.Items, clone.Items); + } + + [Fact] + public void Readonly_Struct_With_Class_Should_Be_Deep_Cloned() + { + var source = new BoxHolder(new Box { Value = 7 }); + + var clone = source.DeepClone(); + + Assert.Equal(7, clone.Box.Value); + Assert.NotSame(source.Box, clone.Box); + } + + [Fact] + public void Readonly_Struct_Inside_Class_Should_Be_Deep_Cloned() + { + var source = new HoldsReadonlyStruct { Holder = new Holder([4, 5]) }; + + var clone = source.DeepClone(); + + Assert.Equal([4, 5], clone.Holder.Items); + Assert.NotSame(source.Holder.Items, clone.Holder.Items); + } + + [Fact] + public void One_Dim_Array_Of_Readonly_Structs_Should_Be_Deep_Cloned() + { + var source = new[] { new Holder([1]), new Holder([2]) }; + + var clone = source.DeepClone(); + + Assert.Equal([2], clone[1].Items); + Assert.NotSame(source[1].Items, clone[1].Items); + } + + [Fact] + public void Two_Dim_Array_Of_Readonly_Structs_Should_Be_Deep_Cloned() + { + var source = new[,] { { new Holder([1]), new Holder([2]) }, { new Holder([3]), new Holder([4]) } }; + + var clone = source.DeepClone(); + + Assert.Equal([4], clone[1, 1].Items); + Assert.NotSame(source[1, 1].Items, clone[1, 1].Items); + } + + [Fact] + public void Deep_Clone_To_Existing_Array_Of_Readonly_Structs_Should_Be_Deep() + { + var source = new[,] { { new Holder([1]), new Holder([2]) }, { new Holder([3]), new Holder([4]) } }; + var target = new Holder[2, 2]; + + source.DeepCloneTo(target); + + Assert.Equal([4], target[1, 1].Items); + Assert.NotSame(source[1, 1].Items, target[1, 1].Items); + } + + [Fact] + public void Shallow_Clone_Of_Readonly_Struct_Should_Share_References() + { + var source = new Holder([1, 2, 3]); + + var clone = source.ShallowClone(); + + Assert.Same(source.Items, clone.Items); + } + + [Fact] + public void Mutable_Struct_With_Array_Should_Still_Be_Deep_Cloned() + { + var source = new MutableHolder { Items = [1, 2, 3] }; + + var clone = source.DeepClone(); + + Assert.Equal([1, 2, 3], clone.Items); + Assert.NotSame(source.Items, clone.Items); + } +} diff --git a/DeepCloner/Helpers/DeepClonerExprGenerator.cs b/DeepCloner/Helpers/DeepClonerExprGenerator.cs index 62ffae6..cf96e10 100644 --- a/DeepCloner/Helpers/DeepClonerExprGenerator.cs +++ b/DeepCloner/Helpers/DeepClonerExprGenerator.cs @@ -225,21 +225,21 @@ private static void AddFieldCloneExpression(List expressionList, Fie { // fieldInfo.SetValue(toLocal, value): FieldInfo.SetValue is an INSTANCE method, so the // FieldInfo constant is the call target, not the first argument. - expressionList.Add(Expression.Call( + expressionList.Add(SetReadonlyField(toLocal, boxed => Expression.Call( fieldConstant, _fieldSetMethod!, - Expression.Convert(toLocal, typeof(object)), - Expression.Convert(call, typeof(object)))); + boxed, + Expression.Convert(call, typeof(object))))); } else { var setMethod = typeof(DeepClonerExprGenerator).GetPrivateStaticMethod("ForceSetField")!; // ForceSetField(fieldInfo, toLocal, value): a STATIC method, so there is no call target. - expressionList.Add(Expression.Call( + expressionList.Add(SetReadonlyField(toLocal, boxed => Expression.Call( setMethod, fieldConstant, - Expression.Convert(toLocal, typeof(object)), - Expression.Convert(call, typeof(object)))); + boxed, + Expression.Convert(call, typeof(object))))); } } else @@ -248,6 +248,26 @@ private static void AddFieldCloneExpression(List expressionList, Fie } } + /// + /// Builds the expression that writes a readonly field through reflection, which needs the target as an object. + /// Boxing a class just passes the reference, but boxing a struct copies it, so the write would land on the copy + /// and be thrown away. For a struct the target is therefore boxed once, written, and unboxed back. + /// + private static Expression SetReadonlyField(Expression toLocal, Func setField) + { + if (!toLocal.Type.IsValueType()) + { + return setField(Expression.Convert(toLocal, typeof(object))); + } + + var boxed = Expression.Variable(typeof(object)); + return Expression.Block( + [boxed], + Expression.Assign(boxed, Expression.Convert(toLocal, typeof(object))), + setField(boxed), + Expression.Assign(toLocal, Expression.Unbox(boxed, toLocal.Type))); + } + private static object GenerateProcessArrayMethod(Type type) { var elementType = type.GetElementType()!; From 5077a3a115dcf095f756343dbd999938fbce35d0 Mon Sep 17 00:00:00 2001 From: David Bond Date: Sun, 4 Oct 2026 14:13:04 +0100 Subject: [PATCH 3/3] test: make ReadonlyStructSpec fixtures record structs (S3898) Co-Authored-By: Claude Sonnet 5.5 --- DeepCloner.Tests/ReadonlyStructSpec.cs | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/DeepCloner.Tests/ReadonlyStructSpec.cs b/DeepCloner.Tests/ReadonlyStructSpec.cs index c375b39..0e21a13 100644 --- a/DeepCloner.Tests/ReadonlyStructSpec.cs +++ b/DeepCloner.Tests/ReadonlyStructSpec.cs @@ -17,12 +17,9 @@ public sealed class Box public int Value { get; set; } } - public readonly struct BoxHolder(Box box) - { - public Box Box { get; } = box; - } + public readonly record struct BoxHolder(Box Box); - public struct MutableHolder + public record struct MutableHolder { public int[] Items { get; set; } }