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

[Uday S Reddy <u.s.reddy@cs.bham.ac.uk>]

Drew Adams writes:

> 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?

No, it wasn't from an actual application.  I just made it up.  And, it
isn't good code either.  To make it good code, one needs another
save-excursion to protect the point movement in buffer A, as below:

     (defun find-something ()
        (....
	   (save-excursion 
              (set-buffer A)  
	      (save-excursion
                 (goto-char y)  
                  ...))))

The outer save-excursion essentially plays the role of
save-current-buffer and it is mostly harmless when used in that way.
The second save-excursion is needed to reverse the excursion of the
point to y.  Without the second save-excursion, I would consider the
code buggy.

Once that bug is fixed, the remaining save-excursions (the ones at the
top-level in `find-something' as well as `my-beautiful-function') can
be safely converted to save-current-buffer.  And, the code will work
fine.  This is what I mean by tracking down the bugs covered up by
redundant save-excursions.  The clean-up procedure involves:

- Converting a few save-excursions flagged up by the compiler to
save-current-buffer.

- Testing to see if those conversions break anything.

- If they do, then hunt down the unprotected excursions that have been
exposed in the process and wrap them in save-excursions.

And repeat.  It is painstaking work.

You might wonder why do this at all.  Why not just leave the code as
it is since it is working fine?  That is a judgement call for you as
the developer.  If it is important code and you expect it to get used
for a long time hence, it pays to clean it up, because you never know
when those unprotected excursions will come back to bite you.  

> 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.

If that code was found in an actual application, then the only thing
that can be said in defence of the original developer is that it must
have been an oversight.  In the pre-20.1 elisp, save-excursion was the
only primitive available to restore the current buffer, which had the
unfortunate effect of covering up all the unprotected excursions.  So,
s?he didn't notice the problem.  If s?he deliberately wrote such code,
then we should probably dump it and go find something else to work on.

> 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.

No way!  If the original author deliberately used the outer
save-excursion to cover up unprotected excursions, then the code is
really bad.  Let me amplify that point because this seems to be the
crux of the discussion:

   (save-excursion
      (set-buffer B)
      ....
      (setq a (find-something))
      ...)

The outer save-excursion is restoring the current buffer's point and
mark.  There is nothing visible in the body that does anything to the
current buffer's point and mark.  So, the ONLY reason the outer
save-excursion needs to restore the current buffer's point and mark is
the expectation that `find-something' is going to do something cheeky.
How can you trust such a programmer?

> 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.

If the code doesn't allow me to alter it "willy-nilly" then it isn't
good code.  Of course, I would need to understand its function, the
data structures and invariants.  But, assuming I have understood all
that, I should be able to look at pieces of code locally and modify
them.  

> 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.

I think I have already made my disagrement plain.  It is a bad idea
for `my-beautiful-function' to be negating the other code's point
movements at the top level.  If it is clear from the way the "other
code" is put together that its purpose is to change buffer and move
point, and m-b-f is deliberately calling that code to achieve those
effects, then why is it reversing all those changes via a
save-excursion?  That doesn't make much sense.  Either

- the other code is being called for its effects, in which case we
have no need to reverse them, or

- the other code is being called for some other results, in which case
it should be cleaning up after itself.

Slapping a save-excursion at the top-level, which deceptively looks
like a save-current-buffer, to do a brute force clean-up after the
fact is not a very clever way of arranging things.

> 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).

No, I didn't say that.  `save-excursion' is definitely used to save
the point and mark.  But when used for that purpose, it is placed
where the point or mark are being moved.  If it is used to save the
buffer, it is placed in front of a set-buffer.  I have never really
seen it being used to do both.  If I have to imagine how it could be
used to do both, it would be something like this:

    (save-excursion
        ...
	(goto-char x)
        ...
        (set-buffer B)
        (save-excursion
           ....))

A second save-excursion would be needed after switching to B, because
whatever is done afterwards is *not* going to be protected by the
original save-excursion.

> > 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'.

We shouldn't call `save-excursion' willy-nilly just because it
exists.  If the buffer A is indeed special and we should restore only
its excursions but not those of other buffers, then the right place to
protect them is wherever the excursions are happening, not at the
top-level.  

Morever, what if that is not what we want?  What if we want to protect
the excursions in all the buffers, not only the current-buffer of
`m-b-f'?  The top-level `save-excursion' is not going to help you
achieve that.  What do you do then?

On a closer look, this kind of programming doesn't look well-thought
out at all.  It seems quite arbitrary.

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

I know.  I was writing to Stefan earlier today saying exactly that.
`save-excursion' should be replaced by `save-current-buffer' only
after ensuring that all excursions are properly protected.  I have
described my clean-up procedure earlier in the message.

> 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).

"Only one buffer" is a mistaken assumption.  Typically, in this kind
of code, there would be save-excursions all over the place.  Whenever
point movements happen, they would be under a thick stack of
save-excursions for all kinds of buffers.  The buffer you are doing
excursions in will typically end up being in that stack accidentaly
and so its excursion will get cancelled before the whole process ends.
Somehow magically the code works, even though you don't know how.
When you tweak the code a little, some buffer might get eliminated
from the stack and, all of a sudden, its excursions will get exposed
to the user.  "No problem!" says the hacker.  Slap on another
save-excursion, and the code starts to work magically again.

> > 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?

What do _I_ do?  I never need to save and restore the point in a
buffer.  I always place my save-excursions next to the excursions.
Only those point movements that are supposed to be the inteded effects
of the operations will be outside any save-excursion.  If I make any
mistakes in doing this, I go find them and fix them.  So, I don't need
any save-excursions at the top-level.  The code that I maintain works
exactly in that way.  And that is what the Elisp manual says
you should do as well:

       It is often useful to move point "temporarily" within a
       localized portion of the program, or to switch buffers
       temporarily.  This is called an "excursion", and it is done
       with the `save-excursion' special form.

Cheers,
Uday