Fix long tuples in PatternLocalRewriter

[Youssef1313]

Fixes #47878

To explain the changes:

loweredInput.Arguments can have at max 8 arguments, with the 8th argument being another tuple. This is due to the available ValueTuples in the runtime:

I fixed the loop to have the exact number of items (which can be greater than 8), and introduced a static local function which gets the BoundExpression for the corresponding index.

1. When index < 7, just return Arguments[index].
2. When index >= 7, gets the corresponding TRest tuple, and apply the same logic for index - 7.

[Youssef1313]

For tuple containing > 7 elements (10 elements for example), the previous value of count will be 8 (the TRest position) instead of 10, causing the loop after a few lines to run a fewer iteration than needed.

[Youssef1313]

Here, we can't just access Arguments[i] for two reasons:

1. If i == 8, the result expression will be a tuple, this is not intended.
2. If i > 8, this will throw an exception due to the other fix I made.

[333fred]

@gafter if you are available, we'd appreciate a review 😄

[333fred]

Please split out this test and verify the IL. I don't think we need to verify IL on all of these variations, but it would be good to have 1 of them verified.

[Youssef1313]

@333fred I feel it's safer to verify the IL for different cases because in the past, the compiler had different behavior between the 8 elements case, and the > 8 elements case.

I'd at least verify the IL for 2 cases:

• 8 elements
• Greater than 8 elements

[333fred]

Done review pass (commit 3).

[333fred]

Also, it looks like another test is failing with this change.

[Youssef1313]

Also, it looks like another test is failing with this change.

I'll give it a look and try to fix that with the test changes you suggested.

[jcouv]

Done with review pass (iteration 3). Minor comments only

[Youssef1313]

Also, it looks like another test is failing with this change.

With the change I introduced, GetArgumentRefKind method is now being called with indices > 7, which sounds correct to me. It just needs a similar logic to the static local function getExpression that I introduced. One small problem is where to define this logic to avoid duplicating it in two places.

Another problem is that I'm unsure of "who" calls GetArgumentRefKind. Is it used for things other than tuples (i.e. normal method parameters) in which case things may get a bit more complex (opened #47911 in a hope that test failures will answer).

Also I spotted an assertion that is now passing just "by luck":

        private void EmitArguments(ImmutableArray<BoundExpression> arguments, ImmutableArray<ParameterSymbol> parameters, ImmutableArray<RefKind> argRefKindsOpt)
        {
            // We might have an extra argument for the __arglist() of a varargs method.
            Debug.Assert(arguments.Length == parameters.Length || arguments.Length == parameters.Length + 1, "argument count must match parameter count");
            Debug.Assert(parameters.All(p => p.RefKind == RefKind.None) || !argRefKindsOpt.IsDefault, "there are nontrivial parameters, so we must have argRefKinds");
            Debug.Assert(argRefKindsOpt.IsDefault || argRefKindsOpt.Length == arguments.Length, "if we have argRefKinds, we should have one for each argument");

            for (int i = 0; i < arguments.Length; i++)
            {
                RefKind argRefKind = GetArgumentRefKind(arguments, parameters, argRefKindsOpt, i);
                EmitArgument(arguments[i], argRefKind);
            }
        }

This assertion:

            // We might have an extra argument for the __arglist() of a varargs method.
            Debug.Assert(arguments.Length == parameters.Length || arguments.Length == parameters.Length + 1, "argument count must match parameter count");

Assumes a max difference of 1 between arguments length and parameters length.

Because the corresponding test uses a tuple of 9 elements, and arguments is 8 (where the 8th argument is a tuple itself holding the 8th and 9th values). the assertion passes. But if it's a tuple of 10 elements. arguments.Length would still be 8 and the assertion will fail.

Given that, I'm now less confident with the change I made. It breaks some existing assumptions. I could just remove the assertion in EmitArguments, but I'm afraid this breaks something.

[333fred]

One small problem is where to define this logic to avoid duplicating it in two places.

Consider doing this as a static method on TupleTypeSymbol. I'll take a look at the code in EmitArguments a bit more.

[Youssef1313]

One small problem is where to define this logic to avoid duplicating it in two places.

Consider doing this as a static method on TupleTypeSymbol. I'll take a look at the code in EmitArguments a bit more.

The existing code needs to get a BoundExpression from BoundObjectCreationExpression based on the index. I'm not sure if I can achieve the same if the extension method is on TupleTypeSymbol.

I found a class named BoundExpressionExtensions, looks like a good place for this extension method.

For the failing assertions, I think I may surround them with an if condition so they are executed for non-tuple parameters only.

[333fred]

            // We might have an extra argument for the __arglist() of a varargs method.
            Debug.Assert(arguments.Length == parameters.Length || arguments.Length == parameters.Length + 1, "argument count must match parameter count");

Perhaps the right thing to do is break this assert out into a conditional method that can verify the right thing. It would verify that:

• arguments.Length == parameters.Length in the general case (and the last parameter is not an __arglist)
• arguments.Length == parameters.Length + 1 when the last parameter is an __arglist
• The tuple logic as appropriate.

You'll have to surround such a method if #if DEBUG blocks for now, since we have a bug with Conditional that isn't going to be fixed in the toolset until RC2 (iirc).

[Youssef1313]

Actually I think I was incorrect when I said:

With the change I introduced, GetArgumentRefKind method is now being called with indices > 7, which sounds correct to me.

This isn't correct. ValueTuple constructor have at most 8 arguments. Even if the elements are greater than that, it doesn't make sense to call ArgumentRefKind with index > 7. So GetArgumentRefKind should be kept as it is with the assertion in it.

The callers & caller's assertion needs some tweaks. Or even I think the original fix shouldn't cascade like this.

[RikkiGibson]

GitHub keeps presenting my name at the top of the completion list, then the "remove all assignees" button pops in..

[Youssef1313]

One thing to notice, when this was working in past, the generated code was using pointers in an unsafe context.

The change in this PR tries to make the code works again, but in non-unsafe fashion.

But I can't get it to work correctly.

@RikkiGibson @jcouv Are you aware of any PRs in the past that was around making the generated code non-unsafe?

[RikkiGibson]

@Youssef1313 I messaged you on gitter, let's figure out the answer to that and how to move forward.

[RikkiGibson]

Summary of the situation:

Pattern matching of tuples of more than 8 elements (in this scenario at least) has not worked for some time. At least not since the 16.5 time frame. For a while, pattern matching tuples with exactly 8 elements has worked, but later the semantics of the emitted code were broken.

Perhaps we should just look at the up-to-date implementation and try to fix it to work with indefinitely many elements, perhaps using a "too many locals" or similar diagnostic if we hit a meaningful limit.

[Youssef1313]

Is the IL for this test and the next test correct?

[Youssef1313]

@RikkiGibson Looks like it should be working now. But my changes removed a whole array that used to exist, which makes me a little bit less confident. Can this change break some scenario that is currently not tested?

[RikkiGibson]

I'm confused by the change to get rid of the newArguments array. I thought that would have undesired effects, particularly related to reuse of user temps or side effects.

[Youssef1313]

@RikkiGibson I was expecting to see some test failures related to this too, but I don't know what scenarios exactly that should fail. Do you know any scenarios that should fail to add tests for them?

[RikkiGibson]

I tried the following scenario to induce a failure but it continued to have the expected side-effects in your branch:

        [Fact, WorkItem(47878, "https://github.com/dotnet/roslyn/issues/47878")]
        public void VerifyIL_8ElementsTuple_SideEffects()
        {
            var text = @"
using System;
public class C
{
    public static void Main()
    {
        int x = (M(1), M(2), M(3), M(4), M(5), M(6), M(7), M(8)) switch
        {
            (1, 2, 3, 4, 5, 6, 7, 7) => 1,
            (1, 2, 3, 4, 5, 6, 7, 8) => 1,
            _ => -1,
        };

        Console.WriteLine(x);
    }

    static int M(int x)
    {
        Console.Write(x);
        return x;
    }
}
";
            CompileAndVerify(text, expectedOutput: "123456781");
        }

I think the next thing I would try would be to make many of the tuple components local variables and see if the lowering code reuses the local variables properly instead of introducing new ones.

I'll try to debug in more detail soon.

[RikkiGibson]

Here's the scenario where we have unexpected side effects:

        [Fact, WorkItem(47878, "https://github.com/dotnet/roslyn/issues/47878")]
        public void VerifyIL_8ElementsTuple_SideEffects_02()
        {
            var text = @"
using System;
using System.Runtime.CompilerServices;
public class C
{
    public static void Main()
    {
        try
        {
            int x = (M(1), M(2), M(3), M(4), M(5), M(6), M(7), M(8)) switch
            {
                (1, 2, 3, 4, 5, 6, 7, 7) => 1,
            };

            Console.WriteLine(x);
        }
        catch (SwitchExpressionException ex)
        {
            Console.Write(""💥"");
        }
    }

    static int M(int x)
    {
        Console.Write(x);
        return x;
    }
}
namespace System.Runtime.CompilerServices
{
    public class SwitchExpressionException : InvalidOperationException
    {
        public SwitchExpressionException() {}
        public SwitchExpressionException(object unmatchedValue) => UnmatchedValue = unmatchedValue;
        public object UnmatchedValue { get; }
    }
}
";
            CompileAndVerify(text, expectedOutput: "12345678💥");
        }

With your change, the actual output is 1234567812345678💥. Let's revert that part of the change.

[Youssef1313]

@RikkiGibson That will get us to the assertion failure too. I'm still not sure what is the appropriate fix. I'll add this test and give it a try again, but not sure if I would be able to get the correct fix.

[RikkiGibson]

I think the problem is that when we "unwrap" the tuple elements as is done in this PR, the length of the arguments no longer matches the length of the arguments ot the top-level object creation node. i.e. loweredInput.Arguments.Length should never be more than 8, and tuples of size 8 or larger must nest their arguments in additional object creation nodes.

I can try and look a bit to see how to do this--the fix isn't coming to mind instantly. But to get the behavior of the scenario fixed in 16.8 we may want to take #48028 and then optimize it later.

[RikkiGibson]

I will move this back to draft status. Feel free to mark it ready for review again if you want us to take another look later.

[Youssef1313]

Closing this stale PR.