mathjs icon indicating copy to clipboard operation
mathjs copied to clipboard

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

Open codegiyu opened this issue 6 months ago • 11 comments

  • 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

codegiyu avatar Jun 30 '25 01:06 codegiyu

@josdejong ready for review

codegiyu avatar Jun 30 '25 01:06 codegiyu

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!

gwhitney avatar Jun 30 '25 15:06 gwhitney

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

codegiyu avatar Jul 01 '25 10:07 codegiyu

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

codegiyu avatar Jul 01 '25 18:07 codegiyu

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

gwhitney avatar Jul 01 '25 20:07 gwhitney

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!

gwhitney avatar Jul 02 '25 18:07 gwhitney

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.

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

  2. 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)
      }
    }
    
    
  3. The AssignmentNode._compile method needs a similar change as AccessorNode._compile

  4. 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 avatar Jul 11 '25 14:07 josdejong

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

josdejong avatar Jul 23 '25 10:07 josdejong

@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 avatar Jul 23 '25 10:07 codegiyu

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

codegiyu avatar Jul 23 '25 10:07 codegiyu

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.

josdejong avatar Jul 23 '25 12:07 josdejong