Expression.New has inconsistent argument checking for the declaring type of the ctor

[hughbe]

There is an inconsistency between the checking of the constructor parameter in various Expression.New() overloads. This is when the constructor has a GenericTypeDefinition
This was discovered in dotnet/corefx#10952, and this is demonstrated below.

Problem

[Fact]
public static void ConstructorDeclaringType_GenericTypeDefinition_ThrowsArgumentException()
{
    ConstructorInfo constructor = typeof(GenericClass<>).GetConstructor(new Type[0]);

    Assert.Throws<ArgumentException>("constructor", () => Expression.New(constructor));
    Assert.Throws<ArgumentException>("constructor", () => Expression.New(constructor, new Expression[0]));
    Assert.Throws<ArgumentException>("constructor", () => Expression.New(constructor, (IEnumerable<Expression>)new Expression[0]));

}

[Fact]
public static void ConstructorDeclaringType_GenericTypeDefintion_Works()
{
    // Should probably throw an ArgumentException, similar to other overloads
    ConstructorInfo constructor = typeof(GenericClass<>).GetConstructor(new Type[0]);

    Expression.New(constructor, new Expression[0], new MemberInfo[0]);
    Expression.New(constructor, new Expression[0], new MemberInfo[0]);
}

Solution

The fix is to add the following code to New(ConstructorInfo, IEnumerable<Expression>, IEnumerable<MemberInfo>) so that it is consistent with New(ConstructorInfo) and New(ConstructorInfo, IEnumerable<Expression>)

ContractUtils.RequiresNotNull(constructor.DeclaringType, nameof(constructor) + "." + nameof(constructor.Decl
TypeUtils.ValidateType(constructor.DeclaringType, nameof(constructor));

This would be a breaking change, but is a bug, as an InvalidOperationException is thrown when compiling a lambda expression containing the New expression.

/cc @stephentoub @JonHanna @VSadov

[bartdesmet]

Good catch. This may be a good candidate for a fix, especially because the behavior only differs for the most complex overload. While users could construct an invalid expression today (and never compile it), this case seems rare.

IIRC, we saw a similar issue for New a while back with the ability to pass in a constructor for an abstract class, which also doesn't compile. In that case, we decided not to make any changes to the exception behavior of factory methods because of compatibility arguments.

[hughbe]

I'll send in a PR once dotnet/corefx#10952 is merged. It seems to me that System.Linq.Expressions has a more lenient breaking change policy than other libraries - is this correct?