Why is the innermost exception in an expression tree not thrown?

Question

I've been trying to create a custom ExpressionVisitor that would generate an expression that (optionaly) throws a NullReferenceException on the first null value. The DebugView of the expression looks fine to me but it doesn't work as exptecte (by me). I thought it would first throw the

.Throw .New System.NullReferenceException("c3")

because the test variable is null but instead this one is thrown

.Throw .New System.NullReferenceException("p")

I cannot understand why it executes the statements backwards. Shouldn't it execute the innermost If first?

DebugView:

.Block() {
    .If (.Block() {
        .If (.Constant<ExpressionTrees.Program+<>c__DisplayClass0_0>(ExpressionTrees.Program+<>c__DisplayClass0_0) == null) {
            .Throw .New System.NullReferenceException("c3")
        } .Else {
            .Default(System.Void)
        };
        .Constant<ExpressionTrees.Program+<>c__DisplayClass0_0>(ExpressionTrees.Program+<>c__DisplayClass0_0).c3
    } == null) {
        .Throw .New System.NullReferenceException("p")
    } .Else {
        .Default(System.Void)
    };
    (.Constant<ExpressionTrees.Program+<>c__DisplayClass0_0>(ExpressionTrees.Program+<>c__DisplayClass0_0).c3).p
}

My complete test code:

namespace ExpressionTrees
{
    class c1
    {
        public c2 c2 { get; set; }
    }

    class c2
    {
        public c3 c3 { get; set; }
    }

    class c3
    {
        public string p { get; set; }
    }

    class Program
    {
        static void Main(string[] args)
        {
            c3 c3 = null; 
            var test_c3 = NullGuard.Check(() => c3.p, true);
        }
    }

    public static class NullGuard
    {
        public static T Check<T>(Expression<Func<T>> expression, bool canThrowNullReferenceException = false)
        {
            var nullGuardVisitor = new NullGuardVisitor(canThrowNullReferenceException);
            var nullGuardExpression = nullGuardVisitor.Visit(expression.Body);            
            var nullGuardLambda = Expression.Lambda<Func<T>>(nullGuardExpression, expression.Parameters);
            var value = nullGuardLambda.Compile()();
            return value;
        }
    }

    public class NullGuardVisitor : ExpressionVisitor
    {
        private readonly bool _canThrowNullReferenceException;

        internal NullGuardVisitor(bool canThrowNullReferenceException)
        {
            _canThrowNullReferenceException = canThrowNullReferenceException;
        }

        protected override Expression VisitMember(MemberExpression node)
        {
            var expression = Visit(node.Expression);

            // expression == null
            var expressionEqualsNull = Expression.Equal(expression, Expression.Constant(null, expression.Type));

            if (_canThrowNullReferenceException)
            {
                var nullReferenceExceptionConstructorInfo = typeof(NullReferenceException).GetConstructor(new[] { typeof(string) });

                // if (expression == null) { throw new NullReferenceException() } else { node }
                var result = 
                    Expression.Block(
                        Expression.IfThen(
                            expressionEqualsNull,
                            Expression.Throw(Expression.New(nullReferenceExceptionConstructorInfo, Expression.Constant(node.Member.Name)))
                        ),
                        node
                    );
                return result;
            }
            else
            {
                var result = Expression.Condition(
                    expressionEqualsNull,
                    Expression.Constant(null, expression.Type),
                    node);
                return result;
            }

        }
    }
}

