fix: parse unary not, when used with function-call syntax, as a function call

[gwhitney]

Previously, not(object).method() would be parsed as not applied to object.method(), which is confusing since it appears to be the same syntax as a unary function call followed by a method call on the result, like say matrix(object).size() which works perfectly well when object is an Array that has no method size, because the result of matrix(object) is a Matrix which does have a method size.

This PR changes the parse so that not(object).method() applies the not operator to object and then calls method() on the result, to be parallel with the parse when not is replaced by any unary function symbol.

In other words, it changes the parse of not(object).method() from not ((object).method()) to (not(object)).method(), so that what appears to be a unary function call in fact behaves like a unary function call.

The parse of not object.method() remains as it was, namely not (object.method()), since not used as a unary operator has lower precedence than the . for method selection and calling.

Resolves #3608.

(Note this PR follows proposed resolution III of #3608; I prepared a PR for it as an attempt to avoid a two-week turnaround if in the end @josdejong is comfortable with that resolution of the problem.)

[josdejong]

Thanks Glen, I finally understand what you mean 😜

I made one inline comment, can you have a look at that?

[josdejong]

Would it be possible to "invert" this code into something like the following? So it is more explicit what happens in the case of not(....

if (name === 'not' || state.token === '(') {
  return parseAssignment(...)
}

const params = [parseUnary(state)]
return new OperatorNode(name, fn, params)

[gwhitney]

OK, I have reversed the sense of the test for not(.... appearing as a unary function-call. But I don't think it makes sense to actually fully parse the unary function-call syntax right there in parseUnary, since we already will hit code later that parses it, and we don't want to essentially duplicate that code here, as that will create a subtle maintenance issue where if we ever need to change the code for parsing (unary) function calls, we will be obliged to make a corresponding change here or else run the risk of creating a bug or parsing inconsistency. Seems better to have the code for parsing (unary) function calls in just one place. What do you think?

Also, now that per-function history is in I thought about it but did not add a HISTORY entry to the parse() function for this change as it seems to be more of a bugfix than a feature change, which is I think mostly what we want to capture in HISTORY. But it is on the edge so if you want something in the parse() HISTORY just let me know.

Thanks!

[josdejong]

@gwhitney I see your update inverting the if. Would it also be possible to call parseAssignment explicitly? (instead of changing state and then continuing).

[gwhitney]

I really don't think that's a good idea, for the code-maintenance reasons outlined above: unary function-call should only be parsed in one place, not two. What's the motivation for replicating the function-call parsing in this location?

[gwhitney]

Ah, I could instead return parseSymbol(savedState) because (at the moment!) we have enough information to know that the tests in parsePow, parseNullishCoalescing, and parseCustomNodes (the intermediate "layers" between where we are and parseSymbol) will all fail and the next test that will succeed will be in parseSymbol. So that would be acceptable if you strongly prefer it to just dropping through and letting the parse continue. But it has its own maintenance cost: what if the layers are rearranged or refactored or new ones that might fire are introduced? Then again, it might become subtly incorrect to just jump to parseSymbol here. So I would prefer to just drop through, and I definitely don't want to reproduce the innards of parseSymbol here, but if you have a strong position that it would be better to short-circuit straight to parseSymbol here I would accept that compromise. Let me know.

[josdejong]

Ah, sorry, yes I mean parseSymbol, not parseAssignment. Sorry for the confusion 😅 .

When replacing Object.assign(state, saveState) with return parseSymbol(savedState) it is explicit how the expression is being parsed.

[gwhitney]

OK, inserted return parseSymbol(state) at the point where not is detected. Note that it's necessary to restore state to what it was before grabbing the ( token; you can't just return parseSymbol(saveState). In other words, the state object that comes into parseUnary must be aliased somewhere, so that if it doesn't agree with the state passed into parseSymbol then the parse is spoiled. Should I file an issue about that, or just not worry about it since the parser is working as long as you properly update the input state object in every parseXxx() function? (I'm inclined toward the latter.)

[josdejong]

Neat solution, thanks, I like it!

The need for restoring the state via Object.assign(state, saveState) is indeed ugly, it means that we pass and mutate state here and there and use the mutated state. If that is the case, we should not pass state as an argument but use it as a global variable 🤔 (global inside of the parser). Yeah, good idea to open a separate issue to have a look at this.

[gwhitney]

I have opened #3629; hopefully that leaves this PR ready to merge.