quick fix for #3700

[matthias314]

This is a quick fix on top of current master. A separate commit (also in my PR #3658) ensures that regexp error messages are displayed. The error message for \Q without \E is slightly weird,

error parsing regexp: missing closing ): `(\Qvw)`

but at least there is an error message instead of a crash.

Fixes #3700

[JoeKar]

I like how it is proposed to simplify in #3913:

	if b.Settings["ignorecase"].(bool) {
		s = "(?i)" + s
	}

Afterwards we can perform your introduced check with:

	r, err = regexp.Compile("(" + s + ")")
	if err != nil {
		return [2]Loc{}, false, err
	}

The benefit is, that we should compile the string only once to find out if it is valid or not.
Would it hurt to have it in that moment already in an additional capture group? If no, then nothing more needs to be added, if yes then the non-grouped compilation (+ additional sanity check for err):

	r, err = regexp.Compile(s)

[matthias314]

I also like the ignorecase approach you mentioned. Regarding the capture group, I guess it doesn't matter, but in order be clean, I would go with a non-capturing group

    r, err = regexp.Compile("(?:" + s + ")")

Shall I update the PR along these lines?

EDIT: In this appraoch, all regexp error messages would look "weird": For example, if a user searches for (x, the message becomes

error parsing regexp: missing closing ): `(?:(x)`

[niten94]

I'm only taking a quick look at the moment, but the check should also probably be done in ReplaceRegex.

[matthias314]

In principle I agree, and that's what is done in #3658. My goal with this PR is to fix the problem with minimal changes to the code so that it will be easy to rebase #3658 (or @niten94's or somebody else's modification of it) on top of this PR.

[niten94]

I was thinking of adding return 0, 0 when the check fails, which seems unlikely to significantly increase the difficulty of the rebase.

If this will be done, shouldn't we also modify > replace to report an error by performing the check? Crashing on > replace or displaying no results despite \Qexample test being valid before seems like a regression as well, and we could stop compiling twice with #3658 at least.

EDIT: In this appraoch, all regexp error messages would look "weird": For example, if a user searches for (x, the message becomes

error parsing regexp: missing closing ): `(?:(x)`

Wouldn't it be fine to return a new error if its type is syntax.Error and Expr matches the whole modified string?

Edit: Combining the actual compilation and check has a flaw, which I cannot thoroughly lookup and explain yet right now.

[JoeKar]

Could you add the check in ReplaceCmd if the change is small enough for 2.0.15?

This question is still open. For consistency it might be better to add it already now.

[matthias314]

This is automatic because now the regexp is checked in findLineParams. All regexps for search and replace operations pass through this function.

[niten94]

Yes, checking the regex and adjusting it in findLineParams is enough.

Just to be slightly clear about the information left here on > replace: \Q without \E is indeed accepted since it doesn't display an error, but without this PR it can crash in ReplaceRegex. Reproduction steps slightly differ with #3700.

[matthias314]

It's not only that it doesn't display an error -- it actually performs the correct replacements.

EDIT: I mean "with this PR".

[niten94]

I think it succeeds in most cases, but this is one minimal example where it crashes without this PR:

1. Open an empty buffer, then insert one space and "a"
2. Select "a" only and run > replace '\Qa' b

[matthias314]

The Go gurus say that \Q without \E is valid, see golang/go#76424. I've adjusted the PR so that now this is accepted here, too. I do this by compiling the regexp twice: first as-is, then with a trailing \E. If both succeed, then I conclude that there is a \Q without closing \E, and I continue with the second regexp.. I think this should work. As before, the PR is minimally invasive at the moment.

[dmaluka]

Looks nice, thanks.