wasm: Fix and re-enable implicit space skipping

Author: pdubroy

packages/wasm/Makefile

@@ -41,7 +41,7 @@ build/es5.wasm: dist/index.js
 	$(NODE) scripts/es5ToWasm.js build/es5.wasm
 
 build/liquid-html.wasm: dist/index.js
-	$(NODE) src/cli.js -g LiquidHTML -o build/liquid-html.wasm test/data/liquid-html-mod.ohm
+	$(NODE) src/cli.js -g LiquidHTML -o build/liquid-html.wasm test/data/liquid-html.ohm
 
 .PHONY: go-test-es5
 go-test-es5: test/go/testmain build/es5.wasm

packages/wasm/runtime/ohmRuntime.ts

@@ -4,7 +4,7 @@ declare function fillInputBuffer(offset: i32, maxLen: i32): i32;
 declare function printI32(val: i32): void;
 declare function isRuleSyntactic(ruleId: i32): bool;
 
-@inline const IMPLICIT_SPACE_SKIPPING = false;
+@inline const IMPLICIT_SPACE_SKIPPING = true;
 
 // TODO: Find a way to share these.
 @inline const WASM_PAGE_SIZE: usize = 64 * 1024;
@@ -84,6 +84,15 @@ function hasMemoizedResult(ruleId: i32): boolean {
   return memoTableGet(pos, ruleId) !== 0;
 }
 
