`foreach` expansion spec doesn't match Microsoft C# compiler behavior

[GeeLaw]

Consider

A a = default;
Console.WriteLine(a.Value); // 0, as expected
((A)(a)).GetEnumerator();
Console.WriteLine(a.Value); // 0, as expected
foreach (var i in a)
  ;
Console.WriteLine(a.Value); // 1, unexpected per spec

public struct A
{
  public int Value;
  public IEnumerator<int> GetEnumerator()
  {
    ++Value;
    return ((IEnumerable<int>)new int[1]).GetEnumerator();
  }
}

Per 13.9.5.2 Synchronous foreach, the correct expansion uses ((C)(x)).GetEnumerator() to acquire the enumerator. In this example, C is A and x is a.

Casting a struct variable to its own type creates a copy, so according to the spec, the correct behavior is to mutate a copy of a.

---

I think the spec is wrong for the following reasons:

• I do expect foreach (var i in a) to call GetEnumerator on a without cast.
• When the resolved GetEnumerator method is an extension method taking this ref A, the expression ((A)(a)).GetEnumerator() (as required by the current spec) doesn't compile. Since extension GetEnumerator is added in or after C# 7.2 (which is when taking receiver by ref is allowed in extension methods), it's intended to work. (Update: It seems foreach by extension method is added in commit 0372a1384efd01df4ed1f7ef6bf2798999c86967 for C# 9.)

In fact, the spec has more incorrect wordings. Suppose struct A implements IEnumerable<int> explicitly without a public GetEnumerator method, the current behavior is to use constrained callvirt on a (hence potentially mutating, and also inside GetEnumerator the struct can check its identity) instead of casting it to the interface then invoking GetEnumerator (which boxes hence only mutates a copy).

[GeeLaw]

Another interesting fact:

A? na = new A();
foreach (var i in na)
  ;

The code above (with the A defined in the OP) compiles in Microsoft's C# compiler, but according to the spec...

The type of na is A?, which does not have a public GetEnumerator method (nor an extension is in scope). Therefore, the spec considers enumerable interfaces, but A doesn't implement IEnumerable, so there's no conversion from A? to IEnumerable or IEnumerable<T> for any T. Therefore, the code should not compile per spec.

I also consider this a defect of the spec, because it's sensible to lift the semantics of foreach over expression of type X to X?.

---

Now, if we change na to default and run the code, the exception thrown is InvalidOperationException, not NullReferenceException as noted:

Note: If expression has the value null, a System.NullReferenceException is thrown at run-time. end note

Clearly, this is caused by na.Value in na.Value.GetEnumerator() (the actual lowered form produced by Roslyn).

[GeeLaw]

Found some related work in progress: dotnet/csharpstandard#1472

[BillWagner]

I transferred this issue from the csharpstandard repo.

Consider this example from @jskeet

A a = new A { Value = 10 };
((A)a).Mutate();
Console.WriteLine(a.Value); // Prints 10 (mutate has acted on a copy)
a.Mutate();
Console.WriteLine(a.Value); // Prints 11 (mutate has acted on the variable a)

struct A
{
    public int Value;

    public void Mutate()
    {
        Value++;
    }
}

From offline conversations:

I think that this is not about conversion creating a copy. This is about a cast expression resulting in an RValue, rather than an LValue. Therefore, I think that the fact that mutations are observable on the expression converted with a cast is a compiler bug.

/cc @AlekseyTs

[GeeLaw]

I think that this is not about conversion creating a copy. This is about a cast expression resulting in an RValue, rather than an LValue.

I'm puzzled by this statement, possibly due to lost of context. AFAIK there's no semantic difference between "creating a copy" and "resulting in an RValue". If the expression being cast is an LValue, then converting it to RValue simply means creating a copy and further operating on the copy. If the expression being cast is an RValue, then creating a copy and further operating on the copy, is equivalent to not creating a copy and simply operating on the original.

Therefore, I think that the fact that mutations are observable on the expression converted with a cast is a compiler bug.

In foreach, the bug appears to be "the compiler did not rewrite it to use a cast at all", not "the compiler rewrites it to a cast but somehow makes the mutation observable at the original location".

[GeeLaw]

The issue has 2 layers:

1. Should foreach be specced to always use a cast on the collection expression?
2. Does the compiler match the spec?

The current answer to (2) is "No".

My opinion about (1) is that it should not be specced to always use a cast on the collection expression, only when necessary. In particular, I suggest revising the spec to

1. If c.GetEnumerator() already satisfies the enumerator pattern required by foreach, then acquire the enumerator as c.GetEnumerator(), with no cast at all.
2. Otherwise, if c is a value type implementing IEnumerable<T> or IEnumerable, then acquire the enumerator as constrained callvirt, with no cast at all.
3. Otherwise, if c is a nullable value type, then try 1 and 2 again with c.Value as c.
4. Otherwise, if c is a reference type implementing IEnumerable<T> or IEnumerable, then do an interface cast and acquire the enumerator.
5. Otherwise, this foreach is not valid.

The standard's way of saying constrained callvirt is "except that the cast of shall not cause boxing to occur" per using statement text. I'd suggest phrasing it as

invokes the interface method ... on ... as if in generics with where T : IEnumerable<T> or where T : IEnumerable

with a note

If the value type implements the interface method GetEnumerator and the collection expression is a variable, the mutation is reflected in that variable. On the other hand, if the value type does not implement it (another interface deriving from IEnumerable<T> or IEnumerable provides a default implementation), the invocation (hence mutation) is done on a boxed copy.

The benefits of my proposal:

• This wording is consistent with the existing behavior.
• Case 1 works with extension GetEnumerator taking this ref TStruct, as well as interface-constrained type parameter types.
• Case 2 works with ref struct without public GetEnumerator.
• No existing implemented feature is lost.

The disadvantages of the current spec:

• There's unnecessary copy required, and such semantics are not yet implemented.
• It doesn't work with explicitly implemented IEnumerable<T>, IEnumerable on ref struct at all. However, current implementation does work with ref struct.
• It requires boxing of Nullable<T> or manual check and throw (because of exception type being NullReferenceException instead of InvalidOperationException).

A a = default;
// Spec requires boxing below, but it compiles fine, as it should.
foreach (var i in a)
  ;

ref struct A : IEnumerable
{
  IEnumerator IEnumerable.GetEnumerator()
  {
    return (new int[1]).GetEnumerator();
  }
}

---

All in all, by default if the compiler doesn't implement the spec, it's "the compiler's bug". But due to the analysis above, I think this really is a defect of the spec, not the compiler.

[GeeLaw]

After some digging, I have a better understanding of

I think that this is not about conversion creating a copy. This is about a cast expression resulting in an RValue, rather than an LValue. Therefore, I think that the fact that mutations are observable on the expression converted with a cast is a compiler bug.

Putting it here in case helpful to passers-by.

---

What that says is that the reason ((A)a).M() works on a copy of a, is not because (A)a creates a copy of a, but because (A)a is an r-value and RValue.M() for value type A always works on a copy of RValue. The statement owes the fact of creating a copy to "invoking a method on r-value" instead of "doing an identity conversion". This adheres to C# spec better. The relevant paragraphs in ECMA C# 7th edition are as follows.

p.232 (cast expressions):

A cast_expression of the form (T)E ... the result is the value produced by the explicit conversion. The result is always classified as a value, even if E denotes a variable.

p.181 (method invocation E.M(...)):

if the type of E is a value-type V, and M is declared or overridden in V...
If E is not classified as a variable...
then a temporary local variable of E’s type is created and the value of E is assigned to that variable. E is then reclassified as a reference to that temporary local variable. The temporary variable is accessible as this within M, but not in any other way.

But my point of "no semantic difference" in #84307 (comment) still stands. I haven't found a case where whether identity conversion "already" creates a copy (still as r-value) alters the semantics of a C# program.

[CyrusNajmabadi]

Seems like a compiler bug to me. The spec is clear here. That said, practically this has been the behavior forever, so i would just doc it a a minor quirk in the compiler that practically impacts no one. If you had a desire for this to work, and your mutable struct collection type is impacted by this, toobadsosad.

There's an easy fix for the author. Just add the manual cast themselves.

--

I'm also ok with an alternate reading indicating that transform in the spec is meant to be read in a slightly looser fashion. Namely that the (C) cast is there primarily to address appropriate lookup through one of the other types mentioned in prior sections (for example looking up an interface member), and isn't intended in the case where the method is found trivially without reconsidering the type of the collection. However, it was felt to be annoying/onerous to have to spell all that out, and it was assumed to be practically equivalent for all #intensiveporpoises. So the simple example was given that is incorrect.

--

Either way, the practical impact is near nil. Either we add a quick clarification statement in teh spec. Or we just mention the quirk in the compiler. We can also just do nothing as this isn't likely of import.