Fix not being able to use and and or inside a function definition

[josdejong]

Fixes #3143.

The issue was that in OperatorNode, we didn't use createSubScope in case of rawArgs functions, like we do in FunctionNode.

This is needed because during evaluation of a custom defined function, we have two scopes:

• "scope" holding the "global" scope variables, the values provided via math.evaluate('...', scope)
• "args" holding the local function arguments of a function, like x in function f(x) = fawFunction(x)^2

When using a rawArgs function, these two scopes where merged into one using createSubScope. Without it, you get an error like "undefined symbol x". However, createSubScope copied all variables from both scopes in a new Map, which made it impossible to do assignments like (a = true) and (b = true), because the assigned variables would end up in the subscope instead of the main scope.

This PR solves this by utilizing a new wrapper Map instance, named PartitionedMap. It holds references to both scope and args. It looks and acts like a normal Map, but it keeps the two scopes separated under the hood, and hence can read/write to the right scope.

As a side effect, we're now not copying the scope anymore inside createSubScope, we only instantiate a wrapper interface around it. This improves the performance of custom defined functions 🚀.

[josdejong]

Fix published now in v12.3.2

[pjanik]

@josdejong, this was released as a minor version bump, but it seems to me that it's a solid breaking change.

Our app was entirely relying on the custom scopes and the createSubScope method used by it. Now it's all gone, and I'm not entirely sure if it's even possible to fix it.

Could we somehow restore support for custom scopes with custom methods instead of passing around this PartitionedMap?

[josdejong]

I'm sorry to hear that Piotr. Can you share a minimal example of the kind of logic that you're using and that now doesn't work anymore?

[pjanik]

@josdejong, thanks! I'll think about a minimal example tomorrow.

I actually managed to fix it on our side, as our custom Scope was still accessible as the .a property of the new PartitionedMap. However, I probably need to test it a bit more to ensure that our original custom scope doesn't eventually get lost. Previously, I wasn't worried about this, as our custom MathJS scope was implementing the expected interface, including:

  createSubScope() {
    return this;
  }

Now, it seems to be available as the .a property, but I suspect that if createSubScope() from src/utils/map.js is called a few times, .a might eventually reference another PartitionedMap.

Essentially, we're implementing an app with formula support, similar to a very simple spreadsheet. We rely heavily on the custom MathJS scope that exposes custom methods necessary for app-specific calculations.

We used to define our formula functions as:

