11

I discovered that iterator methods in value types are allowed to modify this.
However, due to limitations in the CLR, the modifications are not seen by the calling method. (this is passed by value)

Therefore, identical code in an iterator and a non-iterator produce different results:

static void Main() {
    Mutable m1 = new Mutable();
    m1.MutateWrong().ToArray();     //Force the iterator to execute
    Console.WriteLine("After MutateWrong(): " + m1.Value);

    Console.WriteLine();

    Mutable m2 = new Mutable();
    m2.MutateRight();
    Console.WriteLine("After MutateRight(): " + m2.Value);
}

struct Mutable {
    public int Value;

    public IEnumerable<int> MutateWrong() {
        Value = 7;
        Console.WriteLine("Inside MutateWrong(): " + Value);
        yield break;
    }
    public IEnumerable<int> MutateRight() {
        Value = 7;
        Console.WriteLine("Inside MutateRight(): " + Value);
        return new int[0];
    }
}

Output:

Inside MutateWrong(): 7
After MutateWrong(): 0

Inside MutateRight(): 7
After MutateRight(): 7

Why isn't it a compiler error (or at least warning) to mutate a struct in an iterator?
This behavior is a subtle trap which is not easily understood.

Anonymous methods, which share the same limitation, cannot use this at all.

Note: mutable structs are evil; this should never come up in practice.

Community
  • 1
  • 1
