bug#15101: 24.3.50; debugger-eval-expression broken

bug#15101: 24.3.50; debugger-eval-expression broken

[Helmut Eller]

Start Emacs with: emacs -Q --eval '(let ((foo 123)) (debug))'

Then press e. Enter foo to evaluate the local variable foo. But it
doesn't work; it only generates the error:
debugger-eval-expression: Symbol's value as variable is void: foo

This used to work fine in previous versions.


In GNU Emacs 24.3.50.2 (i686-pc-linux-gnu, GTK+ Version 2.24.10)
 of 2013-08-15 on ix
Bzr revision: 113885 xfq.free@gmail.com-20130815082722-3v7q6fyczifh2s97

bug#15101: 24.3.50; debugger-eval-expression broken

[Stefan Monnier]

> Start Emacs with: emacs -Q --eval '(let ((foo 123)) (debug))'
> Then press e. Enter foo to evaluate the local variable foo. But it
> doesn't work; it only generates the error:
> debugger-eval-expression: Symbol's value as variable is void: foo
> This used to work fine in previous versions.

Indeed, this is a change that will trip up users.  Here's what's
happening: `e' will now run the code in the context in which the "code
on the current line" was run.

This refinement can be useful for dynamically bound variables, but was
mostly added for lexically bound variables, where it's indispensable.

So the above recipe works again if you use C-p before `e' so that point
is now on the top-most line, which stands for "in the context that
called `debug'".

I think a good fix is to change debug.el so that point starts on the
first line of the *Debugger* buffer rather than on the second.


        Stefan

bug#15101: 24.3.50; debugger-eval-expression broken

[Drew Adams]

> > Start Emacs with: emacs -Q --eval '(let ((foo 123)) (debug))'
> > Then press e. Enter foo to evaluate the local variable foo. But it
> > doesn't work; it only generates the error:
> > debugger-eval-expression: Symbol's value as variable is void: foo
> > This used to work fine in previous versions.
> 
> Indeed, this is a change that will trip up users.  Here's what's
> happening: `e' will now run the code in the context in which the "code
> on the current line" was run.
> 
> This refinement can be useful for dynamically bound variables, but was
> mostly added for lexically bound variables, where it's indispensable.
> 
> So the above recipe works again if you use C-p before `e' so that point
> is now on the top-most line, which stands for "in the context that
> called `debug'".
> 
> I think a good fix is to change debug.el so that point starts on the
> first line of the *Debugger* buffer rather than on the second.

In your judgment, can both the old `e' behavior and the new `e' behavior
be useful, at least for dynamically bound variables?

If so, perhaps consider having two different keys, for the two different
behaviors.

bug#15101: 24.3.50; debugger-eval-expression broken

[Stefan Monnier]

> In your judgment, can both the old `e' behavior and the new `e' behavior
> be useful, at least for dynamically bound variables?

`e' while on the top line of the buffer gives you the old behavior
(except it handles lexical vars if there were any when `debug' was
called).


        Stefan

bug#15101: 24.3.50; debugger-eval-expression broken

[Drew Adams]

> > In your judgment, can both the old `e' behavior and the new `e'
> > behavior be useful, at least for dynamically bound variables?
> 
> `e' while on the top line of the buffer gives you the old behavior
> (except it handles lexical vars if there were any when `debug' was
> called).

Yes, you said that.  This should be documented, of course, especially
since previously the cursor position was irrelevant for `e'.  So there
is at least a doc bug here, it seems.

You say that "this refinement can be useful for dynamically bound
variables" and is "indispensable" for lexically bound vars.

OK.  So just what is the usefulness?  What is a use case for `e' with
point not on the first line?  That too should be pointed out in the
doc - not obvious to me, at least.

And if it is NOT useful for someone to use `e' anywhere other than the
first line then why not have `e' act as if it were at bob?

Or if it is sometimes, but not typically, useful to use `e' other than
on the first line, how about a message letting an unaware user know that
the cursor is not on the first line and hence evaluation will not be in
the "context that called `debug'"?  That seems the least we could do
about this gotcha.

In sum, my concerns here are:

