match beginning and end of line correctly

[matthias314]

When searching with buf:FindNext, the start of the search region currently always matches ^ and the end of the region $, no matter if they are the indeed the start or end of a line. Similar problems exist with other empty-string pattern like \b. The goal of this PR is to fix this. This is done by modifying the search pattern for the first and last lines (prepending and/or appending ..

Code common to findDown and findUp has been moved to separate functions. One could tweak the specific way it's done, but maybe it's better to first get some feedback if you are willing to go in this direction at all.

[dmaluka]

This "padded regex" approach feels hacky. Maybe it is reliable (I haven't analyzed it carefully yet), but how about a straightforward approach instead: search not just within the given range (start..end) but within the extended range from the beginning of the first line to the end of the last line, and filter out those matches that are outside start..end?

[matthias314]

That doesn't work because FindAll and friends return only non-overlapping matches, see here. For example, for s := "abcd" and re := regexp.MustCompile(".."), the call re.FindAllStringIndex(s, -1) returns [[0 2] [2 4]]. Hence the method you are suggesting would claim that there is no match in s[1:3].

[dmaluka]

Ok, fair point.

[matthias314]

I've force-pushed a new version:

• the first commit is the old PR,
• the second commit should address all your comments,
• the third commit is a variant where I only use the two bitmasks padStart and padEnd instead of an enum with four elements. You may or may nor find this more elegant.

I've also rebased the PR so that one can test it together with the changes to the search functionality done in previous PRs.

[dmaluka]

Thanks, LGTM (apart from my above comment about comments, and the fact that it's better to squash such fix-up changes into one commit, but we can squash them later).

[dmaluka]

Side note: looks like we can deduplicate the code in findUp()/findDown() even more. Looks like both functions are identical except that one uses FindIndex() while the other one uses FindAllIndex() and takes the last match. So we can merge them into one function, e.g. find() or findNext(), with down argument.

[matthias314]

I've combined findUp and findDown to findNext, with an additional argument down. However, I wonder if down is necessary at all. Could we replace it by start.LessEqual(end)? If we keep down, why don't we change the calls of findNext so that start is always less than or equal to end?

Moreover, the code at the beginning of findNext (copied from the old functions) is not fully clear to me. Why may start.X and end.X be modified (to different values!) before we test if start is greater than end?

One might think of making findNext even more general so that it can be used for ReplaceRegex, too (and also for the function FindNextSubmatch that I proposed in #3552).

EDIT: We might also want the first argument to findNext to be the full set of four regexps we're using.

[dmaluka]

Moreover, the code at the beginning of findNext (copied from the old functions) is not fully clear to me. Why may start.X and end.X be modified (to different values!) before we test if start is greater than end?

When start or end is beyond the last line of the buffer, i.e. its Y is greater than b.LinesNum()-1, we clamp it to be exactly at the end of the buffer, so that we don't try to search in non-existent lines. (Even though LineBytes() returns an empty string for non-existent lines so in most cases search would probably work correctly even if we didn't clamp, but e.g. if the search pattern is just ^ or $, we would get unwanted matches in those non-existent lines.) So we set Y to the last line, and that's not enough, we also need to set X to the end of this last line. For example, if the buffer has 100 lines and the last line has 50 characters, and end passed to findDown() is 0, 200, we should change it to 50, 99, not to 0, 99 (so that we search up to the very end of the buffer, including the last line).

[dmaluka]

LGTM, but I've just noticed that it still doesn't fix the issue in the case of replace -a.

So I guess we also need a similar fix in ReplaceRegex().

(Unless we treat this as the intended behavior, but then it's inconsistent with replace without -a...)

[matthias314]

I was thinking of modifying ReplaceRegex in a separate PR. But of course we could also do it here. Whatever you prefer.

[dmaluka]

@JoeKar what do you think about this PR? Maybe any ideas of an alternative approach?

I was thinking of modifying ReplaceRegex in a separate PR. But of course we could also do it here. Whatever you prefer.

I'm fine either way. Although it seems reasonable to address it in this PR, since it's rather a part of the same issue, and the way to fix it is probably just to adopt the same approach as in FindNext().

[JoeKar]

@JoeKar what do you think about this PR? Maybe any ideas of an alternative approach?

This "padded regex" approach feels hacky.

Indeed it is hacky, but it seems to fit the needs via the help of regular expressions on its own. Otherwise we've to solve it with additional code dropping (parts of) results.
The only thing which bothers me right now is, that we (Must)Compile resp. parse the regular expressions twice, while we should be able to prevent this and compile it only once...with the same padding approach (e.g. padRegexp(s string)).

[dmaluka]

The only thing which bothers me right now is, that we (Must)Compile resp. parse the regular expressions twice, while we should be able to prevent this and compile it only once...with the same padding approach (e.g. padRegexp(s string)).

Well, yes, we could compile the padded regexps in FindNext() and pass all 4 regexps to findUp() and findDown()... I'm not sure that's a big deal though. That is not the only waste we have here. For instance, we always compile all 4 regexps, while we never use them all. For example, rPadBoth is never used if the search range consists of more than one line (which we could easily check, and avoid compiling rPadBoth in this case, which would complicate the code, with doubtful usefulness).

Not even mentioning the fact that we compile the same regex every time when calling FindNext() for the same pattern multiple times.

[dmaluka]

Hmm... Throughout the entire loop, fineLineParams() returns padStart at most once (for the first line), and padEnd at most once (for the last line), right? So we could both simplify and optimize the code by removing padRegexp() and instead letting fineLineParams() itself compile and return the padded regexp - the one exact padded regexp that is needed, exactly in the case when it is needed?

That should also address your concern about compiling twice, since even when we call findUp() or findDown() twice, we do that because we wrap, so we still have padStart or padEnd at most once throughout FindNext(), thus findLineParams() will still compile the corresponding padded regex at most once.

[matthias314]

Several questions and comments about the extension to ReplaceRegex():

• ReplaceRegex() uses re.ReplaceAllFunc() instead of re.ReplaceAll(). As far as I can tell, the reason is to get the correct value for netrunes (which counts characters, not runes). This is only used in ReplaceCmd() for interactive replacements. There one could probably get the right count separately, which would simplify ReplaceRegex(). Would that be a good idea? Or rather not because it would change the signature of the function and therefore be breaking?
• One new difficulty is that one cannot use re.ReplaceAllFunc() inside ReplaceRegex() if the regexp is padded. (The reason is again that one would miss matches where the padding leads to an overlap.) Instead, one would have to call FindNext() (or findDown()) to find all matches in such a line and then do the replacement separately for each match. The whole thing would be easier if we had a function like FindAll. I think such a function would be of general use. Would you be interested in that?
• Actually, it would be even more helpful to have something like FindNextSubmatch: return submatches when searching #3552 or FindAllSubmatch. Otherwise one would have to call re.ReplaceAll() instead of re.Expand() for each match on the first/last line because the submatch information is not available. The same currently applies to ReplaceCmd() (where ReplaceRegex() is called instead of re.Expand()). Again, I think a submatch functionality would be useful in general. Interested?

[JoeKar]

So we could both simplify and optimize the code by removing padRegexp() and instead letting fineLineParams() itself compile and return the padded regexp - the one exact padded regexp that is needed, exactly in the case when it is needed?

Sounds good. 👍

[matthias314]

As a first step, I've implemented @dmaluka's suggestion to remove padRegexp.

[dmaluka]

ReplaceRegex() uses re.ReplaceAllFunc() instead of re.ReplaceAll(). As far as I can tell, the reason is to get the correct value for netrunes (which counts characters, not runes). This is only used in ReplaceCmd() for interactive replacements. There one could probably get the right count separately, which would simplify ReplaceRegex(). Would that be a good idea?

Maybe... Hard to say without seeing an actual implementation.

IIUC it is not directly related to the issue addressed by this PR, it would be just an extra improvement?

Or rather not because it would change the signature of the function and therefore be breaking?

I wouldn't worry too much about that. We already changed ReplaceRegex() interface once, in #2954. No one seemed to complain. We are not really strict about preserving compatibility of interfaces that are primarily for micro's internal use and just happen to be exposed to plugins (by virtue of being public methods/fields of objects that are returned to plugins by some other functions). Dogmatically preserving them all would be counter-productive. (Although it's IMO quite unfortunate that we have such vagueness of the APIs for plugins and their stability guarantees and so on, but that's how the plugin system is designed, unfortunately.)

One new difficulty is that one cannot use re.ReplaceAllFunc() inside ReplaceRegex() if the regexp is padded. (The reason is again that one would miss matches where the padding leads to an overlap.) Instead, one would have to call FindNext() (or findDown()) to find all matches in such a line and then do the replacement separately for each match.

Yep, and that seems inevitable. And I imagine we wouldn't need to do that per line, we can just call FindNext() in a loop for the entire range, similarly to what ReplaceCmd() does for interactive replace?

The whole thing would be easier if we had a function like FindAll. I think such a function would be of general use. Would you be interested in that?

I'm not sure. A file may be large, the number of matches may be huge, do we really want to store and return them all and then process them, if we can instead just call FindNext() in a loop and thus process them one by one on the fly?

Actually, it would be even more helpful to have something like #3552 or FindAllSubmatch. Otherwise one would have to call re.ReplaceAll() instead of re.Expand() for each match on the first/last line because the submatch information is not available. The same currently applies to ReplaceCmd() (where ReplaceRegex() is called instead of re.Expand()). Again, I think a submatch functionality would be useful in general. Interested?

Maybe. Again, hard to discuss without an actual implementation.

[dmaluka]

As a first step, I've implemented @dmaluka's suggestion to remove padRegexp.

LGTM.

[matthias314]

Here is a modification of ReplaceRegex. I didn't bother too much about optimizations. For example, one could treat the first and last lines separately only if they are not complete lines. Also, the padded regexps may be compiled several times for these lines, and findAll could be implemented with a callback function, analogously to ReplaceAllFunc. The code assumes that the replacement text has no newlines; I think this assumption was implicitly made before already.

EDIT: Calls of the form util.CharacterCount(b.LineBytes(i)) occur several times in ReplaceRegex and even more so elsewhere in the code. What about a dedicated CharacterCount function for LineArray?

[matthias314]

I've force-pushed a slightly modified version. It works the same, but the code is cleaner now.

[dmaluka]

I've force-pushed a slightly modified version. It works the same, but the code is cleaner now.

So please squash the last 2 commits into one (to let reviewers review the relevant version, not waste time on the outdated one).

[dmaluka]

Still, what is the advantage of using findAll() to remember all matches in some temporary storage to handle them afterwards, instead of just using findDown() and handling each match right away?

[matthias314]

One could do that. I wrote the code with the idea in mind to have some public FindAll function at a later point. That code would be based on the current findAll. One could change findAll to some findAllFunc that takes a callback function as argument (simialr to ReplaceAllFunc). This way the problem of creating temporary lists would disappear. (That would also be a good idea for a public function.) Currently findAll is only called for the first and last lines, so that performance doesn't really matter. That would change of course if we used this function for all lines.

[dmaluka]

Somehow I missed the email notification about your comments a week ago...

One could do that. I wrote the code with the idea in mind to have some public FindAll function at a later point. That code would be based on the current findAll. One could change findAll to some findAllFunc that takes a callback function as argument (simialr to ReplaceAllFunc).

Still, what for? Why not keep things simple?

Currently findAll is only called for the first and last lines, so that performance doesn't really matter.

A single line may be huge. (Although that's an edge case, and currently micro handles huge lines very poorly anyway, for unrelated reasons, due to the dumb data structure with O(n) access, which IMO we should fix some day, because that's a shame.)

But my point is not so much about performance but about simplicity. We have a chance to try to make the logic not just more correct but at the same time simpler, by simply calling findDown() in a loop for the entire range, instead of complicating the code with more special cases, so why not take this chance?

[matthias314]

If we want to call MultipleReplace only once in ReplaceRegex with all deltas at once (as done in master and this PR), then I believe we cannot deal with each match right away (unless we modify MultipleReplace). The problem is that multiple deltas on a single line only work if the last (= rightmost) change comes first. If we simply want to call findDown repeatedly over the whole buffer, then we have to store all matches in a list (currently done in findAll) and then process this list backwards (done in ReplaceRegex). (My understanding is that there is no efficient way in Go to prepend to a slice.)

Apart from this list reversal, this would indeed lead to fairly simple code. Padded repexps would be compiled for almost every call to findDown. If one doesn't want that, then one could create the padded regexps before calling findDown.

Please let me know how you want to proceed.

EDIT: Modifying the processing of deltas should be easy to do (in ExecuteTextEvent). As far as I can tell, MultipleReplace is the only place where a TextEvent with multiple deltas is created, and ReplaceRegex is the only function calling MultipleReplace.

[dmaluka]

If we want to call MultipleReplace only once in ReplaceRegex with all deltas at once (as done in master and this PR), then I believe we cannot deal with each match right away (unless we modify MultipleReplace).

Indeed, good point. Thanks for explanations.

I've analyzed all this in more detail and now it seems to me that the current approach in this PR is already as simple and practical as it can be without further refactoring of EventHandler (which we can do separately, if we want to).

Padded repexps would be compiled for almost every call to findDown.

Yeah, that's also an argument.

[matthias314]

Would you be interested in a separate PR that modifies ExecuteTextEvent? In a separate commit I could modify the search code (precompile padded regexps and use the findDown method only in ReplaceRegex).

[dmaluka]

Would you be interested in a separate PR that modifies ExecuteTextEvent?

If it goes well, why not?

[matthias314]

I've noticed that we used an incorrect correction to the start and end of a match in the case of a padded regexp. Previously we were dealing with incomplete characters if the padding matched a combining character. (For the end we could probably do without the fix because the combining characters come after the base character, but for the start it's important.)