+@inline function maybeSkipSpaces(ruleId: i32): void {
+  // TODO: Find a better way to deal with the bindings here.
+  if (IMPLICIT_SPACE_SKIPPING && isRuleSyntactic(ruleId)) {
+    const origNumBindings = bindings.length;
+    evalApply0(2);
+    bindings.length = origNumBindings;
+  }
+}
+
 export function match(startRuleId: i32): Result {
   // (Re-)initialize globals, clear memo table.
   pos = 0;
@@ -93,15 +102,12 @@ export function match(startRuleId: i32): Result {
 
   // Get the input and do the match.
   let inputLen = fillInputBuffer(0, i32(WASM_PAGE_SIZE));
-  const succeeded = evalApply0(startRuleId) !== 0;
 
-  // Potentially skip trailing spaces before checking for the end.
-  if (IMPLICIT_SPACE_SKIPPING && succeeded && isRuleSyntactic(startRuleId)) {
-    evalApply0(2);
-  }
-
-  if (inputLen === pos) {
-    return succeeded;
+  maybeSkipSpaces(startRuleId);
+  const succeeded = evalApply0(startRuleId) !== 0;
+  if (succeeded) {
+    maybeSkipSpaces(startRuleId);
+    return inputLen === pos;
   }
   return 0;
 }
@@ -218,5 +224,6 @@ export function setBindingsLength(len: i32): void {
 }
 
 export function getCstRoot(): usize {
+  // TODO: Figure out how to handle this w.r.t. leading and trailing space.
   return bindings[0];
 }

packages/wasm/scripts/bench.js

@@ -19,7 +19,7 @@ const inputs = {
   underscore: readFileSync(join(datadir, '_underscore-1.8.3.js'), 'utf-8'),
 };
 
-const liquid = ohm.grammars(readFileSync(join(datadir, 'liquid-html-mod.ohm'), 'utf8'));
+const liquid = ohm.grammars(readFileSync(join(datadir, 'liquid-html.ohm'), 'utf8'));
 
 let liquidHtmlMatcher;
 let es5Matcher;

packages/wasm/scripts/parseLiquid.js

@@ -22,7 +22,7 @@ import {wasmMatcherForGrammar} from '../test/_helpers.js';
 const __dirname = dirname(fileURLToPath(import.meta.url));
 const datadir = join(__dirname, '../test/data');
 
-const liquid = ohm.grammars(readFileSync(join(datadir, 'liquid-html-mod.ohm'), 'utf8'));
+const liquid = ohm.grammars(readFileSync(join(datadir, 'liquid-html.ohm'), 'utf8'));
 
 // Get pattern from command line arguments
 const pattern = process.argv[2];
@@ -76,7 +76,10 @@ const matchWithInput = (m, str) => (m.setInput(str), m.match());
     const start = performance.now();
     assert.equal(matchWithInput(m, input), 1, `failed: ${path}`);
     wasmTimes.push(performance.now() - start);
-    assert.equal(input.length, unparseW(input, m.getCstRoot()).length);
+
+    // Trailing/leading spaces are currently dropped, so trim both
+    // to after unparsing.
+    assert.equal(input.trim().length, unparseW(input, m.getCstRoot()).trim().length);
   }
 
   const sum = arr => arr.reduce((a, b) => a + b, 0);

packages/wasm/src/index.js

@@ -12,7 +12,8 @@ const WASM_PAGE_SIZE = 64 * 1024;
 const DEBUG = process.env.OHM_DEBUG === '1';
 const FAST_SAVE_BINDINGS = true;
 const FAST_RESTORE_BINDINGS = true;
-const IMPLICIT_SPACE_SKIPPING = false;
+
+const IMPLICIT_SPACE_SKIPPING = true;
 
 // When specializing rules, should we emit a generalized version that
 // handles the specific cases? If false, code size will be larger.
@@ -63,7 +64,10 @@ function uniqueName(names, str) {
   return name;
 }
 
-const isSyntactic = ruleName => ruleName[0] === ruleName[0].toUpperCase();
+function isSyntacticRule(ruleName) {
+  assert(ruleName[0] !== '$', ruleName);
+  return ruleName[0] === ruleName[0].toUpperCase();
+}
 
 class IndexedSet {
   constructor() {
@@ -617,11 +621,11 @@ export class Compiler {
     return idx;
   }
 
-  inLexifiedContext() {
-    return this._lexContextStack.at(-1);
+  inLexicalContext() {
+    return checkNotNull(this._lexContextStack.at(-1));
   }
 
-  liftPExpr(exp) {
+  liftPExpr(exp, isSyntactic) {
     assert(!(exp instanceof pexprs.Terminal));
 
     // Note: the same expression might appear in more than one place, and
@@ -656,7 +660,7 @@ export class Compiler {
       formals = newParams.filter(isNonNull).map(p => `__${p.index}`);
     }
     const actuals = freeVars.filter(isNonNull);
-    const ruleInfo = {body, formals, source: exp.source};
+    const ruleInfo = {body, formals, isSyntactic, source: exp.source};
     return [name, ruleInfo, actuals];
   }
 
@@ -733,13 +737,26 @@ export class Compiler {
     const {grammar} = this;
 
     const lookUpRule = name => {
-      if (name in grammar.rules) return grammar.rules[name];
-      if (grammar.superGrammar) return lookUpRule(name, grammar.superGrammar);
+      const isSyntactic = isSyntacticRule(name);
+      if (name in grammar.rules) {
+        return {...grammar.rules[name], isSyntactic};
+      }
+      if (grammar.superGrammar) {
+        return lookUpRule(name, grammar.superGrammar);
+      }
     };
 
     // Begin with all the rules in the grammar + spaces.
-    const rules = Object.entries(this.grammar.rules);
-    rules.push(['spaces', lookUpRule('spaces')]);
+    const rules = Object.entries(this.grammar.rules).map(([name, info]) => {
+      const isSyntactic = isSyntacticRule(name);
+      return [name, {...info, isSyntactic}];
+    });
+    rules.push([
+      'spaces',
+      {
+        ...lookUpRule('spaces'),
+      },
+    ]);
 
     const liftedTerminals = new IndexedSet();
 
@@ -751,25 +768,25 @@ export class Compiler {
 
     // If `exp` is not an Apply or Param, lift it into its own rule and return
     // a new application of that rule.
-    const simplifyArg = exp => {
+    const simplifyArg = (exp, isSyntactic) => {
       if (isApplyLike(exp)) {
-        return simplify(exp);
+        return simplify(exp, isSyntactic);
       }
       if (exp instanceof pexprs.Terminal) {
         return liftTerminal(exp);
       }
 
-      const [name, info, env] = this.liftPExpr(exp);
+      const [name, info, env] = this.liftPExpr(exp, isSyntactic);
       const args = env.map(p => {
         assert(p instanceof pexprs.Param, 'Expected Param');
         return ir.param(p.index);
       });
       rules.push([name, info]);
       return ir.apply(name, args);
     };
-    const simplify = exp => {
+    const simplify = (exp, isSyntactic) => {
       if (exp instanceof pexprs.Alt) {
-        return ir.alt(exp.terms.map(e => simplify(e)));
+        return ir.alt(exp.terms.map(e => simplify(e, isSyntactic)));
       }
       if (exp === pexprs.any) return ir.any();
       if (exp === pexprs.end) return ir.end();
@@ -778,24 +795,24 @@ export class Compiler {
           rules.push([exp.ruleName, checkNotNull(lookUpRule(exp.ruleName))]);
           return ir.apply(
               exp.ruleName,
-              exp.args.map(arg => simplifyArg(arg)),
+              exp.args.map(arg => simplifyArg(arg, isSyntactic)),
           );
         case pexprs.CaseInsensitive:
           return ir.caseInsensitive(exp.obj);
         case pexprs.Lex:
-          return ir.lex(simplify(exp.expr));
+          return ir.lex(simplify(exp.expr, true));
         case pexprs.Lookahead:
-          return ir.lookahead(simplify(exp.expr));
+          return ir.lookahead(simplify(exp.expr, isSyntactic));
         case pexprs.Not:
-          return ir.not(simplify(exp.expr));
+          return ir.not(simplify(exp.expr, isSyntactic));
         case pexprs.Opt:
-          return ir.opt(simplify(exp.expr));
+          return ir.opt(simplify(exp.expr, isSyntactic));
         case pexprs.Plus:
-          return ir.plus(simplify(exp.expr));
+          return ir.plus(simplify(exp.expr, isSyntactic));
         case pexprs.Seq:
-          return ir.seq(exp.factors.map(e => simplify(e)));
+          return ir.seq(exp.factors.map(e => simplify(e, isSyntactic)));
         case pexprs.Star:
-          return ir.star(simplify(exp.expr));
+          return ir.star(simplify(exp.expr, isSyntactic));
         case pexprs.Param:
           return ir.param(exp.index);
         case pexprs.Range:
@@ -818,7 +835,7 @@ export class Compiler {
       if (!newRules.has(name)) {
         newRules.set(name, {
           ...info,
-          body: simplify(info.body),
+          body: simplify(info.body, info.isSyntactic),
         });
       }
     }
@@ -828,10 +845,10 @@ export class Compiler {
 
   compileTerminalRule(name) {
     const {asm} = this;
+    this.beginLexContext(true);
     asm.addFunction(`$${name}`, [w.valtype.i32], [w.valtype.i32], () => {
       asm.addLocal('ret', w.valtype.i32);
       asm.addLocal('tmp', w.valtype.i32);
-
       asm.switch(
           w.blocktype.empty,
           () => asm.localGet('__arg0'),
@@ -842,18 +859,28 @@ export class Compiler {
       );
       asm.localGet('ret');
     });
+    this.endLexContext();
     return this.asm._functionDecls.at(-1);
   }
 
+  beginLexContext(initialVal) {
+    assert(this._lexContextStack.length === 0);
+    this._lexContextStack.push(initialVal);
+  }
+
+  endLexContext() {
+    this._lexContextStack.pop();
+    assert(this._lexContextStack.length === 0);
+  }
+
   compileRule(name) {
     const {asm} = this;
     const ruleInfo = getNotNull(this.rules, name);
     let paramTypes = [];
     if (ruleInfo.patterns) {
       paramTypes = [w.valtype.i32];
     }
-    assert(this._lexContextStack.length === 0);
-    this._lexContextStack.push(!isSyntactic(name));
+    this.beginLexContext(!ruleInfo.isSyntactic);
     asm.addFunction(`$${name}`, paramTypes, [w.valtype.i32], () => {
       asm.addLocal('ret', w.valtype.i32);
       asm.addLocal('tmp', w.valtype.i32);
@@ -862,8 +889,7 @@ export class Compiler {
       asm.emit(`END eval:${name}`);
       asm.localGet('ret');
     });
-    this._lexContextStack.pop();
-    assert(this._lexContextStack.length === 0);
+    this.endLexContext();
     return this.asm._functionDecls.at(-1);
   }
 
@@ -1204,6 +1230,7 @@ export class Compiler {
 
   emitApplyTerm({terminalId}) {
     const {asm} = this;
+    this.maybeEmitSpaceSkipping();
     asm.i32Const(terminalId);
     asm.emit(w.instr.call, this.ruleEvalFuncIdx('$term'));
     asm.localSet('ret');
@@ -1223,7 +1250,7 @@ export class Compiler {
     assert(exp.children.length === 0);
 
     if (exp !== this._applySpaces) {
-      this.maybeEmitSpaceSkipping();
+      this.maybeEmitSpaceSkipping(); // Avoid infinite recursion.
     }
 
     const {asm} = this;
@@ -1340,7 +1367,7 @@ export class Compiler {
   }
 
   maybeEmitSpaceSkipping() {
-    if (IMPLICIT_SPACE_SKIPPING && !this.inLexifiedContext()) {
+    if (IMPLICIT_SPACE_SKIPPING && !this.inLexicalContext()) {
       this.asm.emit('BEGIN space skipping');
       this.emitApply(this._applySpaces);
       this.asm.emit('END space skipping');

packages/wasm/test/test-liquid-html.js

@@ -10,7 +10,7 @@ import {wasmMatcherForGrammar} from './_helpers.js';
 const matchWithInput = (m, str) => (m.setInput(str), m.match());
 
 const scriptRel = relPath => new URL(relPath, import.meta.url);
-const grammarSource = fs.readFileSync(scriptRel('data/liquid-html-mod.ohm'), 'utf8');
+const grammarSource = fs.readFileSync(scriptRel('data/liquid-html.ohm'), 'utf8');
 
 const liquid = ohm.grammars(grammarSource);
 
@@ -36,6 +36,14 @@ test('swatch.liquid', async t => {
   t.is(matchWithInput(m, input), 1);
 });
 
+test('html comment', async t => {
+  const input = `{% if x %}
+    <!-- x -->
+  {% endif %}`;
+  const m = await wasmMatcherForGrammar(liquid.LiquidHTML);
+  t.is(matchWithInput(m, input), 1);
+});
+
 test('book-review.liquid', async t => {
   const input = fs.readFileSync(scriptRel('data/book-review.liquid'), 'utf8');
   let start = performance.now();

packages/wasm/test/test-wasm.js

@@ -891,18 +891,45 @@ test('lifted terminals', async t => {
   t.is(matchWithInput(m, 'yy'), 0);
 });
 
-// eslint-disable-next-line ava/no-skip-test
-test.skip('basic space skipping', async t => {
+test('basic space skipping', async t => {
   const g = ohm.grammar(`
     G {
       Start = ">" (digit "a".."z")*
     }`);
   const m = await wasmMatcherForGrammar(g);
-  t.is(matchWithInput(m, '> 0 a 1 b '), 1);
+  t.is(matchWithInput(m, '> 0 a 1 b'), 1);
+  t.is(matchWithInput(m, ' > 0 a 1 b '), 1);
 });
 
-// eslint-disable-next-line ava/no-skip-test
-test.skip('space skipping & lex', async t => {
+test('space skipping w/ lifted terminals', async t => {
+  // It shouldn't matter that the terminal (as arg) appears in a syntactic
+  // context; only the point of use.
+  const g = ohm.grammar(`
+    G {
+      Start = two<"x">
+      two<t> = t t
+    }`);
+  const m = await wasmMatcherForGrammar(g);
+  t.is(matchWithInput(m, 'xx'), 1);
+  t.is(matchWithInput(m, ' xx'), 1);
+  t.is(matchWithInput(m, 'x x'), 0);
+});
+
+test('space skipping w/ params', async t => {
+  // Make sure space is skipped before params in the body of syntactic rule.
+  const g = ohm.grammar(`
+    G {
+      Start = Reversed<(x | "x"), y, "z".."z"> Reversed<z, y, x>
+      Reversed<a, b, c> = c b a
+      x = "x"
+      y = "y"
+      z = "z"
+    }`);
+  const m = await wasmMatcherForGrammar(g);
+  t.is(matchWithInput(m, ' z y x xyz'), 1);
+});
+
+test('space skipping & lex', async t => {
   {
     const g = ohm.grammar('G { start = ">" digit+ #(space) }');
     const m = await wasmMatcherForGrammar(g);