Here's a first review; there may be more comments later, and @josdejong may weigh in as well. Jos, as the PR stands the nullish coalescing operator is only included in the full mathjs library, not the numbers-only library. Should it be in the latter as well? Also, in JavaScript, it is illegal to mix nullish coalescing with logical "and" as well as with logical "or," unless the expression is parenthesized. I think it is because the sense of the JavaScript designers was that the precedence between these operators is unclear, and so it might be difficult to understand the intention of an expression that was written without parentheses. As this PR stands, an expression like x && y ?? z is accepted in the MathJS Expressions language. Is that OK or would you prefer to match the JavaScript behavior?

Oh and also @ikemHood you may not have noticed that there is a list of operators and their precedences at docs/expressions/syntax.md. That needs to be updated in this PR. Thanks.

Thanks for reviewing Glen!

About the precedence of ??: good to think that through. I see in JavaScript (see docs) it has the same precedence as logical or ||. But according to the docs on MDN, the predecence is "intentionally undefined" (see docs):

However, the precedence between ?? and &&/|| is intentionally undefined, because the short circuiting behavior of logical operators can make the expression's evaluation counter-intuitive.

I don't fully understand, I would think you can just give the operator a different predecence like the PR does right now (like lower than and and or)? The predecence as currently implemented in the PR makes sense to me. @ikemHood @gwhitney do you have any thoughts in this regard?

About the precedence of ??: good to think that through. I see in JavaScript (see docs) it has the same precedence as logical or ||. But according to the docs on MDN, the predecence is "intentionally undefined" (see docs):

However, the precedence between ?? and &&/|| is intentionally undefined, because the short circuiting behavior of logical operators can make the expression's evaluation counter-intuitive.

I don't fully understand, I would think you can just give the operator a different predecence like the PR does right now (like lower than and and or)? The predecence as currently implemented in the PR makes sense to me. @ikemHood @gwhitney do you have any thoughts in this regard?

Basically the discussion in tc39/proposal-nullish-coalescing#26 about what the precedence of ?? should be was not converging, so the TC39 decided to punt and simply make it illegal to combine ?? with || or && at the same level without parentheses, to make the conversation about which way expressions like x ?? y || x "should" associate be moot: you must explicitly specify the order of operations using parentheses.

However, having now read over all the debates in some detail, I actually find the arguments that ?? isn't really analogous to || and && the most compelling, so that a good precedence for ?? would either match Swift/Kotlin (just higher than comparison operators like > and ==), or actually be even higher, such as higher-precedence than exponentiation ^ -- that high precedence is not used in any other language, but has a good justification in that there is no useful meaning for 2 ^ x ?? 1 as (2 ^ x) ?? 1 because (2 ^ x) is never nullish (in JavaScript, it is either 1 or NaN if x is null or undefined, and in MathJS Expressions it is a runtime error), so the only useful association is 2 ^ (x ?? 1). Ergo, the precedence of ?? should be higher than exponentiation. This seems like a really good argument to me, and would be simple to implement, but you may not want MathJS Expressions to be unlike any other programming language. On the other hand, looking overall at all of its features, it probably already doesn't exactly match any other language. On the third hand, you might want it to be as close to JavaScript as is is reasonable, since it is a JavaScript library, in which case you should choose either to exactly match JavaScript's precedence and make it illegal to combine ?? with || or && at the same level, or choose ?? as just lower precedence than ||, which would disambiguate all expressions (but disambiguate arithmetic expressions like x * y ?? z in a way that can never be useful).

To summarize the options that seem reasonable to me, in no particular order:

1. Exactly the same as JavaScript, solely to be compatible with JavaScript: same as ||, but not allowed to be mixed with || or && at the same level.
2. Just lower than ||: close-ish to JavaScript, matching C#, Dart, PHP, and CoffeeScript
3. Just higher than comparisons like < and ==: matching Swift/Kotlin, seems more useful than 1 or 2 in practice
4. Just higher than exponentiation: matches nothing else as far as I know, but actually seems the most useful to me.

I don't think @ikemHood can reasonably complete this PR until @josdejong makes a decision on precedence, so marking it as draft for now.

It makes a lot of sense to me to both (a) give it a different precedence from || and &&, and (b) give it a precedence right above exponentation ^. I indeed think the most common use case is to ensure a default value for a variable, and is typically not applied after a computation like (x * y) ?? z or (x ^ y) ?? z. I would say let's cater for the most common use case, and for other cases you can always use parenthesis.

About associativity: I think in practice the accociativity doesn't matter (like with addition), but it is good to define it officially I think. Left-to-right makes most sense to me: you pick the first non-nullish default value, going through it from left to right like in value ?? default1 ?? default2 ?? ....

@ikemHood do you have any thoughts on this?

And thanks for digging into this @gwhitney , really helpful!

Gwen was detailed. I'd support we give it a precedence higher than exponentiation.

Sounds good, let's go for it :)

