Boolean child expressions evaluate incorrectly when specifying the right operand

[beechj]

I recently updated from version 1.3.1 to 1.4.2, since then child boolean expression no longer evaluate correctly if you specify the right operand.

Steps to reproduce the behavior:

1. Create a child expression that evaluates to false
2. Create a second child expression that also evaluates to false
3. Create an outer expression that references the child expressions and specifies the right operand e.g. "[ChildExpression1] == true || [ChildExpression2] == true"
4. This will incorrectly evaluate to true
5. Change the outer expression to simply: "[ChildExpression1] || [ChildExpression2]"
6. Now the outer expression correctly evaluates to false

Expression variables should respect the right operand regardless of whether they are boolean or not. This was working in 1.3.1

• OS: Windows
• OS Version: 7 SP1
• Version 1.4.2

[beechj]

I did some debugging and found that the errant code is in VariableExpression:

`// Check to see if we have to referred to another expression.
if (variables[this.variableName] is IExpression expression)
{
return expression.Evaluate(variables);
}

return variables[this.variableName];`

Expression.cs does not implement IExpression and so it is not evaluated, instead it is returned as is.
I can send you a pull request with a fix if you wish?

[beechj]

#54 created which addresses the issue

[bijington]

@beechj first off sorry for introducing this bug, there was a fair amount of refactoring that took place to get to 1.4+.

Thank you for not only raising it but also supplying the fix 🙂.

I have had a brief look over the PR as I don’t have my laptop to hand but it looks fine to me. Unfortunately I won’t be likely to actually approve the PR until the end of next week but assuming all is fine then I can aim to get a patch version up on NuGet then also.

[bijington]

Also really nice idea on allowing expressions in the playground! I hadn’t considered that before

[beechj]

@bijington no problem, have a look at it when you get chance. For now I have dropped back to 1.3.1 because we have a lot of child expressions in our use case.

I think you may be missing a few test cases with regard to child expressions which is why it got missed. I have added a few logical ones so hopefully that should cover it.

Was actually a very simple change to allow child expressions in the playground and it was the easiest way to test my change 😉

[bijington]

@beechj apologies for the delay but I am back and have been over the PR which looks fine.

You are certainly correct about a lack of unit testing in this area, I do believe there were some (probably not enough) but they must have been lost during the last bit refactor.

I am currently refactoring the parser internals so that I can boost unit test coverage of that area. My aim is to get as close to 100% coverage as possible at some point :).

[bijington]

This is now available in release https://github.com/bijington/expressive/releases/tag/v1.4.3

[beechj]

@bijington no worries, I'll give this version a go 👍

[bijington]

@beechj great! I did forget to mention that it is also available via NuGet if that is your preferred option 🙂