Content

Not quite a Yegge long.

Beware of exactly what you’re dragging into that closure! (C#)

Tuesday 6 October 2009 - Filed under Uncategorized

I saw an interesting bug report today for OpenRA: A closure constructed in the body of a foreach loop wasn’t capturing the induction variable the way we expected. As usual, the compiler is doing the right thing, and it’s just a weird case you have to watch out for. Here’s a simplified, somewhat contrived version that makes me want to cry:

var xs = new[] { “foo”, “bar”, “baz” };
var ys = new List<Func<string>>();

foreach( var x in xs ) ys.Add( () => x );

foreach( var y in ys ) Console.WriteLine( y() );

One would naively expect that this nonsense would have this output:

foo
bar
baz

Unfortunately, you get this:

baz
baz
baz

Oops. That `x` is being shared between all the closures we created. Apparently, you have to create a “real” local binding within the `foreach` block to separate them:

foreach( var x in xs ) { var y = x; ys.Add( () => y ); }

Uh, right. Let’s expand that foreach manually:

using( var e = xs.Cast<string>().GetEnumerator() )
   while( e.MoveNext() ) {
       var x = e.Current; 
       ys.Add( () => x );
   }

Pay no attention to the Cast<string>() lameness. Foreach has hax. And no, that’s not really what the compiler did. Enjoy:

using (var e = xs.Cast<string>().GetEnumerator())
            {
                string x;
                while (e.MoveNext()) {
                     x = e.Current; ys.Add(() => x); }
            }

This sucks. In fact, this sucks so much that I’m going to throw out all the imperative crap and do it the nice way, which works properly without being weird:

var ys = xs.Select(x => (Func<string>)(() => x)).ToList();

Oops, still a bit of lameness because the type inference engine sucks. You could hide it with an extension method: AsThunk() :: a –> (() –> a) but we won’t bother, since this is pretty contrived anyway.

Oh well. Anyway, that’s why OpenRA currently makes a Radar Dome when you click on Power Plant.

2009-10-06  »  admin

Talkback x 5

  1. Paul Batum
    6 October 2009 @ 6:53 pm

    Hi there,

    I have a few minor points to make:

    1. You can avoid some of the parantheses nastiness by instead explcitly specifying the generic arguments. So this:
    xs.Select(x => (Func)(() => x))
    becomes
    xs.Select<string, Func>(x => () => x)
    An argument can be made for either format, I just wanted to point out that an alternative exists.

    2. I think its a little unfair to say that the cast is required because the type inference engine “sucks”. After all, the lambda
    () => “hi there”
    can be cast to more than one type! Sure, Func works, but if I was to declare this delegate:
    delegate string GetSomeString();
    then this would work too!
    xs.Select(x => () => x);

    In which case the compiler is no longer in the realm of ‘inferring’ a type – it now has to -choose- one! Now I do appreciate that if C# had a more powerful type system this would be a different story…

  2. Paul Batum
    6 October 2009 @ 6:58 pm

    Unfortunately none of my above post came through properly because I forgot to html encode it. Hopefully this one will appear correctly.

    Hi there,

    I have a few minor points to make:

    1. You can avoid some of the parantheses nastiness by instead explcitly specifying the generic arguments. So this:
    xs.Select(x => (Func<string>)(() => x))
    becomes
    xs.Select<string, Func<string>>(x => () => x)
    An argument can be made for either format, I just wanted to point out that an alternative exists.

    2. I think its a little unfair to say that the cast is required because the type inference engine "sucks". After all, the lambda
    () => "hi there"
    can be cast to more than one type! Sure, Func<string> works, but if I was to declare this delegate:
    delegate string GetSomeString();
    then this would work too!
    xs.Select<string, GetSomeString>(x => () => x);

    In which case the compiler is no longer in the realm of ‘inferring’ a type – it now has to -choose- one! Now I do appreciate that if C# had a more powerful type system this would be a different story…

  3. elder_george
    6 October 2009 @ 10:38 pm

    ReSharper makes a warning on such code (in addition to all his niceness).

  4. website design
    7 October 2009 @ 1:35 am

    ReSharper even has a warning on this, since its such a common mistake. Yes, thats how closures a implemented in C#, or more certainly, how foreach is implemented. As far as I understand, if loop variable was recreated for every iteration, that wouldnt be a problem.

  5. admin
    10 December 2009 @ 7:35 pm

    You’re absolutely right on the first point, Paul, although neither technique is ideal. All you can do is shift the problem around :)

    As for the second point, perhaps it is unfortunate that the CLR’s notion of a delegate type need be anything more than an alias for the signature. Func<string> and GetSomeString should just be different names for the same type.

    Yes, I can see the inconsistency between having structural typing here and not elsewhere, and that’s an ugly problem. I can also see problems with attaching attributes to delegate declarations (for example, the ones P/Invoke uses to generate unmanaged wrappers), and more problems with how Reflection sees the type. In your Func<string> vs GetSomeString case, is the reflected type generic, or is it not?

    I cannot see a clean way to solve these things, other than to go back to nominal typing and making inference impossible.

    You seem to have your head screwed on straight, Paul, any thoughts?

Share your thoughts

Re: Beware of exactly what you’re dragging into that closure! (C#)







Tags you can use (optional):
<a href="" title=""> <abbr title=""> <acronym title=""> <b> <blockquote cite=""> <cite> <code> <del datetime=""> <em> <i> <q cite=""> <strike> <strong>