Repository: ember-cli/eslint-plugin-ember
Pull request #1801 (2023-03-09 21:07:27)

Fix issue with token mapping for lint errors on template tokens in gjs/gts files by displaying eslint error on the opening `<template>` tag
**Background**
when using ESLint with gts/gjs files and encountering a lint error _specifically on a template token_, the error maps to the private `__GLIMMER_TEMPLATE` scope syntax, and we need to re-map that back into the template. In previous iterations of trying to map these tokens back to the original source, there were a few issues encountered:

**- token mapping was too eager, and would match the first instance of a token (even if it was part of a comment block)**
```
// comment Foo <-- this gets caught
<template> <Foo /> </template> // <-- this doesn't
```

**- token mapping never handled multiple tokens, eg:**
```
<template>
  <Foo />
  <Foo /> {{! this one never gets reported by eslint }}
</template>
```

**What does this PR do?**
We decided it would be easier (and less bug-prone) to instead just map any eslint errors on scope tokens back to the opening template tag. These error messages are modified to be prefixed with `Error in template: `, and will still give the user a signal that something is wrong in their template. This code will also be more efficient than searching the code for specific instances of tokens and filtering out any instances determined to be contained inside of a comment block.

This PR updates the logic to handle reporting on scope tokens. If we get a diagnostic message from eslint which is on a scope token, we search backwards until we find the first `<template>` tag, then we re-print the error message on this line.




Patch:
diff --git a/lib/preprocessors/glimmer.js b/lib/preprocessors/glimmer.js
index 140b69d7..e68e2ae3 100644
--- a/lib/preprocessors/glimmer.js
+++ b/lib/preprocessors/glimmer.js
@@ -9,11 +9,6 @@ const util = require('ember-template-imports/src/util');
 const TRANSFORM_CACHE = new Map();
 const TEXT_CACHE = new Map();
 
-// source: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Guide/Regular_Expressions#escaping
-function escapeRegExp(string) {
-  return string.replace(/[$()*+.?[\\\]^{|}]/g, '\\$&'); // $& means the whole matched string
-}
-
 function arrayEq(arr1, arr2) {
   return arr1.length === arr2.length && arr1.every((val, idx) => val === arr2[idx]);
 }
