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

Generated code for Expression-tree instantiation crashes when it refers to ValueTuple<A,B>.Equals #27322

Closed
EamonNerbonne opened this issue Jun 1, 2018 · 4 comments · Fixed by #43518 or #43553
Assignees
Labels
Area-Compilers Bug Language-VB New Language Feature - Tuples Tuples Resolution-Fixed The bug has been fixed and/or the requested behavior has been implemented
Milestone

Comments

@EamonNerbonne
Copy link

EamonNerbonne commented Jun 1, 2018

Version Used: Various, including VS15.7.3 and VS15.8.0 preview2

Steps to Reproduce:

var tupleA = (1, 3);
var tupleB = (1, "123".Length);

Expression<Func<int>> ok1 = () => tupleA.Item1;
Expression<Func<int>> ok2 = () => tupleA.GetHashCode();
Expression<Func<Tuple<int, int>>> ok3 = () => tupleA.ToTuple();
Expression<Func<bool>> ok4 = () => Equals(tupleA, tupleB);
Expression<Func<int>> ok5 = () => Comparer<(int, int)>.Default.Compare(tupleA, tupleB);
ok1.Compile()(); //no crash.
ok2.Compile()(); //no crash.
ok3.Compile()(); //no crash.
ok4.Compile()(); //no crash.
ok5.Compile()(); //no crash.

Expression<Func<bool>> err1 = () => tupleA.Equals(tupleB);  //crash 1!
Expression<Func<int>> err2 = () => tupleA.CompareTo(tupleB);  //crash 2!

Expected Behavior:
The code runs without throwing.

Actual Behavior:

