Evaluating 'help(not)' throws SyntaxError

[ovk]

For some reason, evaluating help(not) throws an error. For example:

// Works fine
console.log(math.evaluate('help(and)'));

// "SyntaxError: Value expected (char 9)"
console.log(math.evaluate('help(not)'));

Please see this jsfiddle example.

[josdejong]

Thanks for reporting. That is not helpful 😉 .

I think the reason is that the parser interprets not as operator here, as in not true, so it expects something after the not. In case of the operator and this is no issue because the parser already knows when it's used as operator: when it is preceded by an operand.

Anyone able to have a look into this?

[joao99sb]

For some reason, evaluating help(not) throws an error. For example:

// Works fine
console.log(math.evaluate('help(and)'));

// "SyntaxError: Value expected (char 9)"
console.log(math.evaluate('help(not)'));

Please see this jsfiddle example.

I'm new with contribution on open source project, this will be my first try. i will attempt to fix this bug, but i have a some doubt.

1. i did not understand the utility of function evaluate at all
2. where can i find the file of this function ?

[josdejong]

Thanks for your offer @joao99sb . Solving this issue may not be the easiest to get started with, but it would be great if you can give it a try 👍

The issue is in the expression parser, which can interpret not in two ways: as operator like not true or as function like not(true). The parser expects not to be followed by "something", but in case of help(not) that is not the case.

You can find the code of the expression parser here: https://github.com/josdejong/mathjs/blob/develop/src/expression/parse.js

[gwhitney]

Another example of the same bug: map([true, false], not) which throws rather than returning [false, true] as expected. I would be very happy to address this bug. My recommended resolution process:

(1) First decide if something along the lines of #3379 will be adopted. If so, it will as a side effect eliminate this bug, so just wait for that. I can fold resolving #3379 into #3423 or do it as a follow-on PR or do it as an independent PR, as @josdejong prefers.

(2) If #3379 ends up decided against, then if the refactoring approach represented by #3423 is approved (which I would broadly characterize as switching to "table-driven parsing"), I can easily fold a fix into the final version of that PR for review.

(3) If the approach in #3423 doesn't meet with approval either, then I will need to do a more limited PR to unify OperatorNode and FunctionNode, which was discussed long ago and is long overdue (the reason for my wanting to do it now is that in the current bifurcated state, it's really hard to do special TeX handling for both the // operator and the quotient function without duplicating code -- you can see that duplication existing already with the templates for the divide, pow, and multiply functions in src/utils/latex.js as compared to the switch on the identifier of an OperatorNode in the _toTex method of that class). In this case, I can fold this fix into that unification PR, or do it as a follow-on to that PR.

I guess to summarize: I believe that fixing this will become much easier with/after (1), (2), and/or (3) and so would prefer not to try to fix this bug directly in the parser as it stands. (I would guess that the complexity/organization of the current parser code is at least part of why Jos commented above, in effect, that this is not a "good first issue".)

[josdejong]

I've just looked into #3379, and right now we will not move into supporting unary function calls without parenthesis (like sin x), so I think your option (1) is not possible right now. Option (2) looks good so far and can indeed solve this issue, though it is quite a large refactor and may take time.

I've had a look into fixing this in the parser as it is right now, see #3470. It's not pretty but we could solve it like this for the time being. What do you think @gwhitney? My feeling right now is that we can better wait for a proper solution rather than use this workaround, also be cause this is quite an edge case.

[gwhitney]

I agree that #3470 feels very special-case. Given that mathjs has been unable to parse help(not) for years, I for one am happy to wait for a parsing overhaul to address this. On the other hand, #3470 is also a very small code change, so if you prefer to merge it now just to have this corrected and the tests in the suite for future parsing revisions, I'm not going to object. It makes current parsing spaghetti only marginally more tangled ;-)

[josdejong]

Let's wait for the proper fix rather than implementing a workaround that may never be cleaned up again.