SLaks
  • 868,454
  • 176
  • 1,908
  • 1,964
  • 2
    Iteresing. I think warning is a must. – Lukasz Madon Dec 22 '10 at 23:22
  • I guess it's a side effect of having the compiler generate the iterator class and a use case the team didn't consider. But we see it now and can let them know. – Jeff Mercado Dec 22 '10 at 23:33
  • @Jeff: They did consider it. Read my [blog post](http://blog.slaks.net/2010/12/when-shouldnt-you-write-ref-this.html); the spec _explicitly_ mentions this (§7.6.7). – SLaks Dec 22 '10 at 23:37
  • @0xA3: I'm not asking for it to work. I'm asking for a warning. – SLaks Dec 22 '10 at 23:44
  • I think you could ask why you would want to mutate a value inside an iterator. Is it because you're trying to actually change the original instance? Or are you trying to maintain state within the iterator? – Gabe Dec 22 '10 at 23:47
  • @Gabe: If you want to maintain state, don't do it in `this`. – SLaks Dec 22 '10 at 23:56
  • This is only one of many differences in semantics between classes and structs that you must know when creating a struct, any of which could fall under the category of a subtle trap that's not easily understood. – Gabe Dec 31 '10 at 22:49

4 Answers4

2

In order to justify a warning, it should be in a situation where the programmer is likely to get unexpected results. According to Eric Lippert, "we try to reserve warnings for only those situations where we can say with almost certainty that the code is broken, misleading or useless." Here is an instance where the warning would be misleading.

Let's say you have this perfectly valid – if not terribly useful – object:

struct Number
{
    int value;
    public Number(int value) { this.value = value; }
    public int Value { get { return value; } }
    // iterator that mutates "this"
    public IEnumerable<int> UpTo(int max)
    {
        for (; value <= max; value++)
            yield return value;
    }
}

And you have this loop:

var num = new Number(1);
foreach (var x in num.UpTo(4))
    Console.WriteLine(num.Value);

You'd expect this loop to print 1,1,1,1, not 1,2,3,4, right? So the class works exactly as you expect. This is an instance where the warning would be unjustified.

Since this is clearly not a situation where the code is broken, misleading, or useless, how would you propose that the compiler generate an error or warning?

Peter Duniho
  • 68,759
  • 7
  • 102
  • 136
Gabe
  • 84,912
  • 12
  • 139
  • 238
  • Any iterator that tries to modify the struct for good. (Which would be a sign that the struct ought to be a class) – SLaks Dec 23 '10 at 02:14
  • The big issue here is that two (otherwise) identical methods, one an iterator and one not, behave differently. (aside from lazy execution) – SLaks Dec 23 '10 at 02:15
  • SLaks: It looks like you've identified a quirk in the language. I don't see what the compiler can do about it. – Gabe Dec 23 '10 at 03:36
  • Actually I would expect it to print 1,2,3,4. Each iteration increments the value, why should it stay 1? – Ilya Kogan Dec 24 '10 at 11:48
  • @Ilya: If it printed `x` then you should expect 1,2,3,4. You should expect `x` to get incremented, not `num`. – Gabe Dec 24 '10 at 17:06
  • @Gabe: You could still get this "perfectly valid behavior" by using a "local" variable to count in `UpTo` (of course it wouldn't really be local, it would be hoisted by the compiler processing of `yield return` syntactic sugar). That would avoid the suggested warning and arguably would be easier to understand as well -- there would be no question whatsoever whether the original instance was affected. – Ben Voigt Dec 31 '10 at 21:35
  • Ben: Sure you could, but that's not the point. The point is that warnings aren't for code that might be hard to understand; they're for code that's likely broken, misleading, or useless. This is a trivial example of code that is none of those. – Gabe Dec 31 '10 at 22:40
  • @Gabe: I think it IS misleading. The implementation of `UpTo` appears to modify the member field but actually does not. That this is the desired behavior is irrelevant, it's still confusing. Using a local variable would both give the desired behavior AND be clear. There are lots of cases where a very slight rewrite preserves the behavior and eliminates a false-positive warning. For that matter, C# generates a lot of outright errors for code that's neither wrong nor hard to understand (bit-wise operations on narrow integers, for example) – Ben Voigt Dec 31 '10 at 23:25
2

To cite yourself "mutable structs are evil" :) The same thing as you experienced happens if you implement an extension method for a struct. If you try to modify the struct within the extension method you still will have your original struct unchanged. It is a bit less surprising since the extension method signature looks like:

static void DoSideEffects(this MyStruct x) { x.foo = ...

Looking at it we realize that something like parameter passing happens and therefore the struct is copied. But when you use the extension it looks like:

x.DoSideEffects()

and you will be surprised not to have any effects on your variable x. I suppose that behind the scenes your yield constructs do something similar to extensions. I would phrase the starting sentence harder: "structs are evil" .. in general ;)

fausto
  • 67
  • 3
0

I had a similar thought to what Gabe said. It seems at least theoretically possible that one might opt to use a struct to behave kind of like a method, encapsulating that method's local variables as instance fields:

struct Evens
{
    int _current;

    public IEnumerable<int> Go()
    {
        while (true)
        {
            yield return _current;
            _current += 2;
        }
    }
}

I mean, it's kind of weird, obviously. It kind of reminds me of ideas I've encountered in the past, though, where developers have concocted ever-stranger ways of calling methods, such as wrapping a method's parameters into an object and letting that object call the method—going backwards, in a sense. I'd say that's roughly what this is.

I'm not saying this would be a wise thing to do, but it is at least a way of using this in the way you are describing that might be intentional, and would technically exhibit correct behavior.

Community
  • 1
  • 1
Dan Tao
  • 125,917
  • 54
  • 300
  • 447
  • But you get the exact same behavior letting the compiler hoist a local variable to iterator lifetime, just like it does for closures in anonymous delegates. And, given that option, I'm not convinced anyone would intentionally choose to modify the silently copied member variable. – Ben Voigt Dec 31 '10 at 23:27
  • @Ben: yeah, I can't really dispute that. The thing is, we're trying to discuss why something IS allowed. I feel that, when it's even *possible* to imagine a use for such a feature, it's pretty tough to come up with a concrete argument for or against it. Maybe it's just allowed because it isn't disallowed, you know? To me it's a head-scratcher, that's all. Conversations tend to become more heated when the question is why something *isn't* allowed. – Dan Tao Jan 01 '11 at 00:24
0

It's not really clear what should happen, though I think .net is deficient in not requiring a special attribute for methods which modify 'this'; such an attribute could be useful applied to immutable class types as well as mutable structs. Methods which are tagged with such an attribute should only be usable on structure variables, fields, and parameters, and not on temporary values.

I don't think there's any way to avoid having the iterator capture the struct by value. It's entirely possible that by the time an iterator is used, the original struct upon which it was based may not exist anymore. On the other hand, if the struct implemented an interface that inherited IEnumerable<int>, but also included a function to return Value, casting the struct to that interface type before using the enumerator could in theory allow the iterator to keep a reference to the struct without having to recopy its value; I wouldn't be at all surprised if the enumerator copies the struct by value even in that case, though.

supercat
  • 77,689
  • 9
  • 166
  • 211