Fix source slicing for whitespace-stripping comments

Re-apply #1430 with a more robust implementation and tests Also turns the warning into a `localAssert()` so it gets stripped before reaching downstream consumers. This relates to #1431 in that this removes the warning, so to the extent that #1431 was motivated by sliencing the warning in Ember builds, it avoids the issue. But I haven't investigated the root cause of that issue and whether it is indicative of an actual bug somewhere deeper. Fixes emberjs/ember.js#19392 Co-authored-by: Lee Nave <[email protected]>
glimmerjs · Feb 21, 2025 · 5256314 · 5256314
1 parent f65635c
commit 5256314
Show file tree

Hide file tree

Showing 5 changed files with 142 additions and 19 deletions.
diff --git a/packages/@glimmer/compiler/test/compiler-test.ts b/packages/@glimmer/compiler/test/compiler-test.ts
@@ -314,6 +314,88 @@ test('top-level comments', `<!-- {{foo}} -->`, c` {{foo}} `);
 
 test('handlebars comments', `<div>{{! Better not break! }}content</div>`, ['<div>', [s`content`]]);
 
+test('handlebars comments with whitespace removal', '<div>  {{~! some comment ~}}  content</div>', [
+  '<div>',
+  [s`content`],
+]);
+
+// Strange handlebars comments
+//
+// https://github.com/handlebars-lang/handlebars-parser/blob/a095229e292e814ed9d113d88c827f3509534d1a/lib/helpers.js#L44C26-L44C45
+//
+// These are all the ways I could find to write a comment with a single character 's':
+//
+// {{!s}}
+// {{!s-}}
+// {{!s--}}
+// {{!-s}}
+// {{!-s-}}
+// {{!-s--}}
+// {{!--s--}}
+//
+// Unclear if they are meant to work but the parser accepts them, and the compiler shouldn't break.
+//
+// This is really testing @glimmer/syntax's ASTv2 loc info, but that is currently only exercised by
+// the compiler and does not have its own dedicated test suite.
+const StrangeComments = [
+  // empty comments
+  '!',
+  '!-',
+  '!--',
+  '!---',
+  '!----',
+  // comment consisting of '-'
+  '!-----',
+  // comment consisting of '--'
+  '!------',
+  // comment consisting of '!'
+  '!!',
+  '!-!',
+  '!!-',
+  '!-!-',
+  '!-!--',
+  '!--!--',
+  // comment consisting of '!-'
+  '!!---',
+  '!-!---',
+  '!--!---',
+  // comment consisting of '!--'
+  '!!----',
+  '!-!----',
+  '!--!----',
+  // comment consisting of 's'
+  '!s',
+  '!s-',
+  '!s--',
+  '!-s',
+  '!-s-',
+  '!-s--',
+  '!--s--',
+  // notably does not work, probably a bug:
+  // '!--!',
+  // '!--!-',
+  // '!--s',
+  // '!--s-',
+];
+for (const c of StrangeComments) {
+  test(`strange handlebars comments {{${c}}}`, `<div>{{${c}}}content</div>`, [
+    '<div>',
+    [s`content`],
+  ]);
+  test(`strange handlebars comments {{~${c}}}`, `<div>  {{~${c}}}content</div>`, [
+    '<div>',
+    [s`content`],
+  ]);
+  test(`strange handlebars comments {{${c}~}}`, `<div>{{${c}~}}  content</div>`, [
+    '<div>',
+    [s`content`],
+  ]);
+  test(`strange handlebars comments {{~${c}~}}`, `<div>  {{~${c}~}}  content</div>`, [
+    '<div>',
+    [s`content`],
+  ]);
+}
+
 test('namespaced attribute', `<svg xlink:title='svg-title'>content</svg>`, [
   '<svg>',
   { 'xlink:title': s`svg-title` },

diff --git a/packages/@glimmer/syntax/lib/source/loc/span.ts b/packages/@glimmer/syntax/lib/source/loc/span.ts
@@ -1,3 +1,4 @@
+import { localAssert } from '@glimmer/debug-util';
 import { LOCAL_DEBUG } from '@glimmer/local-debug-flags';
 import { assertNever } from '@glimmer/util';
 
@@ -206,23 +207,16 @@ export class SourceSpan implements SourceLocation {
   }
 
   /**
-   * Convert this `SourceSpan` into a `SourceSlice`. In debug mode, this method optionally checks
-   * that the byte offsets represented by this `SourceSpan` actually correspond to the expected
-   * string.
+   * Convert this `SourceSpan` into a `SourceSlice`.
    */
   toSlice(expected?: string): SourceSlice {
     const chars = this.data.asString();
 
-    if (import.meta.env.DEV) {
-      if (expected !== undefined && chars !== expected) {
-        // eslint-disable-next-line no-console
-        console.warn(
-          `unexpectedly found ${JSON.stringify(
-            chars
-          )} when slicing source, but expected ${JSON.stringify(expected)}`
-        );
-      }
-    }
+    localAssert(
+      expected === undefined || expected === chars,
+      `unexpectedly found ${JSON.stringify(chars)} when slicing source, ` +
+        `but expected ${JSON.stringify(expected)}`
+    );
 
     return new SourceSlice({
       loc: this,

diff --git a/packages/@glimmer/syntax/lib/v2/normalize.ts b/packages/@glimmer/syntax/lib/v2/normalize.ts
@@ -390,17 +390,55 @@ class StatementNormalizer {
 
   MustacheCommentStatement(node: ASTv1.MustacheCommentStatement): ASTv2.GlimmerComment {
     let loc = this.block.loc(node.loc);
-    let textLoc: SourceSpan;
 
-    if (loc.asString().slice(0, 5) === '{{!--') {
-      textLoc = loc.slice({ skipStart: 5, skipEnd: 4 });
+    // If someone cares for these cases to have the right loc, feel free to attempt:
+    // {{!}} {{~!}} {{!~}} {{~!~}}
+    // {{!-}} {{~!-}} {{!-~}} {{~!-~}}
+    // {{!--}} {{~!--}} {{!--~}} {{~!--~}}
+    // {{!---}} {{~!---}} {{!---~}} {{~!---~}}
+    // {{!----}} {{~!----}} {{!----~}} {{~!----~}}
+    if (node.value === '') {
+      return new ASTv2.GlimmerComment({
+        loc,
+        text: SourceSlice.synthetic(''),
+      });
+    }
+
+    let source = loc.asString();
+    let span = loc;
+
+    if (node.value.startsWith('-')) {
+      localAssert(
+        /^\{\{~?!---/u.test(source),
+        `to start a comment's content with a '-', it must have started with {{!--`
+      );
+      span = span.sliceStartChars({
+        skipStart: source.startsWith('{{~') ? 6 : 5,
+        chars: node.value.length,
+      });
+    } else if (node.value.endsWith('-')) {
+      localAssert(
+        /--~?\}\}/u.test(source),
+        `to end a comment's content with a '-', it must have ended with --}}`
+      );
+
+      const skipEnd = source.endsWith('~}}') ? 5 : 4;
+      const skipStart = source.length - node.value.length - skipEnd;
+
+      span = span.slice({
+        skipStart,
+        skipEnd,
+      });
     } else {
-      textLoc = loc.slice({ skipStart: 3, skipEnd: 2 });
+      span = span.sliceStartChars({
+        skipStart: source.lastIndexOf(node.value),
+        chars: node.value.length,
+      });
     }
 
     return new ASTv2.GlimmerComment({
       loc,
-      text: textLoc.toSlice(node.value),
+      text: span.toSlice(node.value),
     });
   }
 

diff --git a/packages/@glimmer/syntax/test/loc-node-test.ts b/packages/@glimmer/syntax/test/loc-node-test.ts
@@ -585,9 +585,12 @@ test('handlebars comment', () => {
       {{!-- derp herky --}}<div></div>
     </div>
     <span {{! derpy }}></span>
+
+      {{~!~}}
+
   `);
 
-  let [, div, , span] = ast.body;
+  let [, div, , span, standaloneComment] = ast.body;
   if (assertNodeType(div, 'ElementNode')) {
     let [comment1, , comment2, trailingDiv] = div.children;
     locEqual(comment1, 2, 9, 2, 39);
@@ -598,6 +601,7 @@ test('handlebars comment', () => {
       locEqual(span, 5, 4, 5, 30);
       locEqual(comment3, 5, 10, 5, 22);
     }
+    locEqual(standaloneComment, 7, 6, 7, 13);
   }
 });
 

diff --git a/packages/@glimmer/syntax/test/parser-node-test.ts b/packages/@glimmer/syntax/test/parser-node-test.ts
@@ -794,6 +794,11 @@ test('a Handlebars comment', () => {
   );
 });
 
+test('a Handlebars comment with whitespace removal', function () {
+  let t = 'before {{~! some comment ~}} after';
+  astEqual(t, b.program([b.text('before'), b.mustacheComment(' some comment '), b.text('after')]));
+});
+
 test('a Handlebars comment in proper element space', () => {
   let t = 'before <div {{! some comment }} data-foo="bar" {{! other comment }}></div> after';
   astEqual(