fix: numeric literals can be set as object keys and used to access objects #3498

codegiyu · 2025-06-30T01:57:25Z

Parser extended to accept numeric literals as object keys in object expressions.
Parser extended to accept numeric literals as object accessors including negative numbers.
Parser extended to accept operations and expressions resulting in numbers as object accessors.
If a number is parsed as an object key or accessor, it is converted to a string before being used as an object key / accessor.
If an operation or expression resulting in a number is parsed as an object accessor, the result of the operation / expression is evaluated before being converted to a string and used as an object accessor
Numeric literals, operations and expressions resulting in numbers are now accepted in square brackets for object assigments. The resulting number is first converted to a string before being used as an object key.
Care was taken to manage numbers with leading zeros either as object keys or accessors. eg 05. They're run through a Number constructor before being converted to strings to ensure there are no preceeding zeros
Dot notation for object assignment and accessing remains unchanged

Closes #3352

…jects

…rs handled

codegiyu · 2025-06-30T01:59:21Z

@josdejong ready for review

.eslintrc.cjs

.prettierrc.cjs

gulpfile.js

gwhitney · 2025-06-30T15:21:55Z

This is just a preliminary review; I have not had a chance to go over the core code changes that implement the new functionality. However, there are no new test cases to demonstrate the new functionality (and help prevent future regressions on this feature), and a few irrelevant changes that do not belong in this PR, which I have commented. Please remove the irrelevant changes and add a thorough suite of unit tests for the new feature, and we can move on to a more thorough code review. Thanks!

codegiyu · 2025-07-01T10:39:42Z

Understood...I'll remove the unnecessary eslint, prettier suggestions and whitespace improvement and add tests for the parser adjustments I've made

… auto" This reverts commit cedc04f.

codegiyu · 2025-07-01T18:01:53Z

@gwhitney unnecessary changes removed and unit test suites added....ready for review

gwhitney · 2025-07-01T20:56:36Z

@josdejong the contributor codegiyu has quite reasonably reflected the idiosyncrasy from JavaScript into the MathJS Expressions language that {2: 3} is a valid object literal with property '2' but {-2: 3} is a syntax error. Is that what MathJS would prefer? Or would you rather that {-2: 3}, for example, be allowed, as a literal for a plain object with property '-2'? I personally am a bit unclear on the motivation for JavaScript to disallow negative numeric literal keys in an object.

josdejong

Thanks a lot for picking this up @codegiyu! Nice.

I made a couple of inline comments, can you have a look at those?

josdejong · 2025-07-02T09:54:40Z

src/expression/node/AccessorNode.js

      const evalIndex = this.index._compile(math, argNames)