1. Is the non-first-line behavior of `e' really useful?  If not, maybe
   have the code work around it (always act as if on the first line).

2. Document the behavior (completely).  Describe the different use cases.

3. Consider a message informing the user that, because point is not
   on the first line, evaluation will not be done in the original context
   (and tell the user just what evaluation context will be used).

AFAICT, there is no mention of this in the doc yet.  I see that "lexical"
is mentioned only briefly in (elisp) `Edebug Eval' - and nothing about
this.  And it is not mentioned at all in the manual sections about the
standard, non-edebug debugger (i.e., command `debug').

bug#15101: 24.3.50; debugger-eval-expression broken

[Helmut Eller]

On Thu, Aug 15 2013, Stefan Monnier wrote:

>> Start Emacs with: emacs -Q --eval '(let ((foo 123)) (debug))'
>> Then press e. Enter foo to evaluate the local variable foo. But it
>> doesn't work; it only generates the error:
>> debugger-eval-expression: Symbol's value as variable is void: foo
>> This used to work fine in previous versions.
>
> Indeed, this is a change that will trip up users.  Here's what's
> happening: `e' will now run the code in the context in which the "code
> on the current line" was run.
>
> This refinement can be useful for dynamically bound variables, but was
> mostly added for lexically bound variables, where it's indispensable.
>
> So the above recipe works again if you use C-p before `e' so that point
> is now on the top-most line, which stands for "in the context that
> called `debug'".
>
> I think a good fix is to change debug.el so that point starts on the
> first line of the *Debugger* buffer rather than on the second.

What does "context" mean?  Intuitively I would say that in the second
line, ie. the one that looks like "(let ((foo 123)) (debug))", foo is
part of the context.

Compare this with:
(let ((foo 1))
  (let ((bar 2))
    (let ((baz 3))
      (debug))))

It seems to me that a better fix would be to adjust linenumber->context
mapping by one.

Helmut

bug#15101: 24.3.50; debugger-eval-expression broken

[Stefan Monnier]

> I think a good fix is to change debug.el so that point starts on the
> first line of the *Debugger* buffer rather than on the second.

I've just made that change.
Thank you,


        Stefan

bug#15101: 24.3.50; debugger-eval-expression broken

[Drew Adams]

> > I think a good fix is to change debug.el so that point starts on the
> > first line of the *Debugger* buffer rather than on the second.
> 
> I've just made that change.

And you closed the bug.  That change does not fix the bug, IMHO.

A user will still be tripped up by the undocumented and unguessable
behavior.  See my previous comments.  Please take care of this properly.

It is enough that a user does something in another window and then clicks
`mouse-1' in the debugger window on a line other than the first, to be
completely thrown off.

bug#15101: 24.3.50; debugger-eval-expression broken

[Stefan Monnier]

> A user will still be tripped up by the undocumented and unguessable
> behavior.

Check the doc before saying it's not documented.
And yes, NEWS is part of the doc.


        Stefan

bug#15101: 24.3.50; debugger-eval-expression broken

[Stefan Monnier]

> What does "context" mean?

That which is *around*.

> Intuitively I would say that in the second
> line, ie. the one that looks like "(let ((foo 123)) (debug))", foo is
> part of the context.

The context in which (let ((foo 123)) (debug)) does not include (yet)
"foo" (well, maybe it does, but that would be another "foo" than the
one inside the let).

> It seems to me that a better fix would be to adjust linenumber->context
> mapping by one.

I don't think this can be done, because the semantics become very weird
and unpredictable.


        Stefan

bug#15101: 24.3.50; debugger-eval-expression broken

[Drew Adams]

> > A user will still be tripped up by the undocumented and unguessable
> > behavior.
> 
> Check the doc before saying it's not documented.
> And yes, NEWS is part of the doc.

I mentioned clearly that I *did* check the doc:

  >> AFAICT, there is no mention of this in the doc yet.  I see that
  >> "lexical" is mentioned only briefly in (elisp) `Edebug Eval' - and
  >> nothing about this.  And it is not mentioned at all in the manual
  >> sections about the standard, non-edebug debugger (i.e., command
  >> `debug').

But yes, I had checked only in the most recent build I have (2013-08-08).
Mea culpa.

If you updated the doc as a result of this bug report (today), and your
update addresses all of the concerns I mentioned, great.  Pat on back.

But if you simply added a blurb to NEWS about this then no, that does not
handle the problem adequately for users.  If that's all you did then
please help users more than that.

Yes, NEWS is a part of the doc.  That doesn't make it the only place to
cover something like this.  (A change log is also doc.  As is a code
comment.)

NEWS is doc that users consult to find out about significant changes, but
it is not the doc they consult to find out about the product features and
how to use them.  You don't look only, or even mainly, in NEWS to find out
what `C-x C-f' does and how to use it.  Likewise, `e' in the debugger.

You expect the manuals and `C-h k C-x C-f' to tell you that.  And you
expect that information to be up-to-date and relatively complete.

Users should not need to consult NEWS to find out why something like `e',
which they have been using for decades, suddenly does not do what they
expect.  Each of the debugger commands/keys, of which `e' is one, is
described in the Elisp manual.  User-visible changes to the behavior
should result in an update of that doc.

FWIW, I have now checked the latest `debugging.texi' in BZR
(http://bzr.savannah.gnu.org/lh/emacs/trunk/annotate/head:/doc/lispref/debugging.texi).

Unfortunately, I see no change there from what has been there for the
description of `e' for a long time.  Nothing about any of the things
discussed in this thread.  No help for a user about cursor on first line;
which evaluation context is used when not on first line; differences
between lexical and dynamic; etc.  Nada.  Not a good thing for users.

bug#15101: 24.3.50; debugger-eval-expression broken

[Helmut Eller]

On Thu, Aug 15 2013, Stefan Monnier wrote:

>> What does "context" mean?
>
> That which is *around*.
>
>> Intuitively I would say that in the second
>> line, ie. the one that looks like "(let ((foo 123)) (debug))", foo is
>> part of the context.
>
> The context in which (let ((foo 123)) (debug)) does not include (yet)
> "foo" (well, maybe it does, but that would be another "foo" than the
> one inside the let).

Consider this example:

(progn
  (defun foo (n)
    (cond ((= n 0) (debug))
      (t (foo (1- n)))))
  (byte-compile 'foo)
  (foo 100))

If we go to the line "foo(10)" I would expect that n is 10. With your
definition n is 11. I would argue that your version is confusing.

>> It seems to me that a better fix would be to adjust linenumber->context
>> mapping by one.
>
> I don't think this can be done, because the semantics become very weird
> and unpredictable.

Can you give an example of what would be weird with this change:

=== modified file 'lisp/emacs-lisp/debug.el'
--- lisp/emacs-lisp/debug.el	2013-08-15 17:21:19 +0000
+++ lisp/emacs-lisp/debug.el	2013-08-16 05:22:02 +0000
@@ -547,7 +547,7 @@
   (interactive
    (list (read--expression "Eval in stack frame: ")))
   (let ((nframe (or nframe
-                    (condition-case nil (1+ (debugger-frame-number 'skip-base))
+                    (condition-case nil (debugger-frame-number 'skip-base)
                       (error 0)))) ;; If on first line.
          (base (if (eq 'debug--implement-debug-on-entry
                       (cadr (backtrace-frame 1 'debug)))


Either way, it would be nice to document why the 1+ was there in the
first place, because it looks like a bug to me.

Helmut

bug#15101: 24.3.50; debugger-eval-expression broken

[Stefan Monnier]

> If we go to the line "foo(10)" I would expect that n is 10. With your
> definition n is 11. I would argue that your version is confusing.

I think the problem comes from the fact that contrary to traditional
debuggers, we don't have the "line-number info".

In a traditional debugger, you'd have "foo(10) at foo.el:25", pointing
out exactly where you are inside `foo'.  Whereas in Elisp backtraces you
only have "the expression that was being evaluated", so "foo(10)"
doesn't tell you where you were inside `foo', instead it tells you that
you were somewhere where a call to `(foo 10) was made.

> Can you give an example of what would be weird with this change:
[...]
> -                    (condition-case nil (1+ (debugger-frame-number 'skip-base))
> +                    (condition-case nil (debugger-frame-number 'skip-base)

I started with this, but found it very weird for example with things like

   (let ((x (toto)) (y (titi))) (tata))

where you'd naturally expect to be able to find the value of `x' or `y',
but that's only true if the debugger was called from `tata'.  If instead
the debugger was called from within `toto' or `titi', `x' and `y' aren't
bound yet.


        Stefan

bug#15101: 24.3.50; debugger-eval-expression broken

[Helmut Eller]

On Fri, Aug 16 2013, Stefan Monnier wrote:

>> If we go to the line "foo(10)" I would expect that n is 10. With your
>> definition n is 11. I would argue that your version is confusing.
>
> I think the problem comes from the fact that contrary to traditional
> debuggers, we don't have the "line-number info".
>
> In a traditional debugger, you'd have "foo(10) at foo.el:25", pointing
> out exactly where you are inside `foo'.  Whereas in Elisp backtraces you
> only have "the expression that was being evaluated", so "foo(10)"
> doesn't tell you where you were inside `foo', instead it tells you that
> you were somewhere where a call to `(foo 10) was made.

In this example, there are two positions of interest:

  1. for the newest stack frame, the one that looks like foo(0), the
     "instruction pointer" would be at the callsite for debug.

  2. for all other stack frames of the form foo(X) the instruction
     pointer would be at the same location: the (recursive) callsite for
     foo.

Are there any other places that foo(X) could possibly denote? I think
not.  So, what value has n at position 1?

>
>> Can you give an example of what would be weird with this change:
> [...]
>> - (condition-case nil (1+ (debugger-frame-number 'skip-base))
>> +                    (condition-case nil (debugger-frame-number 'skip-base)
>
> I started with this, but found it very weird for example with things like
>
>    (let ((x (toto)) (y (titi))) (tata))
>
> where you'd naturally expect to be able to find the value of `x' or `y',
> but that's only true if the debugger was called from `tata'.  If instead
> the debugger was called from within `toto' or `titi', `x' and `y' aren't
> bound yet.

The 1+ doesn't solve/change that problem, does it?

Helmut