Why are you explicitly checking for null and throwing a NRE? If you don't want to propagate the null values, and instead just want to throw, all you need to do is *leave the expression unchanged* and the effect of executing the expression when the value is null will simply result in a NRE being throwing. You're just duplicating the null check *that C# is already doing for you*. — Servy, Jan 13 '16 at 21:56
On a side note, dealing with value types makes solving this problem more complex. Currently if your expression results in a non-nullable value you're still trying to return `null` in you propagate null path, and that won't work. You'll need to use `Nullable` in there. — Servy, Jan 13 '16 at 21:58
@Servy I'm doing this because I want to add a meaninigfull message to the NRE a not just a generic exception. Currently it's just the member name but later it should contain the full path up to the first `null`. — t3chb0t, Jan 13 '16 at 22:02
@Servy Side note to the side note. I've read about it in your answer [here](http://stackoverflow.com/a/30489160/235671) and actually this is based on your code ;-) I've just started to learn expression trees and these are my first attempts to create something useful... but this _evil_ exception won't let me ;-) I guess I will come to the nullable values as soon as I manage to make the exeption throwing to work. — t3chb0t, Jan 13 '16 at 22:04
To your first point, that makes sense. To the side note of the side not, just wanted to make sure that you knew you'd need to solve it eventually. Solving the easy case first is of course a great idea. — Servy, Jan 13 '16 at 22:09
Note that using statements here makes all of this *way* messier. You're way better off keeping everything as expressions. Avoid blocks, use the conditional operator over `if` statements, etc. Also, in your test expression, at least at first, use expressions that don't close over variables, as it results in much simpler and easier to debug expressions. Move onto closures after you get that working. Your problem was small, but very hard to find because it's obfuscated so much by the statements and lambda clutter. — Servy, Jan 13 '16 at 22:38
@Servy I was using conditional at the beginnig but the expression with an exception if true couldn't be compile due to argument type mismatch so I asked about it [here](http://stackoverflow.com/questions/34773933/how-can-i-create-an-expression-that-either-throws-an-exception-or-returns-a-valu) and rewritten it with Shlomo's solution. I guess condition is not that tolerant as far as different types are concerned and I couldn't return just void because it led to problems in the Equal later. — t3chb0t, Jan 14 '16 at 06:20
That's not a very hard problem to solve; I posted an answer to that question with the code to do it. — Servy, Jan 14 '16 at 14:20

score 4 · Accepted Answer · answered Jan 13 '16 at 22:30

It's working as it is supposed to.

Here's that same debug view with different white-spacing & row numbers:

1   .Block() 
2   {
3       .If (.Block() 
4       {
5           .If (.Constant<ExpressionTrees.Program+<>c__DisplayClass0_0>(ExpressionTrees.Program+<>c__DisplayClass0_0) == null) 
6           {
7               .Throw .New System.NullReferenceException("c3")
8           } 
9           .Else 
10          {
11              .Default(System.Void)
12          };
13          .Constant<ExpressionTrees.Program+<>c__DisplayClass0_0>(ExpressionTrees.Program+<>c__DisplayClass0_0).c3
14      } == null) 
15      {
16          .Throw .New System.NullReferenceException("p")
17      } .Else 
18      {
19          .Default(System.Void)
20      };
21      (.Constant<ExpressionTrees.Program+<>c__DisplayClass0_0>(ExpressionTrees.Program+<>c__DisplayClass0_0).c3).p
22  }

Look at line 13: it effectively says if ((closure class).c3 == null) throw new NullReferenceException("p"). Whereas your first check (line 5) effectively says if ((closure class) == null) throw new NullReferenceException("c3"). The problem is in the misleading exception message more than anything.

Thank you. Now I can see it too. I'll completely rewrite it and try to use a `while` loop instead of a custom visitor and recursion. It's hard to decide which answer I should accept. You seem to be both right ;-) — t3chb0t, Jan 14 '16 at 13:29

Servy · Answer 2 · 2016-01-13T22:58:14.133

3

Your issue is in the following snippet:

Expression.Throw(Expression.New(nullReferenceExceptionConstructorInfo,
    Expression.Constant(node.Member.Name)))

Let's say that this member access is a.b. You're checking a and then throwing an exception with a message saying b is null even though you just checked a.

You need to use node.Expression in the exception message if you want to say that a is null, rather than that you couldn't access b because its container was null.

edited Jan 13 '16 at 22:58

answered Jan 13 '16 at 22:34

Servy

202,030
26
332
449

I'm startig to believe that it's not possible to create an expression like that by traversing all parts recursively. Using `node.Expression.Type.Name` also doesn't seem to always work and strangely the last property access is not being checked. I try to rewrite it using a `while` loop over all nested expressions and build a new one without recursion. Maybe then I'll be able to use variables so that the debug view is easier to read and the expressions are not evaluated twice (in the condition and then when returning the value). – t3chb0t Jan 14 '16 at 08:11

Why is the innermost exception in an expression tree not thrown?

2 Answers2