josdejong / mathjs

An extensive math library for JavaScript and Node.js
https://mathjs.org
Apache License 2.0
14.32k stars 1.24k forks source link

null/undefined treated as valid only for left side of comparison operators #1391

Open rcrogers opened 5 years ago

rcrogers commented 5 years ago

Using mathjs@5.4.1

When the null value is on the right side, the correct error is thrown (actual: null):

> require('mathjs').eval('1 > null')
TypeError: Unexpected type of argument in function larger (expected: number or Array or DenseMatrix or SparseMatrix or string or boolean or Matrix or BigNumber or Complex or Fraction, actual: null, index: 1)

However, when the null value is on the left side, the error mistakenly identifies the right side as the source of the problem (actual: number):

> require('mathjs').eval('null > 1')
TypeError: Unexpected type of argument in function larger (expected: Array or DenseMatrix or SparseMatrix or Matrix, actual: number, index: 1)
josdejong commented 5 years ago

Thanks for reporting, the second error isn't very helpful indeed.

It's not a very easy fix, has to do with the way the types of a function are matched against the different signatures of the function.

rcrogers commented 5 years ago

Makes sense. How feasible would it be to change the function def resolution so that a match only occurs if both operator arguments are compatible with the parameters of the function signature?

josdejong commented 5 years ago

The reason in this case is because there are signatures like larger(any, DenseMatrix) to allow multiplying a scalar and a matrix:

https://github.com/josdejong/mathjs/blob/develop/src/function/relational/larger.js#L108-L132

So, thinking about it, the solution may be quite easy (though it requires some labor): replace the any wildcard with explicit types, ie. replace

  const larger = typed('larger', {
    // ...
    'any, DenseMatrix': function (x, y) {
      return algorithm14(y, x, larger, true)
    },
    // ...
  })

with

  const larger = typed('larger', {
    // ...
    'boolean | number | BigNumber | Fraction | Complex | Unit, DenseMatrix': function (x, y) {
      return algorithm14(y, x, larger, true)
    },
    // ...
  })

for all six occurrences, and do this for all operators (not only larger).

Downside is that when you add support for a new data type, you have to add the data type at many places. This can be partly resolved by defining one string on top and use that everywhere like:

  const any = 'boolean | number | BigNumber | Fraction | Complex | Unit'

  const larger = typed('larger', {
    // ...
    `${any}, DenseMatrix`: function (x, y) {
      return algorithm14(y, x, larger, true)
    },
    // ...
  })

Anyone interested in thinking such a solution through and implementing the fix?

nelsongnanaraj commented 3 years ago

I too facing the same issue when I passed the null into the expression..

Please find the error message below ERROR TypeError: Unexpected type of argument in function addScalar (expected: number or string or boolean or BigNumber or Complex or Fraction, actual: null, index: 1)

Do you have any workaround or temporary solution for this case ?

m93a commented 3 years ago

Hey Nelson, The obvious workaround is not passing null into expressions 😅️ Is there a specific reason for why you do it?

nelsongnanaraj commented 3 years ago

I am working for dynamic expression parser using mathjs. So i have to allow the null values in mathjs. Here i shared the simple case of expression.

ifCondition(isNull(dateField1),today(),addDate(dateField1,add(numberField1,numberField2),'DAY'))

ifCondition, isNull, today, addDate are my own defined functions.

IsNull function is highly required for check most of the business cases. Without allowing null in data, How we can identify and return whether the specific expression having null or not.

gwhitney commented 11 months ago

This is definitely a bug, and it definitely still exists. In fact, it's worse, in that if M is a matrix, null > M also reports TypeError: Unexpected type of argument in function larger (expected: Array or DenseMatrix or SparseMatrix or Matrix, actual: number, index: 1) when there isn't even any number in that expression... Hopefully this issue will go away in v13, but if it doesn't, it should be explicitly addressed.