+      // If index contains operator node, evaluate result of the operation and access object with result
+      if (isOperatorNode(this.index.dimensions[0]) && isObjectNode(this.object)) {


This new section in AccessorNode adds support for operators inside the accessor like {2: 6}[2 * 1], but it doesn't allow any expression in general, for example it fails with {2: 6}[multiply(2, 1)].

I think the right place to update this behavior is not here in AccessorNode, but in IndexNode.isObjectProperty and IndexNode.getObjectProperty, see also my other comment in parse.js.

I'm having trouble updating this behaviour in IndexNode.getObjectProperty

I was able to compile the result of the operation because this was happening in a _compile method and I had access to math and argNames, so could call the compile method of the OperatorNode

In IndexNode.getObjectProperty, I don't have access to math and argNames, so can't use the _compile method on an OperatorNode in there. And I can't find another way to compile a result to the operation in an OperatorNode

I've looked into this a bit more, it's more complex than I had expected.

In AccessorNode._compile, there is an if (this.index.isObjectProperty()) { ... }. I think that part of the code must be removed, since it only works for static strings and not expressions inside the brackets. And also, we cannot detect anymore whether we're dealing with an object key (vs a matrix subset) by just looking at the index: when the index now contains just one dimension with one number, it could be an object key too. Therefore, this should be handled by the access(...) function. The logic in the function access that handles the case of an object can be extended to correctly evaluate numeric properties too. To do that, instead of the index.getObjectProperty() we need a method like index.evalObjectProperty(scope, args, context). We cannot use the normal IndexNode._compile(...) because that changes numeric indexes from 1-based to 0-based, and we don't want that in case of an object key.

Similarly, we need to update/extend the logic in AssignmentNode.

Maybe we should also get rid of the "smart" name function of AccessorNode, see get name() { ... }. It's not really needed in practice I think, and is not reliable anymore when it only works in "some" cases.

Does this direction sort of make sense?

Yes, it all make sense. Thank you for the directions. I will try to work it out and revert

josdejong · 2025-07-02T10:07:39Z

test/unit-tests/expression/parse.test.js

+
+    it('should throw an error when negative numbers are applied as keys in an object expression', function () {
+      assert.throws(function () {
+        parseAndEval('{-1: 34}')


I agree with Glen: I cannot come up with a reason why we wouldn't support negative numbers. Can you implement support for negative values too? Or do you have an other opinion?

Yes, I believe I can. I left it out earlier because javascript also didn't seem to accept that

@josdejong I was able to make this happen as well by modifying a part of the parseObject function.

Right before it would throw an error due to not finding a symbol, string or number as an object key, if the current token is '-', it would try to parseUnary and if the resulting node is an operator node with just a constant node with a number value, then it would proceed to use the number preceeded by a '-' as the key

I think you're idea is the same as Glen implemented for the % operator just yesterday in #3505, that indeed works out nicely.

josdejong · 2025-07-02T10:16:50Z

src/expression/parse.js

+        // If param value is number and node is object node, make param value a string
+        if (typeof params[0].value === 'number' && isObjectNode(node) && params.length === 1 && isConstantNode(params[0])) {
+          // Number constructor is first used to manage situations of numbers with preceding zero digit(s)
+          params[0].value = String(Number(params[0].value))


Since it can be the case that the actual (numeric or string) key is evaluated at runtime and not parse time, I think this logic should not be located here but when evaluating the actual key, in functions like Indexnode.isObjectProperty and IndexNode.getObjectProperty. However, these functions currently determine whether dealing with an object node statically. I think this needs to be changed to determine this after evaluating it.

Alright, I will look into effecting the change from IndexNode instead

You're right...making the changes in IndexNode will affect assignments too, which is something I overlooked.

I'm thinking of introducing an optional property like 'forObjectNode'. But since there's already an optional property for dot notation, I'm now thinking of having an optional options object that would contain dotNotation and forObjectNode.

I've been able to mostly move this logic to IndexNode.isObjectProperty and .getObjectProperty

I did attach a boolean property, 'forObjectNode' to params[0] here

I opted for this against modifying the structure of arguments to the IndexNode constructor as I initially intended in my earlier comment, because this felt more controlled and still got the job done

However, using that forObjectNode property I attached to the nodes being fed into the IndexNode constructor, I've been able to recreate this logic you commented on here in IndexNode's isObjectProperty and getObjectProperty

@josdejong Can you please review this update?

I think it makes sense to await reviewing until you've tried out the latest ideas, or is there a specific part of the code on which you already would like to have more feedback?

@josdejong I've pushed my latest changes and I would like your review.

The earlier logic I had in parseAccessor has been completely moved out.
IndexNode.isObjectProperty and getObjectProperty have been restored to only recognize and return strings like before

To be able to handle numbers, operations and expressions in matrix indexes for object accessing and assignments, I relied on a singular DenseMatrix being present in the index._dimensions array recieved in the access and assign functions

Problems are that

Somewhere in the Index transform process, it passes through the _createImmutableMatrix method in MatrixIndex and throws an error if non-integers are used

The tests for getting names accurately for AccessorNode, AssignmentNode, FunctionNode are failing

A few security tests related to not allowing calling Functions via various things (bind, constructor, etc) are failing...however they're failing because the errors gotten now don't exactly match the errors expected. But trying out the test cases still results in an error being thrown

Thanks for the updates, I'll do some testing and debugging with the latest version of your PR soon (I expect tomorrow).

It can indeed be that some of the security tests now give differing error messages, that is fine.

Thank you very much

About (2): the tests for names will need to change if we drop support for these "smart" names. It will be a breaking change.

Thinking about it however, it may be better to leave it as it is for now and mark it deprecated, then we don't introduce a breaking change right now, and it may be better to try focus on getting numbers working as index, which is complex enough already. Let's try one step at a time 😅 .

gwhitney · 2025-07-02T18:47:10Z

OK, now that @josdejong has reviewed the code in more detail, I will leave feedback on your changes to him. But @codegiyu you may want to push your changes so far so that Jos can see them. If you don't think they're ready for merging yet, you should click the "convert to draft" link, and then you can mark it "ready for review" when you think it's back in shape. Thanks!

…x/numeric-object-keys

…ogic moved to IndexNode

…/mathjs into fix/numeric-object-keys

…mber operations/expressions

josdejong · 2025-07-11T14:52:59Z

I'll reply here in the main thread since I get a bit lost in the inline conversations 😅.

Let me try to explain in a bit more detail where I think we need to go. It may be the case that this resolves some of the current questions/issues already.

The method access needs to change from:
```
access(object, index)
```
To:
```
access(object, evalIndex, evalProperty, scope, args)
```
Because access now needs to determine whether to evaluate as index or as property. Depending on whether dealing with an Array, Matrix, or Object, the function access will first call evalIndex(scope, args, context) or evalProperty(scope, args) and then apply it to the array/object.

The AccessorNode._compile method will then look something like this:

_compile (math, argNames) {
  const evalObject = this.object._compile(math, argNames)
  const evalIndex = this.index._compile(math, argNames)
  const evalProperty = this.index._compileProperty(math, argNames)  // DOES NOT YET EXIST

  return function evalAccessorNode (scope, args, context) {
    const object = evalObject(scope, args, context)
    return access(object, evalIndex, evalProperty, scope, args)
  }
}

The AssignmentNode._compile method needs a similar change as AccessorNode._compile
We need a new method IndexNode._compileProperty, which replaces the need for our old isObjectProperty and getObjectProperty. This needs to return an evaluation function which tries to evaluate the index as a property, which can be a string or number. If that is not successful, the function throws an exception.

Does this help?

josdejong · 2025-07-23T10:07:57Z

@codegiyu did you see my last feedback on this PR? Please let me know if you need more pointers or help.

codegiyu · 2025-07-23T10:37:53Z

@codegiyu did you see my last feedback on this PR? Please let me know if you need more pointers or help.

I'm so sorry, I didn't notice your feedback earlier. I'm checking it out now

codegiyu · 2025-07-23T10:43:43Z

Thank you for the guide on a direction on modifying a few of the methods....I'll get it sorted as much as I can before midnight tonight

josdejong · 2025-07-23T12:32:57Z

Thanks for the update! There is no need to hurry, I just wanted to know if it's still on your radar. Take your time.

codegiyu added 3 commits June 28, 2025 19:29

fix: eslint and prettier config updated to allow endOfLine as auto

cedc04f

fix: numeric literals can be set as object keys and used to access ob…

70de523

…jects

fix: number literals with preceeding zeros as object keys and accesso…

384e0fe

…rs handled

gwhitney requested changes Jun 30, 2025

View reviewed changes

.eslintrc.cjs Outdated Show resolved Hide resolved

.prettierrc.cjs Show resolved Hide resolved

gulpfile.js Show resolved Hide resolved

codegiyu added 3 commits July 1, 2025 12:13

Revert "fix: eslint and prettier config updated to allow endOfLine as…

90cb3f6

… auto" This reverts commit cedc04f.

revert: unnecessary change to gulpfile.js removed

349f39d

feat: added thorough suite of unit tests

eafba64

codegiyu requested a review from gwhitney July 1, 2025 18:00

Merge branch 'develop' into fix/numeric-object-keys

e8377d1

josdejong requested changes Jul 2, 2025

View reviewed changes

codegiyu added 3 commits July 3, 2025 09:01

Merge branch 'develop' of https://github.com/josdejong/mathjs into fi…

9d854ba

…x/numeric-object-keys

fix: negative number object keys allowed and number object accessor l…

f95f57f

…ogic moved to IndexNode

Merge branch 'fix/numeric-object-keys' of https://github.com/codegiyu…

8edbfe4

…/mathjs into fix/numeric-object-keys

codegiyu marked this pull request as draft July 3, 2025 13:00

codegiyu requested a review from josdejong July 3, 2025 13:00

codegiyu added 2 commits July 5, 2025 18:58

draft: object assignment and accessing with matrix indexes accepts nu…

12b23bf

…mber operations/expressions

Merge branch 'develop' into fix/numeric-object-keys

59060fd

Uh oh!

Uh oh!

fix: numeric literals can be set as object keys and used to access objects #3498

Are you sure you want to change the base?

fix: numeric literals can be set as object keys and used to access objects #3498

Uh oh!

Conversation

codegiyu commented Jun 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codegiyu commented Jun 30, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gwhitney commented Jun 30, 2025

Uh oh!

codegiyu commented Jul 1, 2025

Uh oh!

codegiyu commented Jul 1, 2025

Uh oh!

gwhitney commented Jul 1, 2025

Uh oh!

josdejong left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gwhitney commented Jul 2, 2025

Uh oh!

josdejong commented Jul 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

josdejong commented Jul 23, 2025

Uh oh!

codegiyu commented Jul 23, 2025

Uh oh!

codegiyu commented Jul 23, 2025

Uh oh!

josdejong commented Jul 23, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

codegiyu commented Jun 30, 2025 •

edited

Loading

josdejong commented Jul 11, 2025 •

edited

Loading