Skip to content
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

Escape all regex special characters in identifiers #596

Closed
wants to merge 2 commits into from

Conversation

oBusk
Copy link

@oBusk oBusk commented Apr 19, 2024

Rather than just escaping $, escape all regex special characters in any identifiers to make sure the RegExp that is created is valid.

Used the list of characters to escape from the regex-escaping proposal

https://github.com/tc39/proposal-regex-escaping/blob/66d654ea6561ea064045c7ecf2da6870c892c9be/polyfill.js#L4

Closes #595

Copy link
Collaborator

@webpro webpro left a comment

Choose a reason for hiding this comment

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

Thanks! Happy to merge, but I do have a question.

@@ -387,7 +387,7 @@ const getImportsAndExports = (

const setRefs = (item: SerializableExport | SerializableExportMember) => {
if (!item.symbol) return;
for (const match of sourceFile.text.matchAll(new RegExp(item.identifier.replace(/\$/g, '\\$'), 'g'))) {
for (const match of sourceFile.text.matchAll(new RegExp(item.identifier.replace(/[\\^$*+?.()|[\]{}]/g, '\\$&'), 'g'))) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This targets (enum) members, perhaps it makes sense to only apply this to members at https://github.com/webpro/knip/blob/main/packages/knip/src/typescript/getImportsAndExports.ts#L43? Ideally we'd create the regex once and reuse it (I should've done that myself here too).

Copy link
Author

Choose a reason for hiding this comment

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

@webpro do you mean to make the escaping of special characters already when creating the serializableMember? I've just had a cursory look, and it looks like modifying the identifier would have lot's of other effects, but perhaps adding another regex property to the serializable members which does identifier->escape->regex and then we can use that pre-created regex here for the matchAll()?

Copy link
Author

Choose a reason for hiding this comment

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

Yeah ok I had another look, and I can't figure out what exactly you mean. I've moved the Regex into it's own file to be reused, also tried to search for anywhere in the code here a similiar regex was used, but couldn't find anywhere.
Let me know what you think!

@webpro webpro force-pushed the main branch 6 times, most recently from 3691e5b to 9bf286f Compare April 26, 2024 10:01
@oBusk oBusk changed the title Escape all regex special characters in identifiers (fix #595) Escape all regex special characters in identifiers May 30, 2024
Copy link

netlify bot commented May 30, 2024

‼️ Deploy request for knip rejected.

Name Link
🔨 Latest commit 8e2a4d3

@oBusk oBusk force-pushed the identifier-regex branch 3 times, most recently from 6531735 to d2f304d Compare May 30, 2024 23:22
@webpro
Copy link
Collaborator

webpro commented Jun 3, 2024

@oBusk Thanks for pushing this, @oBusk! When looking into it myself I figured it could be faster and simpler by doing a literal text-based search without regexes at all. So I implemented that including fixtures and test to prevent regressions.

@webpro webpro closed this Jun 3, 2024
@oBusk
Copy link
Author

oBusk commented Jun 4, 2024

@oBusk Thanks for pushing this, @oBusk! When looking into it myself I figured it could be faster and simpler by doing a literal text-based search without regexes at all. So I implemented that including fixtures and test to prevent regressions.

Oh crap, of course! That's a lot better! Nice!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Arguably terrible enum identifiers causes breakage.
2 participants