refactor/feat: changes and additions to ReplaceOptions ('form', 'direction')#305
Open
samueltlg wants to merge 5 commits into
Open
refactor/feat: changes and additions to ReplaceOptions ('form', 'direction')#305samueltlg wants to merge 5 commits into
samueltlg wants to merge 5 commits into
Conversation
samueltlg
force-pushed
the
pr/replace-options-split
branch
from
e1e8898 to
5a82c5a
Compare
Retain canonical as a deprecated alias, accept both option names for one release, and keep the replacement-form transition separate from the replacement-sameness change.
…ssion 'form' Treat form changes as meaningful during recursive replace iteration so eager form propagation can surface even when structural equality stays the same. This keeps the replacement-form behavior separate from the ReplaceOptions API transition.
…attern variant comparison (rule application)
(this change is marked as a *workaround* rather than a 'fix' per se, since the issue it tackles instead likely has its root in the current behaviour regarding the binding of 'wildcard' symbol variants (-outside the context of pattern matching / replacement.
(See the closing of this message for a further remark on this point.))
Explanation:
- Currently, because canonicalization of expresssions involves wildcard binding as part of symbol binding (this may be undesirable / a bug), it is presently necessary that a new scope be created upon 'match' pattern canonicalization (performed for optimization purposes) in rule application: with this otherwise running the risk of present wildcard symbols being bound to definitions in the current scope...
For example, before this change:
```
ce.expr(['Add', 'x', 3]).replace({ match: ['_', '__'], replace: 'y'});
// →Expectedly captures wildcards, and replaces
// top-level expression.
// *However*, the canonical-request of the 'match' expression within 'applyRule'
// results in symbol/wildcard '_' being attributed a function-definition
// (type='function'): within the scope in which this 'expr.replace()' was called
// Consequently, a subsequent replace call involving a universal wildcard
ce.expr(['Add', 'x', 3]).replace({ match: ['Add', 'x', '_')], replace: 5 });
// → Returns 'null'... (assuming this call made with an unmodified/same scope as the previous)
// Ultimately because the same point-of-canonicalization results in the Universal
// Wildcard in this instance uptaking the previous, 'function' definition, resulting
// in a MathJson-internal type-error for the canonicalized match-pattern variant
// in this instance, resulting in an absent wildcard in the canonical variant,
// and leading to an early exit from rule-application 'applyRule()'
//For illustration, the canonical-variant of the initial, non-canonically boxed
// `['Add', 'x', 3]` pattern results in the canonical-variant as:
[ "Add", "x", [ "Error", [ "ErrorCode", "'incompatible-type'", "number", "function", ], ], ]
```
*Note*:
- This 'fix' may no longer be necessary, if canonicalization of wildcard-containing expressions were to no longer perform name-binding on these (this may be unintentional?)
samueltlg
force-pushed
the
pr/replace-options-split
branch
from
5a82c5a to
8c41a8a
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
(This is all-ready [-and slightly improved])
Summary
This PR breaks out ReplaceOptions-related work into smaller, reviewable commits.
Notes