Fix long tuples in PatternLocalRewriter #47893
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -488,30 +488,47 @@ private BoundDecisionDag RewriteTupleInput( | |
| bool canShareInputs, | ||
| out BoundExpression savedInputExpression) | ||
| { | ||
| var tupleElements = loweredInput.Type.TupleElements; | ||
|
|
||
| // first evaluate the inputs (in order) into temps | ||
| var originalInput = BoundDagTemp.ForOriginalInput(loweredInput.Syntax, loweredInput.Type); | ||
| for (int i = 0; i < tupleElements.Length; i++) | ||
| { | ||
| var field = tupleElements[i].CorrespondingTupleField; | ||
| Debug.Assert(field != null); | ||
| var expr = getExpression(i, loweredInput); | ||
| var fieldFetchEvaluation = new BoundDagFieldEvaluation(expr.Syntax, field, originalInput); | ||
| var temp = new BoundDagTemp(expr.Syntax, expr.Type, fieldFetchEvaluation); | ||
| storeToTemp(temp, expr); | ||
| } | ||
|
|
||
| var rewrittenDag = decisionDag.Rewrite(makeReplacement); | ||
| savedInputExpression = loweredInput.Update( | ||
| loweredInput.Constructor, arguments: loweredInput.Arguments, loweredInput.ArgumentNamesOpt, loweredInput.ArgumentRefKindsOpt, | ||
|
There was a problem hiding this comment. I'm confused by the change to get rid of the There was a problem hiding this comment. @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? There was a problem hiding this comment. 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. There was a problem hiding this comment. 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 There was a problem hiding this comment. @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. There was a problem hiding this comment. 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. 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. |
||
| loweredInput.Expanded, loweredInput.ArgsToParamsOpt, loweredInput.ConstantValueOpt, | ||
| loweredInput.InitializerExpressionOpt, loweredInput.BinderOpt, loweredInput.Type); | ||
|
|
||
| return rewrittenDag; | ||
|
|
||
| static BoundExpression getExpression(int index, BoundObjectCreationExpression expression) | ||
jcouv marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| { | ||
| while (index >= NamedTypeSymbol.ValueTupleRestIndex) | ||
| { | ||
| if (expression.Arguments[NamedTypeSymbol.ValueTupleRestIndex] is BoundObjectCreationExpression restExpression) | ||
| { | ||
| expression = restExpression; | ||
| index -= NamedTypeSymbol.ValueTupleRestIndex; | ||
| } | ||
| else | ||
| { | ||
| // ValueTupleRestIndex should always be another tuple. Hence, it should always have BoundObjectCreationExpression. | ||
| throw ExceptionUtilities.Unreachable; | ||
| } | ||
| } | ||
|
|
||
| return expression.Arguments[index]; | ||
| } | ||
|
|
||
| void storeToTemp(BoundDagTemp temp, BoundExpression expr) | ||
| { | ||
| if (canShareInputs && (expr.Kind == BoundKind.Parameter || expr.Kind == BoundKind.Local) && _tempAllocator.TrySetTemp(temp, expr)) | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here, we can't just access Arguments[i] for two reasons: