bug#5805: 23.1; abbrev-insert does not protext itself with save-excursion

[Maguire, Andrew (GE Infra, Energy)]

Create a global abbrev, abbrv => abbrev

Type the following, ^ indicating point location.

  abbrv ()
          ^
Then press C-x ' to expand the abbrev on the line.
  abbrev ()
        ^
Observe that point ^ is now before the ()s.
In the C version of abbrev expansion in all earlier Emacsen, point is
not affected by expansion,
i.e. it would be left after the ()s which is where the user pressed C-x
'

My current fix is 
;; Emacs 23 has a lisp implementation for abbrevs.
(if (fboundp 'abbrev-insert)
    (defadvice abbrev-insert (around
save-excursion-around-advice-insertion activate)
      "Lisp implementation of advice insertion does not save point
location afterwards.
When looking back over non-word characters to find a word that may be an
abbreviation
if it finds something to replace, it does not save its position."
      (save-excursion
        ad-do-it)))

Thanks,
Andrew


-----Original Message-----
From: Chong Yidong [mailto:cyd@stupidchicken.com] 
Sent: 10 April 2010 17:04
To: Maguire, Andrew (GE Infra, Energy)
Cc: 5805@debbugs.gnu.org
Subject: Re: 23.1; abbrev-insert does not protext itself with
save-excursion

> In our code we use abbrev to dynamically expand certain
> keywords. However, the new lisp implementation is more eager in
> finding things to check, ie. it is able to look back past non-word
> characters to see if an abbrev is to be found.  In this situation it
> is essential to do save-excursion.
>
> Ideally, either insert-abbrev should do a save-excursion itself or all
> calls to it should.  In our code, expand-abbrev is call which
> internally calls abbrev-insert.
>
> Alternatively, is it the recommendation for user code to surround all
> "abbrev" calls with save-excursion now?

Do you have a recipe for reproducing a bug?  By default, the abbrev code
is not supposed to change point, so there should be no advantage adding
a save-excursion.  I don't see why we should constrain the ability of
user-defined functions in `abbrev-expand-functions' to change point, if
they want to.

Or do you mean save-match-data?