Skip to content

Conversation

@nayounsang
Copy link
Contributor

@nayounsang nayounsang commented Jun 28, 2025

PR Checklist

Overview

ref: https://github.com/estree/estree/blob/2bc9ea235184b6164e31b1088575447657e686c6/es2018.md?plain=1#L34

  • change type of cooked in the ast-spec
    • also update snapshot
  • eval string to check for incorrect escaping
  • add test case
  • affected these rule
    • plugin-test-formatting : ignore when cooked is null
    • no-unsafe-assignment: ignore when cooked is null
    • no-duplicate-enum-values: There can be no null case. If value is tagged, it causes ts error and the existing logic only assumes that it is not tagged.

@typescript-eslint
Copy link
Contributor

Thanks for the PR, @nayounsang!

typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community.

The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately.

Thanks again!


🙏 Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently on https://opencollective.com/typescript-eslint.

@netlify
Copy link

netlify bot commented Jun 28, 2025

Deploy Preview for typescript-eslint ready!

Name Link
🔨 Latest commit f43e69b
🔍 Latest deploy log https://app.netlify.com/projects/typescript-eslint/deploys/68c80fa63dc75b0008e3eee1
😎 Deploy Preview https://deploy-preview-11355--typescript-eslint.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
Lighthouse
Lighthouse
1 paths audited
Performance: 85 (🔴 down 1 from production)
Accessibility: 97 (no change from production)
Best Practices: 100 (no change from production)
SEO: 92 (no change from production)
PWA: 80 (no change from production)
View the detailed breakdown and full score reports

To edit notification comments on pull requests, go to your Netlify project configuration.

@nayounsang nayounsang marked this pull request as draft June 28, 2025 08:59
@nayounsang nayounsang changed the title fix(typescript-estree): If the template literal is tagged and the text has an invalid escape, cooked will be null fix(typescript-estree): if the template literal is tagged and the text has an invalid escape, cooked will be null Jun 28, 2025
@nx-cloud
Copy link

nx-cloud bot commented Jun 28, 2025

View your CI Pipeline Execution ↗ for commit f43e69b

Command Status Duration Result
nx run-many -t lint ✅ Succeeded 3m 18s View ↗
nx test eslint-plugin --coverage=false ✅ Succeeded 5m 6s View ↗
nx test typescript-estree --coverage=false ✅ Succeeded 21s View ↗
nx run-many -t typecheck ✅ Succeeded 2m 16s View ↗
nx run types:build ✅ Succeeded 6s View ↗
nx test eslint-plugin-internal --coverage=false ✅ Succeeded 11s View ↗
nx run generate-configs ✅ Succeeded 6s View ↗
nx run integration-tests:test ✅ Succeeded 4s View ↗
Additional runs (27) ✅ Succeeded ... View ↗

☁️ Nx Cloud last updated this comment at 2025-09-15 13:21:30 UTC

@nayounsang
Copy link
Contributor Author

nayounsang commented Jun 28, 2025

It's strange. In my local, I cleared the cache and the changed types were built in the types package. Isn't the CI environment a new virtual machine that runs anyway?


typescript-estree has a dependency on types. TemplateElement comes from types
TemplateElement in types is auto-generated by ast-spec.
In nx cloud, types is hitting cache in CI.
So, changes are not updated and an error occurs.
Do I need to do something separate?

@nayounsang
Copy link
Contributor Author

nayounsang commented Jul 20, 2025

hi @JamesHenry
Thank you for your efforts on this difficult problem.

  • Can I continue working on this PR?
  • Is it okay if I merge with the main branch and resolve conflicts freely?
  • If CI-related work is not completed, can I help?

@JamesHenry
Copy link
Member

@nayounsang Yes, sorry I forgot about this one, I obviously kept iterating on your other one to discover that one wasn't a caching issue. This one might be related to your other one or it might be totally separate.

Please revert my changes when you fix up the conflicts and let's get back to a clean slate. If it's still failing unexpectedly after that I can take another look

@JamesHenry
Copy link
Member

@nayounsang I see you're doing a lot of pushes that result in failed tasks. Are you running these things locally before pushing? Ideally you would iterate locally to get the high priority tasks passing and then push up to CI to verify in a neutral environment

@nayounsang
Copy link
Contributor Author

nayounsang commented Aug 2, 2025

What is this err on CI?
https://cloud.nx.app/runs/Ol9VgADs38/task/eslint-plugin%3Atypecheck

> tsc --build --emitDeclarationOnly

vitest.config.mts(4,34): error TS6305: Output file '/home/runner/work/typescript-eslint/typescript-eslint/dist/vitest.config.base.d.mts' has not been built from source file '/home/runner/work/typescript-eslint/typescript-eslint/vitest.config.base.mts'.
> tsc --build --emitDeclarationOnly

vitest.config.mts(4,34): error TS6305: Output file '/home/runner/work/typescript-eslint/typescript-eslint/dist/vitest.config.base.d.mts' has not been built from source file '/home/runner/work/typescript-eslint/typescript-eslint/vitest.config.base.mts'.
Warning: command "tsc --build --emitDeclarationOnly" exited with non-zero status code

This is my first time seeing this and I don't know how to fix it in local as content of err msg and success in the local.

