refactor glimmer post-process, better handle template tag

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,49 +114,113 @@ 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;
   }
 
   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;
+    }
   });
 }
 

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