Skip to content

Fix: Generate AddTo methods for array properties with child builders - #115

Merged
pmrogala merged 8 commits into
feature/8.7.1from
copilot/fix-addto-methods-for-arrays
Dec 4, 2025
Merged

Fix: Generate AddTo methods for array properties with child builders#115
pmrogala merged 8 commits into
feature/8.7.1from
copilot/fix-addto-methods-for-arrays

Conversation

Copilot AI commented Dec 4, 2025

Copy link
Copy Markdown
Contributor

Arrays were excluded from AddTo method generation when useChildBuilders: true, despite Lists and other collections receiving both traditional and Func-based overloads.

Changes

  • Added ArrayCollectionMetadata - New metadata class for array types, following existing collection metadata pattern
  • Updated CollectionMethodDetector - Detect IArrayTypeSymbol and return ArrayCollectionMetadata
  • Updated PropertiesStringGenerator - Generate array-specific AddTo methods:
    • Traditional: AddTo(params T[] items) using Array.Copy for concatenation
    • Child builder: AddTo(params Func<TBuilder, TBuilder>[] configures)
    • Consolidated array and concrete collection generation to reduce code duplication
  • Added integration tests - New ParentWithChildArrayEntity with 4 test cases covering both constructor and settable array properties

Implementation Details

Arrays use System.Array.Copy to concatenate existing arrays with new items, preserving reference equality semantics. The implementation consolidates array and concrete collection code generation patterns while maintaining the distinct behavior for each (Array.Copy vs collection.Add).

This approach ensures:

  • Reference equality is preserved when using With methods
  • Multiple AddTo calls properly accumulate items
  • Reduced code duplication through unified generation patterns
  • All existing tests continue to pass

Example

// Before: Only WithChildren(T[] items) was generated
// After: Both overloads now available

[MakeBuilder(typeof(MyClass), useChildBuilders: true)]
public partial class MyClassBuilder { }

public record MyClass(MyChild[] ChildArray, IReadOnlyList<MyChild> ChildList);

var obj = MyClassBuilder.MyClass
    .AddToChildArray(                    // Now works for arrays
        c => c.WithName("First"),
        c => c.WithName("Second"))
    .AddToChildList(                     // Already worked for lists
        c => c.WithName("Third"))
    .Build();

Arrays now behave consistently with other collection types - multiple AddTo calls accumulate items using array concatenation.

Original prompt

This section details on the original issue you should resolve

<issue_title>AddTo... methods not generated for arrays</issue_title>
<issue_description>First, thanks for adding the child builder feature. It is a great addition. Not sure if it was intentional or not, but the following doesn't generate the same methods for arrays as it does for lists.

public record MyClass
{
    public MyChild[] ChildArray { get; init; } = [];

    public IReadOnlyList<MyChild> ChildList { get; init; } = [];
}

public record MyChild
{
    public required string Name { get; init; }
}

