ListInitExpression from Expression list fails with multiple expression types.

[JonHanna]

Consider:

private class MixedAddable : IEnumerable
{
    private readonly List<string> _strings = new List<string>();
    private readonly List<int> _ints = new List<int>();

    public void Add(string value)
    {
        _strings.Add(value);
    }

    public void Add(int value)
    {
        _ints.Add(value);
    }

    public IEnumerator GetEnumerator()
    {
        return _strings.Concat(_ints.Select(i => i.ToString())).GetEnumerator();
    }
}

The likes of () => new MixedAddable { 1, "a", 2, "b" } works fine, but the following fails:

Expression.ListInit(
    Expression.New(typeof(MixedAddable)),
    Expression.Constant(0),
    Expression.Constant("a")
)

It's assumed that the correct Add method for the first expression is also correct for the rest, though this doesn't necessarily hold.

[JonHanna]

cc: @VSadov @bartdesmet

[bartdesmet]

Good catch! Maybe it was some kind of optimization to avoid having to resolve an Add method for each element.

[JonHanna]

I'd guess so. Resolving them each time is a waste for the most common cases, and I could see how that could have been the only case considered. In the PR I memoise the type and method so that for such common cases I only add an n - 1 checks that the type is the same, rather than n - 1 method resolutions.

[JonHanna]

Reading over the documentation, this is nearly as documented. The example given failing is as-documented, but it can be strange with some overloads; if there was an overload that took object and one that took string then it would be accepted if the first initializer wasn't a string but the second was, but the second would be passed to the object-taking overload.

To make the behaviour match the documentation we'd have to check subsequent initializers and throw in such a case.

It seems though that it wouldn't be hard to have the expressions match the behaviour the C# compiler uses in picking the Add to use, including also allowing params methods to be called. That would probably need to be considered an API change though.

[JonHanna]

Okay. The documented behaviour as per https://msdn.microsoft.com/en-us/library/bb340226%28v=vs.100%29.aspx suggests that this example is incorrect only in which exception is thrown. Because it says:

In order to use this overload of ListInit(NewExpression, Expression[]), newExpression.Type or its base type must declare a single method named "Add" (case insensitive) that takes exactly one argument. The type of the argument must be assignable from the type represented by the Type property of the first element of initializers.

In the exception list it says for IOE:

More than one argument-compatible method named "Add" (case-insensitive) exists on newExpression.Type and/or its base type.

This doesn't quite match (from the exception list alone we'd expect the call to work, as the two Add methods are mutually-exclusive in which types they accept), but the remarks section certainly suggests that an exception be thrown, and this is the better match.

private class MixedOverlapAssignableAddable : IEnumerable
{
    private readonly List<string> _strings = new List<string>();
    private readonly List<object> _objects = new List<object>();

    public void Add(string value)
    {
        _strings.Add(value);
    }

    public void Add(object value)
    {
        _objects.Add(value);
    }

    public IEnumerator GetEnumerator()
    {
        return _strings.Concat(_objects.Select(i => i.ToString())).GetEnumerator();
    }
}

Then there are some interesting things. The example expression still won't work, but probably shouldn't; while the Compiler dealing with () => new MixedOverlapAssignableAddable{0, "a"} should add an implicit Convert it's not as appropriate for direct use of the Expression class, as those implicit conversions are C# rules, not .NET or Linq.Expression rules.

However, consider that the following sort of works:

Expression.ListInit(
    Expression.New(typeof(MixedOverlapAssignableAddable)),
    Expression.Constant(0, typeof(object)),
    Expression.Constant("a")
);

This though calls the object-taking method with the string, not the string-taking method, as you likely be more appropriate. This example cannot be defended at all by reference to the documentation.

Conversely:

Expression.ListInit(
    Expression.New(typeof(MixedOverlapAssignableAddable)),
    Expression.Constant("a"),
    Expression.Constant(0, typeof(object))
);

Throws an IOE because it can't pick Add method to use with the string, though one is clearly a better match. (The one example that is exactly as documented).

To make the behaviour match the documentation, we would have to change the check for there being two or more unary Add methods that match the signature to be a check as to whether there were two or more unary Add methods at all. This though would have a backwards-compatibility burden, and also reduce available functionality for the sake of compatibility with the spec making it particularly hard to justify the change.

We could though perform a cut-down version of the overload resolution of ECMA 334, considering:

1. Extension methods don't have to be considered.
2. Whether a method is params or not doesn't have to be considered (again, the syntactic sugar hiding the array creation for calling params isn't something that should be implicit here.
3. We are only interested in unary methods.
4. The only implicit conversions we would be interested in would (as per the rest of Expressions) be assignable conversions.

Then the rules for picking the best overload come down to how many steps an argument type is from a parameter type, in terms of inheritance. In particular the only possible ties are with two equally-distance interfaces (since .NET has no multiple inheritance, if a type is inherited from two non-interface types it must be inherited from one via the other).

We would though have to accept private methods, but at a much lower priority than public methods, to maintain compatibility with what is current.

The effect would be more applicable. The only backwards-incompatible case would be like that where the initialisation of MixedOverlapAssignableAddable "worked" in a probably-incorrect way. It's likely any code that did hit this obscure case dependably (not sometimes throwing) is in fact not behaving as expected in any case.

[JonHanna]

Any attempt at this would introduce a burden in checking for the method repeatedly, but this could be memoised against the most common case of each type being the same; if the current argument type is the same as the previous, then the previous method can be used.

[VSadov]

Expression Trees do not need to match the complexities C# binder and overload resolution. For the most scenarios Expression Tree is an already fully bound tree and some factories leave some room for not specifying exact operator methods.
I do not think we should make this more complex than it needs to be. Definitely not to the level that matches C#.

From such prospective, is the current behavior acceptable? Are there workarounds to the tree creating code?

[JonHanna]

I've been thinking about this on and off since I raised this issue. I don't think there's any perfect solution, including leaving as-is.

As-is the behaviour is confusing, and at odds with the documentation which means the confusion won't be resolved by reading them. It's also confusing enough that updating the docs would still leave it confusing.

Moving to the documented behaviour would be a breaking change.

Fixing it as per my suggested behaviour above would also be a breaking change. It's less likely that code is affected, and a good chance that any such code is actually improved (picking a better overload that was in fact intended, as well as becoming robust against exceptions that could happen due to insertion order hitting the sequence that throws) but no guarantee of that. It is also relatively complicated to implement, which has its own obvious downsides for correctness.

If this was a beta project my vote would be for taking the overload out as a convenience overload that isn't quite as convenient as it seems, with implementing as documented my second preference.

In all, I think it should be changed to the more complex binding behaviour I suggest, but none of the options are perfect.