someFunction: {
    evaluateRaw: (args: MathNode[], mathjs: any, scope: CustomMathJSScope) => {
      scope.customMethod()
      // ...
    }

but now they looks more like:

someFunction: {
    evaluateRaw: (args: MathNode[], mathjs: any, partitionedScope: { a: CustomMathJSScope }) => {
      const scope = partitionedScope.a;
      scope.customMethod()
      // ...
    }

[pjanik]

@josdejong, here's a minimal working example that uses MathJS v12.3.1:
https://jsfiddle.net/0arxye24/

The idea is to use a custom Scope class that provides .customMethod(). This used to be possible because we could control how the sub-scopes are created via .createSubScope(). In this particular example, scope is just returning itself, but it could clone itself properly. Our scope is never wrapped in some MathJS class.

I based that implementation on the fact that the official documentation described scope as an object that implements the Map interface, and also on this example (which seems to be outdated now, btw):
https://mathjs.org/examples/advanced/custom_scope_objects.js.html

I believe it would be great to still be able to provide a custom Scope implementation (even if it needs to implement this partitioning) and control how the sub-scopes are created, as in previous implementations.

[josdejong]

I actually managed to fix it on our side

That is good news!

In general: there should be no need to do things using const scope = partitionedScope.a, and doing that can introduce weird issues. Instead, you can get the function out of it using a getter:

someFunction: {
  evaluateRaw: (args: MathNode[], mathjs: any, scope: CustomMathJSScope) => {
    // formerly scope was an Object: 
    //     scope.customMethod()
    // now, it is a Map:
    const customMethod = partitionedScope.get('customMethod')
    customMethod()
  }
}

[josdejong]

Thanks for the jsfiddle. Looking into that I see that you extend the Map interface, that may indeed give difficulties when the Map internally is wrapped by another Map interface.

Isn't it possible to put your customMethod as a regular value in the Map instead? like myMap.put('customMethod', ...)

[pjanik]

Isn't it possible to put your customMethod as a regular value in the Map instead? like myMap.put('customMethod', ...)

In theory, it could work for some simple use cases, although I think it's not very maintainable (and not TypeScript friendly). Here's the custom scope that I'm talking about: https://github.com/concord-consortium/codap/blob/8e7fc861af3d29502d7bf23f678d05cf93c262c2/v3/src/models/formula/formula-mathjs-scope.ts

So, I'm not sure what to do with all the custom functionality implemented in this class.

[josdejong]

I hadn't realized that this method createSubScope() might have been used in public. It was not unit tested nor documented, except in this example custom_scope_objects.js which I hadn't seen, so I thought it was some internal left-over and did remove it, sorry.

Thanks for sharing this class with your code. I will try to understand why createSubScope is and attaching custom methods to a Map interface is needed in the first place. We may come up with a different solution for your case.

[josdejong]

A minimal usage example would help me understand what we're talking about. Do you have any?

[pjanik]

Thanks for sharing this class with your code. I will try to understand why createSubScope is and attaching custom methods to a Map interface is needed in the first place. We may come up with a different solution for your case.

Thanks.

I think the general concept is that a simple Map works for basic math evaluation. However, if you want dynamic evaluation of saved math expressions (like in a spreadsheet), the custom scope object is a powerful concept that can provide that functionality.

Therefore, I think it would be great if we could officially support these custom scopes in MathJS. This would require two things:

• Custom Scope needs to implement whatever MathJS needs (e.g., Map interface + partitioning + whatever else in the future). This could be easily controlled by TypeScript interface.
• Custom Scope needs to control how it's cloned when a subscope is necessary. This ensures it can return itself or clone properly, ensuring that other code always works with one class (rather than some MathJS wrapper).

A minimal usage example would help me understand what we're talking about. Do you have any?

I think this JSFiddle is the best, minimal example I can think of. Of course, it's not very useful by itself.

In a real-life scenario, think about a spreadsheet where formulas are evaluated using MathJS. Some of the custom functions might modify what scope.get("someKey") returns during a single expression evaluation (e.g., providing some cached values). Some complex behaviors would be difficult to achieve with regular, basic maps used as scopes.

[josdejong]

Can you explain what the need is to pass these custom methods, attached to the Map interface, towards a "someFunction"?

i.e. isn't it possible to do something like the following?

const context = {
  customMethod: () => { ... }
}

someFunction: {
  evaluateRaw: (args: MathNode[], mathjs: any, partitionedScope: { a: CustomMathJSScope }) => {
    context.customMethod()
  }
}

Or is this customMethod dynamic or so?

[pjanik]

It's hard to explain all that in isolation.

But maybe I can give another, simpler example - even our .get(key) method is way more complex than regular Map.get():
https://github.com/concord-consortium/codap/blob/8e7fc861af3d29502d7bf23f678d05cf93c262c2/v3/src/models/formula/formula-mathjs-scope.ts#L62-L110

We do not want to pre-populate Map instance with all possible key-values, as this would be a huge performance hit and generally not reasonable in our scenario. So, our custom .get() method looks for the required key and calculates value lazily (and then caches it). This is not something we can do with regular Map.

[josdejong]

But that complexity is nicely contained inside the Map implementation behind the regular .get() method, right?

I think the core of the issue is that you want to attach and invoke custom methods on a Map interface, like scope.customMethod(). Then the question is, is there an alternative way to provide the function where you need to invoke these custom methods? ie. pass a context with these custom methods to your custom someFunction.

UPDATE: and my second question is: why are these custom methods needed in the first place, why isn't the Map interface with get/set enough? (I'm not criticizing or anything but just trying to understand the use case😅 )

[pjanik]

But that complexity is nicely contained inside the Map implementation behind the regular .get() method, right?

Assuming that my custom Map is allowed (as the documentation changed from scope: Object to scope: Map) and it will always be maintained at the top of the map hierarchy, and MathJS never assumes it's a regular Map and e.g. tries to clone it, then yes, you're right. So, it all depends on whether we're pretending to be a Map (and MathJS thinks it's a native Map that can be wrapped in another Map or class) or if we're explicitly allowed to use a custom class implementing the Map interface and MathJS respects that.

Then the question is, is there an alternative way to provide the function where you need to invoke these custom methods? ie. pass a context with these custom methods to your custom someFunction.

This is difficult for me to answer without spending some significant time on refactoring. I bet it's possible (as almost everything is possible 😉), but it's probably not pretty. Since the formulas are evaluated like:

  const compiledFormula = math.compile(formula)
  const result = compiledFormula.evaluate(scope)

