Bug in ExecutionVisitor(BinaryExpression exp) ?

Mar 18, 2010 at 4:46 PM

I have the following JavaScript source:

var _maxDate = null;

if(_maxDate == null || _rowDate.compareTo(_maxDate) >0 ){

 

Now, _rowDate is definitely not null, _maxDate is, yet this code explodes. I'd have thought with left-to-right evaluation, when the first term is evaluated to true, the second term is not evaluated. Looking at the source for ExecutionVisitor(BinaryExpression exp), I can see that for OR, both terms are always evaluated. Is this a bug?

Mar 18, 2010 at 4:59 PM

I've a fix which seems to work for me, it doesn't evaluate the 2nd term if we're doing an OR and the first term is true:

 

 

        public void Visit(BinaryExpression expression)
        {
            JsInstance old = Result;
            // Evaluates the left expression and saves the value
            expression.LeftExpression.Accept(this);
            JsInstance left = Result;
            Result = null;
            // Evaluates the right expression and saves the value
            JsInstance right;
            if ((expression.Type != BinaryExpressionType.Or) || !left.ToBoolean())
            {
                expression.RightExpression.Accept(this);
                right = Result;
            }
            else
            {
                right = JsBoolean.False;
            }

        public void Visit(BinaryExpression expression)

        {

            JsInstance old = Result;

 

            // Evaluates the left expression and saves the value

            expression.LeftExpression.Accept(this);

            JsInstance left = Result;

 

            Result = null;

 

            // Evaluates the right expression and saves the value

            JsInstance right;

            if ((expression.Type != BinaryExpressionType.Or) || !left.ToBoolean())

            {

                expression.RightExpression.Accept(this);

                right = Result;

            }

            else

            {

                right = JsBoolean.False;

            }

 

Mar 18, 2010 at 5:18 PM

I've created an issue in IssueTracker, along with a proposed fix which I've tested. The fix there also fixes short-circuit evaluation problems with AND.