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

Bug: Conditional Operator stores ref result in a non-ref-temporary (Only when SpillSequenceSpiller is used - e.g. for async/await) #74115

Open
bernd5 opened this issue Jun 23, 2024 · 2 comments · May be fixed by #74116
Assignees
Milestone

Comments

@bernd5
Copy link
Contributor

bernd5 commented Jun 23, 2024

Version Used: 847d588

Steps to Reproduce:

Compile and execute the following code:

using System;
using System.Threading.Tasks;
_ = Foo.NumberBuggy().Result;
Console.WriteLine("--------");
_ = Foo.NumberNotBuggy();

public class Foo{
     public static async Task<int> NumberBuggy()
     {
        string str = "a2";
        int x = 1;
        int y = 2;

        ref int r =
              ref await EvalAsync(str, 1) is "a1" ? ref x
            : ref await EvalAsync(str, 2) is "a2" ? ref y
            : ref System.Runtime.CompilerServices.Unsafe.NullRef<int>();
        
         r++;
         r++;
         r++;
         int xxx = r;
         System.Console.WriteLine(xxx);
         System.Console.WriteLine(x);
         System.Console.WriteLine(y); //should be 5 now!
         return xxx;
    }
    
    public static ValueTask<int> NumberNotBuggy()
     {
        string str = "a2";
        int x = 1;
        int y = 2;

        ref int r =
              ref EvalAsync(str, 1).Result is "a1" ? ref x
            : ref EvalAsync(str, 2).Result is "a2" ? ref y
            : ref System.Runtime.CompilerServices.Unsafe.NullRef<int>();
        
         r++;
         r++;
         r++;
         int xxx = r;
         System.Console.WriteLine(xxx);
         System.Console.WriteLine(x);
         System.Console.WriteLine(y);
         return new(xxx);
    }
    static ValueTask<string> EvalAsync(string s, int i)
    {
        System.Console.WriteLine($"{s} {i}");
        return ValueTask.FromResult(s);
    }
}

Expected Behavior:
Same output for both methods (they are not really async)

Actual Behavior:
The result of the conditional expression is stored in a non-ref temp local which causes that the later increment operations do not change the original value - just the temp.

Error cause
The error is caused in SpillSequenceSpiller

The code should be:

var tmp = _F.SynthesizedLocal(node.Type, kind: SynthesizedLocalKind.Spill, syntax: _F.Syntax,
   refKind: node.IsRef ? RefKind.Ref : RefKind.None);

But how can we determine if the result should be ref-readonly or not?

@dotnet-issue-labeler dotnet-issue-labeler bot added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels Jun 23, 2024
@bernd5 bernd5 changed the title Bug: Conditional Expression stores ref result in non ref local temp (Only when Spillseq Bug: Conditional Expression stores ref result in non ref local temp (Only when SpillSequenceSpiller is used - e.g. for async/await) Jun 23, 2024
@bernd5
Copy link
Contributor Author

bernd5 commented Jun 23, 2024

A simpler version (makes use of spilling with general is pattern):

string str = "a2";
int x = 1;
int y = 2;

ref int r =
      ref str is "Hallo" ? 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

@bernd5
Copy link
Contributor Author

bernd5 commented Jun 23, 2024

And a version with ref readonly:

using System;


string str = "a2";
int x = 1;
int y = 2;

ref readonly var xx = ref x; 

ref readonly int r =
      ref Eval(str, 1) is "Hallo" ? ref xx
    : ref Eval(str, 2) is { Length: >= 2 and <= 10 or 22 } ? ref y
    : ref System.Runtime.CompilerServices.Unsafe.NullRef<int>();

ref var rx = ref System.Runtime.CompilerServices.Unsafe.AsRef(in r);

rx++;
rx++;
rx++;
int xxx = r;
System.Console.WriteLine(xxx);
System.Console.WriteLine(x);
System.Console.WriteLine(y);


static T Eval<T>(T s, int i)
{
    System.Console.WriteLine($"{s} {i}");
    return s;
}

@bernd5 bernd5 changed the title Bug: Conditional Expression stores ref result in non ref local temp (Only when SpillSequenceSpiller is used - e.g. for async/await) Bug: Conditional Operator stores ref result in a non ref local temporary (Only when SpillSequenceSpiller is used - e.g. for async/await) Jun 24, 2024
@bernd5 bernd5 changed the title Bug: Conditional Operator stores ref result in a non ref local temporary (Only when SpillSequenceSpiller is used - e.g. for async/await) Bug: Conditional Operator stores ref result in a non-ref-temporary (Only when SpillSequenceSpiller is used - e.g. for async/await) Jun 24, 2024
@jaredpar jaredpar added Bug and removed untriaged Issues and PRs which have not yet been triaged by a lead labels Jun 27, 2024
@jaredpar jaredpar added this to the 17.12 milestone Jun 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants