bug#23949: 25.0.95; Regression in handling error caused by (string-match-p "." nil)

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: Eli Zaretskii <eliz@gnu.org>,  schwab@suse.de,  23949@debbugs.gnu.org
> Date: Tue, 12 Jul 2016 16:27:03 -0400
> 
> Clearly, the problem is that string-match-p uses
> "(let ((inhibit-changing-match-data t))",

Since this is the only difference between string-match and
string-match-p, yes, that's pretty much obvious.  But saying that
doesn't yet point out the code which is affected by this binding in a
way that breaks popping the debugger.

> so the debugger is run with inhibit-changing-match-data bound to t
> and that breaks lots of Elisp code.

Then perhaps the solution is to avoid the effect of binding
inhibit-changing-match-data on the debugger.

> That's a general problem with the use dynamic binding to pass extra
> parameters: you end up passing them not just to that one function but
> also to all other functions called from that one.

This is a strange thing to hear, from you of all the people.
Dynamically binding variables around some expression is standard Emacs
Lisp programming technique, used all over the place.  The doc string
of this particular variable even says so.  How come it is suddenly a
problem?

I also think that the "breaks a lot of Elisp code" part is at least a
tad exaggerated.  We bind this particular variable in 2 functions that
are called from more than 300 different places in the Emacs sources,
so if doing so indeed breaks a lot of Lisp, we are in deep trouble,
which I don't think is the case.

Anyway, a cursory glance at help-function-arglist points to the
problematic code.  Compare this:

  (help-split-fundoc (documentation 'delete-file) nil)
    => ("(nil FILENAME &optional TRASH)" . "Delete file named FILENAME.  If it 
is a symlink, remove the symlink.
  If file has multiple names, it continues to exist with the other names.
  TRASH non-nil means to trash the file instead of deleting, provided
  ‘delete-by-moving-to-trash’ is non-nil.

  When called interactively, TRASH is t if no prefix argument is given.
  With a prefix argument, TRASH is nil.")

with this:

  (let ((inhibit-changing-match-data t))
    (help-split-fundoc (documentation 'delete-file) nil))
    => ("(nilnil" . "Delete fi")

The latter is clearly bogus.  Now, help-function-arglist calls
help-split-fundoc, and then reads from the string produced by the
latter:

          (let* ((doc (condition-case nil (documentation def) (error nil)))
                 (docargs (if doc (car (help-split-fundoc doc nil))))
                 (arglist (if docargs
                              (cdar (read-from-string (downcase docargs)))))

I hope the reason for EOF is now clear.  (I have no idea why it only
happens on master: the above bogus value shows on emacs-25 as well.)

Does the following variant of string-match-p look right?  Its intent
is to limit the effect of inhibit-changing-match-data to the call to
string-match only, leaving the error handling, if any is needed,
outside of that binding.

(defsubst string-match-p (regexp string &optional start)
  "\
Same as `string-match' except this function does not change the match data."
  (condition-case err
      (let ((inhibit-changing-match-data t))
        (string-match regexp string start))
    (error (signal (car err) (cdr err)))))