improving debug output of get-buffer

improving debug output of get-buffer

[StrawberryTea]

Hello.

Following our PR discussion in
https://github.com/akreisher/eshell-syntax-highlighting/pull/15, I was
wondering if anyone thinks it's a good idea to modify the get-buffer C
code so that it returns an error like "(wrong-type-argument
string-or-buffer-p nil)" instead of "(wrong-type-argument stringp nil)".

Sincerely,
StrawberryTea

Re: improving debug output of get-buffer

[Eli Zaretskii]

> From: StrawberryTea <look@strawberrytea.xyz>
> Date: Wed, 01 Nov 2023 13:06:16 -0500
> 
> Following our PR discussion in
> https://github.com/akreisher/eshell-syntax-highlighting/pull/15

Please in the future post the main points of the discussions here.
Some of us don't like (or cannot) accessing GitHub.

> I was wondering if anyone thinks it's a good idea to modify the
> get-buffer C code so that it returns an error like
> "(wrong-type-argument string-or-buffer-p nil)" instead of
> "(wrong-type-argument stringp nil)".

IMO, "wrong-type-argument string-or-buffer-p" is not much better than
what we have now.

The reason why we signal "wrong-type-argument stringp"
is that the argument is almost always a string, because calling the
function with a buffer as an argument is not useful.

Re: improving debug output of get-buffer

[StrawberryTea]

