feat(macro): add basic conditional support #3385

ylemkimon · 2021-10-31T06:27:55Z

\iftrue, \iffalse, and \ifmmode.

More conditionals can be defined using defineConditional.

Fixes partially #1003.

codecov · 2021-10-31T06:29:54Z

Codecov Report

Merging #3385 (44c706e) into main (40109f6) will increase coverage by 0.00%.
The diff coverage is 94.73%.

@@           Coverage Diff           @@
##             main    #3385   +/-   ##
=======================================
  Coverage   93.46%   93.46%           
=======================================
  Files          89       89           
  Lines        6577     6615   +38     
  Branches     1524     1532    +8     
=======================================
+ Hits         6147     6183   +36     
- Misses        399      401    +2     
  Partials       31       31

Impacted Files	Coverage Δ
src/MacroExpander.js	`96.66% <86.66%> (-0.77%)`	⬇️
src/defineMacro.js	`100.00% <100.00%> (ø)`
src/macros.js	`97.40% <100.00%> (+0.08%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a59135f...44c706e. Read the comment docs.

edemaine

Wow, this is great!

As you'll see in the review, it took me a little while to understand how it all works, partly because of the file order presented in GitHub. A few extra comments would help with that.

There are also a couple of design decisions that are at least worth thinking about, but which you can deem worse than the current approach if you like.

And I (maybe) implemented \newif, though I didn't test it. And I reimplemented \TextOrMath in a better way thanks to \ifmmode (also untested).

edemaine · 2021-10-31T19:32:45Z

docs/supported.md

+### Conditionals
+
+`\iftrue`, `\iffalse`, `\else`, `\fi`
+


I could see moving this up above under "Available functions", which is where \TextOrMath gets mentioned.

Either way, \ifmmode is missing here.

This also reminds me to ask: should \TextOrMode get re-implemented via \ifmmode? Indeed, our current definition expands both arguments, whereas LaTeX's definition expands only one of the arguments (whichever applies). I think the following should work (it's identical to LaTeX's definition, but with extra spaces removed):

defineMacro("\\TextOrMath", "\\ifmmode\\expandafter\\@secondoftwo\\else\\expandafter\\@firstoftwo\\fi");

This could be a good test of \if and \ifmmode. 🙂

edemaine · 2021-10-31T19:34:16Z

src/MacroExpander.js

@@ -33,6 +33,7 @@ export default class MacroExpander implements MacroContextInterface {
    macros: Namespace<MacroDefinition>;
    stack: Token[];
    mode: Mode;
+    conditions: boolean[];


I'm surprised to see this additional data structure. I thought conditionals were just macros in the end. Is that incorrect, because of the way they don't always evaluate the body?

Oh, now I see this is really a stack of conditions we're currently "inside". Perhaps this should be renamed to conditionStack? And/or add a comment? (perhaps down below next to the existing stack comment)

...and now I see that this is documented, in defineMacro.js. I suppose it's too much to want MacroContextInterface defined here. Maybe a comment referring the reader to that definition for documentation about all these member variables?

edemaine · 2021-10-31T19:36:56Z

src/defineMacro.js

+    // skipped text.
+    defineMacro(name, function(context) {
+        const condition = evaluate(context);
+        context.conditions.push(condition);


Is this worth a pushCondition method, to mirror pushToken?

edemaine · 2021-10-31T19:40:53Z

src/macros.js

+defineMacro("\\else", function(context) {
+    // When \else is expanded, TeX reads to the end of any text that
+    // ought to be skipped.
+    if (!context.conditions[context.conditions.length - 1]) {


Ah, and now I finally see why you need the conditions stack: to count how many \elses have been seen at this level.

This is a place where I feel like you're working hard not to pop, hence the idea above about removing the pop from skipConditionalText.

edemaine · 2021-10-31T19:43:21Z

src/MacroExpander.js

+     * keeping track of \if...\fi nesting.
+     */
+    skipConditionalText() {
+        const condition = this.conditions.pop();


I think it'd be helpful to document that this function should be called immediately after a conditional has been pushed and it's been determined that it's false.

Alternatively, have you thought about removing this pop from this method? It seems like everywhere that uses this method might be happier to do the poping (or not pushing) itself. And I think it might better align with the method name. On the other hand, I see that condition is actually used in the \else case...

Is the intent that this function gets called only when condition is false (i.e. only when skipping)? Is that worth a comment and/or an assertion? And if the intent is true, perhaps the pop could be removed from this method... And it seems like the conditions stack could instead just be a counter? Below I realize why you need a stack.

edemaine · 2021-10-31T20:06:36Z

src/defineMacro.js

+    defineMacro(name, function(context) {
+        const condition = evaluate(context);
+        context.conditions.push(condition);
+        if (!condition) {


I believe this is the only place where conditions' boolean values are used. Which suggests that you just need a depth counter, not an actual stack, because here it's known from line 144.

edemaine · 2021-10-31T20:07:07Z

src/defineMacro.js

+        if (!condition) {
+            context.skipConditionalText();
+        }
+        return '';


Would return [] be more efficient here, because it doesn't engage the tokenizer? Not sure. (Also applies to a lot of other macros.)

Alternatively, I wonder if it makes sense to check for undefined as a return value from a macro, and expand it to nothing in that case. Then this line could be omitted, here and elsewhere.

edemaine · 2021-10-31T20:19:34Z

src/macros.js

+
+defineConditional("\\iftrue", () => true);
+defineConditional("\\iffalse", () => false);
+defineConditional("\\ifmmode", (context) => context.mode === 'math');


~~I seem to recall when implementing \TextOrMath that this wasn't possible before. I'm guessing it works now though.~~

Here's an attempt at \newif:

defineMacro("\\newif", function(context) { let value = false; const name = context.popToken(); // don't expand following token // LaTeX silently strips off the first two characters, intending them to be 'if'. This is a more strict check: if (!name.text.startsWith('\\if')) { throw new ParseError("\newif followed by something other than \if..."); } const cond = name.text.slice(2); defineConditional(name.text, () => value); defineMacro(`\\${cond}false`, () => { value = false; return ''; }); defineMacro(`\\${cond}true`, () => { value = true; return ''; }); return ''; });

edemaine · 2021-10-31T20:23:01Z

test/katex-spec.js

+
+        it("\\ifmmode should work", function() {
+            expect`\def\foo{\ifmmode math\else text\fi}\foo`.toParseLike`math`;
+            expect`\def\foo{\ifmmode math\else text\fi}\textit{\foo}`.toParseLike`\textit{text}`;


Maybe add expect`\def\foo{\ifmmode math\else text\fi}\text{$\foo$}`.toParseLike`\text{$math$}`;

Or these tests could be combined into something like \foo\text{\foo$\foo$\foo}\foo to make sure math mode is both entered and exited correctly.

edemaine · 2021-10-31T20:28:11Z

src/MacroExpander.js

+                }
+            } else if (token.text === "EOF") {
+                throw new ParseError("Incomplete conditional! " +
+                    "End of input while skipping conditional text.", token);


LaTeX says Incomplete \iffalse; all text was ignored after line <n>. or Incomplete \iftrue; all text was ignored after line <n>. (in the case of \iftrue ... \else ...). Perhaps `Incomplete \\if${condition}` would be appropriate and more informative?

feat(macro): add basic conditional support

44c706e

edemaine reviewed Oct 31, 2021

View reviewed changes

ylemkimon mentioned this pull request Oct 31, 2021

feat(macro): add basic conditional support #2131

Closed

edemaine mentioned this pull request Mar 9, 2022

_* parse bug #3576

Open

edemaine mentioned this pull request Aug 18, 2022

\ensuremath #3687

Open

edemaine mentioned this pull request Aug 28, 2022

fix: Use SVGs for some stacked delims #3686

Merged

ronkok mentioned this pull request Sep 8, 2022

feat: Add derivatives, incl. high-order and mixed partial #3701

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(macro): add basic conditional support #3385

feat(macro): add basic conditional support #3385

ylemkimon commented Oct 31, 2021

codecov bot commented Oct 31, 2021 •

edited

edemaine left a comment •

edited

edemaine Oct 31, 2021 •

edited

edemaine Oct 31, 2021

edemaine Oct 31, 2021

edemaine Oct 31, 2021

edemaine Oct 31, 2021

edemaine Oct 31, 2021

edemaine Oct 31, 2021

edemaine Oct 31, 2021 •

edited

edemaine Oct 31, 2021

edemaine Oct 31, 2021

feat(macro): add basic conditional support #3385

Are you sure you want to change the base?

feat(macro): add basic conditional support #3385

Conversation

ylemkimon commented Oct 31, 2021

codecov bot commented Oct 31, 2021 • edited

Codecov Report

edemaine left a comment • edited

Choose a reason for hiding this comment

edemaine Oct 31, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edemaine Oct 31, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Oct 31, 2021 •

edited

edemaine left a comment •

edited

edemaine Oct 31, 2021 •

edited

edemaine Oct 31, 2021 •

edited