Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
codegiyu wants to merge 12 commits into
base: develop
Choose a base branch
from
Draft

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

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
cedc04f
fix: eslint and prettier config updated to allow endOfLine as auto
codegiyu Jun 28, 2025
70de523
fix: numeric literals can be set as object keys and used to access ob…
codegiyu Jun 30, 2025
384e0fe
fix: number literals with preceeding zeros as object keys and accesso…
codegiyu Jun 30, 2025
90cb3f6
Revert "fix: eslint and prettier config updated to allow endOfLine as…
codegiyu Jul 1, 2025
349f39d
revert: unnecessary change to gulpfile.js removed
codegiyu Jul 1, 2025
eafba64
feat: added thorough suite of unit tests
codegiyu Jul 1, 2025
e8377d1
Merge branch 'develop' into fix/numeric-object-keys
josdejong Jul 2, 2025
9d854ba
Merge branch 'develop' of https://github.com/josdejong/mathjs into fi…
codegiyu Jul 3, 2025
f95f57f
fix: negative number object keys allowed and number object accessor l…
codegiyu Jul 3, 2025
8edbfe4
Merge branch 'fix/numeric-object-keys' of https://github.com/codegiyu…
codegiyu Jul 3, 2025
12b23bf
draft: object assignment and accessing with matrix indexes accepts nu…
codegiyu Jul 5, 2025
59060fd
Merge branch 'develop' into fix/numeric-object-keys
codegiyu Jul 10, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions .eslintrc.cjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,10 @@ module.exports = {
'@typescript-eslint/no-unused-vars': [
'error',
{ argsIgnorePattern: '^_', varsIgnorePattern: '^_' }
],
'prettier/prettier': [
'error',
{ endOfLine: 'auto' }
Comment thread
codegiyu marked this conversation as resolved.
Outdated
]
},
plugins: ['@typescript-eslint'],
Expand Down
3 changes: 2 additions & 1 deletion .prettierrc.cjs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
module.exports = {
semi: false,
singleQuote: true,
trailingComma: 'none'
trailingComma: 'none',
endOfLine: 'auto'
}
Comment thread
codegiyu marked this conversation as resolved.
2 changes: 1 addition & 1 deletion gulpfile.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import log from 'fancy-log'
import webpack from 'webpack'
import babel from 'gulp-babel'
import { mkdirp } from 'mkdirp'
import { cleanup, iteratePath } from './tools/docgenerator.js'
import { cleanup, iteratePath } from './tools/docgenerator.js'
import { generateEntryFiles } from './tools/entryGenerator.js'
Comment thread
codegiyu marked this conversation as resolved.
import { getAllFiles, validateChars } from './tools/validateAsciiChars.js'

Expand Down
10 changes: 10 additions & 0 deletions src/expression/node/AccessorNode.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import {
isIndexNode,
isNode,
isObjectNode,
isOperatorNode,
isParenthesisNode,
isSymbolNode
} from '../../utils/is.js'
Expand Down Expand Up @@ -93,6 +94,15 @@ export const createAccessorNode = /* #__PURE__ */ factory(name, dependencies, ({
const evalObject = this.object._compile(math, argNames)
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)) {
Copy link
Copy Markdown
Owner

@josdejong josdejong Jul 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Author

@codegiyu codegiyu Jul 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Owner

@josdejong josdejong Jul 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Author

@codegiyu codegiyu Jul 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

const operatorNode = this.index.dimensions[0]
return function evalAccessorNode (scope, args, context) {
const result = operatorNode._compile(math, argNames)(scope, args, context)
return getSafeProperty(evalObject(scope, args, context), String(result))
}
}

if (this.index.isObjectProperty()) {
const prop = this.index.getObjectProperty()
return function evalAccessorNode (scope, args, context) {
Expand Down
14 changes: 10 additions & 4 deletions src/expression/parse.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { factory } from '../utils/factory.js'
import { isAccessorNode, isConstantNode, isFunctionNode, isOperatorNode, isSymbolNode, rule2Node } from '../utils/is.js'
import { isAccessorNode, isConstantNode, isFunctionNode, isObjectNode, isOperatorNode, isSymbolNode, rule2Node } from '../utils/is.js'
import { deepMap } from '../utils/collection.js'
import { safeNumberType } from '../utils/number.js'
import { hasOwnProperty } from '../utils/object.js'
Expand Down Expand Up @@ -1413,6 +1413,12 @@ export const createParse = /* #__PURE__ */ factory(name, dependencies, ({
closeParams(state)
getToken(state)

// 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))
Copy link
Copy Markdown
Owner

@josdejong josdejong Jul 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Author

@codegiyu codegiyu Jul 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Author

@codegiyu codegiyu Jul 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Author

@codegiyu codegiyu Jul 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Author

@codegiyu codegiyu Jul 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@josdejong Can you please review this update?

Copy link
Copy Markdown
Owner

@josdejong josdejong Jul 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Author

@codegiyu codegiyu Jul 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

  1. Somewhere in the Index transform process, it passes through the _createImmutableMatrix method in MatrixIndex and throws an error if non-integers are used
  2. The tests for getting names accurately for AccessorNode, AssignmentNode, FunctionNode are failing
  3. 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

Copy link
Copy Markdown
Owner

@josdejong josdejong Jul 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Author

@codegiyu codegiyu Jul 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you very much

Copy link
Copy Markdown
Owner

@josdejong josdejong Jul 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 😅 .

}

node = new AccessorNode(node, new IndexNode(params))
} else {
// dot notation like variable.prop
Expand Down Expand Up @@ -1615,11 +1621,11 @@ export const createParse = /* #__PURE__ */ factory(name, dependencies, ({
// parse key
if (state.token === '"' || state.token === "'") {
key = parseStringToken(state, state.token)
} else if (state.tokenType === TOKENTYPE.SYMBOL || (state.tokenType === TOKENTYPE.DELIMITER && state.token in NAMED_DELIMITERS)) {
key = state.token
} else if (state.tokenType === TOKENTYPE.SYMBOL || (state.tokenType === TOKENTYPE.DELIMITER && state.token in NAMED_DELIMITERS) || state.tokenType === TOKENTYPE.NUMBER) {
key = state.tokenType === TOKENTYPE.NUMBER ? String(Number(state.token)) : state.token
getToken(state)
} else {
throw createSyntaxError(state, 'Symbol or string expected as object key')
throw createSyntaxError(state, 'Symbol, numeric literal or string expected as object key')
}

// parse key/value separator
Expand Down