[-- Attachment #1: Type: text/plain, Size: 1110 bytes --]

That makes sense. But basically, we were using with-current-buffer, which uses
set-buffer and it took time to realize the “(wrong-type-argument stringp nil)”
error was from that with-current-buffer.

Eli Zaretskii <eliz@gnu.org> writes:

>> From: StrawberryTea <look@strawberrytea.xyz>
>> Date: Wed, 01 Nov 2023 13:06:16 -0500
>>
>> Following our PR discussion in
>> <https://github.com/akreisher/eshell-syntax-highlighting/pull/15>
>
> Please in the future post the main points of the discussions here.
> Some of us don’t like (or cannot) accessing GitHub.
>
>> I was wondering if anyone thinks it’s a good idea to modify the
>> get-buffer C code so that it returns an error like
>> “(wrong-type-argument string-or-buffer-p nil)” instead of
>> “(wrong-type-argument stringp nil)”.
>
> IMO, “wrong-type-argument string-or-buffer-p” is not much better than
> what we have now.
>
> The reason why we signal “wrong-type-argument stringp”
> is that the argument is almost always a string, because calling the
> function with a buffer as an argument is not useful.

Re: improving debug output of get-buffer

[Eli Zaretskii]

> From: StrawberryTea <look@strawberrytea.xyz>
> Cc: emacs-devel@gnu.org
> Date: Wed, 01 Nov 2023 15:02:48 -0500
> 
> That makes sense. But basically, we were using with-current-buffer, which uses
> set-buffer and it took time to realize the “(wrong-type-argument stringp nil)”
> error was from that with-current-buffer.

And the backtrace didn't tell you the error was from set-buffer?

In any case, your original question was about get-buffer, not about
set-buffer.  Are you actually asking about set-buffer?

Re: improving debug output of get-buffer

[Rahguzar]

Hi Eli,
I was the other participant in the original discussion that led to this
thread.

Eli Zaretskii <eliz@gnu.org> writes:

>> That makes sense. But basically, we were using with-current-buffer, which uses
>> set-buffer and it took time to realize the “(wrong-type-argument stringp nil)”
>> error was from that with-current-buffer.
>
> And the backtrace didn't tell you the error was from set-buffer?

No backtrace stopped before `set-buffer`.

The backtrace was

Debugger entered--Lisp error: (wrong-type-argument stringp nil)
  eshell-syntax-highlighting--highlight-elisp(113 120)
  eshell-syntax-highlighting--parse-and-highlight(command 120)
  #<subr eshell-syntax-highlighting--enable-highlighting>()
  apply(#<subr eshell-syntax-highlighting--enable-highlighting> nil)
  (condition-case err (apply func args) ((debug error) (signal (car err) (cdr err))))
  cae-debug-reraise-error(#<subr eshell-syntax-highlighting--enable-highlighting>)
  apply(cae-debug-reraise-error #<subr eshell-syntax-highlighting--enable-highlighting> nil)
  eshell-syntax-highlighting--enable-highlighting()

where the function `eshell-syntax-highlighting--highlight-elisp` uses
the `with-current-buffer`.

I have been flummoxed by similar problems a few times before. Usually it
is `(with-current-buffer var body)` where `var` is a variable whose value
is I expect to be a buffer but which actually in nil. The
`(wrong-type-argument stringp nil)` makes me go looking for nil strings
in the body rather than realizing that the problem is with var
especially since `set-buffer` tends not to popup in the backtrace.

> In any case, your original question was about get-buffer, not about
> set-buffer.  Are you actually asking about set-buffer?

`set-buffer` is more relevant yes.

Thanks,
Rahguzar

Re: improving debug output of get-buffer

[Eli Zaretskii]

> From: Rahguzar <rahguzar@zohomail.eu>
> Cc: StrawberryTea <look@strawberrytea.xyz>,  emacs-devel@gnu.org
> Date: Thu, 02 Nov 2023 16:11:48 +0100
> 
> I was the other participant in the original discussion that led to this
> thread.

Once again: too bad that the whole discussion, with all its details,
does not happen on our bug tracker, but on some GitHub site.  The
details should be posted here, all of them, to help us understand and
investigate the issues.

> >> That makes sense. But basically, we were using with-current-buffer, which uses
> >> set-buffer and it took time to realize the “(wrong-type-argument stringp nil)”
> >> error was from that with-current-buffer.
> >
> > And the backtrace didn't tell you the error was from set-buffer?
> 
> No backtrace stopped before `set-buffer`.

Even if you set debug-on-error non-nil?  If so, I'd love to see a
reproduction recipe to understand why this happens.

> The backtrace was
> 
> Debugger entered--Lisp error: (wrong-type-argument stringp nil)
>   eshell-syntax-highlighting--highlight-elisp(113 120)

So you are saying that if instead we'd see

  Debugger entered--Lisp error: (wrong-type-argument string-or-buffer-p nil)
    eshell-syntax-highlighting--highlight-elisp(113 120)

it would have been much better?  Would you mind explaining to me how
would that be much more helpful?  Because I don't see it.

> where the function `eshell-syntax-highlighting--highlight-elisp` uses
> the `with-current-buffer`.

And knowing that eshell-syntax-highlighting--highlight-elisp calls
with-current-buffer, the error message still made no sense?  How so?

> > In any case, your original question was about get-buffer, not about
> > set-buffer.  Are you actually asking about set-buffer?
> 
> `set-buffer` is more relevant yes.

We could improve the diagnostics of set-buffer.  But I'm still at a
loss how that would have helped you here when "stringp nil" did not.

Re: improving debug output of get-buffer

[Rahguzar]

Thanks Eli,

Eli Zaretskii <eliz@gnu.org> writes:

> Once again: too bad that the whole discussion, with all its details,
> does not happen on our bug tracker, but on some GitHub site.  The
> details should be posted here, all of them, to help us understand and
> investigate the issues.

I agree with this but this was me trying to provide a fix for a package
maintained by someone else so it was somewhat out of our control. I did
try to provide some context.

> Even if you set debug-on-error non-nil?  If so, I'd love to see a
> reproduction recipe to understand why this happens.

I will try to come up with a `emacs -Q` recipe.

> So you are saying that if instead we'd see
>
>   Debugger entered--Lisp error: (wrong-type-argument string-or-buffer-p nil)
>     eshell-syntax-highlighting--highlight-elisp(113 120)
>
> it would have been much better?  Would you mind explaining to me how
> would that be much more helpful?  Because I don't see it.

For me at least, I don't always remember than it is possible to pass a
buffer name to `with-current-buffer` and in this particular case I had
expected that argument to be a buffer object but it turned out to
be nil. So when I saw `(wrong-type-argument stringp nil)` I just went
looking for a string in the body of `with-current-buffer` without
realizing that the string, this error is referring to might be buffer
name. With `(wrong-type-argument string-or-buffer-p nil)` I think I
would have looked for a `BUFFER-OR-NAME` argument and found it.

>> where the function `eshell-syntax-highlighting--highlight-elisp` uses
>> the `with-current-buffer`.
>
> And knowing that eshell-syntax-highlighting--highlight-elisp calls
> with-current-buffer, the error message still made no sense?  How so?

See above, I mistakenly thought I was passing a buffer object.

> We could improve the diagnostics of set-buffer.  But I'm still at a
> loss how that would have helped you here when "stringp nil" did not.

As a clumsy programmer I cause quite a lot of errors, a sizable of them
of `stringp nil` variety and mostly they are caused by wrong
manipulation of strings. So when I see it I tend to hunt for strings I
in the code. I think `buffer-or-string-p` will narrow down the places
where I need to look quite substantially.

Thanks,
Rahguzar

Re: improving debug output of get-buffer

[Eli Zaretskii]

> From: Rahguzar <rahguzar@zohomail.eu>
> Cc: look@strawberrytea.xyz, emacs-devel@gnu.org
> Date: Thu, 02 Nov 2023 17:57:03 +0100
> 
> > We could improve the diagnostics of set-buffer.  But I'm still at a
> > loss how that would have helped you here when "stringp nil" did not.
> 
> As a clumsy programmer I cause quite a lot of errors, a sizable of them
> of `stringp nil` variety and mostly they are caused by wrong
> manipulation of strings. So when I see it I tend to hunt for strings I
> in the code. I think `buffer-or-string-p` will narrow down the places
> where I need to look quite substantially.

Patches to improve the error messages from set-buffer will be welcome.
I think we should use something similar to nsberror there.