Skip to content

refactor + fix: cut ClonerToExprGenerator complexity, clear Codacy findings, deep clone readonly structs - #5

Merged
davidnmbond merged 3 commits into
mainfrom
fix/codacy-grade-3
Oct 4, 2026
Merged

davidnmbond merged 3 commits into
mainfrom
fix/codacy-grade-3

Conversation

@davidnmbond

@davidnmbond davidnmbond commented Oct 4, 2026 •

Copy link
Copy Markdown

Summary

  • ClonerToExprGenerator (the worst complexity offender): split GenerateProcessMethod, the 1D/2D/N-D array cloners and the index odometer into small methods; braces everywhere; removed dead assignments and commented-out code. Behaviour is unchanged.
  • ShallowClonerGenerator: braces
  • Tests: scoped suppressions with reasons for deliberate public fields (S1104/S2357) and a deliberately throwing Equals (S3877)
  • .codacy.yml: excludes the vendored Imported/ FastDeepCloner comparison code
  • CLAUDE.md: split a compound bullet
  • Bug fix: DeepClone/DeepCloneTo did not deep clone reference-type fields of readonly structs (details below)

Bug: readonly structs were silently shallow-cloned

DeepClone() and DeepCloneTo() left the reference-type fields of a readonly struct (including readonly record struct) pointing at the same objects as the source.

public readonly record struct Holder(int[] Items);

var source = new Holder([1, 2, 3]);
var clone = source.DeepClone();
ReferenceEquals(source.Items, clone.Items); // was true, expected false

It also affected readonly structs holding class instances, nested in a class, and as 1D/2D array elements (DeepClone and DeepCloneTo). Mutable structs and ShallowClone were correct, so the trigger is the fields being readonly/init-only.

Cause: for a readonly field, DeepClonerExprGenerator.AddFieldCloneExpression emitted FieldInfo.SetValue(Convert(toLocal, typeof(object)), value). For a struct toLocal, Convert boxes a copy; SetValue wrote the cloned value into the copy, which was then discarded. Both the SetValue and ForceSetField paths had this. Classes were unaffected.

Fix: box once, set the field on the box, unbox back into the struct local (SetReadonlyField).

Found by exercising the struct branch of the 2D array cloner while refactoring. The original code behaves identically, so this is long-standing, not a regression. (Issues are disabled on this repository, so the write-up lives here.)

Behaviour preserved by the refactor

Swapped the original ClonerToExprGenerator back in and ran the new struct-array test: identical before and after.

Not changed

  • S3459 in DeepClonerExprGenerator: the field is set by reflection as a runtime capability probe, replacing it could change behaviour
  • The SECURITY.md contact email (PII finding) is intentional

Test plan

  • dotnet test DeepCloner.Tests: 130 passed, 1 skipped, 0 failed, no warnings
  • new ReadonlyStructSpec: 6 of 8 tests failed before the fix, all pass after
  • Confirm Codacy results on this PR

🤖 Generated with Claude Code

… 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 <noreply@anthropic.com>
@codacy-production

codacy-production Bot commented Oct 4, 2026 •

Copy link
Copy Markdown

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🟢 Metrics 13 complexity · 2 duplication

Metric Results
Complexity 13
Duplication 2

View in Codacy

🟢 Coverage 85.00% diff coverage · -0.10% coverage variation

Metric Results
Coverage variation ✅ -0.10% coverage variation (-1.00%)
Diff coverage ✅ 85.00% diff coverage

View coverage diff in Codacy

Coverage variation details
Coverable lines Covered lines Coverage
Common ancestor commit (6966fc5) 1054 862 81.78%
Head commit (5077a3a) 1108 (+54) 905 (+43) 81.68% (-0.10%)

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details
Coverable lines Covered lines Diff coverage
Pull request (#5) 160 136 85.00%

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

AI Reviewer: first review requested successfully. AI can make mistakes. Always validate suggestions.

Run reviewer

TIP This summary will be updated as you push new changes.

@davidnmbond davidnmbond changed the title refactor: cut ClonerToExprGenerator complexity and clear remaining Codacy findings refactor + fix: cut ClonerToExprGenerator complexity, clear Codacy findings, deep clone readonly structs Oct 4, 2026
davidnmbond and others added 2 commits October 4, 2026 14:09
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 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
@davidnmbond
davidnmbond merged commit 1ce15ff into main Oct 4, 2026
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant