Add support for scopes with get and set methods by jhugman · Pull Request #2166 · josdejong/mathjs

jhugman · 2021-04-10T14:42:57Z

This wraps the scop Object in a ObjectWrappingMap and finds all the places where getSafeProperty (and friends) was being used on the scope, and changes it to getMapProperty (and friends).

It also adds an example, some tests and some documentation.

I think it shows a fairly clear path to:

making objects constructed by expressions to be Maps, instead of objects.
wrapping the math object passed through the AST as a Map instead of objects.

These should eliminate the blacklist approach to securing mathjs.

Additionally, it shows a reasonable path to making symbol lookup (including units) to be extensible.

jhugman

customs.js defines some getSafeProperty and setSafeProperty which should be modified to recognized objects with get and set methods.
Add functions for hasSafeProperty(object, prop) and getSafeProperties(object). Between these four functions we should be able to get and set, test membership, and enumerate entries.
Find and replace instances of if (… in scope) or for (… in scope) with the newly minted functions.
Add tests to evaluate.test.js, simplify.test.js and security.test.js.
Add an example of using a custom scope object.

I don't think this is ready for landing yet, (see checklist here), but it is breaking the build, so would appreciate some help with that, and a feedback review. @josdejong

josdejong · 2021-04-17T14:50:36Z

Thanks @jhugman for your PR! I'll review it soon

josdejong

First, sorry for my late response, I try to keep up but sometimes I lack time or energy.

Over all this PR looks really good and well thought through, thanks a lot @jhugman! I made some comments in the code.

One important thing we need to decide upon is whether we already want to start giving users this deprecation warning, I think that may be a bit too early: I would like to have worked out a proof of concept of the evaluator purely based on Map instead of Object before introducing those warnings. That way we know for sure if it's feasible, and at that point we know what the impact is and what will be the breaking changes exactly.

I think completely replacing the internal usage of Object in the expression parser involves:

scope should be Map instead of Object
the math namespace used in the expression parser (mathWithTransform) should be Map instead of Object.
Parser and cli.js should use Map instead of Object (already in the PR)
Input like math.evaluate('{a: 2, b: 3}') should create Map instead of Object. This is very breaking!

I think it would be good to go about this in small steps, for example:

Step 1: first introduction of using Map as scope. Keep everything backward compatible. (This PR)
Step 2: work out a proof of concept fully replacing usage of Object in the expression parser with Map.
Step 3: based on the outcome of Step 2, work out a solution which replaces Object with Map everywhere internally, but still must be backward compatible. This version may give deprecation warnings, or we may decide that we want to introduce a config option to toggle using either Object or Map or something like that.
Step 4: drop the old code in a new breaking release. Create helper functions to ease migration where needed.

jhugman · 2021-04-25T15:44:16Z

Ok, I found a bug: I hadn't actually being doing the right thing with creating sub scopes.

I pulled on a thread, and it kept coming. A series of fairly mechanical refactors makes it look like there's a lot, but not much really.

Once I'd introduced hasSafeProperty and getSafeProperties in previous commits, I could find all the places using SafePropert*(scope to ScopePropert methods, which forward to the equivalent SafeProperty methods. This really unlocks some useful transformations.

Getting these ScopeProperty methods to check if the scope was MapLike, and error if not let me find all the entrance points to evaluate and simplify, wrap and object into a Map like object, moving most of the SafeProperty calls out of evaluate methods, and into the Object wrapper.

From there, a re-organization of Map functions and objects was relatively simple: all the working parts are in a new file ./src/utils/map.js.

At this point, SafeProperty functions from customs.js are only protecting math objects, and in-language created objects. I'm not going to attempt that here, but there's a good path forward now: we can treat the methods on math as a different namespace/scope, and internal implementations of in language objects and arrays can diverge their unsafe Javascript counterparts.

Responding to your review, and some other questions:

I've renamed MapLike to Map. In general, I migrated from SafeProperty to ScopeProperty to MapProperty.
We can inline all calls to MapProperty functions to their map methods. I'd keep them around for now until the migration from internal objects and math namespace has been completed.
I don't understand why parser has a scope. It seems to come up in the security tests quite a lot, but not many places else.

Still to do:

Tests for utils/map.js
Micro-optimizing isMap as suggested.
Write some documentation as suggested.
Consider renaming ObjectWrapperScope as ObjectWrappingMap.
Look at review comments around cli.js
Convert Parser.js to use a Map as suggested.

I'm not quite finished, but the end is in sight.

jhugman

Not quite ready, but would like your feedback @josdejong .

josdejong · 2021-04-28T15:02:30Z

-              return evalArg(scope, args, context)
-            }))
-          }
+        return function evalFunctionNode (scope, args, context) {


The three cases for 1, 2, or more than 2 arguments where here for performance: looping over a map is slower than just calling 1 or 2 functions. It would be nice to simplify the code, but please let's only do that after verifying that it doesn't have negative impact on the performance. It may be a good idea to add a short comment explaining this 😁

Ok, I made a switch statement. How are you benchmarking/profiling to make these micro-optimizing decisions?

There is no full-fledged benchmark suite, but there are some tests for this in the folder test/benchmark. In this case we can run test/benchmark/expression_parser.js against both the current develop branch and this PR and see if there is any noticeable difference.

I see the benchmark files are broken because they're loaded as ES module instead of CommonJS (still using require), will fix that in develop

josdejong · 2021-04-28T15:06:46Z

I really like where this is heading @jhugman!

Consider renaming ObjectWrapperScope as ObjectWrappingMap

ObjectWrapperScope is a really nice solution! I like the name ObjectWrappingMap better indeed :)

jhugman · 2021-05-03T23:16:28Z

Write some documentation in docs/expressions/parsing.md.

Aside from the above documentation, I think this is ready for review! Thanks for your nice feedback so far.

jhugman · 2021-05-08T13:13:37Z

Ok! I've added the last bit, a section in the documentation.

I think this is in a place where it should be sqashed and landed before moving forward with the next block of work for de-objectifying mathjs.

@josdejong When you have some time could you review again, and we can work towards the Big Green Button for me.

josdejong

@jhugman this looks very solid, thanks for the updates. Using a Map interface everywhere works out really nicely, like in Parser.js!

I've updated the benchmark ./test/benchmark/expression_parser.js to make it work and have a proper comparison with plain JS using Map, updating the following code to:

const expr = '2 + 3 * sin(pi / 4) - 4x'
const scope = new Map([
  ['x', 2]
])
const compiled = math.parse(expr).compile(math)

function undefinedSymbol(name) {
  throw new Error('Undefined symbol "' + name + '"')
}

const sin = getSafeProperty(math, 'sin')
const pi = getSafeProperty(math, 'pi')
const compiledPlainJs = {
  evaluate: function (scope) {
    return 2 + 3 * (scope.has('sin') ? scope.get('sin') : sin)((scope.has('pi') ? scope.get('pi') : pi) / 4) - 4 * (scope.has('x') ? scope.get('x') : undefinedSymbol('x'))
  }
}

It looks like you're PR makes the expression parser slightly faster than it was before, at least there is no regression in performance 🎉

I've added some inline feedbacks in the latest version of the PR and closed some outdated conversations, can you have a look?

josdejong · 2021-05-12T09:13:54Z

