Boolean child expressions evaluate incorrectly when specifying the right operand
Closed this issue · 9 comments
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:
- Create a child expression that evaluates to false
- Create a second child expression that also evaluates to false
- Create an outer expression that references the child expressions and specifies the right operand e.g. "[ChildExpression1] == true || [ChildExpression2] == true"
- This will incorrectly evaluate to true
- Change the outer expression to simply: "[ChildExpression1] || [ChildExpression2]"
- 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
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 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.
Also really nice idea on allowing expressions in the playground! I hadn’t considered that before
@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 😉
@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 :).
This is now available in release https://github.com/bijington/expressive/releases/tag/v1.4.3
@bijington no worries, I'll give this version a go 👍