Unless I missed them, there are still no unit tests of nullish the function (i.e., not written as ?? operator), and you haven't entered it into the tables in docs/expressions/syntax.md. Thanks!

Oh, and also, now that we have chosen a very different precedence for ?? than JavaScript does, that fact should probably be mentioned in the bullet list in the syntax.md. Either there should be a bullet that says "Some operator precedences differ" with a couple of examples if there is more than one such precedence difference (you'll need to compare the tables for the two languages), or if this is the only difference, say "The precedence of the nullish coalescing operator ?? is higher so that e.g. 2 ^ x ?? 0 is grouped as 2 ^ (x ?? 0)." Thanks.

there are still no unit tests of nullish the function (i.e., not written as ?? operator)

Sorry if I wasn't clear. There should be a test/unit-tests/function/logical/nullish.test.js, thanks. I will look at the new docs.

@gwhitney

Done,

I added a nullishTransform just like orTransform to achieve a true short-circuit

@gwhitney the last commit fixes the failing CI

@josdejong shouldn't there just be one line in AUTHORS for this contributor? And @ikemHood which of the two addresses would you like to appear? Thanks!

@josdejong shouldn't there just be one line in AUTHORS for this contributor?

I'll clean it up when publishing the next version. Apparently @ikemHood has created commits with two different git accounts.

@josdejong shouldn't there just be one line in AUTHORS for this contributor? And @ikemHood which of the two addresses would you like to appear? Thanks!

ikempeter2020@gmail.com

@gwhitney you've reviewed this PR before. Are there still open issues or is all your feedback addressed?

@josdejong @ikemHood the remaining concerns I can see are as follows:

• The handlers when the first argument is a scalar or SparseMatrix are very clear and crisp, with just one small point remaining: In the (SparseMatrix, Array) implementation, the Array is converted to Matrix just to get its size. But mathjs already has a size() function that handles matrices and arrays uniformly, so it seems to me it would be better to use this function to avoid conversions to create a temporary that's just thrown away after reading its size. Whether you two want to make this small improvement before merging I leave to you.

• After the above handlers in the main implementation of nullish in src/function/logical/nullish.js, the organization gets a bit harder to follow. I would suggest organizing them by type of first argument, since most types of first argument have already been handled. As far as I can tell, the only types left are DenseMatrix and Array, so I would suggest a section for each of these two types. I would suggest in each of these sections, the DenseMatrix, any (or Array, any) handler should be last, given that such functions are only used when the more specific types don't match (and in particular end up only being used for scalars, as all collection types are explicitly handled).

• This perspective makes it clear that all of the handlers with first type any can simply be deleted; all first argument types are handled by the collection of other handlers, making the any handlers redundant/misleading.

• The types returned with mixed Array/Matrix arguments do not seem to be consistent with other binary functions. As far as I can tell, the usual convention (e.g. for or) is that if either operand is a Matrix, the result is a Matrix. With the current set of handlers, it seems to me that if either operand is a SparseMatrix, the result is a (Sparse)Matrix, and if not, then if either operand is an Array, the result is an Array, and finally only if both arguments are DenseMatrix, the result is a DenseMatrix. The handlers should be modified to bring nullish() into conformity with the return type of other binary functions. Moreover, there should be one clean battery of tests in nullish.test.js that verifies that each combo of collection types produces the correct result type.

• By analogy with matrixAlgorithmSuite, it does seem reasonable that the handlers when the first type is Array are going to correspond to the DenseMatrix handlers, with matrix() conversion called on the first operand. By and large that's true, with the exception of the catchall scalar handler. There we currently have

        'DenseMatrix, any': typed.referToSelf(self => (x, y) =>
          matAlgo14xDs(x, y, self, false)
        ),

but

        'Array, any': typed.referToSelf(self => (x, y) => self(matrix(x), y).valueOf()),

which results in double redispatch (first to DenseMatrix, any and then to matAlgo14xDs with self) when nullish is called on an Array and a scalar. It would seem to me preferable to proceed analogously with the other Array handlers and use

       'Array, any': typed.referToSelf(self => (x, y) =>
           matAlgo14xDs(matrix(x), y, self, false).valueOf()),

for this handler, saving one round of redispatch.

My apologies for the large number of iterations of review on this PR, but the addition of a new operator is relatively far-reaching within the design of mathjs, and many of the points raised have only been able to come into focus when prior concerns were dealt with. I've made a strong effort in this comment to collect everything else I can see into a single place. I hope if/when these are all dealt with, the PR will be ready to merge, but I can't 100% promise that there will be nothing further on re-review when these are dealt with. Thanks for understanding!

Thanks for the overview Glen! The PR looks very neat already @ikemHood I hope you'll be able to address the last open ends!

Thanks for the overview Glen! The PR looks very neat already @ikemHood I hope you'll be able to address the last open ends!

Yes, done that.

Published now in v14.8.0, thanks again @ikemHood!