views:

161

answers:

4

Consider the following code:

using System;

namespace ConsoleApplication2
{
    class Program
    {
        static void Main(string[] args)
        {
            var square = new Square(4);
            Console.WriteLine(square.Calculate());
        }
    }

    class MathOp
    {        
        protected MathOp(Func<int> calc) { _calc = calc; }
        public int Calculate() { return _calc(); }
        private Func<int> _calc;
    }

    class Square : MathOp
    {
        public Square(int operand)
            : base(() => _operand * _operand)  // runtime exception
        {
            _operand = operand;
        }

        private int _operand;
    }
}

(ignore the class design; I'm not actually writing a calculator! this code merely represents a minimal repro for a much bigger problem that took awhile to narrow down)

I would expect it to either:

  • print "16", OR
  • throw a compile time error if closing over a member field is not allowed in this scenario

Instead I get a nonsensical exception thrown at the indicated line. On the 3.0 CLR it's a NullReferenceException; on the Silverlight CLR it's the infamous Operation could destabilize the runtime.

A: 

Have you tried using () => operand * operand instead? The issue is that there's no certainty that _operand will be set by the time you call the base. Yes, it's trying to create a closure on your method, and there's no guarantee of the order of things here.

Since you're not setting _operand at all, I'd recommend just using () => operand * operand instead.

David Morton
It will work, but it has a very different meaning...
Thomas Levesque
Suffice to say this defeats the purpose. In my "real" code I have several very complex MathOps. Some of the steps are common to all MathOps, so I place them in the base class. In one particular Op, the first part of the calculation is invariant -- I wanted to optimize it by caching the intermediate result in a member field, then letting the rest of the calculation (which varies based on the parameters to Calculate) proceed as usual.
Richard Berg
@Richard Berg: Perhaps you could work around the issue by using a protected initializer method instead? I'm sure you've already thought of that, but it can't hurt to mention...
Aaronaught
That might work. /// FWIW, the actual scenario is a large inheritance tree rooted on IComparable<T>. Lots of abstract classes building on each other in such a way I feel is highly optimized [assuming proper JIT inlining] yet obeys DRY strictly. By the time I hit today's issue I was out of this framework code, implementing a custom comparer for some complex types in a specific application, head stuck deeply in Expression<> mojo. Guess it took me by surprise that the root cause was so fundamental to a design which by then was working perfectly in many other places.
Richard Berg
I think what you want to do is call a private static function within your derived class to compute the optimization result and return that; like this: `public Square(int operand) : base(ComputeSquare(operand)) {}` along with `private static int ComputeSquare(int operand) { return operand * operand; }`
Gabe
I ended up moving the assignment from the base class' constructors to some protected Init() methods. This also required making a protected no-argument constructor. In many of the derived classes that don't need caching, I left things as-is (all the "work" happening in the constructor initializer) but made sure all such classes were sealed. Pretty happy with the results so far.
Richard Berg
+8  A: 

It's not going to result in a compile-time error because it is a valid closure.

The problem is that this is not initialized yet at the time the closure is created. Your constructor hasn't actually run yet when that argument is supplied. So the resulting NullReferenceException is actually quite logical. It's this that's null!

I'll prove it to you. Let's rewrite the code this way:

class Program
{
    static void Main(string[] args)
    {
        var test = new DerivedTest();
        object o = test.Func();
        Console.WriteLine(o == null);
        Console.ReadLine();
    }
}

class BaseTest
{
    public BaseTest(Func<object> func)
    {
        this.Func = func;
    }

    public Func<object> Func { get; private set; }
}

class DerivedTest : BaseTest
{
    public DerivedTest() : base(() => this)
    {
    }
}

Guess what this prints? Yep, it's true, the closure returns null because this is not initialized when it executes.

Edit

I was curious about Thomas's statement, thinking that maybe they'd changed the behaviour in a subsequent VS release. I actually found a Microsoft Connect issue about this very thing. It was closed as "won't fix." Odd.

As Microsoft says in their response, it is normally invalid to use the this reference from within the argument list of a base constructor call; the reference simply does not exist at that point in time and you will actually get a compile-time error if you try to use it "naked." So, arguably it should produce a compile error for the closure case, but the this reference is hidden from the compiler, which (at least in VS 2008) would have to know to look for it inside the closure in order to prevent people from doing this. It doesn't, which is why you end up with this behaviour.

Aaronaught
Did you try it ? I get a compilation error...
Thomas Levesque
@Thomas Levesque: Yes I did, and it compiled, and I got the same runtime error. Curious that you got a compile error; I'm on VS 2008, are you on VS 2010? Maybe they classified this as a bug and updated the compiler to detect this?
Aaronaught
+1 Good explanation. I suspected it, but couldn't be certain the lack of /this/ pointer in my Watch Window wasn't just a VS quirk (I find it gets confused far too easily).
Richard Berg
Indeed, it compiles with VS2008, not with VS2010...
Thomas Levesque
@Thomas Levesque: That raises further questions, since, as I now note in my edit, this was reported to Microsoft and they closed the issue as "Won't Fix" - and then they fixed it! *Sigh*
Aaronaught
So this must be the justification for if (this != null) { ... }...
Daniel Brückner
@Aaronaught : excellent investigation, I wish I could upvote more than once ;)
Thomas Levesque
@Daniel Brückner: Haha, well, I suppose if the code is inside a closure being supplied as a constructor argument from a derived class then yes. :P If `this` is `null` at any other time then you've got bigger things to worry about!
Aaronaught
+2  A: 

How about this:

using System;
using System.Linq.Expressions;

namespace ConsoleApplication2
{
    class Program
    {
        static void Main(string[] args)
        {
            var square = new Square(4);
            Console.WriteLine(square.Calculate());
        }
    }

    class MathOp
    {
        protected MathOp(Expression<Func<int>> calc) { _calc = calc.Compile(); }
        public int Calculate() { return _calc(); }
        private Func<int> _calc;
    }

    class Square : MathOp
    {
        public Square(int operand)
            : base(() => _operand * _operand)
        {
            _operand = operand;
        }

        private int _operand;
    }
}
Artem Govorov
interesting way to defer resolution. Still doesn't work in 2010 though.
Jimmy
Clever. +1 for the quickest fix (no refactoring needed).
Richard Berg
+7  A: 

It was a compiler bug that has been fixed. The code should never have been legal in the first place, and if we were going to allow it, we should have at least generated valid code. My bad. Sorry about the inconvenience.

Eric Lippert