Bugs and inconsistencies in logical operations

[gwhitney]

Describe the bug
In many cases, calling logical operations on complex numbers or Fractions leads to an error when it appears the value should be well defined; sometimes problems crop up with bigints or units as well

To Reproduce
All of the following fail, with various errors:

math.boolean(math.complex(1,1))
math.boolean(math.fraction(1,1))
math.boolean(100000000000000000000000n)
math.not(math.fraction(1,1))
math.and(math.fraction(1,1), math.fraction(1,1))
math.and(2, math.unit(1, 'g'))
math.evaluate('and(complex(1,1), complex(1,1))')) // math.and(math.complex(1,1), math.complex(1,1)) works!
math.or(math.fraction(1,1), math.fraction(1,1))
math.or(2, math.unit(1, 'g'))
math.evaluate('or(complex(1,1), complex(1,1))')) // ditto to above comment
math.xor(math.fraction(1,1), math.fraction(1,1))
math.xor(2, math.unit(1, 'g'))
// note math.evaluate('xor(complex(1,1), complex(1,1))') works.

Discussion
Note that I am not at all sure the list above is exhaustive of the problematic cases; it's very hard to make certain that a given test list is complete. Also, the fact that complex numbers fail with "and" and "or" in the expression parser but not in direct calls, and work ok with "xor" and "not" in both ways suggests that it has to do with short-circuiting, which only applies in the expression parser and only with "and" and "or".

Note that the logical expressions are defined by a series of implementations for different types. As a suggestion, I think it would be more robust and clearer to focus on the implementation of the boolean() conversion, and then just define the logical expressions by any arguments, converting the arguments to boolean as needed. If such a suggestion is taken, it would be essentially trivial to resolve #2483 at the same time.

[gwhitney]

Worth considering #3415 before deciding how to address this.

[gwhitney]

In addition, suppose n = math.bignumber(NaN). Then we have math.boolean(n) === true and math.evaluate('n ? true : false', {n}) === true but math.and(n, true) === false. These are inconsistent among themselves. The bignumber value n should be consistently treated as falsy or consistently treated as truthy; the former seems better for consistency with the ordinary JavaScript number type.

[gwhitney]

In particular, here's a complete external drop-in replacement for the mathjs functions and, or that solves all of the above problems for those functions, and short-circuits in expressions (so no transforms would be needed), and handles arbitrarily many arguments, and converts to the #3415 convention of returning the argument with the truthy/falsy value computed, rather than just true or false (but it could be trivially changed to return the boolean value instead, if #3415 is not desirable). It doesn't quite pass all of the unit tests because of the inconsistency noted in the previous comment; but resolving that inconsistency would make it pass. If it looks reasonable, you can let me know which way mathjs should go with respect to #3415, and I can turn this into a PR. If I did so, there would be a decision as to whether to delegate to map, as this does, or to use matrixAlgorithmSuite as most internal implementations do -- I would be open to suggestions.

import math from './src/defaultInstance.js'

function logicalFn(invert) {
    function logicalCombiner(...callArgs) {
        let input = []
        let unevaluated = false
        let scope = undefined
        let last = callArgs.length
        /* First check if we are being called from the expression parser via
           rawArgs.
           Is there a more definitive/foolproof way?
         */
        if (
            last === 3
            && Array.isArray(callArgs[0])
            && math.isNode(callArgs[0][0])
        ) {
            input = callArgs[0]
            last = input.length
            unevaluated = true
            scope = callArgs[2]
        }
        let value = invert
        let collection = false
        let terminated = false
        for (let i = 0; i < last; ++i) {
            const arg = unevaluated ? input[i].evaluate(scope) : callArgs[i]
            const argColl = math.isCollection(arg)
            if (collection) {
                if (argColl) value = math.map(value, arg, logicalCombiner)
                else value = math.map(value, elt => logicalCombiner(elt, arg))
            } else {
                if (argColl) {
                    value = math.map(arg, elt => logicalCombiner(value, elt))
                    collection = true
                } else {
                    // scalar-scalar case, in which we don't consider any
                    // more arguments once the truth value is determined,
                    // except that in the non-short-circuiting (direct
                    // JavaScript call) case, we continue in case we encounter
                    // a collection.
                    if (terminated) continue
                    let test = math.boolean(arg)
                    if (invert) test = !test
                    if (test) {
                        if (unevaluated) return arg
                        terminated = true
                    }
                    value = arg
                }
            }
        }
        return value
    }
    logicalCombiner.rawArgs = true
    return logicalCombiner
}

math.import({and: logicalFn(true), or: logicalFn(false)}, {override: true})
math.import({boolean: math.typed('boolean', {
    Complex: z => z.re !== 0 || z.im !== 0,
    Fraction: r => !(r.equals(r.neg())),
    bigint: b => b !== 0n,
    Unit: u => !!u.value,
})})

Note in particular that and vs or differ only in one line of code (whether the logical sense (truthiness or falsiness) of each argument is inverted or not before testing). That high level of similarity is not exploited in existing code; the above code shares virtually the entire implementation for the two functions.

[josdejong]

Fraction and Unit support is missing in these error cases. When a function does not support a data type, mathjs tries to automatically convert to an other data type that is supported by the function. In the case of math.boolean(math.fraction(1,1)), it finds a conversion from Fraction to BigNumber but that conversion throws an exception and hence is not helpful in this case.

In short, I think the solution is to extend the logical functions (boolean, and, not, or, xor) with support for Fraction, Unit, and Complex. The solution may not be a breaking change, if in the end it only adds support for missing data types.

As a suggestion, I think it would be more robust and clearer to focus on the implementation of the boolean() conversion, and then just define the logical expressions by any arguments, converting the arguments to boolean as needed

That may work indeed! Or maybe we should only implement the boolean signature like and(boolean, boolean), and make sure all automatic conversions from any type to a boolean are in place. Not sure if that would work out nicely, just a thought.

Thanks for the example code for a solution Glen!

Help working out a PR would be welcome!

[gwhitney]

Sadly this comment shows that any resolution of this issue will be a breaking change, hence part of v15 presumably. I of course prefer to let all the arguments into the logical operator rather than coerce them into boolean in hopes you will relent on #3415 ;-) but more practically because it means we don't have to worry about whether every conversion to boolean will be installed as an allowed implicit conversion -- it's hard for me to gauge whether or not we will want all such conversions to be implicit, long-term. But there's no problem with executing explicit conversion to boolean inside the logical operators, if they take any arguments.

[josdejong]

Maybe we can split this into two separate PR's: one which is non-breaking and adds support for Fraction, Unit, and Complex to the boolean operations, and a second PR which can contain breaking changes and focuses on inconsistencies.