StefH/FluentBuilder

Wrong initialization is generated for ReadOnlyCollection<> properties

iam-mholle opened this issue · 2 comments

When generating a builder for a class containing a ReadOnlyCollection<> property, the generated code initializes the property with a new List<>().

Consider the following class:

[AutoGenerateBuilder]
public partial class SomeDTO
{
  public ReadOnlyCollection<string> Strings { get; set; }
}

The following builder is being generated, where the _strings initialization and the WithStrings(() => new List<string>()) call cause the compilation to fail:

public partial class SomeDTOBuilder : Builder<Library.SomeDTO>
{
    private bool _stringsIsSet;
    private Lazy<System.Collections.ObjectModel.ReadOnlyCollection<string>> _strings = new Lazy<System.Collections.ObjectModel.ReadOnlyCollection<string>>(() => new List<string>());
    public SomeDTOBuilder WithStrings(System.Collections.ObjectModel.ReadOnlyCollection<string> value) => WithStrings(() => value);
    public SomeDTOBuilder WithStrings(Func<System.Collections.ObjectModel.ReadOnlyCollection<string>> func)
    {
        _strings = new Lazy<System.Collections.ObjectModel.ReadOnlyCollection<string>>(func);
        _stringsIsSet = true;
        return this;
    }
    public SomeDTOBuilder WithStrings(Action<ClassLibrary1.FluentBuilder.IListBuilder<string>> action, bool useObjectInitializer = true) => WithStrings(() =>
    {
        var builder = new ClassLibrary1.FluentBuilder.IListBuilder<string>();
        action(builder);
        return (System.Collections.ObjectModel.ReadOnlyCollection<string>) builder.Build(useObjectInitializer);
    });
    public SomeDTOBuilder WithoutStrings()
    {
        WithStrings(() => new List<string>());
        _stringsIsSet = false;
        return this;
    }


    public override SomeDTO Build(bool useObjectInitializer = true) { /*...*/ }
    public static SomeDTO Default() => new SomeDTO();
}

Looking at the FluentBuilder source, the issue seems to be that FluentBuilderGenerator.Extensions.TypeSymbolExtensions.GetDefault calls FluentBuilderGenerator.Extensions.TypeSymbolExtensions.GetFluentTypeKind to check for an adequate default initializer, but GetFluentTypeKind returns interface types found for implementing types, meaning that ReadOnlyCollection : System.Collections.Generic.IList<T> /*, ...*/ causes FluentTypeKind.IList to be returned from GetFluentTypeKind which then leads to new List<>() to be chosen as default initializer.

I think the 4 interface implementation checks in GetFluentTypeKind should only happen if typeSymbol.TypeKind == TypeKind.Interface like so:

public static FluentTypeKind GetFluentTypeKind(this ITypeSymbol typeSymbol)
{
    if (typeSymbol.SpecialType == SpecialType.System_String)
    {
        return FluentTypeKind.String;
    }

    if (typeSymbol.TypeKind == TypeKind.Array)
    {
        return FluentTypeKind.Array;
    }

    if (typeSymbol.TypeKind == TypeKind.Interface)
    {
        if (typeSymbol.ImplementsInterfaceOrBaseClass(typeof(IDictionary<,>)) || typeSymbol.ImplementsInterfaceOrBaseClass(typeof(IDictionary)))
        {
            return FluentTypeKind.IDictionary;
        }

        if (typeSymbol.ImplementsInterfaceOrBaseClass(typeof(IList<>)) || typeSymbol.ImplementsInterfaceOrBaseClass(typeof(IList)))
        {
            return FluentTypeKind.IList;
        }

        if (typeSymbol.ImplementsInterfaceOrBaseClass(typeof(ICollection<>)) || typeSymbol.ImplementsInterfaceOrBaseClass(typeof(ICollection)))
        {
            return FluentTypeKind.ICollection;
        }

        if (typeSymbol.AllInterfaces.Any(i => i.SpecialType == SpecialType.System_Collections_IEnumerable))
        {
            return FluentTypeKind.IEnumerable;
        }
    }

    return FluentTypeKind.Other;
}
StefH commented

@iam-mholle
This is solved in next release.