Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Async method on struct mutating copy #4135

Closed
YuvalItzchakov opened this issue Jul 27, 2015 · 3 comments
Closed

Async method on struct mutating copy #4135

YuvalItzchakov opened this issue Jul 27, 2015 · 3 comments

Comments

@YuvalItzchakov
Copy link

Following this question on SO: http://stackoverflow.com/questions/31642535/structs-private-field-value-is-not-updated-using-an-async-method/31646624#31646624

The following program will print 35, and not 45. This happens because the async generated state machine lifts a copy of the struct by copying the this parameter.

I know that using mutable structs are evil, but still, this can lead to some confusion. Was this intentional behavior?

public static void Main(string[] args)
{
     var sInstance = new Structure(25);
     sInstance.Change(35);
     await sInstance.ChangeAsync(45);
}

public struct Structure 
{
   private int _Value;

   public Structure(int iValue) 
   {
      _Value = iValue;
   }

   public void Change(int iValue)
   {
      _Value = iValue;
   }

   public async Task ChangeAsync(int iValue)
   {
      await Task.Delay(1);
      _Value = iValue;
   }
}

This is the decompiled method:

[DebuggerStepThrough, AsyncStateMachine(typeof(Program.Structure.<ChangeAsync>d__4))]
public Task ChangeAsync(int iValue)
{
   Program.Structure.<ChangeAsync>d__4 <ChangeAsync>d__;
   // Lifting the copy here.
   <ChangeAsync>d__.<>4__this = this;
   <ChangeAsync>d__.iValue = iValue;
   <ChangeAsync>d__.<>t__builder = AsyncTaskMethodBuilder.Create();
   <ChangeAsync>d__.<>1__state = -1;
   AsyncTaskMethodBuilder <>t__builder = <ChangeAsync>d__.<>t__builder;
   <>t__builder.Start<Program.Structure.<ChangeAsync>d__4>(ref <ChangeAsync>d__);
   return <ChangeAsync>d__.<>t__builder.Task;
}
@YuvalItzchakov YuvalItzchakov changed the title async method on struct mutating copy Async method on struct mutating copy Jul 27, 2015
@controlflow
Copy link

In short: unfortunately, it is impossible to implement this mutation in CLR :)

In C#, we have some protection from this kind of struct mutability confusion when using closures inside struct declaration members:

struct S {
  public int Value;

  void M() {
    Action f = () => {
      Value ++; // CS1673
    };
  }
}

Error CS1673: Anonymous methods, lambda expressions, and query expressions inside structs cannot access instance members of 'this'. Consider copying 'this' to a local variable outside the anonymous method, lambda expression or query expression and using the local instead

Since execution of every closure can be arbitrarily delayed, we need some way to also delay the lifetime of all the members captured into closure. There is no way to do it in general for this of value type, since value type can be allocated on stack (local variables of value types) and stack space will be reused on method execution exit.

In theory, when value type is stored as a field of some managed object/element of array, C# can emit closure code to do struct mutation inplace. Unfortunately, there is no knowledge on where this value is located when emitting struct member code, so C# decided simply to force users handle this situation manually (by copying the this value most of the time, as error message suggested).

Unfortunately, different decision was made for iterator methods declared on struct declarations:

using System.Collections;

struct S {
    int Value;

    IEnumerable I() {
        Value++;
        yield break;
    }
}

This code compiles fine and always operates on S value copy, since there is no way to put reference to arbitrary S value inside compiler-generated iterator class that implements IEnumerable interface. I was unable to find mentions of this behavior in C# 4.0 spec...

Absolutely the same story happens with async methods. There are ways to reference arbitrary values in CLR via managed references types (they are used for ref/out-parameters in CLR), but storing such references inside closure class fields will produce unverifiable MSIL, because of unpredictable-in-general lifetime of the value I described above.

@jakobbotsch
Copy link
Member

Absolutely the same story happens with async methods. There are ways to reference arbitrary values in CLR via managed references types (they are used for ref/out-parameters in CLR), but storing such references inside closure class fields will produce unverifiable MSIL, because of unpredictable-in-general lifetime of the value I described above.

Actually, you are not even allowed to do that (ECMA-335 section II.14.4.2):

Managed pointers cannot be stored in static variables, array elements, or fields of
objects or value types.

The problem does indeed seem impossible to solve. Seems the compiler would have to pass the structure itself by reference to the enumerator returned, or do something clever with operating on a boxed instance and then unbox it by itself.

I definitely think the compiler should emit a warning if it cannot be a compiler error because of backwards compatibility. The compiler magic right now makes for some pretty confusing behavior.

@gafter
Copy link
Member

gafter commented Mar 20, 2017

We are now taking language feature discussion on https://github.com/dotnet/csharplang for C# specific issues, https://github.com/dotnet/vblang for VB-specific features, and https://github.com/dotnet/csharplang for features that affect both languages.

See also #18002 for a general discussion of closing and/or moving language issues out of this repo.

@gafter gafter closed this as completed Mar 20, 2017
# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

No branches or pull requests

5 participants