Skip to content

Commit 82b5f10

Browse files
samueltlgarnog
andauthored
refactor/feat: changes and additions to ReplaceOptions ('form', 'direction') (#305)
* feat: ReplaceOptions.direction * refactor (+fix): update ReplaceOptions.canonical to 'form' 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. * fix: when considering replacement expr. sameness, also consider expression '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. * fix (workaround): ensure intended canonical - non-canonical - match pattern 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?) * doc: ReplaceOptions; and CHANGELOG --------- Co-authored-by: Arno Gourdol <arno@arno.org>
1 parent ae0f981 commit 82b5f10

7 files changed

Lines changed: 268 additions & 61 deletions

File tree

CHANGELOG.md

Lines changed: 21 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,22 @@
1+
## Upcoming changes
2+
3+
### Added
4+
5+
- **ReplaceOptions.direction** — replacement traversal can now be controlled
6+
from left-to-right or right-to-left.
7+
8+
### Fixed
9+
10+
- **ReplaceOptions form handling**`Expression.replace()` now accepts the new
11+
`form` option, while keeping deprecated `canonical` as a backward- compatible
12+
alias for one release.
13+
- **Replacement form propagation** — recursive replacements preserve and
14+
propagate the requested form upward when child operands already share it.
15+
16+
### Changes
17+
18+
- **ReplaceOptions.canonical is deprecated** — prefer `form`; specifying both
19+
`form` and `canonical` now raises an error.
120
### [Unreleased]
221

322
- **2-arg `\arctan(y, x)` / `\tan^{-1}(y, x)``Arctan2`** — both `\arctan` and
@@ -1253,8 +1272,8 @@ GPU compile).
12531272
`oklab(L a b / alpha)` syntax, matching the existing `oklch()` support.
12541273
- **GPU compilation**: `ColorMix`, `ColorContrast`, `ContrastingColor`,
12551274
`ColorToColorspace`, and `ColorFromColorspace` now compile to GLSL and WGSL.
1256-
Preamble functions provide sRGB ↔ OKLab ↔ OKLCh conversion, color mixing with
1257-
shorter-arc hue interpolation, and APCA contrast on the GPU.
1275+
Preamble functions provide sRGB ↔ OKLab ↔ OKLCh conversion, color mixing
1276+
with shorter-arc hue interpolation, and APCA contrast on the GPU.
12581277
- Added `rgbToHsl()` conversion function. Exported `hslToRgb()` (previously
12591278
private).
12601279

src/compute-engine/boxed-expression/rules.ts

Lines changed: 183 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ import type {
1515
Expression,
1616
ReplaceOptions,
1717
ExpressionInput,
18+
FormOption,
1819
} from '../global-types';
1920

2021
import {
@@ -657,16 +658,9 @@ function boxRule(
657658
);
658659
}
659660

660-
// Push a clean scope that only inherits from the system scope (index 0),
661-
// not from the global scope or user-defined scopes. This prevents user-defined
662-
// symbols (like `x` used as a function name in `x(y+z)`) from interfering with
663-
// rule parsing. The system scope contains all built-in definitions.
664-
const systemScope = ce.contextStack[0]?.lexicalScope;
665-
if (systemScope) {
666-
ce.pushScope({ parent: systemScope, bindings: new Map() });
667-
} else {
668-
ce.pushScope();
669-
}
661+
// Ensure a clean scope (that only inherits from the system scope) before boxing or parsing:
662+
// preventing wildcards & user-defined from inheriting definitions in rules.
663+
pushSafeScope(ce);
670664

671665
let matchExpr: Expression | undefined;
672666
let replaceExpr: Expression | RuleReplaceFunction | RuleFunction | undefined;
@@ -742,6 +736,25 @@ function boxRule(
742736
};
743737
}
744738