and custom functions are defined in a clean, stateless way:

const customFunction = (args, mathjs, scope) => { ... }

I don't see a better place for custom functionality than this custom scope, as that's the only context that we actually control. Setting a module or global variable isn't a pretty approach, even though it could work. Another non-pretty approach would be to do scope.set("objectWithCustomMethods", objectWithCustomMethods) and then get it inside the function. So surely there are ways. But not necessarily ones that I'd like to use. But I'd need to think about it more. It's a bit difficult for me atm too, as I cannot spend more time on such experiments and refactoring on our side.

and my second question is: why are these custom methods needed in the first place, why isn't the Map interface with get/set enough? (I'm not criticizing or anything but just trying to understand the use case😅 )

Imagine our custom scope has two methods: scope.enableCaching() and scope.disableCaching(). These methods toggle a flag that affects how scope.get() works. Achieving this is difficult when working with an object limited to the Map interface and a separate object with custom methods. While we could do scope.set("__secret_key__cachingEnabled", true), this is not a long-term solution. I would rather explore other options than be forced to implement most of the functionality in this way.

[josdejong]

Assuming that my custom Map is allowed (as the documentation changed from scope: Object to scope: Map) and it will always be maintained at the top of the map hierarchy, and MathJS never assumes it's a regular Map and e.g. tries to clone it, then yes, you're right.

That is indeed the case: mathjs does not clone or copy the scope, it only wraps around it to add a sub-scope on top of it for keeping say the variable x in a custom defined function f(x) = x^2 when executing this function. The old implementation indeed copied all variables into a new Map instance, and it did call parentScope.createSubScope() for that, but that is now not needed anymore.

Thanks for your explanation about the context, I think you're right, it sounds like the most logic way to pass context to your custom function (I expect the alternative would require jumping though hoops).

Imagine our custom scope has two methods: scope.enableCaching() and scope.disableCaching(). These methods toggle a flag that affects how scope.get() works.

Thanks, this helps me a lot understanding the case 😅 👍 . This makes sense indeed, it is very neat to keep these custom funcions are stateless/standalone. I also agree that using something like scope.set("__secret_key__cachingEnabled", true) is ugly.

So, understanding your use case now and thinking about a solution, I think your earlier solution will indeed work perfectly fine. It would be good though to write a (recursive) helper function for this I think to make it more robust, something like:

// usage:
someFunction: {
    evaluateRaw: (args: MathNode[], mathjs: any, scope: Map | PartitionedMap) => {
      const rootScope = getRootScope(scope);
      rootScope.customMethod()
      // ...
    }
}

function getRootScope(scope) {
  return isPartitionedMap(scope)
    ? getRootScope(scope.a) // recurse
    : scope
}

function isPartitionedMap(map) {
  return isMap(map) && isMap(map.a) && isMap(map.b)
}

function isMap(map) {
  // ... check existence of methods get, set, has, etc...
}

[pjanik]

Thanks! This recursive approach gives me peace of mind, as it should indeed be safe. Things seemed to work even without it, but I was worried that at some point, our custom scope would be buried too deep and we wouldn't find it at the .a property.

This seems safe enough for us to move forward. However, I wonder if we're still in some gray, undocumented area, and if these interfaces shouldn't be documented somewhere. I could imagine adding a few words about this in the documentation, and maybe the .a and .b names could be more descriptive, or there could be some design that hides these details from use cases like ours. But obviously, this doesn't need to happen now, just something to think about long-term.

Actually, one of the easiest approaches could be MathJS exporting getRootScope itself and ensuring it always works. This way, we wouldn't need to worry about what kind of scope we're currently dealing with, and no one would be locked into the current implementation with .a and .b properties (or whatever they become in the future).

Thanks a lot for all the responses!

[josdejong]

Sounds good 💪

I'll write about this in the docs, add unit tests that verify that the passed scope always is a Map or PartitionedMap, update the example custom_scope_objects.js, and see whether I can export at least helper functions isMap and isPartitionedMap.

I'll give a refactoring of the PartitionedMap some more thought too, maybe expose two getters for the two internal maps but it is indeed not the biggest concern right now.

For your solution: I would also add an extra check in the code to verify whether getRootScope() indeed returned a FormulaMathJsScope instance, and if not throw an exception that you can capture in unit tests. That should guard against a new type of Map instance is added in the future that requires a different way to get to the root scope (I can't think of anything in that direction but you never know).