The above snippet crashes at line crash 1 with System.ArgumentException : Cannot resolve method Boolean Equals(System.ValueTuple`2[System.Int32,System.Int32]) because the declaring type of the method handle System.ValueTuple`2[T1,T2] is generic. Explicitly provide the declaring type to GetMethodFromHandle.

If you remove that line, it crashes at line crash 2 with System.ArgumentException : Cannot resolve method Int32 CompareTo(System.ValueTuple`2[System.Int32,System.Int32]) because the declaring type of the method handle System.ValueTuple`2[T1,T2] is generic. Explicitly provide the declaring type to GetMethodFromHandle.


Note (from @gafter):
This bug has been confirmed fixed in C# (#43518), but not in VB. See discussion below.

@EamonNerbonne
Copy link
Author

EamonNerbonne commented Jun 1, 2018

Perhaps helpful too are two decompiles, one using ValueTuple, and one using something similar, highlighting the difference in generated code:

Decompiles of a broken scenario and a working one:

//Broken: ValueTuple                                                          //Working - MyValueTuple                                                                
                                                                              
IL_00A3:  ldloc.0     // CS$<>8__locals0                                      IL_0031:  ldloc.0     // CS$<>8__locals0
IL_00A4:  ldtoken     UserQuery.<>c__DisplayClass0_0                          IL_0032:  ldtoken     UserQuery.<>c__DisplayClass0_0
IL_00A9:  call        System.Type.GetTypeFromHandle                           IL_0037:  call        System.Type.GetTypeFromHandle
IL_00AE:  call        System.Linq.Expressions.Expression.Constant             IL_003C:  call        System.Linq.Expressions.Expression.Constant
IL_00B3:  ldtoken     UserQuery+<>c__DisplayClass0_0.tupleA                   IL_0041:  ldtoken     UserQuery+<>c__DisplayClass0_0.myTupleA
IL_00B8:  call        System.Reflection.FieldInfo.GetFieldFromHandle          IL_0046:  call        System.Reflection.FieldInfo.GetFieldFromHandle
IL_00BD:  call        System.Linq.Expressions.Expression.Field                IL_004B:  call        System.Linq.Expressions.Expression.Field
IL_00C2:  ldtoken     System.ValueTuple<System.Int32,System.Int32>.Equals     IL_0050:  ldtoken     UserQuery+MyValueTuple<System.Int32,System.Int32>.Equals
                                                                              IL_0055:  ldtoken     UserQuery.MyValueTuple`2
IL_00C7:  call        System.Reflection.MethodBase.GetMethodFromHandle        IL_005A:  call        System.Reflection.MethodBase.GetMethodFromHandle
IL_00CC:  castclass   System.Reflection.MethodInfo                            IL_005F:  castclass   System.Reflection.MethodInfo
IL_00D1:  ldc.i4.1                                                            IL_0064:  ldc.i4.1    
IL_00D2:  newarr      System.Linq.Expressions.Expression                      IL_0065:  newarr      System.Linq.Expressions.Expression
IL_00D7:  dup                                                                 IL_006A:  dup         
IL_00D8:  ldc.i4.0                                                            IL_006B:  ldc.i4.0    
IL_00D9:  ldloc.0     // CS$<>8__locals0                                      IL_006C:  ldloc.0     // CS$<>8__locals0
IL_00DA:  ldtoken     UserQuery.<>c__DisplayClass0_0                          IL_006D:  ldtoken     UserQuery.<>c__DisplayClass0_0
IL_00DF:  call        System.Type.GetTypeFromHandle                           IL_0072:  call        System.Type.GetTypeFromHandle
IL_00E4:  call        System.Linq.Expressions.Expression.Constant             IL_0077:  call        System.Linq.Expressions.Expression.Constant
IL_00E9:  ldtoken     UserQuery+<>c__DisplayClass0_0.tupleA                   IL_007C:  ldtoken     UserQuery+<>c__DisplayClass0_0.myTupleA
IL_00EE:  call        System.Reflection.FieldInfo.GetFieldFromHandle          IL_0081:  call        System.Reflection.FieldInfo.GetFieldFromHandle
IL_00F3:  call        System.Linq.Expressions.Expression.Field                IL_0086:  call        System.Linq.Expressions.Expression.Field
IL_00F8:  stelem.ref                                                          IL_008B:  stelem.ref  
IL_00F9:  call        System.Linq.Expressions.Expression.Call                 IL_008C:  call        System.Linq.Expressions.Expression.Call
IL_00FE:  call        System.Array.Empty<ParameterExpression>                 IL_0091:  call        System.Array.Empty<ParameterExpression>
IL_0103:  call        System.Linq.Expressions.Expression.Lambda<Func`1>       IL_0096:  call        System.Linq.Expressions.Expression.Lambda<Func`1>
IL_0108:  stloc.2     // err21                                                IL_009B:  stloc.1     // ok

(Corresponding to the following linqpad snippet:)

void Main()
{
    var tupleA = (1, 3);
    var myTupleA = ToMyValueTuple(tupleA);
    tupleA.Dump();
    Expression<Func<bool>> ok = () => myTupleA.Equals(myTupleA);//no crash
    ok.Dump();
    Expression<Func<bool>> err21 = () => tupleA.Equals(tupleA);//crash
    err21.Dump();
    
}
static MyValueTuple<T1, T2> ToMyValueTuple<T1, T2>((T1, T2) tuple) => new MyValueTuple<T1, T2>(tuple);
public struct MyValueTuple<T1, T2> : IEquatable<MyValueTuple<T1, T2>> {
    public readonly T1 v1;
    public readonly T2 v2;
    public MyValueTuple((T1 v1, T2 v2) tuple) => (v1, v2) = tuple;
    public bool Equals(MyValueTuple<T1, T2> other) => Equals(v1, other.v1) && Equals(v2, other.v2);
}

In short, it's calling the wrong overload of GetMethodFromHandle (the one for non-generic handles, instead of the one for a method handle with a generic type handle).

@EamonNerbonne
Copy link
Author

Possibly related: #12897 and #12797

@gafter
Copy link
Member

gafter commented Jun 1, 2018

This is likely a symptom of #20648

@gafter gafter modified the milestones: 15.8, 16.0 Jun 4, 2018
@jinujoseph jinujoseph modified the milestones: 16.0, 16.3 Jun 9, 2019
@jcouv jcouv modified the milestones: 16.3, Compiler.Next Jul 11, 2019
@gafter gafter self-assigned this Apr 17, 2020
gafter pushed a commit to gafter/roslyn that referenced this issue Apr 20, 2020
gafter pushed a commit that referenced this issue Apr 21, 2020
* Add a test to verify that a bug is fixed.
Fixes #27322 (C# only)
VB fix depends on #20648
@gafter gafter reopened this Apr 21, 2020
@gafter
Copy link
Member

gafter commented Apr 21, 2020

@AlekseyTs wrote in #43518 (review)

It doesn't look like VB fix really depends on #20648. There is a safe and simple fix that we can make in CodeGenerator.EmitMethodInfoExpression by adjusting the target method symbol as follows:

            Dim method As MethodSymbol = node.Method

            If method.IsTupleMethod Then
                method = method.TupleUnderlyingMethod
            End If

Please add the fix and the following test:

        <Fact>
        Public Sub Issue27322()
            Dim source0 = "
Imports System
Imports System.Collections.Generic
Imports System.Linq.Expressions

Module Module1

    Sub Main()
        Dim tupleA = (1, 3)
        Dim tupleB = (1, ""123"".Length)

        Dim ok1 As Expression(Of Func(Of Integer)) = Function() tupleA.Item1
        Dim ok2 As Expression(Of Func(Of Integer)) = Function() tupleA.GetHashCode()
        Dim ok3 As Expression(Of Func(Of Tuple(Of Integer, Integer))) = Function() tupleA.ToTuple()
        Dim ok4 As Expression(Of Func(Of Boolean)) = Function() Equals(tupleA, tupleB)
        Dim ok5 As Expression(Of Func(Of Integer)) = Function() Comparer(Of (Integer, Integer)).Default.Compare(tupleA, tupleB)
        ok1.Compile()()
        ok2.Compile()()
        ok3.Compile()()
        ok4.Compile()()
        ok5.Compile()()

        Dim err1 As Expression(Of Func(Of Boolean)) = Function() tupleA.Equals(tupleB)
        Dim err2 As Expression(Of Func(Of Integer)) = Function() tupleA.CompareTo(tupleB)

        err1.Compile()()
        err2.Compile()()

        System.Console.WriteLine(""Done"")
    End Sub

End Module
"

            Dim comp1 = CreateCompilation(source0, options:=TestOptions.DebugExe)
            CompileAndVerify(comp1, expectedOutput:="Done")
        End Sub

@AlekseyTs AlekseyTs added the Resolution-Fixed The bug has been fixed and/or the requested behavior has been implemented label Apr 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Compilers Bug Language-VB New Language Feature - Tuples Tuples Resolution-Fixed The bug has been fixed and/or the requested behavior has been implemented
Projects
None yet
5 participants