739+
/**
740+
* Push a clean scope - safe for the boxing of rules - that only inherits from the system scope
741+
* (index 0), not from the global scope or user-defined scopes. This prevents user-defined symbols
742+
* (like `x` used as a function name in `x(y+z)`) from interfering with rule parsing. The system
743+
* scope contains all built-in definitions.
744+
*
745+
* This also crucially prevents wildcards from being given definitions where captured & bound.
746+
*
747+
* @param ce
748+
*/
749+
function pushSafeScope(ce: ComputeEngine) {
750+
const systemScope = ce.contextStack[0]?.lexicalScope;
751+
if (systemScope) {
752+
ce.pushScope({ parent: systemScope, bindings: new Map() });
753+
} else {
754+
ce.pushScope();
755+
}
756+
}
757+
745758
/**
746759
* Create a boxed rule set from a collection of non-boxed rules
747760
*/
@@ -774,6 +787,23 @@ export function boxRules(
774787
return { rules };
775788
}
776789

790+
function normalizeReplaceForm(
791+
options?: Readonly<Partial<ReplaceOptions>>
792+
): FormOption | undefined {
793+
if (options?.canonical !== undefined && options?.form !== undefined)
794+
throw new Error(
795+
'replace(): options.canonical and options.form are mutually exclusive'
796+
);
797+
798+
if (options?.canonical !== undefined) {
799+
if (options.canonical === true) return 'canonical';
800+
if (options.canonical === false) return 'raw';
801+
return options.canonical;
802+
}
803+
804+
return options?.form;
805+
}
806+
777807
/**
778808
* Apply a rule to an expression, assuming an incoming substitution
779809
* @param rule the rule to apply
@@ -789,15 +819,20 @@ export function applyRule(
789819
options?: Readonly<Partial<ReplaceOptions>>
790820
): RuleStep | null {
791821
if (!rule) return null;
792-
let canonical = options?.canonical ?? (expr.isCanonical || expr.isStructural);
822+
const requestedForm = normalizeReplaceForm(options);
793823

794824
// eslint-disable-next-line prefer-const
795825
let { match, replace, condition, id, onMatch, onBeforeMatch } = rule;
796826
const because = id ?? '';
797827

798828
const ce = expr.engine;
799829

800-
if (canonical && match) {
830+
const canonicalRequested =
831+
requestedForm !== undefined &&
832+
requestedForm !== 'raw' &&
833+
requestedForm !== 'structural';
834+
835+
if ((canonicalRequested || expr.isCanonical) && match) {
801836
const awc = getWildcards(match);
802837
const canonicalMatch = match.canonical;
803838
const bwc = getWildcards(canonicalMatch);
@@ -809,29 +844,39 @@ export function applyRule(
809844
let operandsMatched = false;
810845

811846
if (isFunction(expr) && options?.recursive) {
847+
const direction = options?.direction ?? 'left-right';
848+
let newOps =
849+
direction === 'left-right' ? expr.ops : [...expr.ops].reverse();
850+
812851
// Apply the rule to the operands of the expression
813-
const newOps = expr.ops.map((op) => {
852+
newOps = newOps.map((op) => {
814853
const subExpr = applyRule(rule, op, {}, options);
815854
if (!subExpr) return op;
816855
operandsMatched = true;
817856
return subExpr.value;
818857
});
819858

859+
if (direction === 'right-left') (newOps as Expression[]).reverse();
860+
820861
// At least one operand (directly or recursively) matched: but continue onwards to match against
821862
// the top-level expr., test against any 'condition', et cetera.
822863
if (operandsMatched) {
823-
// If new/replaced operands are all canonical, and options do not explicitly specify canonical
824-
// status, then should be safe to mark as fully-canonical
825-
if (
826-
!canonical &&
827-
options?.canonical === undefined &&
828-
newOps.every((x) => x.isCanonical)
829-
)
830-
canonical = true;
831-
832-
expr = ce.function(expr.operator, newOps, {
833-
form: canonical ? 'canonical' : 'raw',
834-
});
864+
// (note: so not consult the input-expr 'form' because, assuming that replaced operands assume
865+
// the same form, this will be upcast in the subsequent branches.
866+
// ^Another reason to avoid this, is if the form of replacements differ from the input expr.,
867+
// then likely it is not the intention to preserve the form of the parent)
868+
let form: FormOption = 'raw';
869+
// The current policy for applying a form according to 'options.form' is for this to apply to
870+
// *replacements only* (this ultimately allowing for finer control of replacement operations).
871+
// ...However, if all child operands bear the same form, 'eagerly' assume this form for the
872+
// present expression (if this present expression also later matches, form may be updated
873+
// according to 'options.form'.)
874+
//(@note: check 'canonical' first, because numbers may be jointly marked as structural and
875+
//canonical).
876+
if (newOps.every((x) => x.isCanonical)) form = 'canonical';
877+
else if (newOps.every((x) => x.isStructural)) form = 'structural';
878+
879+
expr = ce.function(expr.operator, newOps, { form });
835880
}
836881
}
837882

@@ -881,41 +926,81 @@ export function applyRule(
881926
}
882927
}
883928

929+
/** The computed form value to be assumed by the *directly replaced* expression: assuming either an
930+
'enforced' value (options), or consultation to the form of the input expression */
931+
let formValue =
932+
requestedForm ??
933+
(expr.isStructural ? 'structural' : expr.isCanonical ? 'canonical' : 'raw');
934+
935+
// If `true`, then the form is not 'enforced' (via options) and therefore, the prior computed
936+
// form only applies wherein the initially-produced replacement expression has a 'raw' form
937+
// (else retaining whichever form of the replacement)
938+
const dynamicForm = requestedForm === undefined;
939+
940+
/** Get the overall form type from *formValue* (raw/structural/canonical), accounting for
941+
* 'canonical' potentially assuming multiple values. */
942+
const getFormType = () =>
943+
formValue === 'structural'
944+
? 'structural'
945+
: formValue === 'raw'
946+
? 'raw'
947+
: 'canonical';
948+
884949
// Have a (direct) match: in this case, consider the canonical-status of the replacement, too.
885950
if (
886-
!canonical &&
887-
options?.canonical === undefined &&
951+
formValue === 'raw' &&
952+
dynamicForm &&
888953
replace instanceof _BoxedExpression &&
889-
replace.isCanonical
954+
(replace.isCanonical || replace.isStructural)
890955
)
891-
canonical = true;
956+
formValue = replace.isCanonical ? 'canonical' : 'structural';
892957

