Repository: ember-cli/eslint-plugin-ember
Pull request #1795 (2023-03-02 23:31:46)

refactor glimmer post-process, better handle template tag
refactor postprocessing of template placeholders and re-map back to source more appropriately.

This PR makes many changes based on the assumption that the number of lines in the original code equals the number of lines in the transformed code. The code now throws an error if this assumption is not met. With this assumption in place, a lot of mapping from transformed -> original becomes a lot simpler: for a lint error on line X, when the original source content at line X matches the transformed source content at line X, we can return the original diagnostic message with no changes.

The remainder of the work in this PR is around mapping placeholder template values (`[__GLIMMER_TEMPLATE` back into its original form of `<template>`, and adjusting the `column` field in the diagnostic to match the changes. Same goes for the "closing tag" placeholder.

We still currently use the error prone "find token and loop through source" method when the diagnostic "token" is inside a template tag, but there is a failing test (commented out currently) which exhibits how this code is currently broken.

Other big change here is to generally add support for **multiline lint errors**, which never existed in the original implementation.



Patch:
diff --git a/lib/preprocessors/glimmer.js b/lib/preprocessors/glimmer.js
index 295f1aa9..140b69d7 100644
--- a/lib/preprocessors/glimmer.js
+++ b/lib/preprocessors/glimmer.js
@@ -14,6 +14,19 @@ 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]);
+}
+
+// regex to capture entire template with placeholder tags
+const oneLineTemplateRegex = /\[__GLIMMER_TEMPLATE\(`(.*)`, {.*}\)]/;
+
+// regex to capture opening template tag
+const openingTemplateTagRegex = /\[__GLIMMER_TEMPLATE\(`$/;
+
+// regex to capture closing template tag
+const closingTemplateTagRegex = /`, {.*}\)]/;
+
 /**
  * This function is responsible for running the embedded templates transform
  * from ember-template-imports.
@@ -101,6 +114,8 @@ function mapRange(messages, filename) {
     return flattened;
   }
 
+  // if there are no instances of the placeholder value, then this template
+  // did not contain a <template> tag, so preprocessed === postprocessed
   if (!transformed.includes(util.TEMPLATE_TAG_PLACEHOLDER)) {
     return flattened;
   }
@@ -108,42 +123,104 @@ function mapRange(messages, filename) {
   const lines = transformed.split('\n');
   const originalLines = original.split('\n');
 
+  // this function assumes the original output and transformed output
+  // are identical, minus the changes to transform `<template>` into the
+  // placeholder `[__GLIMMER_TEMPLATE()]
+  //
+  // in the future, for a usecase like integrating prettier --fix,
+  // this assumption will no longer be valid, and should be removed/refactored
+  if (lines.length !== originalLines.length) {
+    throw new Error(
+      'eslint-plugin-ember expected source file and transform file to have the same line length, but they differed'
+    );
+  }
+
   return flattened.map((message) => {
-    if (lines[message.line - 1] === originalLines[message.line - 1]) {
-      // original line and transformed line match exactly. return original diagnostic message
-      return message;
-    }
+    // 1. handle eslint diagnostics on single lines.
+    if (message.line === message.endLine) {
+      const originalLine = originalLines[message.line - 1];
+      const transformedLine = lines[message.line - 1];
+      if (originalLine === transformedLine) {
+        // original line and transformed line match exactly. return original diagnostic message
+        return message;
+      }
 
-    const line = lines[message.line - 1];
-    const token = line.slice(message.column - 1, message.endColumn - 1);
-
-    // Now that we have the token, we need to find the location
-    // in the original text
-    //
-    // TODO: Long term, we should use glimmer syntax parsing to find
-    // the correct node -- otherwise it's too easy to
-    // partially match on similar text
-    let originalLineNumber = 0;
-    let originalColumnNumber = 0;
-
-    for (const [index, line] of originalLines.entries()) {
-      const column = line.search(new RegExp(`\\b${escapeRegExp(token)}\\b`));
-      if (column > -1) {
-        originalLineNumber = index + 1;
-        originalColumnNumber = column + 1;
-        break;
+      // 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);
+
+      // lint error is specifically on JUST the opening <template> tag
+      if (openingTemplateTagRegex.test(token)) {
+        token = `<${util.TEMPLATE_TAG_NAME}>`;
+
+        // 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)) {
+        // 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}>`;
+
+        // 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;
+          }
+        }
       }
-    }
 
-    const modified = {
-      ...message,
-      line: originalLineNumber,
-      column: originalColumnNumber,
-      endLine: originalLineNumber,
-      endColumn: originalColumnNumber + token.length,
-    };
+      return modified;
+    } else {
+      // 2. handle multi-line diagnostics from eslint
+      const originalRange = originalLines.slice(message.line - 1, message.endLine);
+      const transformedRange = lines.slice(message.line - 1, message.endLine);
+      if (arrayEq(originalRange, transformedRange)) {
+        // original line range and transformed linerange match exactly. return original diagnostic message
+        return message;
+      }
 
-    return modified;
+      // for ranges containing template tag placeholders, the only change we should need to make is
+      // on the endColumn field, as this is the only one guaranteed to be different. The start column
+      // field does not need to be transformed, because it should be the same regardless of if we are
+      // using a <template> tag or the placeholder.
+      //
+      // we modify the endColumn below by finding the index of the closing placeholder tag, and
+      // mapping the placeholder syntax back into the ending column of the original `</template>` tag
+
+      const modified = { ...message };
+
+      const endLine = lines[message.endLine - 1];
+      if (closingTemplateTagRegex.test(endLine)) {
+        const originalEndLine = originalLines[message.endLine - 1];
+        const closingTemplateTag = `</${util.TEMPLATE_TAG_NAME}>`;
+        const closingTagIndex = originalEndLine.indexOf(closingTemplateTag);
+
+        if (closingTagIndex) {
+          modified.endColumn = closingTagIndex + closingTemplateTag.length;
+        }
+      }
+
+      return modified;
+    }
   });
 }
 


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 1bc27ba5..572661bc 100644
--- a/tests/lib/rules-preprocessor/gjs-gts-processor-test.js
+++ b/tests/lib/rules-preprocessor/gjs-gts-processor-test.js
@@ -37,6 +37,7 @@ function initESLint(options) {
       rules: {
         'lines-between-class-members': 'error',
         'no-undef': 'error',
+        'no-unused-vars': 'error',
         'ember/no-get': 'off',
         'ember/no-array-prototype-extensions': 'error',
       },
@@ -145,6 +146,47 @@ const invalid = [
       },
     ],
   },
+  {
+    filename: 'my-component.gjs',
+    code: `
+      import Component from '@glimmer/component';
+
+      export default class MyComponent extends Component {
+        foo = "bar";
+        <template>"hi"</template>
+      }
+    `,
+    errors: [
+      {
+        message: 'Expected blank line between class members.',
+        line: 6,
+        endLine: 6,
+        column: 9,
+        endColumn: 33,
+      },
+    ],
+  },
+  {
+    filename: 'my-component.gjs',
+    code: `
+      import Component from '@glimmer/component';
+
+      export default class MyComponent extends Component {
+        foo = "bar";
+        <template>"hi"
+        </template>
+      }
+    `,
+    errors: [
+      {
+        message: 'Expected blank line between class members.',
+        line: 6,
+        endLine: 7,
+        column: 9,
+        endColumn: 19,
+      },
+    ],
+  },
 ];
 
 describe('template-vars', () => {
@@ -207,7 +249,7 @@ describe('line/col numbers should be correct', () => {
     const code = `
     import Component from '@glimmer/component';
     import { get } from '@ember/object';
-    
+
     export default class MyComponent extends Component {
       constructor() {
         super(...arguments);
@@ -270,7 +312,38 @@ describe('lint errors on the exact line as the <template> tag', () => {
       foo = 'bar';
       <template>
         <div>
-          some totally random, non-meaningful text 
+          some totally random, non-meaningful text
+        </div>
+      </template>
+    }
+    `;
+    const results = await eslint.lintText(code, { filePath: 'my-component.gjs' });
+
+    const resultErrors = results.flatMap((result) => result.messages);
+    expect(resultErrors).toHaveLength(1);
+    expect(resultErrors[0].message).toBe('Expected blank line between class members.');
+  });
+});
+
+describe('multiple tokens in same file', () => {
+  it('correctly maps duplicate <template> tokens to the correct lines', async () => {
+    const eslint = initESLint();
+    const code = `
+    import Component from '@glimmer/component';
+
+    export const fakeTemplate = <template>
+      <div>"foo!"</div>
+    </template>
+
+    export default class MyComponent extends Component {
+      constructor() {
+        super(...arguments);
+      }
+
+      foo = 'bar';
+      <template>
+        <div>
+          some totally random, non-meaningful text
         </div>
       </template>
     }
@@ -280,5 +353,6 @@ describe('lint errors on the exact line as the <template> tag', () => {
     const resultErrors = results.flatMap((result) => result.messages);
     expect(resultErrors).toHaveLength(1);
     expect(resultErrors[0].message).toBe('Expected blank line between class members.');
+    expect(resultErrors[0].line).toBe(14);
   });
 });