feat(macro): improve argument parsing by ylemkimon · Pull Request #2085 · KaTeX/KaTeX

ylemkimon · 2019-08-21T17:31:44Z

1. Primitive v.s. Macro
LaTeX has two types of command (control sequence): TeX primitive and macro. TeX primitives are provided by TeX engine. Macros, in contrast, are defined using primitives in the LaTeX kernel (latex.ltx) and packages such as amsmath. (Macro may expand into primitive commands without taking arguments. They are considered as primitives hereinafter.) Primitives scan argument as it goes, i.e., expanding macros, whereas macros take a properly nested group or non-blank token without expanding. For instance,

\def\foo{12}
\sqrt\foo
\underline\foo

is equivalent to

\sqrt12
\underline{12}

However, KaTeX uses primitive argument parsing for functions. This PR introduces a new argument type primitive, which uses the existing method, and uses it for parsing arguments of primitive commands. It also changes other group parsers such as parseArgumentGroup (new) and parseStringGroup to use macro argument parsing method, which scans the argument and denotes the end of the argument at the MacroExpander level before parsing. This fixes #924, fixes #1027, fixes #1720, and fixes #2319. Closes #611.

2. Macro
Delimited parameters in macro and brace handling are implemented in the new consumeArg function. Braced control sequence and unbraced replacement text no longer works.[1]

Optional arguments are implemented using a delimited parameter at the MacroExpander level. This fixes #1419 and #2288.

Whitespace handling is also improved to consume only in undelimited macro arguments and required/optional arguments. This broke \\ and this PR fixes it along with fix #2119.

Some changes may result in BREAKING CHANGEs such as [1], and issues linked above, however they probably were originally wrong behavior.

ylemkimon · 2019-08-21T17:35:33Z

src/MacroExpander.js

+                    ", expected '" + (delims && isDelimited ? delims[match] : "}") +
+                    "'", tok);
+            }
+            if (delims && isDelimited) {


delims && is required for Flow to recognize delims != null.

ylemkimon · 2019-08-21T17:39:47Z

src/Parser.js

-                if (isOptional) {
+
+            // \sqrt is a macro only if an optional argument exists
+            if (funcData.type === "sqrt" && i === 1 && optArgs[0] == null) {


I can't seem to find a better way to handle \sqrt than hard-coding it.

for non-strict mode and \genfrac, respectively.

ylemkimon · 2019-10-11T17:28:18Z

This breaks the example at the main page:

% \f is defined as f(#1) using the macro
\f{x} = \int_{-\infty}^\infty
    \hat \f\xi\,e^{2 \pi i \xi x}
    \,d\xi

Any suggestion to replace it is welcome!

Parser mode affects neither fetch nor consume.

codecov-commenter · 2020-07-13T13:54:35Z

Codecov Report

Merging #2085 into master will decrease coverage by 0.01%.
The diff coverage is 98.23%.

@@            Coverage Diff             @@
##           master    #2085      +/-   ##
==========================================
- Coverage   93.83%   93.81%   -0.02%     
==========================================
  Files          84       84              
  Lines        6112     6128      +16     
  Branches     1254     1260       +6     
==========================================
+ Hits         5735     5749      +14     
- Misses        346      348       +2     
  Partials       31       31

Impacted Files	Coverage Δ
src/functions/delimsizing.js	`97.39% <ø> (ø)`
src/functions/kern.js	`84.21% <ø> (ø)`
src/functions/mathchoice.js	`94.11% <ø> (ø)`
src/functions/mclass.js	`98.03% <ø> (ø)`
src/functions/op.js	`98.91% <ø> (ø)`
src/functions/styling.js	`100.00% <ø> (ø)`
src/parseNode.js	`73.33% <ø> (ø)`
src/Parser.js	`96.26% <95.00%> (-0.73%)`	⬇️
src/MacroExpander.js	`97.82% <100.00%> (+0.49%)`	⬆️
src/defineFunction.js	`95.65% <100.00%> (+0.41%)`	⬆️
... and 9 more

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 8578d74...a2df241. Read the comment docs.

k4b7 · 2020-08-02T19:10:40Z

@ylemkimon in #2288 you commented:

Actually, #2085 will add support for delimited macros.

but earlier in this PR you also commented:

I wanted to demonstrate KaTeX supports a delimited macro, but it's currently not possible to define a delimited macro. But I think this is out of this PR's scope. I've also added a migration guide.

Looking at the code it looks delimiters are supported. Is that correct? If so, could you add some tests that demonstrate this?

ylemkimon · 2020-08-02T19:16:38Z

@kevinbarabash I meant it's not possible to define a delimited macro using options.macros.

ylemkimon · 2020-08-02T19:17:35Z

test/katex-spec.js

+    it("\\gdef defines macros with delimited parameter", function() {
+        expect`\gdef\foo|#1||{#1}\text{\foo| x y ||}`.toParseLike`\text{ x y }`;
+        expect`\gdef\foo#1|#2{#1+#2}\foo 1 2 |34`.toParseLike`12+34`;
+        expect`\gdef\foo#1#{#1}\foo1^{23}`.toParseLike`1^{23}`;
+        expect`\gdef\foo|{}\foo`.not.toParse();
+        expect`\gdef\foo#1|{#1}\foo1`.not.toParse();
+        expect`\gdef\foo#1|{#1}\foo1}|`.not.toParse();


Tests for delimited macro are already added here.

I was wondering what the | were for. I'm so used to brackets and parens as delimiters, I forgot that | is a delimiter as well. 🙃

k4b7

I've done a partial review. I'll pick up at def.js when I have some more time.

docs/migration.md

k4b7 · 2020-08-09T18:52:54Z

docs/migration.md

+## `\newline` and `\cr`
+`\newline` and `\cr` no longer takes an optional size argument. To specify vertical
+spacing, `\\` should be used.


Does this change in behaviour arise from the macro/primitive changes?

@kevinbarabash Partly, yes. Those changed space handling and they are updated to match their definitions.

This is a late comment, sorry. But there is a breaking change in that \raisebox previously accepted unbraced dimensions and now it does not. That might be worth a note in the migration guide.

@ronkok Thank you for finding it! That was an unexpected change, but might be fixed with #2139. I'll add a note to the migration guide.

I don't think there is anything to "fix". LaTeX \raisebox does not accept unbraced dimensions, so the new behavior is not a bug. It's just needs a note in the migration guide.

src/MacroExpander.js

k4b7 · 2020-08-09T19:09:12Z

src/MacroExpander.js

+            tokens.shift();
+        }
+        tokens.reverse(); // to fit in with stack order
+        return {tokens, start, end: tok};


start and end can be outside the range of tokens, correct?

@kevinbarabash No, they are always the first and last or the second and second to last tokens.

If the case where there's a { and a } at the start and end of tokens are the start and end the braces and tokens the stuff inside the braces (after the pop() and shift())?

@kevinbarabash Sorry, my mistake. end can be outside the range of tokens, when it's delimited. In that case, end is the last delimiter.

src/MacroExpander.js

src/Parser.js

k4b7 · 2020-08-09T19:46:51Z

src/Parser.js

+        const expression = this.parseExpression(false, "EOF");
+        this.expect("EOF"); // expect the end of the argument


I wonder if we could use a different token to mark the end of arguments. Overloading EOF seems like it could be a bit confusing. Maybe something for a future PR given that there's already so much going on in this one.

@kevinbarabash I agree. This is my major concern, too. This will probably cause a lot of issues that are hard to debug, but I can't seem to find a simple better way. I'll add TODO for now.

k4b7 · 2020-08-09T19:49:21Z

src/defineFunction.js

+export const normalizeArgument = function(arg: AnyParseNode): AnyParseNode {
+    return arg.type === "ordgroup" && arg.body.length === 1 ? arg.body[0] : arg;
+};


Is it possible that arg could be a series of nested ordgroups? If so, would we want to unwrap each of them?

@kevinbarabash I'm not sure whether it's possible, but that's not caused by changes in this PR.

k4b7 · 2020-08-09T19:51:44Z

src/environments/array.js

+    // \cr is equivalent to \\ without the optional size argument (see below)
+    // TODO: provide helpful error when \cr is used outside the array environment


Can you raise a ticket for this TODO?

k4b7 · 2020-08-09T19:56:12Z

src/environments/array.js

+            // \def\Let@{\let\\\math@cr}
+            // \def\math@cr{...\math@cr@}
+            // \def\math@cr@{\new@ifnextchar[\math@cr@@{\math@cr@@[\z@]}}
+            // \def\math@cr@@[#1]{...\math@cr@@@...}
+            // \def\math@cr@@@{\cr}


Does this collapse multiple \crs in a row or something like that?

@kevinbarabash No, they are just how \\ is defined in LaTeX and amsmath.

I don't think I'll ever get used to reading .dtx files. 😓

k4b7

I didn't know that LaTeX treated primitives differently from macros. The summary you posted at the top of this PR was quite enlightening. Thank you for noticing this discrepancy and figuring out how to correct it. I've finished my first pass. I don't see any blocking issues, but I do have a fair number of questions about various things so I'm holding off on approving until you have a chance to respond to them.

k4b7 · 2020-08-09T22:52:45Z

src/functions/cr.js

-// same signature, we implement them as one megafunction, with newRow
-// indicating whether we're in the \cr case, and newLine indicating whether
-// to break the line in the \newline case.
-
+// \DeclareRobustCommand\\{...\@xnewline}


What does the ... stand for?

@kevinbarabash They are omitted commands for clarity.

k4b7 · 2020-08-09T22:59:43Z

src/functions/def.js

+                // If the very last character of the <parameter text> is #, so that
+                // this # is immediately followed by {, TeX will behave as if the {
+                // had been inserted at the right end of both the parameter text
+                // and the replacement text.


An example of this situation would be helpful.

@kevinbarabash

KaTeX/test/katex-spec.js

Line 3195 in 35470fd

expect`\gdef\foo#1#{#1}\foo1^{23}`.toParseLike`1^{23}`;

k4b7 · 2020-08-09T23:03:55Z

src/functions/def.js

+            } else if (tok.text === "EOF") {
+                throw new ParseError("Expected a macro definition");
+            } else {
+                delimiters[numArgs].push(tok.text);


Can tok.text be any string or should we be checking it against a list of allowed delimiters?

@kevinbarabash AFAIK, every valid TeX token can be used as delimiters.

k4b7 · 2020-08-09T23:04:39Z

src/functions/def.js

+            } else if (tok.text === "EOF") {
+                throw new ParseError("Expected a macro definition");


How would this case arise?

@kevinbarabash If the end is reached without the macro body, e.g., \def\abc.

This seems really obvious now. 😅

src/macros.js

k4b7 · 2020-08-09T23:16:46Z

test/errors-spec.js

+                   "Unexpected end of input in a macro argument," +
+                   " expected '}' at end of input: …r{#ffffff{text}");


Removing the position information may make it hard to track down errors in some cases. The underlining was also helpful because it made it really easy to see the range of the issue. Looking at some of the test cases below, we still have both of these features in those test cases. Is it hard to maintain those features in these test cases because how macro expansion has changed?

@kevinbarabash In case of macros, arguments should be scanned (fetched) first, so scanning errors are detected before parsing errors, hence changes in error position.

I see, this is just how we've been reporting parse for some time now.

k4b7 · 2020-08-09T23:20:48Z

test/errors-spec.js

            expect`\textcolor{1a2}{foo}`.toFailWithParseError(
                   "Invalid color: '1a2'" +
-                   " at position 12: \\textcolor{1̲a̲2̲}{foo}");
+                   " at position 11: \\textcolor{̲1̲a̲2̲}̲{foo}");


Why is there an extra space inside of \textcolor{} in the error message?

@kevinbarabash It is probably combining underline not shown properly.

I don't see an extra space anymore. 🤷‍♂️

k4b7 · 2020-08-09T23:27:56Z

test/katex-spec.js

-describe("A document fragment", function() {
-    it("should have paddings applied inside an extensible arrow", function() {
-        const markup = katex.renderToString("\\tiny\\xrightarrow\\textcolor{red}{x}");
-        expect(markup).toContain("x-arrow-pad");
-    });
-
-    it("should have paddings applied inside an enclose", function() {
-        const markup = katex.renderToString(r`\fbox\textcolor{red}{x}`);
-        expect(markup).toContain("boxpad");
-    });
-
-    it("should have paddings applied inside a square root", function() {
-        const markup = katex.renderToString(r`\sqrt\textcolor{red}{x}`);
-        expect(markup).toContain("padding-left");
-    });
-});


Why were these test cases removed?

@kevinbarabash They're test cases added in #1707, which relied on edge-case and previous (wrong) behaviors.

test/katex-spec.js

This reverts commit f96fba6.

k4b7

Thanks for answering all my questions and for all of your hard work on this PR.

src/MacroExpander.js

k4b7 · 2020-09-05T22:52:49Z

src/MacroExpander.js

+            tokens.shift();
+        }
+        tokens.reverse(); // to fit in with stack order
+        return {tokens, start, end: tok};


If the case where there's a { and a } at the start and end of tokens are the start and end the braces and tokens the stuff inside the braces (after the pop() and shift())?

k4b7 · 2020-09-05T22:53:52Z

src/MacroExpander.js

                "need to increase maxExpand setting");
        }
        let tokens = expansion.tokens;
