Support for optional chaining #3547
Conversation
|
Awesome Nils, I love optional chaining. I'll review your PR coming week. |
…or function calls. Added more tests for parsing.
…ests for optional chaining with function nodes.
src/expression/node/AccessorNode.js
Outdated
| * Forces evaluate to undefined if the given object is undefined or null. | ||
| */ | ||
| constructor (object, index) { | ||
| constructor (object, index, optionalChaining) { |
There was a problem hiding this comment.
Can you define the default value at construction? Like
constructor (object, index, optionalChaining = false) {
// ...
}
src/expression/node/FunctionNode.js
Outdated
| const object = evalObject(scope, args, context) | ||
|
|
||
| // Optional chaining: if the base object is nullish, short-circuit to undefined | ||
| if (object === null || object === undefined) { |
There was a problem hiding this comment.
In order to reduce the amount of code duplication, can we maybe keep use a single evalFunctionNode, and add there the option chaining part:
if (fromOptionalChaining && (object === null || object === undefined)) {
// ...
}I think the reason to duplicate it is for performance. I'm not sure though if this has any impact on performance, so I think for now it's best to either (a) go for the solution with simplest code or (b) validate with a benchmark wether this has performance impact and only if so duplicate the code.
| const b = new SymbolNode('b') | ||
| const add = new OperatorNode('+', 'add', [a, b]) | ||
| const bar = new AccessorNode(add, new IndexNode([new ConstantNode('bar')]), true) | ||
| assert.strictEqual(bar.toString(), '(a + b)?.["bar"]') |
There was a problem hiding this comment.
This example a bit made up I think. How about an example like (cond ? obj1 : obj2)?.["bar"]?
| const a = new SymbolNode('a') | ||
| const foo = new AccessorNode(a, new IndexNode([new ConstantNode('foo')]), true) | ||
| const bar = new AccessorNode(foo, new IndexNode([new ConstantNode('bar')]), true) | ||
| assert.strictEqual(bar.toString(), 'a?.["foo"]?.["bar"]') |
There was a problem hiding this comment.
Can you add a test for a case like 'a?.foo?.bar' too?
I tried math.parse('obj?.b').toString() and this returns 'obj?..b' instead of obj?.b so I think something needs to be fixed there.
src/expression/node/AccessorNode.js
Outdated
| return function evalAccessorNode (scope, args, context) { | ||
| // get a property from an object evaluated using the scope. | ||
| return getSafeProperty(evalObject(scope, args, context), prop) | ||
| if (this.optionalChaining) { |
There was a problem hiding this comment.
The number of evalAccessorNode implementations now grows from 2 to 6. I think they all make sense, but I'm wondering if we can do something smarter here to prevent this growth and duplication of code, to make it easier to maintain. Any ideas?
We could try out if whether it has a negative performance impact if we create only 1 or 2 versions of evalAccessorNode that contain a few conditional cases.
|
I see some tests are failing on Firefox. Can you have a look at that? See https://github.com/josdejong/mathjs/actions/runs/18316821660/job/52338944661 |
|
I will have a look for the given points at the weekend or next week. I don't like the duplications of the eval funtions either, but I wanted to achieve the best possible eval-performance and don't do any comparisons there, which can be done at compile time. But I will look if there is a better solution and will run some benchmark to check the impact . |
|
Thanks! |
|
@NilsDietrich did you manage to have a look at the last open ends? I'm looking forward to have this PR merged :). Please let me know if you need help. |
AccessorNode-constructor optionalChaining = false as default. Used object == null instead of (object === null || object === undefined)
…es dot-notation. Improved tests of AccessorNode.
…t/optional-chaining
…surable differences.
|
I fixed the things you mentioned. I've also added a benchmark for the AccessorNode. I was unable to measure any performance benefits from having different callback-variants, so I simplified the code to only have the original callbacks and check the optional-chaining paths inside. |
|
Thanks Nils! That sounds good. I'll review your PR next week (time for weekend now 😅). |
2 participants
This pull request adds support for optional chaining '.?' in the expression parser. Till now it's hard to securely access paths of objects which may be undefined:
Scope: { a: { b: { c: 3 } } }
Expression: 'result = (a==undefined ? undefined : (a.b==undefined ? undefined: a.b.c ))'
With optional chaining this can be simplified to:
'result = a?.b?.c'
This feature helps to make the expression language more handy (after support for the nullish coalescing operator (??) was added in #3497).
The new functionalty is fully provided by changes to the AccessorNode.