893958
//@note: '.subs()' acts like an expr. 'clone' here (in case of an empty substitution)
894959
const result =
895960
typeof replace === 'function'
896961
? replace(expr, sub)
897-
: replace.subs(sub, { canonical });
962+
: // @todo: 'expr.subs()' to eventually also assume a 'form' option
963+
// : replace.subs(sub, { form: dynamicForm ? undefined : formValue });
964+
replace.subs(sub, { canonical: getFormType() === 'canonical' });
898965

899-
if (!result)
900-
return operandsMatched
901-
? { value: canonical ? expr.canonical : expr, because }
902-
: null;
966+
if (!result) return operandsMatched ? { value: expr, because } : null;
903967

904968
// To aid in debugging, invoke onMatch when the rule matches
905969
onMatch?.(rule, expr, result);
906970

971+
/** Return the final *expression* with the correctly computed form. */
972+
const computeValue = (result: Expression) => {
973+
// If 'raw', leave the expression as-is
974+
// (note that if result has produced a 'non-raw' form, this may not be 'undone'...)
975+
if (formValue === 'raw') return result;
976+
// Non option-enforced form; let replacement/result expression form override
977+
if (dynamicForm === true && (result.isStructural || result.isCanonical))
978+
return result;
979+
// Enforced form
980+
return getFormType() === 'canonical'
981+
? result.isCanonical
982+
? result
983+
: ce.expr(result, { form: formValue }) //Re-box (instead of 'x.canonical'), case of 'CanonicalForm'
984+
: result.structural;
985+
};
986+
987+
// (Need to request a 'form' variant (canonical/structural) to account for case of a custom
988+
// replace: which may not have returned the same 'form' calculated here)
907989
if (isRuleStep(result))
908-
return canonical ? { ...result, value: result.value.canonical } : result;
990+
return getFormType() === 'raw'
991+
? result
992+
: { ...result, value: computeValue(result.value) };
909993

910994
if (!isExpression(result)) {
911995
throw new Error(
912996
'Invalid rule replacement result: expected a Expression or RuleStep'
913997
);
914998
}
915999

916-
// (Need to request the canonical variant to account for case of a custom replace: which may not
917-
// have returned canonical.)
918-
return { value: canonical ? result.canonical : result, because };
1000+
return {
1001+
value: computeValue(result),
1002+
because,
1003+
};
9191004
}
9201005

