-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
fix(typescript-estree): if the template literal is tagged and the text has an invalid escape, cooked will be null
#11355
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
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. |
✅ Deploy Preview for typescript-eslint ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
cooked will be nullcooked will be null
|
View your CI Pipeline Execution ↗ for commit f43e69b
☁️ Nx Cloud last updated this comment at |
|
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?
|
|
hi @JamesHenry
|
|
@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 |
|
@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 |
|
What is this err on CI? 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) { |
There was a problem hiding this comment.
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]; |
There was a problem hiding this comment.
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.
There was a problem hiding this 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; |
There was a problem hiding this comment.
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
}
`;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?
There was a problem hiding this comment.
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 { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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"
There was a problem hiding this comment.
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.
|
CI is failed again.. I guess more work is needed |
| "copy-ast-spec" | ||
| ], | ||
| "inputs": [ | ||
| "{projectRoot}/src/generated/**/*" |
There was a problem hiding this comment.
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?
|
|
||
| expect(templateElement.value.cooked).toBe(`\\uXXXX`); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.

PR Checklist
TemplateElementinTaggedTemplateExpression#11353Overview
ref: https://github.com/estree/estree/blob/2bc9ea235184b6164e31b1088575447657e686c6/es2018.md?plain=1#L34
cookedin theast-spec