The code below causes an infinite loop in method Foo. I think the
problem is that EnumeratorHelper.Next is invoked on a temporary rather
that on the instance en. This causes any changes to be lost and
therefore the loop will not terminate. Depending on the kind of data
source this will either cause an infinite loop or an
InvalidOperationException.


..method private hidebysig newslot virtual final instance bool MoveNext()
cil managed
{
.override [mscorlib]System.Collections.IEnumerator::MoveNext
.maxstack 4
.locals init (
[0] bool flag,
[1] int32 num,
[2] bool flag2,
[3] valuetype Test/EnumeratorHelper<!T, !K> helper)
....
L_009e: ldarg.0
L_009f: ldfld valuetype Test/EnumeratorHelper<!0, !1>
Test/<Foo>d__0<!T, !K, !R>::<en>5__1
L_00a4: stloc.3
L_00a5: ldloca.s helper
L_00a7: call instance void Test/EnumeratorHelper<!T, !K>::Next()
....


I know the code makes not too much sense in /this/ small example. My
real use case is the synchronized enumeration of several ordered data
sources of different types and a common key (something like a join). It
make the code really unreadable if I need to expand the number of
variables and the code in EnumeratorHelper inline.


using System;
using System.Collections.Generic;


public class Test
{
public static IEnumerable<R> Foo<T,K,R>(IEnumerable<T> source,
Func<T,K> selector, Func<T,R> result)
{ using (var en = new EnumeratorHelper<T,K>(source, selector))
{ while (true)
{ // Do some logic with en and en.Key
if (!en.Valid)
yield break;
yield return result(en.En.Current);
en.Next();
}
}
}

public static void Bar<T,K>(IEnumerable<T> source, Func<T,K> selector,
Action<T> action)
{ using (var en = new EnumeratorHelper<T,K>(source, selector))
{ while (true)
{ // Do some logic with en and en.Key
if (!en.Valid)
return;
action(en.En.Current);
en.Next(); // Operates on a temporary copy!!!
}
}
}

private struct EnumeratorHelper<T,K> : IDisposable
{ private readonly Func<T,K> Selector;
public readonly IEnumerator<T> En;
public bool Valid;
public K Key;

public EnumeratorHelper(IEnumerable<T> list, Func<T,K> selector)
{ Selector = selector;
En = list.GetEnumerator();
Key = (Valid = En.MoveNext()) ? Selector(En.Current) : default(K);
}
public void Next()
{ Key = (Valid = En.MoveNext()) ? Selector(En.Current) : default(K);
}

public void Dispose()
{ if (En != null)
En.Dispose();
}
}

static void Main()
{
var data = new int[] { 1,3,6,7,8 };

// OK
Bar(data, x => x, x => Console.WriteLine(x));

// Fails...
foreach(var item in Foo(data, x => x, x => x))
Console.WriteLine(item);
}
}

On 09.02.2012 16:34, Peter Duniho wrote:
> On Thu, 09 Feb 2012 09:01:04 +0100, Marcel Müller wrote:
>
>> The code below causes an infinite loop in method Foo. I think the
>> problem is that EnumeratorHelper.Next is invoked on a temporary rather
>> that on the instance en. This causes any changes to be lost and
>> therefore the loop will not terminate. Depending on the kind of data
>> source this will either cause an infinite loop or an
>> InvalidOperationException.
>
> I don't understand the IL you posted. It's not what the code you posted
> generates on my system (there's not even a "helper" variable in the C# code
> you posted). What version of the C# compiler are you using?

Oh, I knew I missed something important. It is VS2008 / .Net 3.5.

> Also, you comment about "temporary copy" appears to be in the wrong method.

The complete code of MoveNext of the the generated class is -> see below.
The local slot 3 is the problem. It should not exist, since the
EnumeratorHelper is already a field of the generated class.

But from your statements above it sounds that this has been fixed in a
newer version. Unfortunately we have no choice to upgrade so far.


> That said, yes...making your EnumeratorHelper type a struct is breaking
> your code. Mutable structs can be very dangerous and non-intuitive in
> behavior. That certainly is what's going on here, as can easily be
> demonstrated simply by making EnumeratorHelper a class.

Well, the .NET runtime uses structs for enumerators at several places
too. E.g.
System.Collections.Generic.Dictionary<TKey,TValue>+Enumerator<TKey,TValue>

I see no problem with mutable structs in /this/ case. The struct is
private, never copied, never boxed and it is only used as local
variable. In case of the yield return its fields are in fact expanded
inline into the generated class, but still the above constraints are
true. And all of that is exactly my intention. I need this kind of
EnumerationHelper multiple times in a class providing generalized outer
join semantics. The code would really look not pretty if I need to
repeat myself for this purpose.

So what is wrong with my code?

> Did you have a question? Or are you just sharing your mistake for others
> to benefit from?

If I have a bug in my code, I would like to know it.
But if it is a bug in the compiler it is not likely that I will get a
fix. So it is mainly for information.

I took me a while to catch the problem. The debugger was no help here,
because it did not show the temporary. Only the content of 'en' suddenly
jumped back to the original values after the call to Next(). Also the C#
decompiler was no help, because it said
this.<en>5__1.Next();
which reflects still my intention.

My current (and acceptable) work around is to replace struct by class.
This keeps the code readable. But I am still suprised of the behavior.
It makes absolutely no sense to create a copy of a value type and then
invoke a member function on that temporary.


IL code ...

.method private hidebysig newslot virtual final instance bool
MoveNext() cil managed
{
.override [mscorlib]System.Collections.IEnumerator::MoveNext
.maxstack 4
.locals init (
[0] bool flag,
[1] int32 num,
[2] bool flag2,
[3] valuetype Test/EnumeratorHelper<!T, !K> helper)
L_0000: ldarg.0
L_0001: ldfld int32 Test/<Foo>d__0<!T, !K, !R>::<>1__state
L_0006: stloc.1
L_0007: ldloc.1
L_0008: switch (L_001d, L_001f, L_001b)
L_0019: br.s L_001f
L_001b: br.s L_0097
L_001d: br.s L_0024
L_001f: br L_00b2
L_0024: ldarg.0
L_0025: ldc.i4.m1
L_0026: stfld int32 Test/<Foo>d__0<!T, !K, !R>::<>1__state
L_002b: nop
L_002c: ldarg.0
L_002d: ldflda valuetype Test/EnumeratorHelper<!0, !1>
Test/<Foo>d__0<!T, !K, !R>::<en>5__1
L_0032: ldarg.0
L_0033: ldfld class
[mscorlib]System.Collections.Generic.IEnumerable`1<!0>
Test/<Foo>d__0<!T, !K, !R>::source
L_0038: ldarg.0
L_0039: ldfld class [System.Core]System.Func`2<!0, !1>
Test/<Foo>d__0<!T, !K, !R>::selector
L_003e: newobj instance void Test/EnumeratorHelper<!T,
!K>::.ctor(class [mscorlib]System.Collections.Generic.IEnumerable`1<!0>,
class [System.Core]System.Func`2<!0, !1>)
L_0043: stobj Test/EnumeratorHelper<!T, !K>
L_0048: ldarg.0
L_0049: ldc.i4.1
L_004a: stfld int32 Test/<Foo>d__0<!T, !K, !R>::<>1__state
L_004f: nop
L_0050: br.s L_00ae
L_0052: nop
L_0053: ldarg.0
L_0054: ldfld valuetype Test/EnumeratorHelper<!0, !1>
Test/<Foo>d__0<!T, !K, !R>::<en>5__1
L_0059: ldfld bool Test/EnumeratorHelper<!T, !K>::Valid
L_005e: stloc.2
L_005f: ldloc.2
L_0060: brtrue.s L_006b
L_0062: ldarg.0
L_0063: call instance void Test/<Foo>d__0<!T, !K,
!R>::System.IDisposable.Dispose()
L_0068: nop
L_0069: leave.s L_00b2
L_006b: ldarg.0
L_006c: ldarg.0
L_006d: ldfld class [System.Core]System.Func`2<!0, !2>
Test/<Foo>d__0<!T, !K, !R>::result
L_0072: ldarg.0
L_0073: ldfld valuetype Test/EnumeratorHelper<!0, !1>
Test/<Foo>d__0<!T, !K, !R>::<en>5__1
L_0078: ldfld class
[mscorlib]System.Collections.Generic.IEnumerator`1<!0>
Test/EnumeratorHelper<!T, !K>::En
L_007d: callvirt instance !0
[mscorlib]System.Collections.Generic.IEnumerator`1<!T>::get_Current()
L_0082: callvirt instance !1 [System.Core]System.Func`2<!T,
!R>::Invoke(!0)
L_0087: stfld !2 Test/<Foo>d__0<!T, !K, !R>::<>2__current
L_008c: ldarg.0
L_008d: ldc.i4.2
L_008e: stfld int32 Test/<Foo>d__0<!T, !K, !R>::<>1__state
L_0093: ldc.i4.1
L_0094: stloc.0
L_0095: leave.s L_00bf
L_0097: ldarg.0
L_0098: ldc.i4.1
L_0099: stfld int32 Test/<Foo>d__0<!T, !K, !R>::<>1__state
L_009e: ldarg.0
L_009f: ldfld valuetype Test/EnumeratorHelper<!0, !1>
Test/<Foo>d__0<!T, !K, !R>::<en>5__1
L_00a4: stloc.3
L_00a5: ldloca.s helper
L_00a7: call instance void Test/EnumeratorHelper<!T, !K>::Next()
L_00ac: nop
L_00ad: nop
L_00ae: ldc.i4.1
L_00af: stloc.2
L_00b0: br.s L_0052
L_00b2: nop
L_00b3: ldc.i4.0
L_00b4: stloc.0
L_00b5: leave.s L_00bf
L_00b7: ldarg.0
L_00b8: call instance void Test/<Foo>d__0<!T, !K,
!R>::System.IDisposable.Dispose()
L_00bd: nop
L_00be: endfinally
L_00bf: nop
L_00c0: ldloc.0
L_00c1: ret
.try L_0000 to L_00b7 fault handler L_00b7 to L_00bf
}

Fields ...

.field private int32 <>1__state

.field private !R <>2__current

.field public class [System.Core]System.Func`2<!T, !R> <>3__result

.field public class [System.Core]System.Func`2<!T, !K> <>3__selector

.field public class
[mscorlib]System.Collections.Generic.IEnumerable`1<!T> <>3__source

.field private int32 <>l__initialThreadId

.field public valuetype Test/EnumeratorHelper<!T, !K> <en>5__1

.field public class [System.Core]System.Func`2<!T, !R> result

.field public class [System.Core]System.Func`2<!T, !K> selector

.field public class
[mscorlib]System.Collections.Generic.IEnumerable`1<!T> source

On 11.02.2012 02:48, Peter Duniho wrote:
> I checked with an expert and got some excellent details that I'd
> overlooked. I was reminded that the variable declared in a "using"
> statement is read-only.

Oh, so using is the problem?

In this case I would say that there is still a compiler bug, but when
compiling Bar. The second implementation should fail for the same reason.

> So, because of the "using" statement in the iterator method, the compiler
> intentionally copies the value to a local variable before using it, to make
> sure that whatever happens during the call does not mutate the variable
> declared in the "using" statement.

Hmm, a warning would be nice.

> If you change your iterator method to something like this, it works fine:
>
> public static IEnumerable<R> Foo<T, K, R>(IEnumerable<T> source, Func<T,
> K> selector, Func<T, R> result)
> {
> var en = new EnumeratorHelper<T, K>(source, selector);
>
> try
> {
> while (true)
> { // Do some logic with en and en.Key
> if (!en.Valid)
> yield break;
> yield return result(en.En.Current);
> en.Next();
> }
> }
> finally
> {
> en.Dispose();
> }
> }
>
> Hope that helps.

Interesting point. But it still raises the question why this does not
apply to Bar too.


Marcel