Second attempt at landing #1552 (part 1/4)

The original commit has a few issues:

- Type safety
- Added a new AST node, which is unnecessary since we already have
  `VarHead`, but is also a breaking change and is kind of breakage
  we have been careful to avoid in this area thus far
- The parsing is robust against a few classes of edge cases

This rework the same feature (on the `Block`/`BlockStatement` side)
and adds more test coverage for the feature. Also reanmed the field
to `block.params`.

Author: chancancode

packages/@glimmer/syntax/lib/parser.ts

@@ -76,6 +76,8 @@ export abstract class Parser {
     // node.loc = node.loc.withEnd(end);
   }
 
+  abstract parse(node: HBS.Program, locals: string[]): ASTv1.Template;
+
   abstract Program(node: HBS.Program): HBS.Output<'Program'>;
   abstract MustacheStatement(node: HBS.MustacheStatement): HBS.Output<'MustacheStatement'>;
   abstract Decorator(node: HBS.Decorator): HBS.Output<'Decorator'>;
@@ -149,14 +151,6 @@ export abstract class Parser {
     return node;
   }
 
-  acceptTemplate(node: HBS.Program): ASTv1.Template {
-    let result = this.Program(node);
-    assert(result.type === 'Template', 'expected a template');
-    return result;
-  }
-
-  acceptNode(node: HBS.Program): ASTv1.Block | ASTv1.Template;
-  acceptNode<U extends HBS.Node | ASTv1.Node>(node: HBS.Node): U;
   acceptNode<T extends HBS.NodeType>(node: HBS.Node<T>): HBS.Output<T> {
     return (this[node.type as T] as (node: HBS.Node<T>) => HBS.Output<T>)(node);
   }

packages/@glimmer/syntax/lib/parser/handlebars-node-visitors.ts

@@ -1,8 +1,9 @@
 import type { Nullable, Recast } from '@glimmer/interfaces';
 import type { TokenizerState } from 'simple-html-tokenizer';
-import { getLast, isPresentArray, unwrap } from '@glimmer/util';
+import { assert, getLast, isPresentArray, unwrap } from '@glimmer/util';
 
 import type { ParserNodeBuilder, Tag } from '../parser';
+import type { SourceSpan } from '../source/span';
 import type * as ASTv1 from '../v1/api';
 import type * as HBS from '../v1/handlebars-ast';
 
@@ -24,46 +25,62 @@ export abstract class HandlebarsNodeVisitors extends Parser {
     return this.elementStack.length === 0;
   }
 