+  if (isMap(mapOrObject)) {
+    return mapOrObject
+  }
+  if (typeof mapOrObject === 'object') {


I think it is better to use the isObject util function here.

typeof mapOrObject === 'object' is also true when mapOrObject is null, one of those JS pitfalls

josdejong · 2021-05-12T09:16:54Z

+}
+
+/*
+ * These wrapper functions are temporary. They provide scaffolding around calling the methods


Are those "temporary" functions like setMapProperty still needed at this point, or can the be replaced with a direct call map.set(...)? Since the expression parser now works against a Map interface everywhere, there should be no need for wrapper functions anymore I think?

I think we can get rid of these, though they may come back as scaffold for near future refactors.

Let's then clean them up for now, and when needed we can recreate them (it's not rocket science), OK?

Ok, I've done this.

(btw: I use the 👍 reaction as a check mark to myself that I've addressed the issue).

josdejong · 2021-05-12T09:22:10Z

@@ -0,0 +1,40 @@
+import { createEmptyMap, isMap } from './map.js'
+
+function assign (scope, ...objects) {


Minor detail, but this functionality like assign and createSubScope are not "scope" specific, they are very generic util functions for objects in general. We could move them to utils/object.js maybe and rename "scope" to "target", etc

I wondered about this.

assign could live in utils/object or utils/map, though my preference is map: if it's analogue is Object.assign(target, ...objs). This assign, target is always a Map, even though objs is (Map|Object)[].

createSubScope does feels scope specific, which is why I put it here. It should forward to the scope if it can.

makes sense to rename the file scope.js to map.js, you're right! Maybe createSubScope can be renamed to createSubMap :)

I've moved assign to map.js, and changed the body of createSubScope, and changed the examples/custom_scope_object.js example. Hopefully this should be instructive.

josdejong · 2021-05-12T09:24:02Z

+  it('should provide isMap, a function to tell maps from non-maps', () => {
+    assert.ok(isMap(new Map()))
+    assert.ok(!isMap([]))
+    assert.ok(!isMap({}))


Can you add some tests like isMap(null), isMap(undefined), isMap(42), etc?

josdejong · 2021-05-12T09:31:44Z

-              return evalArg(scope, args, context)
-            }))
-          }
+        return function evalFunctionNode (scope, args, context) {


There is no full-fledged benchmark suite, but there are some tests for this in the folder test/benchmark. In this case we can run test/benchmark/expression_parser.js against both the current develop branch and this PR and see if there is any noticeable difference.

josdejong · 2021-05-12T09:37:40Z

-              return evalArg(scope, args, context)
-            }))
-          }
+        return function evalFunctionNode (scope, args, context) {


I see the benchmark files are broken because they're loaded as ES module instead of CommonJS (still using require), will fix that in develop

josdejong · 2021-05-12T09:41:33Z

@@ -0,0 +1,113 @@
+import { setSafeProperty, hasSafeProperty, getSafeProperty } from './customs.js'
+
+export class ObjectScopeWrapper {


Just some more thoughts on the name: shouldn't it reflect that is a Map? I still think ObjectWrappingMap or ObjectWrapperMap is quite clear, but if you want to keep it like this (ObjectScopeWrapper) I'm fine with that too.

This is renamed ObjectWrappingMap.

jhugman · 2021-05-12T12:54:27Z

Ok, I think I've addressed everything today.

josdejong · 2021-05-16T08:33:28Z

Thanks @jhugman , looks good!

I think everything is resolved and ready. On small thing: I do not see a unit test for isMap(null) and isMap(undefined) for example, or am I overlooking them?

jhugman · 2021-05-16T09:17:31Z

https://github.com/josdejong/mathjs/pull/2166/files#diff-5c678b2623ae91fc4802b34d1bde80ff249f70d89c1e352e1348adb463415accR26-R29

…

a unit test for isMap(null) and isMap(undefined) for example, or am I overlooking them?

josdejong · 2021-05-16T11:25:57Z

https://github.com/josdejong/mathjs/pull/2166/files#diff-5c678b2623ae91fc4802b34d1bde80ff249f70d89c1e352e1348adb463415accR26-R29
…
a unit test for isMap(null) and isMap(undefined) for example, or am I overlooking them?

Ahh, yes, that's perfect 👍 . Ready to merge then 😄

josdejong · 2021-05-16T12:07:54Z

Published now in v9.4.0, thanks again @jhugman, nice job !

jhugman · 2021-05-16T22:01:19Z

@josdejong Thank you for your help!

…

On Sun, May 16, 2021 at 12:08 PM Jos de Jong ***@***.***> wrote: Published now in v9.4.0, thanks again @jhugman <https://github.com/jhugman>, nice job ! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2166 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAYICTG4RJDL2JNT62ELGTTN6YSPANCNFSM42WSLXDQ> .

* Add support for scopes with get and set methods * Fix build for node v12 * Fixup cli and parser tests * Add tests for simplify and evaluate * Add example for a custom scope object * Function calls need child scopes * Transitionary step: Separate Safe and Scope Property calls * Renamed identifiers in FunctionNode * Evaluate with ObjectScopeWrapper * Simplify tests passing * Assume all scopes are map-like. Except parser * Remove isMapLike check in customs.*SafeProperty() methods * Change MapLike to Map * Move keywords from an Object to a Set * Move ScopeProperty functions in to scope.js * Removed deprecation warning * Rename scope.js to map.js * Rename ScopeProperty to MapProperty * Add tests and docs for map.js * Put back the micro-optimization of function calls * Use Map in the parser * Called scope methods directly in cli.js * Coercing of scope into a Map is done in Node, not evaluate * Move createSubScope to its own file * Fixup following self-review * Add scope docs * Final self-review changes * Address reviewer comments * Remove MapProperty witness marks * Converted broken benchmark possibly lost in a rebase * Use bare map as scope in benchmark Co-authored-by: Jos de Jong <wjosdejong@gmail.com>

jhugman marked this pull request as draft April 10, 2021 14:43

jhugman mentioned this pull request Apr 10, 2021

Remove support for Object literals as scope objects #2165

Open

jhugman commented Apr 10, 2021

View reviewed changes

Comment thread src/expression/node/Node.js

Comment thread test/unit-tests/expression/security.test.js

Comment thread src/expression/function/evaluate.js Outdated

jhugman force-pushed the 2143-map-like-scopes branch from 5c74330 to f08e9b3 Compare April 10, 2021 15:00

jhugman commented Apr 11, 2021

View reviewed changes

Comment thread src/core/function/typed.js Outdated

Comment thread src/expression/node/SymbolNode.js

Comment thread src/expression/function/evaluate.js Outdated

jhugman marked this pull request as ready for review April 11, 2021 15:43

josdejong mentioned this pull request Apr 12, 2021

Latest version is missing @babel/runtime #2169

Closed

josdejong requested changes Apr 25, 2021

View reviewed changes

jhugman force-pushed the 2143-map-like-scopes branch from 60bdaa3 to 85fdf30 Compare April 25, 2021 15:01

jhugman commented Apr 25, 2021

View reviewed changes

jhugman requested a review from josdejong April 25, 2021 16:11

josdejong reviewed Apr 28, 2021

View reviewed changes

jhugman requested a review from josdejong May 7, 2021 20:21

josdejong requested changes May 12, 2021

View reviewed changes

jhugman force-pushed the 2143-map-like-scopes branch from 59a608f to 0a04461 Compare May 12, 2021 23:09

jhugman added 9 commits May 15, 2021 15:01

Add support for scopes with get and set methods

35b2334

Fix build for node v12

6e764e6

Fixup cli and parser tests

88a5279

Add tests for simplify and evaluate

954ce24

Add example for a custom scope object

bf241dd

Function calls need child scopes

c45b8f7

Transitionary step: Separate Safe and Scope Property calls

c7e89da

Renamed identifiers in FunctionNode

c9f1c8d

Evaluate with ObjectScopeWrapper

482fb96

jhugman added 18 commits May 15, 2021 15:01

Move keywords from an Object to a Set

97a88ad

Move ScopeProperty functions in to scope.js

2d12e5a

Removed deprecation warning

08ddd93

Rename scope.js to map.js

2a80528

Rename ScopeProperty to MapProperty

93cac3a

Add tests and docs for map.js

b5c537f

Put back the micro-optimization of function calls

3b9dbc3

Use Map in the parser

08e5ee8

Called scope methods directly in cli.js

032aeb9

Coercing of scope into a Map is done in Node, not evaluate

3ccb08d

Move createSubScope to its own file

8461720

Fixup following self-review

472d065

Add scope docs

424100c

Final self-review changes

4c4fe9d

Address reviewer comments

88dec02

Remove MapProperty witness marks

67f72e3

Converted broken benchmark possibly lost in a rebase

b719e39

Use bare map as scope in benchmark

3f58aaf

jhugman force-pushed the 2143-map-like-scopes branch from e4a5ed6 to 3f58aaf Compare May 15, 2021 14:02

Merge branch 'develop' into 2143-map-like-scopes

025ecf6

josdejong approved these changes May 16, 2021

View reviewed changes

josdejong merged commit e80995d into josdejong:develop May 16, 2021

jhugman deleted the 2143-map-like-scopes branch June 26, 2021 12:09

jhugman mentioned this pull request Jun 26, 2021

Change node access to mathWithTransform to a map interface #2264

Closed

		@@ -0,0 +1,40 @@
		import { createEmptyMap, isMap } from './map.js'

		function assign (scope, ...objects) {

		@@ -0,0 +1,113 @@
		import { setSafeProperty, hasSafeProperty, getSafeProperty } from './customs.js'

		export class ObjectScopeWrapper {

Uh oh!

Conversation

jhugman commented Apr 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jhugman left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

josdejong commented Apr 17, 2021

Uh oh!

josdejong left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jhugman commented Apr 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jhugman left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

josdejong commented Apr 28, 2021

Uh oh!

jhugman commented May 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jhugman commented May 8, 2021

Uh oh!

josdejong left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

jhugman commented Apr 10, 2021 •

edited

Loading

jhugman left a comment •

edited

Loading

jhugman commented Apr 25, 2021 •

edited

Loading

jhugman commented May 3, 2021 •

edited

Loading