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

[Drew Adams]

You repeated a lot Uday, but that's OK if it helps you understand or express
yourself.

> I am rewriting the text surrounding the example, based on your questions.
> This `my-beautiful-function' was designed to be called in a buffer A.
> 
> (defun my-beautiful-function ()
>     (save-excursion
>        (set-buffer B)
>        (goto-char x)
>        (setq a (find-something))
>        ...))
> 
> (defun find-something ()
>         (....
>          (save-excursion 
>               (set-buffer A)  
>               (goto-char y)
>               ...)))
> 
> The assumption is that find-something is *buggy* here.  It is moving
> point in buffer A though it wasn't supposed to.

Though you did not state this as part of your scenario, since you have mentioned
it otherwise may I add the assumption that this hypothetical code came from an
existing application written by a good programmer, and that that code generally
works well and has done so for quite some time?

IOW, I think you are trying to point out that code that has worked well can
nevertheless be fragile.  So let's assume that with no changes the code works,
and the person who wrote it was no fool - s?he wrote what s?he intended and
needed.

Let's imagine further that this code was inherited by someone else, and that
s?he is not necessarily familiar with all of the details.

> The `save-excursion' at the outer level in `my-beautiful-function' was
> presumably placed there to restore the current buffer.

Given the assumptions that I added (fairly, I hope), I would say that presumably
the `save-excursion' is there NOT ONLY to restore which buffer was current but
also restore point and mark in that buffer.  That's what `save-excursion' DOES,
and there is no reason not to suppose that the original author did not intend
that.

Even if the inheritor is not sure of things, we must give the benefit of the
doubt to the original designer/programmer, if for no other reason than the code
works.

IOW, given inherited code that seems to work, assume as a start that it works
_because_ of the code that is there.  Do not assume that the code can just be
altered willy-nilly, replacing some functionality by a subset that does not do
the whole job.  That might be the case, and perhaps the inheritor can improve
the code, but it would be a mistake to just assume that things can be changed
willy-nilly.

> Just looking at its body, we don't see any point movements in buffer A.
> So, we might feel justified in thinking that the `save-excursion' can be
> replaced by `save-current-buffer'.  But let us leave that aside for
> the moment.

I think that would be a big mistake - a very rash assumption.  A wiser and more
conservative assumption would be that even if you see no A-buffer point
movements directly in `m-b-f', somewhere in some code invoked within the
`save-excursion' there are or probably are A-buffer movements.  In fact,
precisely because you do not see point-A movements in `m-b-f' itself, you can be
almost sure there could be such in code called withing the `save-excursion'.
Why?  Because the original programmer used `save-excursion'.

IOW, again, do NOT assume that the `save-excursion' is intending to only
`save-current-buffer'.  Such an assumption is unwarranted based only on a glance
at the `m-b-f' code - after all, it does call other code.

> As long as you use `my-beautiful-function' in buffer A, it will appear
> to work fine, because the unprotected point movement in
> `find-something' is cancelled by the `save-excursion' at the
> top-level.  If you call it in some other buffer C, it will
> unexpectedly move point in buffer A.

Correct.

> My position is that `find-something' is *inherently buggy* because of
> its unprotected point movement in buffer A.

Well of course that's your position - and mine too, since you _posited_ that as
your hypothesis!  You seem to be trying to conclude what you hypothesized. (??)

> The fact that it appears to work fine as long as it is called
> from `my-beautiful-function' invoked in buffer A, doesn't make it right.

Dunno about "right".  But it works (by hypothesis).

Again, I think you are saying that code that works can nevertheless be fragile.
I might add: especially if the code is maintained by someone other than the
original author - or even, as sometimes happens to me, if the original author
(me) has simply forgotten some of the logic/details. ;-)

> The `save-excursion' at the top-level of `my-beautiful-function'
> serves to cover up the bug, whether intentionally or unintentionally.