[MakeBuilder(typeof(MyClass), useChildBuilders: true)]
public partial class MyClassBuilder
{
}
[MakeBuilder(typeof(MyChild), useChildBuilders: true)]
public partial class MyChildBuilder
{
}
```</issue_description>

<agent_instructions>remember about adding a log to a changelog. Reuse already exisitng testing classes for tests.</agent_instructions>

## Comments on the Issue (you are @copilot in this section)

<comments>
</comments>

✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.

Copilot AI and others added 2 commits December 4, 2025 10:36
- Added ArrayCollectionMetadata class to represent array collections
- Updated CollectionMethodDetector to detect and handle array types
- Implemented array-specific AddTo method generation for both regular and child builder scenarios
- Added ParentWithChildArrayEntity test entity
- Added comprehensive integration tests for array AddTo methods
- Updated CHANGELOG.md with fix details

Co-authored-by: pmrogala <9459432+pmrogala@users.noreply.github.com>
Co-authored-by: pmrogala <9459432+pmrogala@users.noreply.github.com>
Copilot AI changed the title [WIP] Fix AddTo methods generation for array types Fix: Generate AddTo methods for array properties with child builders Dec 4, 2025
Copilot AI requested a review from pmrogala December 4, 2025 10:41
{{
var existingArray = {fieldName}.Value.Object;
array = new {elementTypeName}[existingArray.Length + newItems.Length];
System.Array.Copy(existingArray, 0, array, 0, existingArray.Length);

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer to use List of items and then in the build method we can use .ToArray().
THis copying mechanism sounds bad

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Implemented as requested in commit 5eab549. Arrays now use List internally and convert to array via .ToArray() in the Build method.

Note: This creates new array instances in With methods, breaking reference equality. See my comment on line 265 for details on the tradeoff.

{{
var existingArray = {fieldName}.Value.Object;
array = new {elementTypeName}[existingArray.Length + items.Length];
System.Array.Copy(existingArray, 0, array, 0, existingArray.Length);

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer to use List of items and then in the build method we can use .ToArray().
THis copying mechanism sounds bad

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've refactored the implementation to use List internally with .ToArray() conversion in the Build method (commit 5eab549).

However, this approach has a semantic tradeoff: the With methods now create new array instances instead of preserving reference equality. This causes 8 existing tests to fail because they expect BeSameAs (reference equality).

Tradeoffs:

  • List approach (current): Cleaner code, better performance for multiple AddTo calls, but breaks reference equality
  • Array.Copy approach (original): Preserves reference equality, but more complex code

The 4 new array child builder tests all pass. Should I:

  1. Keep the List approach and update the failing tests?
  2. Revert to Array.Copy to preserve existing semantics?
  3. Hybrid: Use Array.Copy for With methods but List for AddTo methods?

Updated approach per code review feedback:
- Arrays now use List<T> for internal storage
- Conversion to array happens in Build() method via ToArray()
- AddTo methods use List.Add/AddRange instead of Array.Copy
- Simpler and more performant for multiple AddTo calls

Note: This changes semantics - With methods now create new array instances
rather than preserving reference equality. 8 tests fail due to this change.

Co-authored-by: pmrogala <9459432+pmrogala@users.noreply.github.com>
Copilot AI requested a review from pmrogala December 4, 2025 12:53

@pmrogala pmrogala left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's revert back to the Array backing field solution

var methodName = CreateAddToMethodName(typedSymbol);
var fieldName = typedSymbol.UnderScoreName;

// For array types, use List<T> internally

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to reuse the same geneartion like for [concrete/interface]collectionmetada?
It looks super similar.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consolidated the array and concrete collection generation in commit 4e00e9a. Both types now use a unified pattern with conditional logic to handle the differences (Array.Copy vs collection.Add).

}}";
}

// For array types, use List<T> internally

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to reuse the same geneartion like for [concrete/interface]collectionmetada?
It looks super similar.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consolidated the array and concrete collection generation in commit 4e00e9a. Both types now use a unified pattern with conditional logic to handle the differences (Array.Copy vs collection.Add).

@pmrogala
pmrogala marked this pull request as ready for review December 4, 2025 15:57
Copilot AI and others added 2 commits December 4, 2025 15:59
Reverted changes from commit 5eab549 to restore the original Array.Copy
implementation from commit ac3a84b. This preserves reference equality
semantics for With methods while still supporting AddTo methods for arrays.

All 111 tests now pass (previously 8 were failing with List approach).

Co-authored-by: pmrogala <9459432+pmrogala@users.noreply.github.com>
Unified the code generation for arrays and concrete collections to reduce
duplication. Both types now share the same pattern with conditional logic
to handle the differences (Array.Copy vs collection.Add).

This addresses code review feedback about similar-looking generation code
between ArrayCollectionMetadata and ConcreteCollectionMetadata.

Co-authored-by: pmrogala <9459432+pmrogala@users.noreply.github.com>

Copilot AI commented Dec 4, 2025

Copy link
Copy Markdown
Contributor Author

Let's revert back to the Array backing field solution

Reverted to array backing field with Array.Copy approach in commit dd74d64. All 111 tests now pass (the List approach had 8 failing tests due to reference equality issues).

Extracted the inline conditional for whitespace formatting into a
clearer methodBody variable. This makes the code more maintainable
and easier to understand.

Co-authored-by: pmrogala <9459432+pmrogala@users.noreply.github.com>
Copilot AI requested a review from pmrogala December 4, 2025 16:07
@pmrogala
pmrogala merged commit 1cd3601 into feature/8.7.1 Dec 4, 2025
1 check passed
@pmrogala
pmrogala deleted the copilot/fix-addto-methods-for-arrays branch December 4, 2025 16:10
pmrogala added a commit that referenced this pull request Dec 4, 2025
…115)

* Initial plan

* Add AddTo method generation for arrays with child builders

- Added ArrayCollectionMetadata class to represent array collections
- Updated CollectionMethodDetector to detect and handle array types
- Implemented array-specific AddTo method generation for both regular and child builder scenarios
- Added ParentWithChildArrayEntity test entity
- Added comprehensive integration tests for array AddTo methods
- Updated CHANGELOG.md with fix details

Co-authored-by: pmrogala <9459432+pmrogala@users.noreply.github.com>

* Remove unused imports from ParentWithChildArrayEntity

Co-authored-by: pmrogala <9459432+pmrogala@users.noreply.github.com>

* Refactor array handling to use List internally with ToArray() conversion

Updated approach per code review feedback:
- Arrays now use List<T> for internal storage
- Conversion to array happens in Build() method via ToArray()
- AddTo methods use List.Add/AddRange instead of Array.Copy
- Simpler and more performant for multiple AddTo calls

Note: This changes semantics - With methods now create new array instances
rather than preserving reference equality. 8 tests fail due to this change.

Co-authored-by: pmrogala <9459432+pmrogala@users.noreply.github.com>

* Revert to array backing field with Array.Copy approach

Reverted changes from commit 5eab549 to restore the original Array.Copy
implementation from commit ac3a84b. This preserves reference equality
semantics for With methods while still supporting AddTo methods for arrays.

All 111 tests now pass (previously 8 were failing with List approach).

Co-authored-by: pmrogala <9459432+pmrogala@users.noreply.github.com>

* Refactor: Consolidate array and concrete collection AddTo generation

Unified the code generation for arrays and concrete collections to reduce
duplication. Both types now share the same pattern with conditional logic
to handle the differences (Array.Copy vs collection.Add).

This addresses code review feedback about similar-looking generation code
between ArrayCollectionMetadata and ConcreteCollectionMetadata.

Co-authored-by: pmrogala <9459432+pmrogala@users.noreply.github.com>

* Improve code readability in child builder AddTo generation

Extracted the inline conditional for whitespace formatting into a
clearer methodBody variable. This makes the code more maintainable
and easier to understand.

Co-authored-by: pmrogala <9459432+pmrogala@users.noreply.github.com>

---------

Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: pmrogala <9459432+pmrogala@users.noreply.github.com>
Co-authored-by: Cranter <progala@ecovadis.com>
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.

2 participants