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.

Sign up for GitHub

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

fix(transformerMetaWordHighlight): use regex instead of substring to find matching indices #909

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix(transformerMetaWordHighlight): use regex instead of substring to find matching indices #909

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 7 additions & 5 deletions packages/transformers/src/transformers/meta-highlight-word.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,11 +55,13 @@ export function transformerMetaWordHighlight(
}
}

function findAllSubstringIndexes(str: string, substr: string): number[] {
export function findAllSubstringIndexes(str: string, substr: string): number[] {
const indexes = []
let i = -1
// eslint-disable-next-line no-cond-assign
while ((i = str.indexOf(substr, i + 1)) !== -1)
indexes.push(i)
const re = new RegExp(substr, 'g')
Copy link
Member

Choose a reason for hiding this comment

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

I guess we need to properly escape the substr if we want to use that approach.

Copy link
Author

Choose a reason for hiding this comment

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

Not sure if this is already handled by this function parseMetaHighlightWords. If you could help give some examples that would be awesome.

https://github.com/shikijs/shiki/blob/0b9137f8413eb1f276928707c0ac92de0664a314/packages/transformers/src/transformers/meta-highlight-word.ts#L3C1-L13C2

And, do we want to use this approach?

Copy link
Collaborator

Choose a reason for hiding this comment

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

The safest way to escape regex special characters is to use Regex+ and interpolate the string to escape. Nearly as safe but with noisier output would be to use native RegExp.escape (an ES proposal), which isn't supported by browsers yet. The most popular way to escape regex special characters (based on npm download stats) is to use the unsafe but lightweight escape-string-regexp. Context safety isn't relevant when the entire regex pattern is an escaped string, though, so no worries.

If it was me, I might just add an inline .replace(/[|\\{}()[\]^$+*?.]/g, '\\$&') with no libraries. This doesn't escape chars that might need to be escaped based on context (-, ,, digits, etc.), since no context awareness is needed when the escaped string is being used as the entire regex pattern.

But then, I don't understand why you're moving from string search to regex search in the first place.

parseMetaHighlightWords isn't escaping special characters; it seems to be matching JS regex literals? I also notice it has a couple minor issue issues with that: it will inappropriately match /foo\/ (no unescaped trailing /) and it will not correctly match /[/]/ (unescaped / is allowed in JS character classes, unless the regex uses flag v).

Copy link
Author

Choose a reason for hiding this comment

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

But then, I don't understand why you're moving from string search to regex search in the first place.

Problem from #908. But yes, this is one out of the many ways to solve it. And it doesn't solve all the problems. Maybe I'll close this first and move it to discussion to see what people think first.

let match = re.exec(str)
while (match !== null) {
Copy link
Collaborator

@slevithan slevithan Feb 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

This will loop forever if re matches an empty string. Is that possible here?

indexes.push(match.index)
match = re.exec(str)
}
return indexes
}
4 changes: 3 additions & 1 deletion packages/transformers/test/meta-word-highlight.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,11 @@
import { expect, it } from 'vitest'
import { parseMetaHighlightWords } from '../src/transformers/meta-highlight-word'
import { findAllSubstringIndexes, parseMetaHighlightWords } from '../src/transformers/meta-highlight-word'

it('parseHighlightWords', () => {
expect(parseMetaHighlightWords('')).toEqual([])
expect(parseMetaHighlightWords('/hello/')).toEqual(['hello'])
expect(parseMetaHighlightWords('/ /f /hello/')).toEqual([' ', 'hello'])
expect(parseMetaHighlightWords('/foo bar/ /foo.bar\\/foo/')).toEqual(['foo bar', 'foo.bar/foo'])
expect(findAllSubstringIndexes('xxx', 'xx')).toEqual([0])
expect(findAllSubstringIndexes('xxxx', 'xx')).toEqual([0, 2])
})
Loading