Add proposal for "ref and unsafe in iterators and async"

[jjonescz]

cc @jaredpar @AlekseyTs

Rendered markdown

[CyrusNajmabadi]

nit: use ```diff blocks

[terrajobst]

Maybe it's just me, but for text/spec changes strikethrough renders more nicely (because other text formatting is still applied). Diff blocks are rendered as plain text, which doesn't seem great for this purpose.

[AlekseyTs]

Is this only for legacy lock? I assume it will be an error to yield out of a "modern" lock. #Closed

[jjonescz]

Yes, the modern lock cannot work with yield since it's lowered to using of a ref struct. I will make it clearer.

[AlekseyTs]

Is this a breaking change? Is this going to require a user to use fixed depending on whether the local is hoisted or not? #Closed

[jjonescz]

Yes, it is a breaking change. No, the intention is to disallow taking pointers of such locals entirely - that's consistent with how variables captured in lambdas and local functions work today. Although allowing to use fixed with those variables (both captured and hoisted) is a possible alternative or future work. I guess there are also other options:

• Require fixed for all locals in async methods (no need to distinguish hoisted or not).
• Leave it as is, maybe emit only a warning.

[jjonescz]

I discovered there is already a warning for this in C# 12 warning wave (which is likely why it's not visible in sharplab): dotnet/roslyn#66915

Given that we already decided to go with a warning here, I will remove this part of the feature, only leaving a note in alternatives.

[jaredpar]

There are a class of ref struct errors that are demoted to warnings inside an unsafe context. Think we should call out explicitly that this is not one of them. Essentially that this will remain and error even in an unsafe context.

These type of values cannot be safely manipulated in unsafe without relying on implementation details of how the state machine rewrite works. That falls outside the boundaries of what we've allowed to demote to warning in unsafe.

[jjonescz]

safely manipulated in unsafe

:D

[jaredpar]

Instead of saying the specific items we'll fix consider having a more general "unify behavior between iterators and async methods" statement

[jjonescz]

I will add that summarizing sentence, that's a good idea, but I will still leave the specific items as well because it's otherwise hard to tell at glance which specific things change.

[jaredpar]

Consider the following code:

async Task<int> M()
{
  int x = 42;
  ref int y = ref x;
  y.ToString();
  await Task.Delay();
  y = ref x; // error or okay?
  y.ToString(); // error or okay?

In this case we're reassigning the ref local so it doesn't need to be hoisted. Is this allowed or not?

I would lean towards "do not allow" until a user could provide fairly compelling reasons why it should be. But this part of the proposal is leaning on allowing behavior when the ref doesn't need to be hoisted hence I think we should call out specifically this case in the spec. Essentially when we re-assign after await / yield is that legal?

[jjonescz]

Are there any specific concerns why we should disallow this? We could certainly disallow ref reassignments after await/yield and potentially lift the restriction later, but we are already lifting restrictions here, so I would lean towards allowing this.

Note that this would be allowed by the most straightforward implementation (removing some binding errors but leaving the after-lowering walker which finds usages of ref-like locals across awaits/yields).

Also your example seems equivalent to the following, so we would presumably need to disallow that as well:

int x = 42;
Span<int> y = new(ref x);
y.ToString();
await Task.Yield();
y = new(ref x);
y.ToString();

[jaredpar]

Are there any specific concerns why we should disallow this?

My concerns are complications of implementation. If this is straight forward to do then I'd be more okay with going this direction. Consider though that there seem to be real challenges with doing this.

// This will be legal in C# 13
ref struct S : IEnumerable<int> { .. } 

async Task M(S s)
{
  foreach (var i in s) {
    await Task.Delay(); 
  }
}

The value s here is definitely used after the await but the current spec language seemingly allows it. This same problem comes up when a ref struct is used in any block level construct: foreach, using, lock, etc ... Is the expectation that for all of these types of constructs that if they involve a ref struct that we error when a await / yield occurs inside them?

Also I'm wondering about the mechanism by which this feature is implemented. The spec says it's an error to use it. Did you consider as an alternative using definite assignment as a mechanism? One approach I was thinking of is essentially the following:

After a statement with a await or yield all ref struct or ref locals in scope are considered definitely unassigned.

That would seemingly provide all the behavior that we want but I haven't really dug into the implementation to see if it's practical.

[jjonescz]

The value s here is definitely used after the await but the current spec language seemingly allows it.

It was my intention to disallow this but the spec language should be clarified, you're right. Thanks.

Is the expectation that for all of these types of constructs that if they involve a ref struct that we error when a await / yield occurs inside them?

Yes.

Also I'm wondering about the mechanism by which this feature is implemented. The spec says it's an error to use it. Did you consider as an alternative using definite assignment as a mechanism?

That's exactly what I want to do. There is already a definite assignment walker (IteratorAndAsyncCaptureWalker) which does what you suggest. It only supports ref structs now, so needs some small tweaking to support ref locals as well. Plus I will remove all the binding errors that restrict async with ref/ref struct locals, etc. And that's it. Note that the walker runs after lowering, so it automatically handles foreach/using/lock etc.

I'm not sure if the "definite assignment" mechanism should be part of the spec language as well. But I will try to incorporate it to clarify things.

[AlekseyTs]

LGTM (commit 14)

[cston]

, for example:

Let's remove if there is no example.