Correct.  The original programmer's use of `save-excursion' does what s?he
wants: ensure that buffer A and its point and mark are restored, regardless of
what the code in the body of the `save-excursion' might do.  IOW, the `m-b-f'
code _ensures_ against any A-buffer point movements, whether from bugs or
otherwise.

The code in `f-s' is buggy by hypothesis, since you said that it was not
intended that it move point in A, but even if it were not buggy and that
movement were intentional, if the intent of using the `save-excursion' is what
we must assume it is (restore point as well as the buffer), then things work as
they should.  Call it a "cover up" if you want, but that's the intention behind
and the documented behavior of `save-excursion'.

> The solution is to change the `save-excursion' to
> `save-current-buffer', exposing the bug, and then to track it down.

That would not be my solution, but feel free.  Rather, that might be one way to
help you find the bug, if it really need be found.  But it is also problematic -
be sure to remember to return to using `save-excursion' if needed.

I would probably let the sleeping dog lie, and wait until the `f-s' bug actually
manifested itself.  After all, the `save-excursion' code does just what you want
here (by hypothesis): it ensures that _even if_ some code in the body (rightly
or wrongly) moves point in A, point in A will get restored.

If you later call `m-b-f' from some other buffer (e.g. Z) than A, then hopefully
you would ask yourself this immediately:

 "Hey, this `save-excursion' restores things for the buffer
 that was current before.  What buffer was that?  A.  So if I
 now call it from buffer Z then I probably need to wrap things
 in `with-current-buffer' or some such.  I certainly cannot just
 assume that now I should restore things in Z instead of A."

Or else, if you do not want to just wrap that way, you will want to analyze
things in detail to find out just what point movements are involved, why the
code restored point in A, etc.

Or if you do not reflect this way, hopefully you will see some change in
behavior and track down the bug in `f-s'.

The point is that you should _not_ just _assume_ that `save-excursion' is used
only in a way that treats it as `save-current-buffer'.  Assume instead that all
of its behavior is needed, then work from there.

You can, as you suggest, try substituting `save-current-buffer' to see what
happens (i.e. as a diagnostic exercise), but be prepared for (look out for)
behavior changes.

> > 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).
> 
> Why do you think m-b-f "wants" to restore point in A, and not in other
> buffers?

Because the original programmer said so, a priori, by choosing to use
`save-excursion' in buffer A.  That's what `save-excursion' does.  We must
presume that the programmer knew what s?he was doing and knew that point in A
needed to be restored.

On what basis could you presume anything different?  You seem to be so convinced
(unlike Stefan, BTW) that `save-excursion' is evil that you assume that its
point-saving behavior is not needed (in code that works).

> What makes the buffer A special?  Why is it that you should
> always restore the point in the current buffer and leave all the other
> buffers to their fate?

That is what `save-excursion' does.  If you want something different, then code
something different.  By hypothesis, the original programmer used
`save-excursion', and knew what s?he was doing.  Don't ask me why s?he felt that
A needed restoring and not other buffers.  But s?he did (by hypothesis), or s?he
wouldn't have used `save-excursion'.

> > 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 `...').
> 
> It may not may have been intended to be called in buffer C, but
> nothing in the design might indicate that it is intended to be called
> only in buffer A.  So, you would be justified in calling it in
> another buffer.

Ooops.  No, but thanks for playing.  Something in the (inherited) implementation
is crying loud and clear that it is intended to be called from A and it is
intended to restore A.  What is that?  `save-excursion' was called from buffer
A, and its job is to restore the current buffer.

This couldn't be clearer.  You ignore such a clear indication of programmer
intention (implementation and probably design) at your own peril.

Let me repeat: `save-excursion' is _not_ `save-current-buffer'.  You substitute
the latter for the former willy-nilly at your own peril.

> > 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.
> 
> So, how much detail would one need to see before one can infer basic
> facts about point movements and buffer changes?