-  Program(program: HBS.Program): ASTv1.Block;
-  Program(program: HBS.Program): ASTv1.Template;
-  Program(program: HBS.Program): ASTv1.Template | ASTv1.Block;
-  Program(program: HBS.Program): ASTv1.Block | ASTv1.Template {
-    const body: ASTv1.Statement[] = [];
-    let node;
-
-    if (this.isTopLevel) {
-      node = b.template({
-        body,
-        loc: this.source.spanFor(program.loc),
-      });
-    } else {
-      node = b.blockItself({
-        body,
-        blockParams: program.blockParams,
-        chained: program.chained,
-        loc: this.source.spanFor(program.loc),
-      });
-    }
+  parse(program: HBS.Program, locals: string[]): ASTv1.Template {
+    let node = b.template({
+      body: [],
+      locals,
+      loc: this.source.spanFor(program.loc),
+    });
 
-    let i,
-      l = program.body.length;
+    return this.parseProgram(node, program);
+  }
 
-    this.elementStack.push(node);
+  Program(program: HBS.Program, blockParams?: ASTv1.VarHead[]): ASTv1.Block {
+    // The abstract signature doesn't have the blockParams argument, but in
+    // practice we can only come from this.BlockStatement() which adds the
+    // extra argument for us
+    assert(
+      Array.isArray(blockParams),
+      '[BUG] Program in parser unexpectedly called without block params'
+    );
 
-    if (l === 0) {
-      return this.elementStack.pop() as ASTv1.Block | ASTv1.Template;
+    let node = b.blockItself({
+      body: [],
+      params: blockParams,
+      chained: program.chained,
+      loc: this.source.spanFor(program.loc),
+    });
+
+    return this.parseProgram(node, program);
+  }
+
+  private parseProgram<T extends ASTv1.ParentNode>(node: T, program: HBS.Program): T {
+    if (program.body.length === 0) {
+      return node;
     }
 
-    for (i = 0; i < l; i++) {
-      this.acceptNode(unwrap(program.body[i]));
+    let poppedNode;
+
+    try {
+      this.elementStack.push(node);
+
+      for (let child of program.body) {
+        this.acceptNode(child);
+      }
+    } finally {
+      poppedNode = this.elementStack.pop();
     }
 
     // Ensure that that the element stack is balanced properly.
-    const poppedNode = this.elementStack.pop();
-    if (poppedNode !== node) {
-      const elementNode = poppedNode as ASTv1.ElementNode;
-
-      throw generateSyntaxError(`Unclosed element \`${elementNode.tag}\``, elementNode.loc);
+    if (node !== poppedNode) {
+      if (poppedNode?.type === 'ElementNode') {
+        throw generateSyntaxError(`Unclosed element \`${poppedNode.tag}\``, poppedNode.loc);
+      } else {
+        // If the stack is not balanced, then it is likely our own bug, because
+        // any unclosed Handlebars blocks should already been caught by now
+        assert(poppedNode !== undefined, '[BUG] empty parser elementStack');
+        assert(false, `[BUG] mismatched parser elementStack node: ${node.type}`);
+      }
     }
 
     return node;
@@ -83,6 +100,66 @@ export abstract class HandlebarsNodeVisitors extends Parser {
     }
 
     const { path, params, hash } = acceptCallNodes(this, block);
+    const loc = this.source.spanFor(block.loc);
+
+    // Backfill block params loc for the default block
+    let blockParams: ASTv1.VarHead[] = [];
+
+    if (block.program.blockParams?.length) {
+      // Start from right after the hash
+      let span = hash.loc.collapse('end');
+
+      // Extend till the beginning of the block
+      if (block.program.loc) {
+        span = span.withEnd(this.source.spanFor(block.program.loc).getStart());
+      }
+      if (block.program.body[0]) {
+        span = span.withEnd(this.source.spanFor(block.program.body[0].loc).getStart());
+      } else {
+        // ...or if all else fail, use the end of the block statement
+        // this can only happen if the block statement is empty anyway
+        span = span.withEnd(loc.getEnd());
+      }
+
+      // Now we have a span for something like this:
+      //
+      //   {{#foo bar baz=bat as |wow wat|}}
+      //                     ~~~~~~~~~~~~~~~
+      //
+      // Or, if we are unlucky:
+      //
+      // {{#foo bar baz=bat as |wow wat|}}{{/foo}}
+      //                   ~~~~~~~~~~~~~~~~~~~~~~~
+      //
+      // Either way, within this span, there should be exactly two pipes
+      // fencing our block params, neatly whitespace separated and with
+      // legal identifiers only
+      const content = span.asString();
+      let skipStart = content.indexOf('|') + 1;
+      const limit = content.indexOf('|', skipStart);
+
+      for (const name of block.program.blockParams) {
+        let nameStart: number;
+        let loc: SourceSpan;
+
+        if (skipStart >= limit) {
+          nameStart = -1;
+        } else {
+          nameStart = content.indexOf(name, skipStart);
+        }
+
+        if (nameStart === -1 || nameStart + name.length > limit) {
+          skipStart = limit;
+          loc = this.source.spanFor(NON_EXISTENT_LOCATION);
+        } else {
+          skipStart = nameStart;
+          loc = span.sliceStartChars({ skipStart, chars: name.length });
+          skipStart += name.length;
+        }
+
+        blockParams.push(b.var({ name, loc }));
+      }
+    }
 
     // These are bugs in Handlebars upstream
     if (!block.program.loc) {
@@ -93,8 +170,8 @@ export abstract class HandlebarsNodeVisitors extends Parser {
       block.inverse.loc = NON_EXISTENT_LOCATION;
     }
 
-    const program = this.Program(block.program);
-    const inverse = block.inverse ? this.Program(block.inverse) : null;
+    const program = this.Program(block.program, blockParams);
+    const inverse = block.inverse ? this.Program(block.inverse, []) : null;
 
     const node = b.block({
       path,
@@ -126,7 +203,7 @@ export abstract class HandlebarsNodeVisitors extends Parser {
 
     if (isHBSLiteral(rawMustache.path)) {
       mustache = b.mustache({
-        path: this.acceptNode<ASTv1.Literal>(rawMustache.path),
+        path: this.acceptNode<(typeof rawMustache.path)['type']>(rawMustache.path),
         params: [],
         hash: b.hash({ pairs: [], loc: this.source.spanFor(rawMustache.path.loc).collapse('end') }),
         trusting: !escaped,
@@ -388,7 +465,7 @@ export abstract class HandlebarsNodeVisitors extends Parser {
     const pairs = hash.pairs.map((pair) =>
       b.pair({
         key: pair.key,
-        value: this.acceptNode(pair.value),
+        value: this.acceptNode<HBS.Expression['type']>(pair.value),
         loc: this.source.spanFor(pair.loc),
       })
     );
@@ -536,7 +613,7 @@ function acceptCallNodes(
   }
 
   const params = node.params
-    ? node.params.map((e) => compiler.acceptNode<ASTv1.Expression>(e))
+    ? node.params.map((e) => compiler.acceptNode<HBS.Expression['type']>(e))
     : [];
 
   // if there is no hash, position it as a collapsed node immediately after the last param (or the

packages/@glimmer/syntax/lib/parser/tokenizer-event-handlers.ts

@@ -441,7 +441,10 @@ export function preprocess(
     end: offsets.endPosition,
   };
 
-  let template = new TokenizerEventHandlers(source, entityParser, mode).acceptTemplate(ast);
+  let template = new TokenizerEventHandlers(source, entityParser, mode).parse(
+    ast,
+    options.locals ?? []
+  );
 
   if (options.strictMode && options.locals?.length) {
     template = b.template({ ...template, locals: options.locals });

packages/@glimmer/syntax/lib/source/loc/span.ts

@@ -190,7 +190,7 @@ export class SourceSpan implements SourceLocation {
   /**
    * Create a new span with the current span's beginning and a new ending.
    */
-  withEnd(this: SourceSpan, other: SourceOffset): SourceSpan {
+  withEnd(other: SourceOffset): SourceSpan {
     return span(this.data.getStart(), other.data);
   }
 

packages/@glimmer/syntax/lib/v1/handlebars-ast.ts

@@ -13,7 +13,7 @@ export interface CommonNode {
 }
 
 export interface NodeMap {
-  Program: { input: Program; output: ASTv1.Template | ASTv1.Block };
+  Program: { input: Program; output: ASTv1.Block };
   MustacheStatement: { input: MustacheStatement; output: ASTv1.MustacheStatement | void };
   Decorator: { input: Decorator; output: never };
   BlockStatement: { input: BlockStatement; output: ASTv1.BlockStatement | void };
@@ -50,7 +50,7 @@ export interface Position {
 export interface Program extends CommonNode {
   type: 'Program';
   body: Statement[];
-  blockParams: string[];
+  blockParams?: string[];
   chained?: boolean;
 }
 

packages/@glimmer/syntax/lib/v1/nodes-v1.ts

@@ -16,8 +16,13 @@ export interface CommonProgram extends BaseNode {
 
 export interface Block extends CommonProgram {
   type: 'Block';
-  blockParams: string[];
+  params: VarHead[];
   chained?: boolean;
+
+  /**
+   * string accessor for params.name
+   */
+  blockParams: string[];
 }
 
 export type EntityEncodingState = 'transformed' | 'raw';
@@ -302,6 +307,34 @@ export type Nodes = {
 export type NodeType = keyof Nodes;
 export type Node = Nodes[NodeType];
 
+// These "sub-node" cannot appear standalone, they are only used inside another
+// "real" AST node to provide richer information. The distinction mostly exists
+// for backwards compatibility reason. These nodes are not traversed and do not
+// have visitor keys for them, so it won't break existing AST consumers (e.g.
+// those that implemented an `All` visitor may not be expecting these new types
+// of nodes).
+//
+// Conceptually, the idea of "sub-node" does make sense, and you can say source
+// locations are another kind of these things. However, in these cases, they
+// actually fully implement the `BaseNode` interface, and only not extending
+// `BaseNode` because the `type` field is not `keyof Nodes` (which is circular
+// reasoning). If these are not "real" nodes because they can only appear in
+// very limited context, then the same reasoning probably applies for, say,
+// HashPair.
+//
+// If we do eventually make some kind of breaking change here, perhaps with
+// some kind of opt-in, then we can consider upgrading these into "real" nodes,
+// but for now, this is where they go, and it isn't a huge problem in practice
+// because there are little utility in traversing these kind of nodes anyway.
+export type SubNodes = {
+  ThisHead: ThisHead;
+  AtHead: AtHead;
+  VarHead: VarHead;
+};
+
+export type SubNodeType = keyof SubNodes;
+export type SubNode = SubNodes[SubNodeType];
+
 export type Statement = Nodes[StatementName];
 export type Statements = Pick<Nodes, StatementName>;
 export type Literal = Nodes[LiteralName];