Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

C# 7.0 tuples - Invalid code generation for expression trees #12797

Closed
bartdesmet opened this issue Jul 28, 2016 · 5 comments
Closed

C# 7.0 tuples - Invalid code generation for expression trees #12797

bartdesmet opened this issue Jul 28, 2016 · 5 comments
Assignees
Milestone

Comments

@bartdesmet
Copy link

Repro steps:

C:\Projects\roslyn\Binaries\Debug>type e.cs
using System;
using System.Linq.Expressions;

class E
{
    static void Main()
    {
        Expression<Func<(int, int)>> f = () => new (int, int)(1, 2);
    }
}
C:\Projects\roslyn\Binaries\Debug>csc e.cs ..\..\src\Compilers\Test\Resources\Core\NetFX\ValueTuple\*.cs
Microsoft (R) Visual C# Compiler version 42.42.42.42
Copyright (C) Microsoft Corporation. All rights reserved.


C:\Projects\roslyn\Binaries\Debug>e

Unhandled Exception: System.ArgumentException: Cannot resolve method Void .ctor(Int32, Int32) because the declaring type of the method handle System.ValueTuple`2[T1,T2] is generic. Explicitly provide the declaring type to GetMethodFromHandle.
   at System.Reflection.MethodBase.GetMethodFromHandle(RuntimeMethodHandle handle)
   at E.Main()

C:\Projects\roslyn\Binaries\Debug>ildasm e.exe

Quick inspection shows that the generated IL code has:

  IL_0001:  ldtoken    method instance void valuetype System.ValueTuple`2<int32,int32>::.ctor(!0,
                                                                                              !1)
  IL_0006:  call       class [mscorlib]System.Reflection.MethodBase [mscorlib]System.Reflection.MethodBase::GetMethodFromHandle(valuetype [mscorlib]System.RuntimeMethodHandle)

This should call the GetMethodFromHandle overload with a RuntimeTypeHandle parameter that should be set to ldtoken valuetype System.ValueTuple2<int32,int32>`. In contrast, the following works fine:

        Expression<Func<(int, int)>> f = () => (1, 2);

It seems the difference is noticed here:

NamedTypeSymbol.AllTypeArgumentCount()
SyntheticBoundNodeFactory.GetMethodFromHandleMethod(...)
SyntheticBoundNodeFactory.ConstructorInfo(...)
ExpressionLambdaRewriter.VisitObjectCreationExpressionInternal(...)

That method returns 2 for the use of the tuple literal (1, 2) but 0 for the use of new (int, int)(1, 2). Looking higher up in the LocalRewriter it seems that VisitObjectCreationExpression receives the constructor of the tuple type but it never gets retargeted to the TupleUnderlyingType unlike what happens in the VisitTupleLiteral case.

@bartdesmet
Copy link
Author

At first sight, it seems a substitution a la node.Constructor.TupleUnderlyingMethod ?? node.Constructor inside the if (_inExpressionLambda) in VisitObjectCreationExpression can address the issue. It'd require a change from node.UpdateArgumentsAndInitializer to the full-blown node.Update to pass in the rewritten constructor.

@bartdesmet
Copy link
Author

I have a spot fix in my fork for expression tree futures @ bartdesmet@994f964. I haven't studied the code base extensively for the places where tuple types are retargeted to their underlying types, so the right fix may be in a different location. I'm planning to play around more with the intersection of expression trees and tuple types in the next few weeks.

@gafter
Copy link
Member

gafter commented Aug 1, 2016

The syntax new ( is to be forbidden.

@gafter gafter added the Bug label Aug 1, 2016
@gafter gafter added this to the 2.0 (RTM) milestone Aug 1, 2016
@bartdesmet
Copy link
Author

Great, that will fix it :-). Do we have an up-to-date document on the C# 7.0 features and their language surface, reflecting latest decisions? I'm basing my investigations on the docs/features MD files right now.

@VSadov
Copy link
Member

VSadov commented Oct 31, 2016

#10891 should make this problem unreachable since the code would be cause a compile error

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants