Always use a by-value temp to capture a reference type receiver

[jcouv]

Relates to #73501

Baseline for test InterpolatedStringHandlerArgumentAttribute_ThisParameter_ReferenceTypeReferenceReceiver is added in commit 1 and updated in commit 2.
The main change in that test is V_3 is now of type C instead of type C&, and it doesn't need to be dereferenced with ldind.ref when that variable is used, just before the invocation of CustomHandler..ctor(int, int, C). Instead it happens during the initialization of the variable.

Fixes #73667

[AlekseyTs]

ref otherC

I do not think this scenario is interesting. I.e. redirecting c2 to a different location is not interesting because the old implementation was capturing the reference and changing that reference couldn't have any effect afterwards. What would be interesting is a change to a value that the reference points to, c2 = otherC. After making this change to the test scenarios in this unit-test and the unit-test below, we will be able to observe that the implementation change is not merely an optimization, but actually a fix for a real problem. Scenario from InterpolatedStringHandlerArgumentAttribute_ThisParameter_ReferenceTypeReferenceReceiver_ReverseOrder is going to crash at runtime without the implementation change and with the implementation change it is going to exhibit expected behavior. Therefore, I suggest opening an issue with a "broken" repro, adjusting description of this PR to say that we are attempting to fix that issue, and adjusting the added unit-tests accordingly. #Closed

[AlekseyTs]

I would even suggest to simplify the scenario by using c2 = default and getting rid of otherC local

[AlekseyTs]

Also, please add tests for generic scenarios like:

            var code = $$"""
using System;
using System.Runtime.CompilerServices;

class Program
{
    static void Main()
    {
        C c = new C(5);
        Test(ref c);
    }

    static void Test<T>(ref T c2) where T : I1
    {
        c2.M(c2 = default, {{expression}});
    }
}

public interface I1
{
    int Prop { get; }
    void M(I1 cNull, [InterpolatedStringHandlerArgumentAttribute("")] CustomHandler c);
}

public class C : I1
{
    public int Prop { get; }
    public C(int i) => Prop = i;
    public void M(I1 cNull, [InterpolatedStringHandlerArgumentAttribute("")] CustomHandler c)
    {
        Console.WriteLine(cNull is null);
        Console.WriteLine(c.ToString());
    }
}

public partial struct CustomHandler
{
    public CustomHandler(int literalLength, int formattedCount, I1 c) : this(literalLength, formattedCount)
    {
        _builder.AppendLine("c.Prop:" + c.Prop.ToString());
    }
}
""";

and

            var code = $$"""
using System;
using System.Runtime.CompilerServices;

class Program
{
    static void Main()
    {
        C c = new C(5);
        Test(ref c);
    }

    static void Test<T>(ref T c2) where T : class, I1
    {
        c2.M(c2 = default, {{expression}});
    }
}

public interface I1
{
    int Prop { get; }
    void M(I1 cNull, [InterpolatedStringHandlerArgumentAttribute("")] CustomHandler c);
}

public class C : I1
{
    public int Prop { get; }
    public C(int i) => Prop = i;
    public void M(I1 cNull, [InterpolatedStringHandlerArgumentAttribute("")] CustomHandler c)
    {
        Console.WriteLine(cNull is null);
        Console.WriteLine(c.ToString());
    }
}

public partial struct CustomHandler
{
    public CustomHandler(int literalLength, int formattedCount, I1 c) : this(literalLength, formattedCount)
    {
        _builder.AppendLine("c.Prop:" + c.Prop.ToString());
    }
}
""";

[AlekseyTs]

Done with review pass (commit 2)

[AlekseyTs]

LGTM (commit 4)

[jcouv]

@dotnet/roslyn-compiler for second review. Thanks

[jcouv]

@dotnet/roslyn-compiler for second review. Thanks

[cston]

!rewrittenReceiver.Type.IsReferenceType &&

Is this always true now?

[cston]

[Theory]

Consider adding [WorkItem] attribute to this test and the type parameter test that hit the bug. #Resolved

[jcouv]

Copying comment from extensions/receiver PR in case it is useful:

            // If we need to capture a receiver (for compound assignment, null-coalescing assignment, extension receiver adjustment, interpolation `this` capture, ...)
            // we need to make sure the capturing temp preserves the semantics of the receiver.
            //
            // For instance:
            //   Struct s = ...;
            //   s.Method(s.field = 42)
            // must be rewritten as:
            //   ref Struct temp = ref s;
            //   temp.Method(s.field = 42)
            // so that Method can have and observe the proper side-effects on the struct.
            //
            // Similarly:
            //   Class c = ...;
            //   ref Class c2 = ref c;
            //   Class otherC = null;
            //   c2.Method(c2 = ref otherC)
            // must be rewritten as:
            //   Class temp = c2; // dereferences to c
            //   temp.Method(c2 = ref otherC)
``` #Resolved

[cston]

[Theory]

Consider adding [WorkItem] attribute to this test. #Resolved

[cston]

LGTM. Minor suggestions only.

[cston]

WorkItem("#73631")

Should the link be "issues/73667" instead? #Resolved