See above.  Infer from the presence of `save-excursion' that there is a _need_
to restore the original buffer, which was A, as well as its point and mark.

That is your best assumption, since the code has been working and the programmer
was no idiot.  Assume anything else by way of shortcut at your own peril.  Tread
carefully and analyze closely.

Again, it's fine to try substituting `save-current-buffer' as a probing
operation, to try to see what will happen - what might break, but be aware that
such a switch automatically changes the behavior: you are no longer saving and
restoring point (and mark).

> > > 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.)
> 
> I have no idea what it means for m-b-f "wanting" to restore A's point.

The presence of `save-excursion' indicates that the current buffer's point will
be restored.  By hypothesis the code reflects the programmer's intention (it
works and has been used for a long, long time).

`save-excursion' "wants" to restore point, and it does so.

> I don't see why it should care about the buffer A at all.  The way I
> view things, m-b-f should restore whatever temporary point movements
> it makes.

Well you are jumping to a wild conclusion then, one not based on the existing
code.  The existing code says only that `m-b-f' should restore point for the
original buffer (which is A).

If the intent had been to restore point movements in all buffers then a very
different program would have to have been written.  `save-excursion' does NOT do
that.  One of Stefan's points (and a/the reason for the warning) was that some
people can get fooled into thinking it does, but at this point at least _you_
should not be so fooled.

Nothing in that code indicates any intention to restore point movements in all
buffers.  Absolutely nothing.  Quite the contrary.  The code is telling us loud
and clear that the intention is to restore point _only_ in the original buffer
(e.g., A).

> It shouldn't be covering up the bugs in other functions
> like `find-something'.  But the way the save-excursion/set-buffer
> combination works, everything is covering up everything else's bugs.
> The code might be completely infested with bugs.  But we will never
> know, because it is also constantly covering them up!

Dunno what to say, Uday, without repeating myself more.  `save-excursion' saves
and restores point in only one buffer.  If that is not the intention then don't
use it.  What you call "covering up" is exactly the ignoring of temporary point
movements (for _one_ buffer).

> > `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.
> 
> Great, I seem to have hit a home run.  `find-something' bug will not
> surface as long as it is called inside a `save-excursion' for buffer
> A.  You said it!

Glad you got it.  Whether buggy or intended, any A-buffer point movements in
`f-s' are ultimately lost if it is called within a `save-excursion' invoked from
A.  That's the intention behind `save-excursion'.

> Now, what plausible reason is there for m-b-f to use `save-excursion'
> if not for covering up the `find-something' bug?

Uh, to do what it does?  To ignore point movements in the buffer it's called
from?  We have to presume that it _means_ to do that, unless you have some
reason not to.

> If the bug wasn't there, we can simply convert `save-excursion'
> to `save-current-buffer' and we will be fine.

If `save-current-buffer' were all that was needed here to start with, that is,
if the programmer did _not_ intend/need to restore point for buffer A, then
`save-excursion' would not have been used.  Even in code written long, long ago
the programmer would have simply saved and restored the buffer and not also the
values of point and mark.

> My belief is that all save-excursions used with set-buffer are
> precisely there to cover up bugs.

What can I say, Uday?

> If there is any other conceivable reason why `save-excursion' should
> be used with `set-buffer', then please show me!

Please read the original thread if you don't get it by now.

What if you need to save and later restore point in a buffer, Uday, what do
_you_ do?

You can use `save-excursion' with `save-current-buffer' or with
`with-current-buffer' instead of with `set-buffer', but you still need to use
`save-excursion' (or roll your own point-saving/restoring code).

Your beef seems to be with `save-excursion' and the fact that it saves and
restores point for a buffer.  Your beef _should_ be with `set-buffer', I would
think.  I would think you would simply be arguing for using 1 instead of 2, in
general:

1. (save-excursion (with-current-buffer Z...))
2. (save-excursion (set-buffer Z)...)

But #1, like #2 has all of the bad "cover-up" behavior you decry.
`save-excursion' saves point for the buffer in which it is invoked.  Invoke it
from a different buffer with no other changes and you are likely to get some
behavior you don't want.