bug#25777: 25.1; [PATCH] `rectangle--pos-cols' should not move point

[Drew Adams <drew.adams@oracle.com>]

> > As I mentioned, this is a regression.  I think the bug and the patch
> > are pretty self-explanatory, but if you feel you need a recipe to
> > show that this causes cursor movement then let me know.
> 
> Yes, an example would be useful.  Perhaps it's the callers of
> rectangle--pos-cols that should use save-excursion.

I can certainly wrap all calls of `rectangle--pos-cols' with
`save-excursion'.  But that should not be necessary.  There
is no reason for that function to leave point moved.  It's
only aim in moving point is to do so temporarily.

Point is moved in Emacs 25, and it was not moved before.
It's a gratuitous regression.  `rectangle--pos-cols' does
not benefit in any way by leaving point moved.

Anyway, thanks for looking at this.  Here is a recipe.  You
can try to make it more minimal if you like, but it is pretty
simple and it shows the problem easily enough.

Download library `modeline-posn.el', from here:
https://www.emacswiki.org/emacs/download/modeline-posn.el

Evaluate these 3 variables: option `modelinepos-style' and
defvars `modelinepos-region-acting-on' and `modelinepos-rect-p'.

Evaluate this:

(defadvice rectangle-mark-mode
 (after bind-modelinepos-region-acting-on activate)
    (setq modelinepos-region-acting-on  rectangle-mark-mode
          modelinepos-rect-p            rectangle-mark-mode)  
    (redisplay 'force))

And evaluate the setq-default sexp for `mode-line-position'
(the one for Emacs > 22).

In buffer *scratch*:

M-x show-paren-mode RET
M-x set-variable RET show-paren-style RET mixed RET

Type this at the beginning of a line at the end of the buffer
(in *scratch*):

() RET () RET

So you see this, with the cursor (_) at eob, i.e., after the
final newline:

()
()
_

Then do `C-x SPC C-p'.  The cursor moves briefly up one line
and then immediately jumps back to where mark was, at the end
of the buffer.  It should stay where `C-p' put it, on the
second left paren.  The problem seems to come from an error
being raised in the context of the show-paren code.

An easier way to repro it is to just load file modeline-posn.el
and then do what I described starting with "In buffer *scratch:".
In that case you will see the intended effect of the library
when the region is active in `rectangle-mark-mode': The mode
line says how many rows & columns are in the rectangular region.
Of course, for the recipe the cursor will not move, but stays
stuck at eob, so the mode line just reads "0 rows, 0 cols".
Actually, you can see it briefly change to "1 rows, 0 cols"
and then change back to "0 rows, 0 cols".

To debug this, since I could not use the debugger (the code
is initiated while updating the mode-line during redisplay),
I had to just insert calls to `message' etc.  I commented
out parts of `modelinepos-style', to find the part that
matters.  Commenting out this, and replacing it by 999,
prevents the problem:

(let ((rpc (rectangle--pos-cols (region-beginning)
                                (region-end))))
  (abs (- (car rpc) (cdr rpc))))

I then replaced just the (abs...) with this:

(message "RPC: %S" rpc)
999

That printed (0 . 0) for RPC, but the problem was reproduced.
That told me that the problem came from just calling
`rectangle--pos-cols'.  Changing the above to just this
removed the problem:

(let ((rpc '(0 . 0))) (abs (- (car rpc) (cdr rpc))))

I checked the code of `rectangle--pos-cols' and saw that
it now does `goto-char' (and it did not do so before
Emacs 25.1).  Wrapping it in `save-excursion' fixed the
problem, without any negative effect on the function
(it does not need point to remain moved).

Such code should not gratuitously add cursor motion.
Other `goto-char' calls were also added, in other parts
of the same file, for Emacs 25.  Perhaps the person who
updated the code did not think about the consequence.

Point is not moved by the code in order to leave it in
a different place.  It is changed only for a temporary
effect.  That point-moving code should be wrapped in
`save-excursion'.  My advice is to check the other
such bare `goto-char' occurrences that were added in
Emacs 25.  I'm guessing that none of them need to leave
point moved.

Code should be able to call `rectangle--pos-cols' just
for its value, without getting side effects like cursor
movement.  If `rectangle--pos-cols' needed, or intended,
to leave point moved somewhere for some reason then yes,
of course, any calling code that doesn't want that should
be responsible for wrapping the calls with `save-excursion'.
But that's not the case: There is no reason for the function
to have the side effect of leaving point in a different position.