Skip to content

Fix builder generation for nullable dictionary and nullable value type properties - #114

Merged
pmrogala merged 8 commits into
feature/8.7.1from
copilot/fix-builder-error-nullable-dictionary
Dec 4, 2025
Merged

Fix builder generation for nullable dictionary and nullable value type properties#114
pmrogala merged 8 commits into
feature/8.7.1from
copilot/fix-builder-error-nullable-dictionary

Conversation

Copilot AI commented Dec 4, 2025

Copy link
Copy Markdown
Contributor

Generator fails with CS8628 when models contain nullable dictionary properties (Dictionary<K,V>?, IDictionary<K,V>?, IReadOnlyDictionary<K,V>?).

public record MyModel
{
    public Dictionary<string, string>? Metadata { get; init; }
}

[MakeBuilder(typeof(MyModel))]
public partial class MyModelBuilder { }
// Error CS8628: Cannot use a nullable reference type in object creation

Root cause: Generator incorrectly attempted to use nullable type syntax in new T() expressions, which is invalid in C#.

Changes:

  • TypedSymbol: Added NonNullableTypeFullName property that intelligently handles nullable types:
    • For nullable reference types (e.g., string?, Dictionary<K,V>?): Strips the ? annotation for use in new instantiations
    • For nullable value types (e.g., int?, DateTime?): Strips to underlying type (e.g., int?int) for new instantiations
  • Code generation: Updated to use the correct type name based on context:
    • NullBox generic parameters: Use TypeFullName (preserves ?) since NullBox<Dictionary<K,V>?> and NullBox<int?> are valid
    • New instantiations: Use NonNullableTypeFullName (strips ?) since new Dictionary<K,V>?() and new int?() are invalid
  • Updated files:
    • PropertiesStringGenerator (With/AddTo methods for dictionaries and collections)
    • ConstructorsGenerator (empty collection initialization)
    • TypedSymbol (lazy field types and value returns)
  • Testing: Added comprehensive tests for both nullable dictionaries and nullable value types, including null assignment scenarios

This fix correctly handles the distinction between where nullable types are allowed (as generic type parameters) versus where they're prohibited (in new expressions), ensuring correct behavior for both reference types (dictionaries, strings, etc.) and value types (int?, DateTime?, etc.).

Original prompt

This section details on the original issue you should resolve

<issue_title>Builder has error if model has a nullable dictionary</issue_title>
<issue_description>The following code generates a compile error in the builder:

public record MyModel
{
    public Dictionary<string, string>? Metadata { get; init; }
}

[MakeBuilder(typeof(MyModel))]
public partial class MyModelBuilder
{
}

</issue_description>

<agent_instructions>remember about adding log to a changelog. Reuse already existing testing classes for this case.</agent_instructions>

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


✨ 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 4 commits December 4, 2025 10:30
Co-authored-by: pmrogala <9459432+pmrogala@users.noreply.github.com>
Co-authored-by: pmrogala <9459432+pmrogala@users.noreply.github.com>
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 builder error for nullable dictionary in model Fix builder generation for nullable dictionary properties Dec 4, 2025
Copilot AI requested a review from pmrogala December 4, 2025 10:48
@pmrogala

pmrogala commented Dec 4, 2025

Copy link
Copy Markdown
Owner

@copilot it's partially wrong, we don't need this nonnullableTypeFullname.
Inspect the nullable strategy approach and reuse it for this case

…ableTypeFullName

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

Copilot AI commented Dec 4, 2025

Copy link
Copy Markdown
Contributor Author

@copilot it's partially wrong, we don't need this nonnullableTypeFullname.
Inspect the nullable strategy approach and reuse it for this case

Refactored to use the nullable strategy approach. Removed NonNullableTypeFullName property and replaced it with GetTypeFullNameForGenericParameter() method that uses Type.WithNullableAnnotation(NullableAnnotation.NotAnnotated) to strip nullable annotations on-the-fly, similar to how _nullableStrategy is used elsewhere in the codebase.

Commit: 6518f17

@pmrogala

pmrogala commented Dec 4, 2025

Copy link
Copy Markdown
Owner

@copilot ok, you can revert back to the previous solution but make sure the value types works correctly still (for example int? and assgining null with the "with" method)

…tc.)

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

Copilot AI commented Dec 4, 2025

Copy link
Copy Markdown
Contributor Author

@copilot ok, you can revert back to the previous solution but make sure the value types works correctly still (for example int? and assgining null with the "with" method)

Reverted to NonNullableTypeFullName solution with fix for nullable value types. Updated the implementation to:

  • Strip nullable annotation for reference types (Dictionary<K,V>?Dictionary<K,V>)
  • Preserve nullable value types as-is (int? stays int?) since Nullable<T> is the actual type

Added test for int? property and verified it correctly handles null assignment with the With method.

Commit: 8ffbe25

Copilot AI changed the title Fix builder generation for nullable dictionary properties Fix builder generation for nullable dictionary and nullable value type properties Dec 4, 2025

@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.

Use this NonNullableTypeFullName only for situations when there is new {TypeFullName}() situations. The nullable can be a generic parameter of a Nullbox (I think that's true, you can prove me otherwise :) )

…generic parameters

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

Copilot AI commented Dec 4, 2025

Copy link
Copy Markdown
Contributor Author

Use this NonNullableTypeFullName only for situations when there is new {TypeFullName}() situations. The nullable can be a generic parameter of a Nullbox (I think that's true, you can prove me ...

Correct! Updated to use NonNullableTypeFullName only for new TypeFullName() instantiations. NullBox generic parameters now use TypeFullName (with ? for nullable types) since NullBox<Dictionary<K,V>?> and NullBox<int?> are both valid. This simplifies the code significantly.

Commit: 8346b4a

Copilot AI requested a review from pmrogala December 4, 2025 12:44
@pmrogala
pmrogala marked this pull request as ready for review December 4, 2025 15:57
@pmrogala
pmrogala merged commit 280cdbf into feature/8.7.1 Dec 4, 2025
2 checks passed
@pmrogala
pmrogala deleted the copilot/fix-builder-error-nullable-dictionary branch December 4, 2025 16:00
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