9211006
/**
@@ -936,6 +1021,7 @@ export function replace(
9361021
const iterationLimit = options?.iterationLimit ?? 1;
9371022
let iterationCount = 0;
9381023
const once = options?.once ?? false;
1024+
normalizeReplaceForm(options);
9391025

9401026
// Normalize the ruleset
9411027
let ruleSet: ReadonlyArray<BoxedRule>;
@@ -956,7 +1042,7 @@ export function replace(
9561042
if (
9571043
result !== null &&
9581044
result.value !== expr &&
959-
!result.value.isSame(expr)
1045+
(!result.value.isSame(expr) || varyingForm(expr, result.value))
9601046
) {
9611047
// If `once` flag is set, bail on first matching rule
9621048
if (once) return [result];
@@ -977,6 +1063,64 @@ export function replace(
9771063
iterationCount += 1;
9781064
}
9791065
return steps;
1066+
1067+
/*
1068+
* Local f.
1069+
*/
1070+
/**
1071+
* Assuming *x* and *x2* are **structurally (symbolically) equivalent**, and considering
1072+
* expression forms 'structural' and 'canonical':
1073+
*
1074+
* - If option 'recursive' equals `true` or `'functions-only'` (**default** = `'functions-only'`),
1075+
* then, if either 'x' or 'x2', or one of the matching sub-expression pairs of these has a
1076+
* differing 'structural' or 'canonical' status, then return `true`.
1077+
* (if 'functions-only', then only function-expression operands are considered)
1078+
*
1079+
* - If 'recursive' === `false`, then this status comparison applies only to/between `x` and `x2`
1080+
* directly.
1081+
*
1082+
* For both cases, if neither `x` nor `x2` (nor compared sub-expressions if recursive) is
1083+
* structural or canonical, then return `false`.
1084+
*
1085+
* **Warning**: will throw an error if it is determined, in case of `recursive !== false`, that
1086+
* `x` and `x2` are not structurally equivalent/have an identical tree/branching structure.
1087+
* (It is therefore the responsibility of the caller to ensure this beforehand)
1088+
*/
1089+
function varyingForm(
1090+
x: Expression,
1091+
x2: Expression,
1092+
{
1093+
recursive = 'functions-only',
1094+
}: { recursive?: boolean | 'functions-only' } = {}
1095+
): boolean {
1096+
if (varies(x, x2)) return true;
1097+
1098+
if (recursive === false) return false;
1099+
1100+
if (isFunction(x) && isFunction(x2)) {
1101+
if (x.ops.length !== x2.ops.length)
1102+
throw new Error(
1103+
`'x' and 'x2' detected to not be structurally equivalent`
1104+
);
1105+
if (x.nops === 0) return false;
1106+
1107+
return x.ops.some((op, index) =>
1108+
recursive === true || (!isFunction(op) && !isFunction(x2.ops[index]))
1109+
? false
1110+
: varyingForm(op, x2.ops[index], { recursive })
1111+
);
1112+
} else if (isFunction(x) || isFunction(x2)) return true;
1113+
1114+
return false;
1115+
1116+
function varies(x: Expression, x2: Expression): boolean {
1117+
if (x.isStructural || x.isCanonical) {
1118+
if (x.isStructural) return !x2.isStructural;
1119+
return !x2.isCanonical;
1120+
}
1121+
return x2.isStructural || x2.isCanonical ? true : false;
1122+
}
1123+
}
9801124
}
9811125

9821126
/**

src/compute-engine/boxed-expression/simplify.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -341,7 +341,7 @@ function simplifyExpression(
341341
if (isSymbol(expr)) {
342342
const result = replace(expr, rules, {
343343
recursive: false,
344-
canonical: true,
344+
form: 'canonical',
345345
useVariations: false,
346346
});
347347
if (result.length > 0) return [...steps, ...result];
@@ -394,7 +394,7 @@ function simplifyNonCommutativeFunction(
394394
): RuleSteps {
395395
const result = replace(expr, rules, {
396396
recursive: false,
397-
canonical: true,
397+
form: 'canonical',
398398
useVariations: options.useVariations ?? false,
399399
});
400400

0 commit comments

Comments
 (0)