fix - #51
Conversation
There was a problem hiding this comment.
🟢 Approval recommended
The generator changes are guarded, well-covered by new tests, and the remaining feedback is limited to minor nullable-warning cleanups in the new test file.
Pull request overview
This PR fixes source-generator collection cloning for issue #50 by switching from CollectionKind-based assumptions to verifying the actual constructor/API surface of the concrete type the generator will instantiate, preventing invalid capacity/copy-ctor/CollectionsMarshal fast paths and falling back to the runtime cloner when codegen can’t be proven correct.
Changes:
- Add regression tests for issue #50 covering
Collection<T>,BindingList<T>, derived collection/dictionary types, getter-only collections, stack ordering, and a non-verifiable custom enumerable fallback. - Introduce
CollectionCapabilitiesand generation guards (CanGenerateCollectionHelper/CanGenerateDictionaryHelper) so helpers are only emitted when construction + population are guaranteed to compile and behave correctly. - Tighten fast paths in
CollectionHelperGeneratorto require verified capacity/copy constructors and exactList<T>before usingCollectionsMarshal.
File summaries
| File | Description |
|---|---|
| src/FastCloner.Tests/SourceGeneratorIssue50Tests.cs | Adds comprehensive regression coverage for issue #50 scenarios and edge cases. |
| src/FastCloner.SourceGenerator/TypeAnalyzer.cs | Adds CollectionCapabilities, helper-generation viability checks, and fixes immutable set kind ordering. |
| src/FastCloner.SourceGenerator/NonPublicAccessorEmitter.cs | Adds null-forgiving operators to suppress nullability warnings in generated accessor code. |
| src/FastCloner.SourceGenerator/NestedTypeCollector.cs | Gates helper registration on verified collection/dictionary capabilities and propagates capability flags into MemberModel. |
| src/FastCloner.SourceGenerator/MemberModel.cs | Extends MemberModel to carry verified concrete collection capability flags. |
| src/FastCloner.SourceGenerator/MemberCollector.cs | Reworks getter-only collection “populatable” detection to be API-based instead of a whitelist. |
| src/FastCloner.SourceGenerator/MemberCloneGenerator.cs | Adds null-forgiving on Cloner<T>.Clone(...) argument (safe due to null-guard in Clone). |
| src/FastCloner.SourceGenerator/CollectionHelperGenerator.cs | Requires verified ctor surfaces for capacity/copy ctor fast paths and restricts CollectionsMarshal usage to exact List<T>. |
Review details
Suppressed comments (3)
src/FastCloner.Tests/SourceGeneratorIssue50Tests.cs:237
clone.Itemsis nullable (DerivedIntDictionary?), but it’s indexed/assigned without!after only runtime assertions, which will trigger nullable warnings.
await Assert.That(clone.Items["b"]).IsEqualTo(2);
clone.Items["c"] = 3;
src/FastCloner.Tests/SourceGeneratorIssue50Tests.cs:274
clone.Numbersis nullable (DerivedIntHashSet?), but it’s used without!after only runtime assertions, which will trigger nullable warnings.
clone.Numbers.Add(4);
src/FastCloner.Tests/SourceGeneratorIssue50Tests.cs:256
clone.Itemsis nullable (DerivedObservableCollection?), but it’s indexed without!after only runtime assertions, which will trigger nullable warnings.
await Assert.That(clone.Items[0]).IsNotSameReferenceAs(original.Items![0]);
await Assert.That(clone.Items[0].Name).IsEqualTo("First");
- Files reviewed: 8/8 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| await Assert.That(clone.Items["a"]).IsNotSameReferenceAs(original.Items!["a"]); | ||
| await Assert.That(clone.Items["b"].Name).IsEqualTo("B"); |
Deep Clone Benchmarks
Current FastCloner vs DeepCloner
FastCloner vs latest
|
| Status | Benchmark | Delta Time | Delta Alloc |
|---|---|---|---|
| 🟢 | DynamicWithArray | -7% faster | ~same |
| ⚪ | DynamicWithDictionary | -3% faster | ~same |
| ⚪ | DynamicWithNestedObject | ~same | ~same |
| 🔴 | FileSpec | +6% slower | ~same |
| ⚪ | LargeEventDocument_10MB | -2% faster | ~same |
| 🔴 | LargeLogBatch_10MB | +7% slower | ~same |
| ⚪ | MediumNestedObject | -2% faster | ~same |
| ⚪ | ObjectDictionary_50 | +4% slower | ~same |
| ⚪ | ObjectList_100 | +4% slower | ~same |
| 🟢 | SmallObject | -15% faster | ~same |
| 🔴 | SmallObjectWithCollections | +6% slower | ~same |
| ⚪ | StringArray_1000 | ~same | ~same |
Regressions
FileSpec: time +6% slower, alloc ~sameLargeLogBatch_10MB: time +7% slower, alloc ~sameSmallObjectWithCollections: time +6% slower, alloc ~same
Improvements
DynamicWithArray: time -7% faster, alloc ~sameSmallObject: time -15% faster, alloc ~same
Mixed changes
- none
No description provided.