@@ -22,11 +17,16 @@ function arrayEq(arr1, arr2) {
 const oneLineTemplateRegex = /\[__GLIMMER_TEMPLATE\(`(.*)`, {.*}\)]/;
 
 // regex to capture opening template tag
-const openingTemplateTagRegex = /\[__GLIMMER_TEMPLATE\(`$/;
+const openingTemplateTagRegex = /\[__GLIMMER_TEMPLATE\(`/;
 
 // regex to capture closing template tag
 const closingTemplateTagRegex = /`, {.*}\)]/;
 
+// regex with capture group on scope token variables. In the following example:
+// `, { strictMode: true, scope: () => ({on,noop}) })]
+// the capture group would contain `on,noop`
+const getScopeTokenRegex = /scope: \(\) => \({(.*)}\) }\)/;
+
 /**
  * This function is responsible for running the embedded templates transform
  * from ember-template-imports.
@@ -145,50 +145,57 @@ function mapRange(messages, filename) {
         return message;
       }
 
-      // when the lines do not match, we've hit a lint error on the line containing the
-      // <template> tag, meaning we need to re-work the message
-      const modified = { ...message };
-      let token = transformedLine.slice(message.column - 1, message.endColumn - 1);
+      // when the lines do not match, we've hit a lint error on a line containing the
+      // <template> tag, the closing </template> tag, or possibly both.
 
-      // lint error is specifically on JUST the opening <template> tag
-      if (openingTemplateTagRegex.test(token)) {
-        token = `<${util.TEMPLATE_TAG_NAME}>`;
+      const modified = { ...message };
+      const token = transformedLine.slice(message.column - 1, message.endColumn - 1);
+      const lineHasOpeningTag = openingTemplateTagRegex.test(transformedLine);
+      const lineHasClosingTag = closingTemplateTagRegex.test(transformedLine);
 
-        // this case is simple: we know the starting column will be correct, so we just
-        // need to adjust the endColumn for the difference in length between the transformed
-        // token and the original token.
-        modified.endColumn = modified.column + token.length;
-      } else if (oneLineTemplateRegex.test(token)) {
+      if (oneLineTemplateRegex.test(token)) {
         // lint error is on a full, one-line <template>foo</template>
         const templateContext = token.match(oneLineTemplateRegex)[1];
-        token = `<${util.TEMPLATE_TAG_NAME}>${templateContext}<${util.TEMPLATE_TAG_NAME}>`;
+        const newToken = `<${util.TEMPLATE_TAG_NAME}>${templateContext}<${util.TEMPLATE_TAG_NAME}>`;
 
         // this case is simple: we know we have a one-line template invocation, and the
         // start `column` will be the same regardless of syntax. simply calculate the
         // length of the full token `<template>...</template>` and set the endColumn.
-        modified.endColumn = modified.column + token.length;
-      } else {
-        // if we haven't fallen into one of the cases above, we are likely dealing with
-        // a scope token in the template placeholder, typically for being used but not
-        // defined. the `token` should be the specific template API which the error is on,
-        // so we need to find the usage of the token in the template
-
-        // TODO: this is still bug-prone, and should be refactored to do something like:
-        // 1. for given token, find associated template tag
-        // 2. conduct search for token only within associated template
-        // 3. ensure we are not matching token inside comments
-        for (const [index, line] of originalLines.entries()) {
-          const newColumn = line.search(new RegExp(`\\b${escapeRegExp(token)}\\b`));
-          if (newColumn > -1) {
-            modified.line = index + 1;
-            modified.column = newColumn + 1;
-            modified.endColumn = newColumn + token.length + 1;
-            break;
+        modified.endColumn = modified.column + newToken.length + 1;
+        return modified;
+      }
+
+      if (lineHasClosingTag) {
+        const scopeTokens = transformedLine.match(getScopeTokenRegex)?.[1]?.split(',') ?? [];
+        if (scopeTokens.includes(token)) {
+          // when we have an error specifically with a scope token, we output the error
+          // on the starting <template> tag. Currently, we do not know the position of
+          // the token in the template, so there were efforts to do regex searches to
+          // find the token. Long story short, these all were very bug-prone, so now we
+          // just modify the message slightly and return it on the opening template tag.
+
+          let idx = message.line - 1;
+          let curLine = lines[idx];
+
+          while (idx) {
+            const templateTag = curLine.search(openingTemplateTagRegex);
+            if (templateTag > -1) {
+              modified.line = idx + 1;
+              modified.endLine = idx + 1;
+              modified.column = templateTag + 1;
+              modified.endColumn = templateTag + `<${util.TEMPLATE_TAG_NAME}>`.length + 1;
+              modified.message = `Error in template: ${message.message}`;
+              return modified;
+            }
+            curLine = lines[--idx];
           }
         }
+      } else if (lineHasOpeningTag) {
+        // token is before the <template> tag, no modifications necessary
+        if (transformedLine.indexOf(token) < transformedLine.search(openingTemplateTagRegex)) {
+          return message;
+        }
       }
-
-      return modified;
     } else {
       // 2. handle multi-line diagnostics from eslint
       const originalRange = originalLines.slice(message.line - 1, message.endLine);
@@ -215,12 +222,16 @@ function mapRange(messages, filename) {
         const closingTagIndex = originalEndLine.indexOf(closingTemplateTag);
 
         if (closingTagIndex) {
-          modified.endColumn = closingTagIndex + closingTemplateTag.length;
+          modified.endColumn = closingTagIndex + closingTemplateTag.length + 1;
         }
       }
 
       return modified;
     }
+
+    // if all else fails: return the original message. It may not have the correct line/col,
+    // but it is better to return a mis-aligned diagnostic message than none at all.
+    return message;
   });
 }
 


Test patch:
diff --git a/tests/lib/rules-preprocessor/gjs-gts-processor-test.js b/tests/lib/rules-preprocessor/gjs-gts-processor-test.js
index 572661bc..61c53150 100644
--- a/tests/lib/rules-preprocessor/gjs-gts-processor-test.js
+++ b/tests/lib/rules-preprocessor/gjs-gts-processor-test.js
@@ -110,9 +110,10 @@ const invalid = [
     `,
     errors: [
       {
-        message: "'on' is not defined.",
-        line: 5,
-        column: 11,
+        message: "Error in template: 'on' is not defined.",
+        line: 4,
+        column: 7,
+        endColumn: 17,
       },
     ],
   },
@@ -125,9 +126,10 @@ const invalid = [
     `,
     errors: [
       {
-        message: "'noop' is not defined.",
-        line: 3,
-        column: 11,
+        message: "Error in template: 'noop' is not defined.",
+        line: 2,
+        column: 7,
+        endColumn: 17,
       },
     ],
   },
@@ -140,9 +142,26 @@ const invalid = [
     `,
     errors: [
       {
-        message: "'Foo' is not defined.",
-        line: 3,
-        column: 10,
+        message: "Error in template: 'Foo' is not defined.",
+        line: 2,
+        column: 7,
+        endColumn: 17,
+      },
+    ],
+  },
+  {
+    filename: 'my-component.gjs',
+    code: `
+      <template>
+        <F_0_O />
+      </template>
+    `,
+    errors: [
+      {
+        message: "Error in template: 'F_0_O' is not defined.",
+        line: 2,
+        column: 7,
+        endColumn: 17,
       },
     ],
   },
@@ -162,7 +181,7 @@ const invalid = [
         line: 6,
         endLine: 6,
         column: 9,
-        endColumn: 33,
+        endColumn: 34,
       },
     ],
   },
@@ -183,7 +202,7 @@ const invalid = [
         line: 6,
         endLine: 7,
         column: 9,
-        endColumn: 19,
+        endColumn: 20,
       },
     ],
   },
@@ -326,6 +345,82 @@ describe('lint errors on the exact line as the <template> tag', () => {
 });
 
 describe('multiple tokens in same file', () => {
+  it('correctly maps duplicate tokens to the correct lines', async () => {
+    const eslint = initESLint();
+    const code = `
+      // comment one
+      // comment two
+      // comment three
+      const two = 2;
+
+      const three = <template> "bar" </template>
+    `;
+    const results = await eslint.lintText(code, { filePath: 'my-component.gjs' });
+
+    const resultErrors = results.flatMap((result) => result.messages);
+    expect(resultErrors).toHaveLength(2);
+
+    expect(resultErrors[0]).toStrictEqual({
+      column: 13,
+      endColumn: 16,
+      endLine: 5,
+      line: 5,
+      message: "'two' is assigned a value but never used.",
+      messageId: 'unusedVar',
+      nodeType: 'Identifier',
+      ruleId: 'no-unused-vars',
+      severity: 2,
+    });
+
+    expect(resultErrors[1]).toStrictEqual({
+      column: 13,
+      endColumn: 18,
+      endLine: 7,
+      line: 7,
+      message: "'three' is assigned a value but never used.",
+      messageId: 'unusedVar',
+      nodeType: 'Identifier',
+      ruleId: 'no-unused-vars',
+      severity: 2,
+    });
+  });
+
+  it('handles duplicate template tokens', async () => {
+    const eslint = initESLint();
+    const code = `
+      // comment Bad 
+
+      const tmpl = <template><Bad /></template>
+    `;
+    const results = await eslint.lintText(code, { filePath: 'my-component.gjs' });
+
+    const resultErrors = results.flatMap((result) => result.messages);
+    expect(resultErrors).toHaveLength(2);
+
+    expect(resultErrors[0]).toStrictEqual({
+      column: 13,
+      endColumn: 17,
+      endLine: 4,
+      line: 4,
+      message: "'tmpl' is assigned a value but never used.",
+      messageId: 'unusedVar',
+      nodeType: 'Identifier',
+      ruleId: 'no-unused-vars',
+      severity: 2,
+    });
+
+    expect(resultErrors[1]).toStrictEqual({
+      column: 20,
+      endColumn: 30,
+      endLine: 4,
+      line: 4,
+      message: "Error in template: 'Bad' is not defined.",
+      messageId: 'undef',
+      nodeType: 'Identifier',
+      ruleId: 'no-undef',
+      severity: 2,
+    });
+  });
   it('correctly maps duplicate <template> tokens to the correct lines', async () => {
     const eslint = initESLint();
     const code = `