-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Do not store temp of ref conditional operator by value #74116
base: main
Are you sure you want to change the base?
Conversation
src/Compilers/CSharp/Portable/BoundTree/BoundExpressionExtensions.cs
Outdated
Show resolved
Hide resolved
src/Compilers/CSharp/Portable/BoundTree/BoundExpressionExtensions.cs
Outdated
Show resolved
Hide resolved
It looks like there are no tests. |
Done with review pass (commit 2) |
@AlekseyTs do you have an idea for a test? |
The scenarios in the issue being fixed should be included as tests, for starters. |
src/Compilers/CSharp/Portable/BoundTree/BoundExpressionExtensions.cs
Outdated
Show resolved
Hide resolved
src/Compilers/CSharp/Portable/BoundTree/BoundExpressionExtensions.cs
Outdated
Show resolved
Hide resolved
src/Compilers/CSharp/Portable/BoundTree/BoundExpressionExtensions.cs
Outdated
Show resolved
Hide resolved
src/Compilers/CSharp/Portable/BoundTree/BoundExpressionExtensions.cs
Outdated
Show resolved
Hide resolved
Done with review pass (commit 5) |
@AlekseyTs can you write a test or help me to do so for the linked issue. I explain what I would want to test: compile and execute the following code: string str = "a2";
int x = 1;
int y = 2;
ref int r =
ref str is "whatever" ? ref x
: ref str is { Length: >= 2 and <= 10 or 22 } ? ref y
: ref System.Runtime.CompilerServices.Unsafe.NullRef<int>();
r++;
r++;
r++;
int xxx = r;
System.Console.WriteLine(xxx); //5
System.Console.WriteLine(x); //1
System.Console.WriteLine(y); //expected 5 - but we get 2 It should print (because
currently it prints:
And it already works if no "Or Pattern" is used (without The reason is because the alternative ref str is { Length: >= 2 and <= 10 or 22 } ? ref y
: ref System.Runtime.CompilerServices.Unsafe.NullRef<int>() needs spilling and stores its result in a temporary by value. |
For writing a test, take a look at |
I added the following test method: [Fact]
public void TestRefConditionalWithSpilling()
{
var source = """
class C
{
public static void Main()
{
string str = "a2";
int x = 1;
int y = 2;
int z = 777;
ref int r =
ref str is "whatever" ? ref x
: ref str is { Length: >= 2 and <= 10 or 22 } ? ref y
: ref z;
r++;
r++;
r++;
int xxx = r;
System.Console.WriteLine(xxx); //5
System.Console.WriteLine(x); //1
System.Console.WriteLine(y); //expected 5 - but we get 2
}
}
""";
var comp = CompileAndVerify(source,
expectedOutput: """
5
1
5
""");
comp.VerifyDiagnostics();
} But if I try to execute it, I get:
Do you know why? I got it: it must be a ref-assignment... |
verify: Verification.Skipped, removed
@AlekseyTs can you review again? |
//or the alternative is ref readonly otherwise it just by ref (not readonly) | ||
// | ||
//without this check we loose the readonly specifier | ||
//See Binder.CheckValueKind |
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.
I am not sure if the reference to Binder.CheckValueKind
is helpful. The new helper is used after initial binding, so I doubt we are going to call Binder.CheckValueKind
after that. I also glanced at the Binder.CheckValueKind
method and nothing stood out to me as an obvious explanation of why what we are doing here is important.
// | ||
//without this check we loose the readonly specifier | ||
//See Binder.CheckValueKind | ||
if (tryGetReadOnlyRefKind(conditionalOperator.Consequence) is { } readOnlyRefKindOfConsequence) |
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.
Do we have a test that is going to fail when this if
statement is removed?
{ | ||
return readOnlyRefKindOfConsequence; | ||
} | ||
if (tryGetReadOnlyRefKind(conditionalOperator.Alternative) is { } readOnlyRefKindOfAlternative) |
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.
Do we have a test that is going to fail when this if
statement is removed?
case RefKindExtensions.StrictIn: | ||
return RefKind.RefReadOnly; | ||
case RefKind.None: | ||
return null; //could be e.g. a throw expression |
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.
Can a throw
expression be used in a ref
conditional operator?
return null; //could be e.g. a throw expression | ||
case var unhandled: | ||
Debug.Assert(false, $"unhandled refKind: {unhandled}"); | ||
return RefKind.RefReadOnly; |
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.
I do not think we should have a default
case like this. Consider using a switch expression without "default" instead. If it is not exhaustive because a named enum value is not handled, the switch expression is going to produce a warning like
warning CS8509: The switch expression does not handle all possible values of its input type (it is not exhaustive). For example, the pattern 'RefKind.In' is not covered.
Otherwise, it is going to produce a warning like following:
warning CS8524: The switch expression does not handle some values of its input type (it is not exhaustive) involving an unnamed enum value. For example, the pattern '(RefKind)6' is not covered.
We should suppress CS8524 with pragma
return null; | ||
case RefKind.RefReadOnlyParameter: | ||
case RefKind.RefReadOnly: //same as RefKind.In | ||
case RefKindExtensions.StrictIn: |
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.
In some cases RefKindExtensions.StrictIn
is actually handled differently by comparison to RefKind.RefReadOnly. I think we should return RefKindExtensions.StrictIn
for it and, in case of an ambiguity between RefKindExtensions.StrictIn
and RefKind.RefReadOnly
across branches, we should prefer RefKindExtensions.StrictIn
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.
Probably always using RefKindExtensions.StrictIn
instead of RefKind.RefReadOnly
is a better alternative.
/// if the expression is a "by-ref" expression like conditional operator or InlineArrayAccess the RefKind is calculated and returned | ||
/// otherwise this function returns RefKind.None | ||
/// </summary> | ||
public static RefKind GetRefKindEx(this BoundExpression node) |
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.
I suggest to move this helper to SpillSequenceSpiller.cs right after VisitConditionalOperator
, it doesn't have to be an extension method.
@@ -1015,6 +1015,241 @@ .maxstack 1 | |||
"); | |||
} | |||
|
|||
[Fact] | |||
public void TestRefConditionalWithSpilling() |
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.
Please add a WorkItem
attribute.
ref readonly int r = | ||
ref str is "whatever" ? ref roy | ||
: ref str is { Length: >= 2 and <= 10 or 22 } ? ref y | ||
: ref System.Runtime.CompilerServices.Unsafe.NullRef<int>(); |
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.
It looks like this test is failing in CI because Unsafe
type isn't available in TargetFramework.NetLatest
when the test is executed on Desktop framework. Consider using TargetFramework.NetCoreApp
instead and conditionally supplying expected output:
expectedOutput: !ExecutionConditionUtil.IsMonoOrCoreClr ? null : """
5
1
5
""");
var tmp = _F.SynthesizedLocal(node.Type, kind: SynthesizedLocalKind.Spill, syntax: _F.Syntax); | ||
var isRef = node.IsRef; | ||
var tmp = _F.SynthesizedLocal(node.Type, kind: SynthesizedLocalKind.Spill, syntax: _F.Syntax, | ||
refKind: isRef ? node.GetRefKindEx() : RefKind.None |
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.
It looks like this change introduces a compiler crash for a scenario that worked fine before:
[Fact]
public void Test1()
{
var src1 = @"
using System.Threading.Tasks;
class C
{
int F;
static async Task Test(bool b)
{
Test(b ? ref (await GetC()).F : ref (await GetC()).F, await GetC());
}
static void Test(int x, C y) {}
static async Task<C> GetC()
{
await Task.Yield();
return new C();
}
}
";
var comp1 = CreateCompilation(src1, options: TestOptions.ReleaseDll);
comp1.VerifyEmitDiagnostics();
}
This scenario also crashes compiler:
[Fact]
public void Test2()
{
var src1 = @"
using System.Threading.Tasks;
class C
{
int F;
static async Task Test(bool b)
{
Test(ref b ? ref (await GetC()).F : ref (await GetC()).F, await GetC());
}
static void Test(ref int x, C y) {}
static async Task<C> GetC()
{
await Task.Yield();
return new C();
}
}
";
var comp1 = CreateCompilation(src1, options: TestOptions.ReleaseDll);
comp1.VerifyEmitDiagnostics();
}
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.
I suggest trying an alternative rewrite here, a rewrite that doesn't introduce ref locals. Instead of creating a local for result, creating a local for condition might work:
<condition side-effects>
bool conditionValue = condition;
if (conditionValue)
{
<consequence side-effects>
}
else
{
<alternative side-effects>
}
return conditionValue ? [ref] consequence : [ref] alternative;
Done with review pass (commit 11) |
Fixes #74115