+        const args = this.consumeArgs(expansion.numArgs, expansion.delimiters);


I didn't realize there could be multiple character delimiters. Thanks for the explanation.

k4b7 · 2020-09-05T22:57:13Z

src/Parser.js

+        argToken.text = str;
+        return argToken;


If that's the case, maybe we should have an assertion that argToken.text.length === str.length and throw if that's not the case. The error reporting locality would be better than waiting for an EOF mismatch.

k4b7 · 2020-09-05T23:00:28Z

src/environments/array.js

+            // \def\Let@{\let\\\math@cr}
+            // \def\math@cr{...\math@cr@}
+            // \def\math@cr@{\new@ifnextchar[\math@cr@@{\math@cr@@[\z@]}}
+            // \def\math@cr@@[#1]{...\math@cr@@@...}
+            // \def\math@cr@@@{\cr}


I don't think I'll ever get used to reading .dtx files. 😓

k4b7 · 2020-09-05T23:02:25Z

src/functions/def.js

+            } else if (tok.text === "EOF") {
+                throw new ParseError("Expected a macro definition");


This seems really obvious now. 😅

k4b7 · 2020-09-05T23:06:50Z

test/errors-spec.js

+                   "Unexpected end of input in a macro argument," +
+                   " expected '}' at end of input: …r{#ffffff{text}");


I see, this is just how we've been reporting parse for some time now.

k4b7 · 2020-09-05T23:08:16Z

test/errors-spec.js

            expect`\textcolor{1a2}{foo}`.toFailWithParseError(
                   "Invalid color: '1a2'" +
-                   " at position 12: \\textcolor{1̲a̲2̲}{foo}");
+                   " at position 11: \\textcolor{̲1̲a̲2̲}̲{foo}");


I don't see an extra space anymore. 🤷‍♂️

ylemkimon · 2020-09-06T03:56:27Z

@kevinbarabash Thank you for the review!

introduced in #2085: https://github.com/KaTeX/KaTeX/pull/2085/files#diff-d351bc0c29817ef51ebb3022b15c0704L67 Co-authored-by: ylemkimon <y@ylem.kim>

Improve macro argument parsing

5686ab0

ylemkimon commented Aug 21, 2019

View reviewed changes

ylemkimon added 3 commits August 22, 2019 02:51

Make \above a primitive command

c868eef

Fix screenshotter data

03ed68e

Normalize argument where necessary

e1f4f68

edemaine mentioned this pull request Oct 10, 2019

\limits inside \mathchoice doesn't do what I expect #2114

Closed

ylemkimon added 8 commits October 11, 2019 01:51

Improve argument location info

9298b92

Update comments

f0a6d9b

Minor refactor

6f2165d

