fix: simplify.resolve detects reference loop and throws error by gwhitney · Pull Request #2405 · josdejong/mathjs

gwhitney · 2022-02-03T17:45:52Z

Also adds reference documentation and embedded documentation for simplify.resolve (with docs for simplify.simplifyCore coming along for the ride). Making these additions involved extending both doc facilities to handle functions "one level down" from the top level, and to support descriptions of what errors a function might throw (which were already present in the reference docs for several other functions, and so will now be displayed).

Also extends simplify.resolve to perform variable substitution within all node types (formerly it would be blocked by, e.g., an ArrayNode and not perform variable resolution within).

Adds tests for all new behaviors, including simplify.resolve throwing ReferenceErrors.

Resolves #2383.

josdejong

Really nice Job @gwhitney , thanks. I like that you took the time to improve on the automatically generated docs too.

I made a few minor inline remarks, and have one thought: shall we can flatten the function names simplify.simplifyCore and simplify.resolve into simplifyCore and simplifyResolve? So far the library doesn't have nested functions, and that is just of simple and straightforward :).

josdejong · 2022-02-16T07:55:08Z

      scope = createMap(scope)
    }
    if (isSymbolNode(node)) {
+      if (within.indexOf(node.name) !== -1) {


Generally I prefer not to mutate function input arguments, but here with within (and only internally) it makes sense.

Would it make sense for performance to change make within a Set instead of an Array?

Ah I was not thinking of a case with 100s of variables, but I suppose someone may want to use mathjs for such a case. Will change.

josdejong · 2022-02-16T07:56:18Z

@@ -17,3 +17,39 @@ export const simplifyDocs = {
    'derivative', 'parse', 'evaluate'


Can you add refrences to simplify.resolve and simplify.simplifyCore in simplify?

Will do, to their new top-level names.

josdejong · 2022-02-16T07:57:51Z

        '\n\n\n'
  }

+  if (doc.mayThrow) {


Nice improvements documenting the exceptions that a function may throw!

gwhitney · 2022-02-16T16:38:36Z

I made a few minor inline remarks, and have one thought: shall we can flatten the function names simplify.simplifyCore and simplify.resolve into simplifyCore and simplifyResolve? So far the library doesn't have nested functions, and that is just of simple and straightforward :).

I think naming should be entirely at your judgment and I can see in this comment and elsewhere that you favor a single layer of public names in mathJS. So I will revert the portions of this that track documentation down to a second layer, and move these two functions to the top layer. As for their names there, I'd suggest simplifyCore and resolve; the latter has a clear meaning and purpose on its own, independent of simplify, and the name does not conflict.

For example, prior to this commit, docgenerator.js would miss simplify.resolve because it is not a direct key of the math object. Also incorporates any "throws" attributes in the comments into the generated documentation, and uses this to document the new error-case behavior of simplify.resolve to be added in the next commit.

Also extends resolve to work inside all node types. Adds tests for both changes.

To support finding the embedded doc from the `math.simplify.resolve` function itself, required extending the search for objects with documentation one level deeper in the `help()` function. Added test for this search. Also added support for documenting throws in embedded docs.

Also reverts changes searching for docs and embedded docs one level down in the naming hierarchy. Also splits tests for resolve and simplifyCore into their own files, reflecting their new top-level status.

Also removed a stray blank line inadvertently introduced in docgenerator.js

gwhitney · 2022-02-16T19:02:47Z

OK, I think that should take care of all of the feedback. Thanks.

josdejong · 2022-02-16T20:17:28Z

Thanks

As for their names there, I'd suggest simplifyCore and resolve

That sounds good 👍 . I do not see a conflict with some other function that we would want to call resolve.

gwhitney · 2022-02-17T01:40:38Z

Oh, I just realized that now that resolve and simplifyCore are at the top level, simplify should just register them as dependencies. Give me a minute and I will fix.

... rather than explicitly loading them, which is unnecessary now that they are at top level.

Also register simplifyCore as a dependency to rationalize

gwhitney · 2022-02-17T02:00:34Z

Oh and I have to do the same for rationalize, and then add the dependencies to factoriesNumber so the build won't fail. Now it should be OK to merge.

josdejong

Thanks, looks good!

josdejong · 2022-03-01T13:27:12Z

This is published now in v10.2.0. Thanks Glen!

gwhitney mentioned this pull request Feb 3, 2022

Export the simplify utils as math.simplify.utils #2282

Open

josdejong requested changes Feb 16, 2022

View reviewed changes

gwhitney added 4 commits February 16, 2022 09:34

fix(resolve): Detect and throw errors for reference loops

7d9bbbb

Also extends resolve to work inside all node types. Adds tests for both changes.

refactor(simplify): Move resolve and simplifyCore to top-level

732f86d

Also reverts changes searching for docs and embedded docs one level down in the naming hierarchy. Also splits tests for resolve and simplifyCore into their own files, reflecting their new top-level status.

gwhitney force-pushed the resolve_loop branch from 269b141 to ad93609 Compare February 16, 2022 18:55

fix(resolve): Remaining changes as requested

1ac7d49

Also removed a stray blank line inadvertently introduced in docgenerator.js

gwhitney force-pushed the resolve_loop branch from ad93609 to 1ac7d49 Compare February 16, 2022 19:01

This was referenced Feb 16, 2022

Question: Simplify order of terms for 5i+4 #2407

Closed

Don't convert between fractions and decimals in simplify #2398

Open

gwhitney added 2 commits February 16, 2022 17:48

refactor: Declare resolve and simplifyCore as dependencies of simplify

a225fb1

... rather than explicitly loading them, which is unnecessary now that they are at top level.

fix: Add dependencies to factoriesNumber

89e2a2b

Also register simplifyCore as a dependency to rationalize

josdejong approved these changes Feb 28, 2022

View reviewed changes

Merge branch 'develop' into resolve_loop

c5143c2

josdejong merged commit 151926c into josdejong:develop Feb 28, 2022

josdejong mentioned this pull request Mar 1, 2022

simplify.resolve blows up call stack in case of recursive references #2383

Closed

		@@ -17,3 +17,39 @@ export const simplifyDocs = {
		'derivative', 'parse', 'evaluate'

Uh oh!

Conversation

gwhitney commented Feb 3, 2022

Uh oh!

josdejong left a comment

Choose a reason for hiding this comment

Uh oh!

josdejong Feb 16, 2022

Choose a reason for hiding this comment

Uh oh!

gwhitney Feb 16, 2022

Choose a reason for hiding this comment

Uh oh!

josdejong Feb 16, 2022

Choose a reason for hiding this comment

Uh oh!

gwhitney Feb 16, 2022

Choose a reason for hiding this comment

Uh oh!

josdejong Feb 16, 2022

Choose a reason for hiding this comment

Uh oh!

gwhitney commented Feb 16, 2022

Uh oh!

gwhitney commented Feb 16, 2022

Uh oh!

josdejong commented Feb 16, 2022

Uh oh!

gwhitney commented Feb 17, 2022

Uh oh!

gwhitney commented Feb 17, 2022

Uh oh!

josdejong left a comment

Choose a reason for hiding this comment

Uh oh!

josdejong commented Mar 1, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants