Repository: KaTeX/KaTeX (js)

Issue:
First character of array doesn't get its own group
The following should render 111, but it currently renders as 22:
```latex
\def\x{1}\begin{matrix}\def\x{2}&\x\end{matrix}\x
```

As described in `test/katex-spec.js` (see TODO), before the environment gets called, `{matrix}` gets consumed which means that the `\def` gets executed, before we can create a group. :-(

This is a general issue with the way `nextToken` works, so it's going to be difficult to fix.

Pull request description:
Fix parse timing by separating consume() into fetch() and consume()
Fix #1989, and generally cleanup parse timing (many fewer hoops to jump through) by defining two methods on parser:

1. `consume()` marks the current token (`nextToken`) as "done", but doesn't yet fetch the next token (setting `nextToken` to `null`).
2. `fetch()` fetches the next token if we don't already have one (e.g., if last token was `consume()`d).

Before this change, `consume()` did both actions.  By separating them, and allowing the parser to live in a state for a while where `nextToken` is `null`, it is far easier to change settings (in particular, math/text mode and catcodes) before reading the next token, in a way that depends on what we're parsing.  For example, if an argument should be treated in text mode, we can just set the mode in the argument parser, instead of when the previous token was consumed.  Similarly, if an argument should be treated as a URL, we can just set the catcode of `%` in the URL argument parser, and reset it after.  We no longer have to take care to reset things before calling `consume()`.

This change mostly involves changing `this.nextToken` to `this.fetch()`. In a perfect world, we could use slightly fewer calls to `this.fetch()`, but Flow doesn't realize that `this.nextToken` will be non-null after a
call to `this.fetch()`, so we need to use a few more calls to `this.nextToken()` or a few more local `nextToken` variables.

I'm kind of impressed how such a major change could be relatively easy to do.  It's been extremely helpful to have so many tests and type checking in our codebase.  For reference, I previously tried to change all the methods to load the token when they need them, and not bother marking used tokens as consumed, so that `nextToken` is never `null`.  This proved very difficult (maybe impossible) because we sometimes get nested deep within `parseExpression` and a single token needs to trigger all of them to terminate (e.g., `\fbox{$\displaystyle x$}`, which is what `\boxed{x}` expands to, the last `$` token ends the `\displaystyle` implicit group and the math mode). So I opted for this approach, which is a much smaller (and more obviously correct) change to the code.

Patch:
diff --git a/src/Parser.js b/src/Parser.js
index ba6ab750..b9a9e97a 100644
--- a/src/Parser.js
+++ b/src/Parser.js
@@ -55,7 +55,7 @@ export default class Parser {
     gullet: MacroExpander;
     settings: Settings;
     leftrightDepth: number;
-    nextToken: Token;
+    nextToken: ?Token;
 
     constructor(input: string, settings: Settings) {
         // Start in math mode
@@ -74,10 +74,9 @@ export default class Parser {
      * appropriate error otherwise.
      */
     expect(text: string, consume?: boolean = true) {
-        if (this.nextToken.text !== text) {
+        if (this.fetch().text !== text) {
             throw new ParseError(
-                "Expected '" + text + "', got '" + this.nextToken.text + "'",
-                this.nextToken
+                `Expected '${text}', got '${this.fetch().text}'`, this.fetch()
             );
         }
         if (consume) {
@@ -86,11 +85,22 @@ export default class Parser {
     }
 
     /**
-     * Considers the current look ahead token as consumed,
-     * and fetches the one after that as the new look ahead.
+     * Discards the current lookahead token, considering it consumed.
      */
     consume() {
-        this.nextToken = this.gullet.expandNextToken();
+        this.nextToken = null;
+    }
+
+    /**
+     * Return the current lookahead token, or if there isn't one (at the
+     * beginning, or if the previous lookahead token was consume()d),
+     * fetch the next token as the new lookahead token and return it.
+     */
+    fetch(): Token {
+        if (this.nextToken == null) {
+            this.nextToken = this.gullet.expandNextToken();
+        }
+        return this.nextToken;
     }
 
     /**
@@ -117,11 +127,10 @@ export default class Parser {
         }
 
         // Try to parse the input
-        this.consume();
         const parse = this.parseExpression(false);
 
         // If we succeeded, make sure there's an EOF at the end
-        this.expect("EOF", false);
+        this.expect("EOF");
 
         // End the group namespace for the expression
         this.gullet.endGroup();
@@ -159,7 +168,7 @@ export default class Parser {
             if (this.mode === "math") {
                 this.consumeSpaces();
             }
-            const lex = this.nextToken;
+            const lex = this.fetch();
             if (Parser.endOfExpression.indexOf(lex.text) !== -1) {
                 break;
             }
@@ -246,11 +255,12 @@ export default class Parser {
     handleSupSubscript(
         name: string,   // For error reporting.
     ): AnyParseNode {
-        const symbolToken = this.nextToken;
+        const symbolToken = this.fetch();
         const symbol = symbolToken.text;
         this.consume();
-        this.consumeSpaces(); // ignore spaces before sup/subscript argument
-        const group = this.parseGroup(name, false, Parser.SUPSUB_GREEDINESS);
+        const group = this.parseGroup(name, false, Parser.SUPSUB_GREEDINESS,
+            undefined, undefined, true);
+                                 // ignore spaces before sup/subscript argument
 
         if (!group) {
             throw new ParseError(
@@ -311,7 +321,7 @@ export default class Parser {
             this.consumeSpaces();
 
             // Lex the first token
-            const lex = this.nextToken;
+            const lex = this.fetch();
 
             if (lex.text === "\\limits" || lex.text === "\\nolimits") {
                 // We got a limit control
@@ -355,14 +365,14 @@ export default class Parser {
                 const primes = [prime];
                 this.consume();
                 // Keep lexing tokens until we get something that's not a prime
-                while (this.nextToken.text === "'") {
+                while (this.fetch().text === "'") {
                     // For each one, add another prime to the list
                     primes.push(prime);
                     this.consume();
                 }
                 // If there's a superscript following the primes, combine that
                 // superscript in with the primes.
-                if (this.nextToken.text === "^") {
+                if (this.fetch().text === "^") {
                     primes.push(this.handleSupSubscript("superscript"));
                 }
                 // Put everything into an ordgroup as the superscript
@@ -398,12 +408,14 @@ export default class Parser {
         name?: string, // For error reporting.
         greediness?: ?number,
     ): ?AnyParseNode {
-        const token = this.nextToken;
+        const token = this.fetch();
         const func = token.text;
         const funcData = functions[func];
         if (!funcData) {
             return null;
         }
+        this.consume(); // consume command token
+
         if (greediness != null && funcData.greediness <= greediness) {
             throw new ParseError(
                 "Got function '" + func + "' with no arguments" +
@@ -416,22 +428,6 @@ export default class Parser {
                 "Can't use function '" + func + "' in math mode", token);
         }
 
-        // hyperref package sets the catcode of % as an active character
-        if (funcData.argTypes && funcData.argTypes[0] === "url") {
-            this.gullet.lexer.setCatcode("%", 13);
-        }
-
-        // Consume the command token after possibly switching to the
-        // mode specified by the function (for instant mode switching),
-        // and then immediately switch back.
-        if (funcData.consumeMode) {
-            const oldMode = this.mode;
-            this.switchMode(funcData.consumeMode);
-            this.consume();
-            this.switchMode(oldMode);
-        } else {
-            this.consume();
-        }
         const {args, optArgs} = this.parseArguments(func, funcData);
         return this.callFunction(func, args, optArgs, token, breakOnTokenText);
     }
@@ -486,28 +482,23 @@ export default class Parser {
             // "After you have said ‘\def\row#1#2{...}’, you are allowed to
             //  put spaces between the arguments (e.g., ‘\row x n’), because
             //  TeX doesn’t use single spaces as undelimited arguments."
-            if (i > 0 && !isOptional) {
-                this.consumeSpaces();
-            }
+            const consumeSpaces = (i > 0 && !isOptional) ||
             // Also consume leading spaces in math mode, as parseSymbol
             // won't know what to do with them.  This can only happen with
             // macros, e.g. \frac\foo\foo where \foo expands to a space symbol.
-            // In LaTeX, the \foo's get treated as (blank) arguments).
+            // In LaTeX, the \foo's get treated as (blank) arguments.
             // In KaTeX, for now, both spaces will get consumed.
             // TODO(edemaine)
-            if (i === 0 && !isOptional && this.mode === "math") {
-                this.consumeSpaces();
-            }
-            const nextToken = this.nextToken;
-            const arg = this.parseGroupOfType("argument to '" + func + "'",
-                argType, isOptional, baseGreediness);
+                (i === 0 && !isOptional && this.mode === "math");
+            const arg = this.parseGroupOfType(`argument to '${func}'`,
+                argType, isOptional, baseGreediness, consumeSpaces);
             if (!arg) {
                 if (isOptional) {
                     optArgs.push(null);
                     continue;
                 }
                 throw new ParseError(
-                    "Expected group after '" + func + "'", nextToken);
+                    `Expected group after '${func}'`, this.fetch());
             }
             (isOptional ? optArgs : args).push(arg);
         }
@@ -523,22 +514,30 @@ export default class Parser {
         type: ?ArgType,
         optional: boolean,
         greediness: ?number,
+        consumeSpaces: boolean,
     ): ?AnyParseNode {
         switch (type) {
             case "color":
+                if (consumeSpaces) {
+                    this.consumeSpaces();
+                }
                 return this.parseColorGroup(optional);
             case "size":
+                if (consumeSpaces) {
+                    this.consumeSpaces();
+                }
                 return this.parseSizeGroup(optional);
             case "url":
-                return this.parseUrlGroup(optional);
+                return this.parseUrlGroup(optional, consumeSpaces);
             case "math":
             case "text":
-                return this.parseGroup(name, optional, greediness, undefined, type);
+                return this.parseGroup(
+                    name, optional, greediness, undefined, type, consumeSpaces);
             case "hbox": {
                 // hbox argument type wraps the argument in the equivalent of
                 // \hbox, which is like \text but switching to \textstyle size.
-                const group = this.parseGroup(
-                    name, optional, greediness, undefined, "text");
+                const group = this.parseGroup(name, optional, greediness,
+                    undefined, "text", consumeSpaces);
                 if (!group) {
                     return group;
                 }
@@ -551,7 +550,10 @@ export default class Parser {
                 return styledGroup;
             }
             case "raw": {
-                if (optional && this.nextToken.text === "{") {
+                if (consumeSpaces) {
+                    this.consumeSpaces();
+                }
+                if (optional && this.fetch().text === "{") {
                     return null;
                 }
                 const token = this.parseStringGroup("raw", optional, true);
@@ -562,21 +564,25 @@ export default class Parser {
                         string: token.text,
                     };
                 } else {
-                    throw new ParseError("Expected raw group", this.nextToken);
+                    throw new ParseError("Expected raw group", this.fetch());
                 }
             }
             case "original":
             case null:
             case undefined:
-                return this.parseGroup(name, optional, greediness);
+                return this.parseGroup(name, optional, greediness,
+                    undefined, undefined, consumeSpaces);
             default:
                 throw new ParseError(
-                    "Unknown group type as " + name, this.nextToken);
+                    "Unknown group type as " + name, this.fetch());
         }
     }
 
+    /**
+     * Discard any space tokens, fetching the next non-space token.
+     */
     consumeSpaces() {
-        while (this.nextToken.text === " ") {
+        while (this.fetch().text === " ") {
             this.consume();
         }
     }
@@ -592,27 +598,27 @@ export default class Parser {
     ): ?Token {
         const groupBegin = optional ? "[" : "{";
         const groupEnd = optional ? "]" : "}";
-        const nextToken = this.nextToken;
-        if (nextToken.text !== groupBegin) {
+        const beginToken = this.fetch();
+        if (beginToken.text !== groupBegin) {
             if (optional) {
                 return null;
-            } else if (raw && nextToken.text !== "EOF" &&
-                    /[^{}[\]]/.test(nextToken.text)) {
-                // allow a single character in raw string group
-                this.gullet.lexer.setCatcode("%", 14); // reset the catcode of %
+            } else if (raw && beginToken.text !== "EOF" &&
+                    /[^{}[\]]/.test(beginToken.text)) {
                 this.consume();
-                return nextToken;
+                return beginToken;
             }
         }
         const outerMode = this.mode;
         this.mode = "text";
         this.expect(groupBegin);
         let str = "";
-        const firstToken = this.nextToken;
+        const firstToken = this.fetch();
         let nested = 0; // allow nested braces in raw string group
         let lastToken = firstToken;
-        while ((raw && nested > 0) || this.nextToken.text !== groupEnd) {
-            switch (this.nextToken.text) {
+        let nextToken;
+        while ((nextToken = this.fetch()).text !== groupEnd ||
+               (raw && nested > 0)) {
+            switch (nextToken.text) {
                 case "EOF":
                     throw new ParseError(
                         "Unexpected end of input in " + modeName,
@@ -624,13 +630,12 @@ export default class Parser {
                     nested--;
                     break;
             }
-            lastToken = this.nextToken;
+            lastToken = nextToken;
             str += lastToken.text;
             this.consume();
         }
-        this.mode = outerMode;
-        this.gullet.lexer.setCatcode("%", 14); // reset the catcode of %
         this.expect(groupEnd);
+        this.mode = outerMode;
         return firstToken.range(lastToken, str);
     }
 
@@ -645,12 +650,13 @@ export default class Parser {
     ): Token {
         const outerMode = this.mode;
         this.mode = "text";
-        const firstToken = this.nextToken;
+        const firstToken = this.fetch();
         let lastToken = firstToken;
         let str = "";
-        while (this.nextToken.text !== "EOF" &&
-                regex.test(str + this.nextToken.text)) {
-            lastToken = this.nextToken;
+        let nextToken;
+        while ((nextToken = this.fetch()).text !== "EOF" &&
+               regex.test(str + nextToken.text)) {
+            lastToken = nextToken;
             str += lastToken.text;
             this.consume();
         }
@@ -695,7 +701,7 @@ export default class Parser {
     parseSizeGroup(optional: boolean): ?ParseNode<"size"> {
         let res;
         let isBlank = false;
-        if (!optional && this.nextToken.text !== "{") {
+        if (!optional && this.fetch().text !== "{") {
             res = this.parseRegexGroup(
                 /^[-+]? *(?:$|\d+|\d+\.\d*|\.\d*) *[a-z]{0,2} *$/, "size");
         } else {
@@ -731,10 +737,13 @@ export default class Parser {
     }
 
     /**
-     * Parses an URL, checking escaped letters and allowed protocols.
+     * Parses an URL, checking escaped letters and allowed protocols,
+     * and setting the catcode of % as an active character (as in \hyperref).
      */
-    parseUrlGroup(optional: boolean): ?ParseNode<"url"> {
+    parseUrlGroup(optional: boolean, consumeSpaces: boolean): ?ParseNode<"url"> {
+        this.gullet.lexer.setCatcode("%", 13); // active character
         const res = this.parseStringGroup("url", optional, true); // get raw string
+        this.gullet.lexer.setCatcode("%", 14); // comment character
         if (!res) {
             return null;
         }
@@ -768,27 +777,35 @@ export default class Parser {
         greediness?: ?number,
         breakOnTokenText?: BreakToken,
         mode?: Mode,
+        consumeSpaces?: boolean,
     ): ?AnyParseNode {
-        const outerMode = this.mode;
-        const firstToken = this.nextToken;
-        const text = firstToken.text;
         // Switch to specified mode
+        const outerMode = this.mode;
         if (mode) {
             this.switchMode(mode);
         }
+        // Consume spaces if requested, crucially *after* we switch modes,
+        // so that the next non-space token is parsed in the correct mode.
+        if (consumeSpaces) {
+            this.consumeSpaces();
+        }
+        // Get first token
+        const firstToken = this.fetch();
+        const text = firstToken.text;
 
-        let groupEnd;
         let result;
         // Try to parse an open brace or \begingroup
         if (optional ? text === "["  : text === "{" || text === "\\begingroup") {
-            groupEnd = Parser.endOfGroup[text];
+            this.consume();
+            const groupEnd = Parser.endOfGroup[text];
             // Start a new group namespace
             this.gullet.beginGroup();
             // If we get a brace, parse an expression
-            this.consume();
             const expression = this.parseExpression(false, groupEnd);
-            const lastToken = this.nextToken;
-            // End group namespace before consuming symbol after close brace
+            const lastToken = this.fetch();
+            // Check that we got a matching closing brace
+            this.expect(groupEnd);
+            // End group namespace
             this.gullet.endGroup();
             result = {
                 type: "ordgroup",
@@ -824,10 +841,6 @@ export default class Parser {
         if (mode) {
             this.switchMode(outerMode);
         }
-        // Make sure we got a close brace
-        if (groupEnd) {
-            this.expect(groupEnd);
-        }
         return result;
     }
 
@@ -878,10 +891,10 @@ export default class Parser {
 
     /**
      * Parse a single symbol out of the string. Here, we handle single character
-     * symbols and special functions like verbatim
+     * symbols and special functions like \verb.
      */
     parseSymbol(): ?AnyParseNode {
-        const nucleus = this.nextToken;
+        const nucleus = this.fetch();
         let text = nucleus.text;
 
         if (/^\\verb[^a-zA-Z]/.test(text)) {
diff --git a/src/defineFunction.js b/src/defineFunction.js
index af80f981..286d6284 100644
--- a/src/defineFunction.js
+++ b/src/defineFunction.js
@@ -5,7 +5,7 @@ import type Parser from "./Parser";
 import type {ParseNode, AnyParseNode, NodeType, UnsupportedCmdParseNode}
     from "./parseNode";
 import type Options from "./Options";
-import type {ArgType, BreakToken, Mode} from "./types";
+import type {ArgType, BreakToken} from "./types";
 import type {HtmlDomNode} from "./domTree";
 import type {Token} from "./Token";
 import type {MathDomNode} from "./mathMLTree";
@@ -85,14 +85,6 @@ export type FunctionPropSpec = {
 
     // Must be true if the function is an infix operator.
     infix?: boolean,
-
-    // Switch to the specified mode while consuming the command token.
-    // This is useful for commands that switch between math and text mode,
-    // for making sure that a switch happens early enough.  Note that the
-    // mode is switched immediately back to its original value after consuming
-    // the command token, so that the argument parsing and/or function handler
-    // can easily access the old mode while doing their own mode switching.
-    consumeMode?: ?Mode,
 };
 
 type FunctionDefSpec<NODETYPE: NodeType> = {|
@@ -138,7 +130,6 @@ export type FunctionSpec<NODETYPE: NodeType> = {|
     allowedInMath: boolean,
     numOptionalArgs: number,
     infix: boolean,
-    consumeMode: ?Mode,
 
     // FLOW TYPE NOTES: Doing either one of the following two
     //
@@ -197,7 +188,6 @@ export default function defineFunction<NODETYPE: NodeType>({
             : props.allowedInMath,
         numOptionalArgs: props.numOptionalArgs || 0,
         infix: !!props.infix,
-        consumeMode: props.consumeMode,
         handler: handler,
     };
     for (let i = 0; i < names.length; ++i) {
diff --git a/src/environments/array.js b/src/environments/array.js
index 6eaceac5..5dca8ffa 100644
--- a/src/environments/array.js
+++ b/src/environments/array.js
@@ -34,12 +34,12 @@ function getHLines(parser: Parser): boolean[] {
     // Each element in the array tells if the line is dashed.
     const hlineInfo = [];
     parser.consumeSpaces();
-    let nxt = parser.nextToken.text;
+    let nxt = parser.fetch().text;
     while (nxt === "\\hline" || nxt === "\\hdashline") {
         parser.consume();
         hlineInfo.push(nxt === "\\hdashline");
         parser.consumeSpaces();
-        nxt = parser.nextToken.text;
+        nxt = parser.fetch().text;
     }
     return hlineInfo;
 }
@@ -110,7 +110,7 @@ function parseArray(
             };
         }
         row.push(cell);
-        const next = parser.nextToken.text;
+        const next = parser.fetch().text;
         if (next === "&") {
             parser.consume();
         } else if (next === "\\end") {
diff --git a/src/functions/math.js b/src/functions/math.js
index 7d7b04a6..e0d08d57 100644
--- a/src/functions/math.js
+++ b/src/functions/math.js
@@ -10,18 +10,14 @@ defineFunction({
         numArgs: 0,
         allowedInText: true,
         allowedInMath: false,
-        consumeMode: "math",
     },
     handler({funcName, parser}, args) {
         const outerMode = parser.mode;
         parser.switchMode("math");
         const close = (funcName === "\\(" ? "\\)" : "$");
         const body = parser.parseExpression(false, close);
-        // We can't expand the next symbol after the closing $ until after
-        // switching modes back.  So don't consume within expect.
-        parser.expect(close, false);
+        parser.expect(close);
         parser.switchMode(outerMode);
-        parser.consume();
         return {
             type: "styling",
             mode: parser.mode,
diff --git a/src/functions/text.js b/src/functions/text.js
index a5fbe392..486f1ca1 100644
--- a/src/functions/text.js
+++ b/src/functions/text.js
@@ -50,7 +50,6 @@ defineFunction({
         argTypes: ["text"],
         greediness: 2,
         allowedInText: true,
-        consumeMode: "text",
     },
     handler({parser, funcName}, args) {
         const body = args[0];


Test patch:
diff --git a/test/katex-spec.js b/test/katex-spec.js
index 31009a63..b0e35e36 100644
--- a/test/katex-spec.js
+++ b/test/katex-spec.js
@@ -3079,7 +3079,7 @@ describe("A macro expander", function() {
     // which doesn't treat all four letters as an argument.
     //it("\\TextOrMath should work in a macro passed to \\text", function() {
     //    expect`\text\mode`.toParseLike(r`\text{text}`, new Settings({macros:
-    //        {"\\mode": "\\TextOrMath{text}{math}"});
+    //        {"\\mode": "\\TextOrMath{text}{math}"}});
     //});
 
     it("\\gdef defines macros", function() {
@@ -3144,20 +3144,12 @@ describe("A macro expander", function() {
     });
 
     it("array cells generate groups", () => {
-        expect`\def\x{1}\begin{matrix}\x&\def\x{2}\x&\x\end{matrix}`
-            .toParseLike`\begin{matrix}1&2&1\end{matrix}`;
+        expect`\def\x{1}\begin{matrix}\x&\def\x{2}\x&\x\end{matrix}\x`
+            .toParseLike`\begin{matrix}1&2&1\end{matrix}1`;
+        expect`\def\x{1}\begin{matrix}\def\x{2}\x&\x\end{matrix}\x`
+            .toParseLike`\begin{matrix}2&1\end{matrix}1`;
     });
 
-    // TODO: This doesn't yet work; before the environment gets called,
-    // {matrix} gets consumed which means that the \def gets executed, before
-    // we can create a group. :-(  Issue #1989
-    /*
-    it("array cells generate groups", () => {
-        expect`\def\x{1}\begin{matrix}\def\x{2}&\x\end{matrix}`
-            .toParseLike`\begin{matrix}&1\end{matrix}`;
-    });
-    */
-
     it("\\gdef changes settings.macros", () => {
         const macros = {};
         expect`\gdef\foo{1}`.toParse(new Settings({macros}));