Modularize group parsers

0b20825

Allow braced and blank size argument

653d665

for non-strict mode and \genfrac, respectively.

Minor refactor & update comments

c9ccd32

Remove raw option in parseStringGroup

93f069c

Update tests

5d83c09

ylemkimon requested review from edemaine and k4b7 October 11, 2019 15:55

ylemkimon added 4 commits October 12, 2019 01:48

Fix { delimited parameter

32acb4a

Update tests

6203288

Merge branch 'master' into arg-parser

a15f9fb

Update tests

bddb1f1

ylemkimon marked this pull request as ready for review October 11, 2019 17:20

ylemkimon added 7 commits October 12, 2019 11:35

Normalize argument in \genfrac

2b9cfb1

Update tests

c9eda04

Consume space before scanning an optional argument

9c9ece3

Fix \\, \newline, and \cr behavior

aac6b5b

Fix flow error

24d9d95

Update comments

d6a2105

Remove unnecessary mode switching

c5fb5b1

Parser mode affects neither fetch nor consume.

edemaine mentioned this pull request Jul 17, 2020

Math operators require braces when used in superscript/subscript #2319

Closed

ylemkimon changed the title ~~Improve macro argument parsing~~ feat(macro): improve argument parsing Jul 17, 2020

ylemkimon added 2 commits July 26, 2020 17:35

Merge branch 'master' into arg-parser

f1245f1

Update screenshots

35470fd

ylemkimon mentioned this pull request Jul 29, 2020

feat(parser): add integer, dimen, glue group parsers #2139

Draft

ylemkimon commented Aug 2, 2020

View reviewed changes

edemaine mentioned this pull request Aug 3, 2020

picture environment #1346

Open

k4b7 reviewed Aug 9, 2020

View reviewed changes

ylemkimon and others added 4 commits September 1, 2020 04:15

Merge branch 'master' into arg-parser

9a35f46

Revert "Document \def doesn't support delimiters (#2288) (#2289)"

f8a1b67

This reverts commit f96fba6.

Update comments, errors, and tests

55e726d

Update screenshots

c7bdf76

k4b7 approved these changes Sep 5, 2020

View reviewed changes

Merge branch 'master' into arg-parser

a2df241

ylemkimon merged commit dc5f97a into master Sep 6, 2020

ylemkimon deleted the arg-parser branch September 6, 2020 03:56

ylemkimon added a commit that referenced this pull request Nov 14, 2020

fix: typo in example on homepage (#2577)

ebd86b9

introduced in #2085: https://github.com/KaTeX/KaTeX/pull/2085/files#diff-d351bc0c29817ef51ebb3022b15c0704L67 Co-authored-by: ylemkimon <y@ylem.kim>

		const expression = this.parseExpression(false, "EOF");
		this.expect("EOF"); // expect the end of the argument

		// \cr is equivalent to \\ without the optional size argument (see below)
		// TODO: provide helpful error when \cr is used outside the array environment

		} else if (tok.text === "EOF") {
		throw new ParseError("Expected a macro definition");

		"Unexpected end of input in a macro argument," +
		" expected '}' at end of input: …r{#ffffff{text}");

Uh oh!

Conversation

ylemkimon commented Aug 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ylemkimon Aug 21, 2019 • 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!

Choose a reason for hiding this comment

Uh oh!

ylemkimon commented Oct 11, 2019

Uh oh!

codecov-commenter commented Jul 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

k4b7 commented Aug 2, 2020

Uh oh!

ylemkimon commented Aug 2, 2020

Uh oh!

ylemkimon Aug 2, 2020 • 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!

Choose a reason for hiding this comment

Uh oh!

k4b7 left a comment

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!

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!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ylemkimon Aug 31, 2020 • 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!

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!

k4b7 left a comment

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

ylemkimon commented Aug 21, 2019 •

edited

Loading

ylemkimon Aug 21, 2019 •

edited

Loading

codecov-commenter commented Jul 13, 2020 •

edited

Loading

ylemkimon Aug 2, 2020 •

edited

Loading

ylemkimon Aug 31, 2020 •

edited

Loading