RE: `save-excursion' defeated by `set-buffer'

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

> It takes quite a bit of insight into the code to understand that it is
> actually *harmful* redundancy.

What redundancy?  What harm?

> The unintentional uses of save-excursion cover up potential
> bugs inside the code which don't get noticed because of this
> redundant saving.

Redundant saving?  What redundant saving?  Each use of `save-excursion' saves
and restores the buffer that was originally current along with its point and
mark.  Where's the redundancy?  Where's the harm?

> So, my test is pure and simple.  Convert all the save-excursion's that
> the compiler complains about to save-current-buffer.

OMG.

> If your code continues to run perfectly, you are my hero.
> If your code falls over, then you should accept that your code is
> buggy

If it wasn't before, it probably is after your near-global search-and-replace.

And what if your code doesn't seem to fall over immediately, and it takes you 2
years to discover that it was misguided after all not to save and restore point
in that particular case?

> and it is only able to stand up on the crutches of what you thought
> was "harmless redundancy".

Please tell us more about the harmless redundancy and the harmful redundancy.
Or even just about the redundancy.

> Then it is up to you whether you want to fix the code or
> continue to live with it.  But there are many of us who won't touch
> such code with a tadpole because we regard it as buggy.
>
> Unfortunately, it is more than a question of clarity.  It is a
> question of correctness.  Let me restate my example from Friday a bit
> more explicitly
> 
> (defun my-beautiful-function ()
>     (save-excursion
>        (set-buffer B)
>        (goto-char x)
>        (setq a (find-something))
>        ...))
> 
> (defun find-something ()
>         (....
>            (set-buffer A)
>            (goto-char y)
>          ...))
> 
> find-something is *buggy* here.  It is moving point in buffer A though
> it wasn't supposed to.

Why wasn't it supposed to?  How can you know from this skeleton whether it was
supposed to move point in A or not?

Or are you just telling us that it is _hypothetically_ buggy, that
_hypothetically_ it is not supposed to move point in A, but it does?

Tell us more.  What buffer is `m-b-f' called in?  Presumably, if it is intended
to be called in buffer A, then it wants to restore point in A when it is done
(assuming the `save-excursion' is the last thing it does).

Presumably, if it is intended instead to be called in buffer C then it wants to
restore point in C when it is done.  In this case, presumably (if we don't just
assume that it is buggy) `find-something' wants to leave point in A at Y
(assuming it doesn't move it in the last `...').

There is no way to know from your skeleton what the purpose or the behavior is.
The devil is in the details...which are missing.

There is nothing intrinsically wrong with a function that moves point to some
position in some buffer and leaves it there.  I probably wouldn't name such a
function just `find-something', however, and I would document that part of its
purpose is to move point in buffer A or whatever.  Assuming that is the case.

> However, my-beautiful-function works fine as
> long as I call it in buffer A because the "harmlessly redundant"
> save-excursion at its top-level restores A's point.

Oh, so you were presuming that `m-b-f' wants to restore A's point.  OK.  Where's
the problem?  (So far nothing is _redundant_, however.)

If `m-b-f' intends to always do things in buffer A and afterward restore point
in A to what it was initially, and if it doesn't know which buffer it will be
called from, then it should wrap (with-current-buffer A ...) around its
`save-excursion'.

> Tomorrow, you decide to call my-beautiful-function from some other
> buffer C, and all hell breaks loose.  You suddenly find that the point
> is moving in the unrelated buffer A and you have no idea why.

Well you should know.  You should know from the name of `find-something' (which
needs a better name) and from its doc that it moves point in A.

Or if by hypothesis `f-s' is buggy and should not in fact move point in A, then
you should be able to see by debugging that it in fact does move point in A,
even when `m-b-f' is called in A.  And you should be able to tell just by
looking at the code that it moves point in A.

I don't deny that if the overall code is complex such a bug might not be
obvious, but I submit that this is a general problem that has nothing special to
do with `save-excursion' or `set-buffer'.  It is simply about keeping track of
the side effects performed by your code.

`find-something' as written has the side effect of moving point in buffer A -
always.  If that is not the intention, then yes, its bug will not surface as
long as it is called inside a `save-excursion' for buffer A.

The same problem would exist for a function that increments a variable X by 10
and is called by a function within a `let' that binds it to 42.  You might never
notice the incrementing as long as that `let' binding is there.  Call the
incrementing function from another context and you might suddenly discover the
incrementing.

There are a zillion cases of such things in a language like Lisp.  Given both
its side effects and dynamic binding there are all kinds of possible pitfalls in
Lisp.

It should be very clear that a construct that restores point for the current
buffer will restore point for the current buffer (!).  Restoring point for a
given buffer hides point movements that took place between saving and restoring
- of course.  The same would be true if you instead used your proposed
`save-point-and-mark' to do the saving and restoring.  Nothing remarkable here
at all.

Nothing mysterious.  Except for the various `...' and the context of use
(invocation).  Without knowing those things, without knowing the intended
behavior, the behavior could be nearly anything.

> So, rewrite the code as follows:
> 
> (defun my-beautiful-function ()
>     (save-current-buffer
>        (set-buffer B)
>        (goto-char x)
>        ...
>        (setq a (find-something))
>        ...))
> 
> (defun find-something ()
>         (....
>            (save-current-buffer
>               (set-buffer A)
>               (save-excursion
>                  (goto-char y)
>                  ...)))
> 
> Not only would you have mollified the compiler, but you will have much
> more robust code that will continue to function when used in
> unforeseen ways.

Oh, so you meant that `m-b-f' doesn't need to do anything specifically in A, and
it need not be called from A.  Such details were missing.  And `f-s' should not
move point in A (the buggy hypothesis).  In that case, sure, that's a perfectly
fine way to do what you apparently want.

It all depends on what the intended behavior is.  You might well want `f-s' to
move point in A.  You might, as part of its contract, know that `m-b-f' will
always be called from A, or from C, or whatever.