Certain regexps cause error in vr--compose-messages

benma / visual-regexp.el

A regexp/replace command for Emacs with interactive visual feedback

400 stars 29 forks source link

Certain regexps cause error in vr--compose-messages #26

Closed monsanto closed 9 years ago

monsanto commented 9 years ago

To reproduce: In any buffer, M-x vr/replace, paste \(\\\)?\(\). You will get the following error:

Debugger entered--Lisp error: (wrong-type-argument sequencep integer-or-marker-p)
  mapconcat(identity (integer-or-marker-p) " - ")
  vr--compose-messages(integer-or-marker-p nil)
  vr--feedback()
  vr--update(2 13 0)
  insert-for-yank-1("\\(\\\\\\)?\\(\\)")
  insert-for-yank("\\(\\\\\\)?\\(\\)")
  yank(nil)
... rest elided ...

Any subsequent interaction with visual-regexp will raise Wrong type argument: overlap, nil.

EDIT: just to be clear, this error comes up for me fairly often on real world queries, the one above is just a minimal example. I've been trying to reproduce this one for a while now

benma commented 9 years ago

Thank you for taking the time to come up with a good example. @syohex has provided a patch, which I will merge as soon.

The root of the error is that optional groups (\(...\)?) result in (match-data) with nil entries, which was not handled correctly. I never use optional groups, which explains why I never encountered this.

For my information: why do you use optional groups like those? You only need groups if you need to reference them in the replace string. I can't come up with a good use case.

monsanto commented 9 years ago

Excellent, thanks for the speedy resolution.

@benma Three reasons:

Groups aren't used just for backreferences; you need them if you want to use a postfix operation like *, +, or ? on a group of characters. One can use the \(?: construct but it is unwieldy for interactive use.
Even in the case of where I need to use a backref, \(...\)? is a character shorter than \(...\|\)
The error can arise even when one does not want to use an optional group; consider the case where you have \(...\)x?\(...\), you want to go to \(...\)\(...\), and your current cursor position is before x. You might instinctively want to type delete twice, but after the first keypress everything breaks.

benma commented 9 years ago

Good points!

The PR is merged now. Please let me know if it works for you.