All CLA requirements met.

Thanks for the PR! We are wrapping up the April release this week but I will review it next week for May. It looks good at first glance.

I spent some time yesterday looking through the implementations of ReplacePattern in vs/editor/contrib/find/replacePattern.ts and vs/workbench/services/search/common/replace.ts. Each of these implements different mechanisms for the CharCode switches, so a small CharCode-aware-only helper function doesn't seem viable.

The alternative IMO is to choose (or combine) one of the ReplacePattern implementations, move it to vs/base/common/search.ts (or perhaps .../replace.ts) and tweak the small handful of callers.

I'm probably not the best choice to perform a nuanced refactor since I'm a TS noob; however I can likely implement the replacement change in .../replace.ts as a parallel to .../replacePattern.ts.

Thoughts?

Yeah, I see why it will be a little complicated to do that and why it is different than what we did for buildReplaceStringWithCasePreserved. Maybe we can merge this PR and just do a separate one with a parallel implementation for search? Or I could do some refactoring and try to get one implementation for both.

I'd be happy to do the parallel PR on the workbench path. It's sort of continuing artificial code separation, but it will at least eliminate a new difference between the implementations.

That sounds good to me!

I've spent a couple of hours on implementing case operations in .../replace.ts, but I've realized that I would need to essentially reimplement String.replace(), since that's what it fully relies on. .../replacePattern.ts already has this type of piecemeal replacement logic, so I'm thinking it would be /less/ work (and far cleaner) to use replacePattern.ReplacePattern everywhere that replace.ReplacePattern is currently used (seems like only .../searchModel.ts).

I guess I don't really understand the difference between the two implementations @alexdima and @sandy081, why is the editor's ReplacePattern manually resolving $ references in the replace string instead of just doing a String.replace like search's version?

I am sorry that I don't remember everything. Here is a comment I could find -- #18111 (comment) ...

I think I found the issue #18111 (searching for /\b\s{3}\b/) to be fundamentally caused by the researching in the matched portion of a string. In other words, searching for a regex in a large string does not yield the same result/groups as searching for the same regex in the substring that did match because regular expressions sometimes refer to characters around the matched text (like \b does here).

So I have changed the text buffer search to return captured groups while searching, so the capture groups are those of searching in a large string. But the search across files could not do that, so I believe that is the point where I forked things.

Oh, that's right. So you are able to get the groups as you go. Search is not able to do that because ripgrep doesn't provide that detailed info, so we have to rerun the regex on the frontend, with hacks to make it work for lookaround. So given that limitation I think we would have to either

• rerun the regex to figure out what the groups are, construct the same info, then go through the replacePattern
• or maintain the totally separate implementation, possibly using String.replace with a function callback to implement the case changing

Haven't looked closely enough to eliminate either of those, I think the second sounds easier, what do you think @irridia?

I'm not yet familiar with the regex handoffs between TS and ripgrep—would ripgrep have to support the same case change operations, or is TS performing the replacement and ripgrep is more of a file buffer, match-only iterator?

A String.replace() replacer() function defeats the internal parameter handling AFAICT, which means that the replacer has to walk the replacement pattern character at a time and replace the various $N instances. This isn't hugely challenging but it's very reinvent-the-wheel-y and duplicates a lot of the work and logic I see in .../replacePattern.ts.

So while I agree the second sounds easier, it also feels like it's fairly significantly increasing the difference between these fundamentally identical code paths. The current codepath is already applying the regex twice, once into _regExp and once with he text.replace()es.

Moving the vs/editor/contrib/find logic to vs/base/common might be the best use of time, even if it delays the case ops support. Just IMHO, still very much a noob to this codebase.

Ripgrep wouldn't have to support it. It only tells us what the matches are. So we are stuck at least rerunning the regex. And I wouldn't call the two code paths fundamentally identical, they have the same apparent effect but have to do it in different ways.

Using a replacement function would be extra work but at least then it parses the replacement patterns for you, which I think is a lot of the work.

And I am ok with implementing this feature separately but still I would support you if you want to look into trying to merge the two code paths, that would be awesome if it's possible, as long as it's not going to lead to breaking a bunch of stuff.