Fix issue with token mapping for lint errors on template tokens in gjs/gts files by displaying eslint error on the opening <template> tag (#1801)

* fix issue with token detection eagerly matching the wrong instance of token

* display scope token lint errors on opening template tag

* fix linting

* remove TODO from now passing test

* update regex to allow more characters in scope token search

* fix overly permissive regex

---------

Co-authored-by: Henry Majoros <[email protected]>
Co-authored-by: Lucy Lin <[email protected]>

Author: 3 people

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;
   });
 }
 

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 = `