text: string | null,
expr: TSESTree.TaggedTemplateExpression,
): void {
if (text == null) {
Copy link
Contributor Author

@nayounsang nayounsang Aug 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change affects three rules.
Would it be better to simply ignore it? How should I handle it?
Once this is decided, I will also add tests.

continue;
}

const nextChar = text[index + 1];
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't care about if nextChar doesn't exist because of typescript error & parsing error.

@nayounsang nayounsang marked this pull request as ready for review August 3, 2025 14:23
Copy link
Member

@JoshuaKGoldberg JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A good start!

}

export class Converter {
#isInTaggedTemplate = false;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Bug] Tagged templates can have arbitrary nodes as children. This is totally valid:

`
${new (class {
    value = `
  
  ${(() => `C:\D`)()}}
  `
  })().value
  }
`;

https://typescript-eslint.io/play#ts=5.8.2&showAST=ts&fileType=.tsx&code=AYKAJA3gdgpg7gAgBQGMA2BDAzlhEQKEIBuGaArjAgLwKiEEKRJICUNAfHQMIBcAOgBFgrNgF8xjegjGjWAOlIUYjScADcQA&eslintrc=N4KABGBEBOCuA2BTAzpAXGYBfEWg&tsconfig=N4KABGBEDGD2C2AHAlgGwKYCcDyiAuysAdgM6QBcYoEEkJemy0eAcgK6qoDCAFutAGsylBm3TgwAXxCSgA&tokens=false

I don't think any kind of state on Converter is going to scale. Instead, I think checking node.parent might be the only way to do it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

node.parent is more safe & simple. I committed.

}
}

#isValidEscape(text: string): boolean {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😬 this and #isInTaggedTemplate are a lot of logic to add. If it does stay in, I think this should at least be extracted to its own separate function to keep Converter from getting even more complex. Finding a package on npm that already does this would be even better.

But, can we get away with checking if template.text === template.rawText?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think template.text === template.rawText is difficult to solve this issue.
As node.text(node: ts.NoSubstitutionTemplateLiteral | ts.TemplateHead | ts.TemplateMiddle | ts.TemplateTail) is handle its escape char and , this condition is difficult when valid and invalid escape chrs are mixed.

So, find package is better. I found unraw package that parse escape chr and throws err when it is invalid. (I wanted to compare different packages, but I couldn't find any other suitable ones.)

I committed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we can eval node.rawText it directly?

try {
  new Function(`return \`${node.rawText}\`;`)();
  return true;
} catch {
  return false;
}

I see they are stored in .templateSpans

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, that's better. Thank you. For me, it's a much lighter and simpler way than adding dependencies, so I committed it. However, I know that eval has performance and security issues. Will this be okay? @JoshuaKGoldberg


p.s I tried to use ts.createSourceFile, but it fails in test cases.

ts.createSourceFile(
   'temp.ts',
   `const str = \`${text}\`;`,
    ts.ScriptTarget.Latest,
);
convert > tagged template literal cooked > should set cooked to null for invalid escape sequences in tagged template literals
AssertionError: expected '\n\uXXXXᄑ\t' to be null

- Expected: 
null

+ Received: 
"
\\uXXXXᄑ       "

convert > tagged template literal cooked > should set cooked to null for mixed valid and invalid escape sequences
AssertionError: expected '\uXXXX\xQW' to be null

- Expected: 
null

+ Received: 
"\\uXXXX\\xQW"

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I should defer to @bradzacher on this. I'm quite weary of taking on eval or new Function. Even if we assume there are no security concerns, both are quite slow. I think we'd need to see benchmarks showing they don't significantly change parsing times for very large files that include many large template literals and edge cases.

@JoshuaKGoldberg JoshuaKGoldberg added the awaiting response Issues waiting for a reply from the OP or another party label Sep 3, 2025
@nayounsang
Copy link
Contributor Author

CI is failed again.. I guess more work is needed

"copy-ast-spec"
],
"inputs": [
"{projectRoot}/src/generated/**/*"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since there is no ast-spec dependency in types, need to set inputs to update the cache hash.
+I think also enhance copy-ast-spec cache.

      "copy-ast-spec": {
        "cache":true,
        "command": "tsx tools/copy-ast-spec.mts",
        "dependsOn": [
          "ast-spec:build"
        ],
        "options": {
          "cwd": "{projectRoot}"
        },
        "outputs": [
          "{projectRoot}/src/generated"
        ]
       "inputs":[
         "file of types..."
       ]
      },

By the way, should I create a separate issue for this task?

@github-actions github-actions bot removed the awaiting response Issues waiting for a reply from the OP or another party label Sep 14, 2025
@nayounsang nayounsang requested a review from fisker September 14, 2025 16:42

expect(templateElement.value.cooked).toBe(`\\uXXXX`);
});
});
Copy link
Contributor

@fisker fisker Sep 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should add some tests for template with expressions.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, thanks to you I found and fixed the bug.

@JoshuaKGoldberg JoshuaKGoldberg added the awaiting response Issues waiting for a reply from the OP or another party label Sep 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

awaiting response Issues waiting for a reply from the OP or another party

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bug: Wrong cooked value of TemplateElement in TaggedTemplateExpression

4 participants