bug#48479: 28.0.50; Crash on `read--expression'

bug#48479: 28.0.50; Crash on `read--expression'

[Jean Louis]

This function is not complete, I was just testing things. But when I
invoke interactively M-x rcd-hash-edit then Emacs crashes. I did not
want to place "nil" down, this comes from testing. It crashes with emacs
-Q when you evaluate this function and invoke it interactively.

(defun rcd-hash-edit ()
  "Interactively choose a hash to edit."
  (interactive)
  (let ((hash (condition-case nil
		  (read--expression "Hash to edit: ")
		nil)))
    (if (eq (type-of (symbol-value hash)) 'hash-table)
	(rcd-hash-edit-hash hash)
      (message "Symbol is not a hash"))))

Fatal error 11: Segmentation fault
Backtrace:
emacs(+0x14a014)[0x558271bec014]
emacs(+0x43835)[0x558271ae5835]
emacs(+0x43d0a)[0x558271ae5d0a]
emacs(+0x1482dd)[0x558271bea2dd]
emacs(+0x148359)[0x558271bea359]
/usr/lib/libpthread.so.0(+0x13960)[0x7f8dd29c2960]
emacs(+0x1b1f9b)[0x558271c53f9b]
emacs(+0x1b0832)[0x558271c52832]
emacs(+0x1b1b0b)[0x558271c53b0b]
emacs(+0x1b0832)[0x558271c52832]
emacs(+0x1b1455)[0x558271c53455]
emacs(+0x1adfc7)[0x558271c4ffc7]
emacs(+0x1aa701)[0x558271c4c701]
emacs(+0x1ae08b)[0x558271c5008b]
emacs(+0x1b0249)[0x558271c52249]
emacs(+0x1abdfa)[0x558271c4ddfa]
emacs(+0x1ae08b)[0x558271c5008b]
emacs(+0x1eaaa0)[0x558271c8caa0]
emacs(+0x1adfc7)[0x558271c4ffc7]
emacs(+0x1eaaa0)[0x558271c8caa0]
emacs(+0x1adfc7)[0x558271c4ffc7]
emacs(+0x1aa701)[0x558271c4c701]
emacs(+0x1ae08b)[0x558271c5008b]
emacs(+0x1b0168)[0x558271c52168]
emacs(+0x1abdfa)[0x558271c4ddfa]
emacs(+0x1ae08b)[0x558271c5008b]
emacs(+0x1eaaa0)[0x558271c8caa0]
emacs(+0x1adfc7)[0x558271c4ffc7]
emacs(+0x1ae14a)[0x558271c5014a]
emacs(+0x13f60d)[0x558271be160d]
emacs(+0x1acfe7)[0x558271c4efe7]
emacs(+0x12ff94)[0x558271bd1f94]
emacs(+0x1af713)[0x558271c51713]
emacs(+0x12ff3b)[0x558271bd1f3b]
emacs(+0x1356e6)[0x558271bd76e6]
emacs(+0x135a12)[0x558271bd7a12]
emacs(+0x4c03f)[0x558271aee03f]
/usr/lib/libc.so.6(__libc_start_main+0xd5)[0x7f8dd265bb25]
emacs(+0x4c64e)[0x558271aee64e]



In GNU Emacs 28.0.50 (build 2, x86_64-pc-linux-gnu, X toolkit, cairo version 1.17.4, Xaw3d scroll bars)
 of 2021-05-13 built on protected.rcdrun.com
Repository revision: ec574a72f7198d9793b466f33382fff397ac4ce1
Repository branch: master
Windowing system distributor 'The X.Org Foundation', version 11.0.12011000
System Description: Parabola GNU/Linux-libre

Configured using:
 'configure --prefix=/package/text/emacs --with-x-toolkit=lucid'

Configured features:
ACL CAIRO DBUS FREETYPE GIF GLIB GMP GNUTLS GPM GSETTINGS HARFBUZZ JPEG
JSON LCMS2 LIBOTF LIBSYSTEMD LIBXML2 M17N_FLT MODULES NOTIFY INOTIFY
PDUMPER PNG RSVG SECCOMP SOUND THREADS TIFF TOOLKIT_SCROLL_BARS X11
XAW3D XDBE XIM XPM LUCID ZLIB

Important settings:
  value of $LC_ALL: en_US.UTF-8
  value of $LANG: de_DE.UTF-8
  value of $XMODIFIERS: @im=exwm-xim
  locale-coding-system: utf-8-unix

Major mode: Shell

Minor modes in effect:
  shell-dirtrack-mode: t
  tooltip-mode: t
  global-eldoc-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  tool-bar-mode: t
  menu-bar-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  blink-cursor-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t
  line-number-mode: t
  transient-mark-mode: t

Load-path shadows:
None found.

Features:
(shadow sort mail-extr emacsbug message rmc puny rfc822 mml mml-sec epa
derived epg epg-config gnus-util rmail rmail-loaddefs auth-source cl-seq
eieio eieio-core cl-macs eieio-loaddefs password-cache json map
time-date subr-x mm-decode mm-bodies mm-encode mail-parse rfc2231
mailabbrev gmm-utils mailheader sendmail rfc2047 rfc2045 ietf-drums
mm-util mail-prsvr mail-utils shell pcomplete comint ansi-color ring
cus-edit cus-start cus-load wid-edit misearch multi-isearch dired-aux
cl-loaddefs cl-lib dired dired-loaddefs bookmark seq byte-opt gv
bytecomp byte-compile cconv text-property-search pp iso-transl tooltip
eldoc electric uniquify ediff-hook vc-hooks lisp-float-type mwheel
term/x-win x-win term/common-win x-dnd tool-bar dnd fontset image
regexp-opt fringe tabulated-list replace newcomment text-mode elisp-mode
lisp-mode prog-mode register page tab-bar menu-bar rfn-eshadow isearch
easymenu timer select scroll-bar mouse jit-lock font-lock syntax
font-core term/tty-colors frame minibuffer cl-generic cham georgian
utf-8-lang misc-lang vietnamese tibetan thai tai-viet lao korean
japanese eucjp-ms cp51932 hebrew greek romanian slovak czech european
ethiopic indian cyrillic chinese composite charscript charprop
case-table epa-hook jka-cmpr-hook help simple abbrev obarray
cl-preloaded nadvice button loaddefs faces cus-face macroexp files
window text-properties overlay sha1 md5 base64 format env code-pages
mule custom widget hashtable-print-readable backquote threads dbusbind
inotify lcms2 dynamic-setting system-font-setting font-render-setting
cairo x-toolkit x multi-tty make-network-process emacs)

Memory information:
((conses 16 89766 5983)
 (symbols 48 9629 3)
 (strings 32 27590 1395)
 (string-bytes 1 857514)
 (vectors 16 17777)
 (vector-slots 8 224289 8351)
 (floats 8 37 63)
 (intervals 56 1292 193)
 (buffers 992 16))

-- 
Thanks,
Jean Louis

Take action in Free Software Foundation campaigns:
https://www.fsf.org/campaigns

bug#48479: 28.0.50; Crash on `read--expression'

[Eli Zaretskii]

> From: Jean Louis <bug@gnu.support>
> Date: Mon, 17 May 2021 14:39:14 +0300
> 
> 
> This function is not complete, I was just testing things. But when I
> invoke interactively M-x rcd-hash-edit then Emacs crashes. I did not
> want to place "nil" down, this comes from testing. It crashes with emacs
> -Q when you evaluate this function and invoke it interactively.
> 
> (defun rcd-hash-edit ()
>   "Interactively choose a hash to edit."
>   (interactive)
>   (let ((hash (condition-case nil
> 		  (read--expression "Hash to edit: ")
> 		nil)))
>     (if (eq (type-of (symbol-value hash)) 'hash-table)
> 	(rcd-hash-edit-hash hash)
>       (message "Symbol is not a hash"))))
> 
> Fatal error 11: Segmentation fault
> Backtrace:

Thanks, this was caused by a recent addition of the :success handler.
I tried to fix that on the master branch now.

bug#48479: 28.0.50; Crash on `read--expression'

[Mattias Engdegård]

17 maj 2021 kl. 17.14 skrev Eli Zaretskii <eliz@gnu.org>:

> Thanks, this was caused by a recent addition of the :success handler.
> I tried to fix that on the master branch now.

Thank you for fixing that, and sorry about the oversight.

It prompts the question why we are so permissive in the first place -- a nil handler is malformed and always a mistake. Raising an error is probably more helpful. Although someone may be relying on this undocumented behaviour it sounds unlikely. Given that it has elicited a byte-compiler warning for a very long time (probably something like 17 years), we ought to be forgiven for turning it into an error now, wouldn't we?

bug#48479: 28.0.50; Crash on `read--expression'

[Eli Zaretskii]

> From: Mattias Engdegård <mattiase@acm.org>
> Date: Mon, 17 May 2021 22:01:10 +0200
> Cc: 48479@debbugs.gnu.org
> 
> 17 maj 2021 kl. 17.14 skrev Eli Zaretskii <eliz@gnu.org>:
> 
> > Thanks, this was caused by a recent addition of the :success handler.
> > I tried to fix that on the master branch now.
> 
> Thank you for fixing that, and sorry about the oversight.

No sweat.

> It prompts the question why we are so permissive in the first place -- a nil handler is malformed and always a mistake. Raising an error is probably more helpful. Although someone may be relying on this undocumented behaviour it sounds unlikely. Given that it has elicited a byte-compiler warning for a very long time (probably something like 17 years), we ought to be forgiven for turning it into an error now, wouldn't we?

What would be the advantage of making it an error?

bug#48479: 28.0.50; Crash on `read--expression'

[Mattias Engdegård]

18 maj 2021 kl. 13.29 skrev Eli Zaretskii <eliz@gnu.org>:

> What would be the advantage of making it an error?

Detecting mistakes, early. There are many possible reasons for making ones; misplaced brackets is but one.

In the Lisp implementation tradition we do not benefit from a monolithic parser that verifies all aspects of the grammar in one go, so extra care has to be taken in each special form. A sloppy parse is unhelpful: it hides mistakes and impedes backward-compatible language evolution. There is no advantage to it.

Furthermore, reporting syntax errors as warnings but still accepting the code really makes no sense, other as a temporary measure to avoid breaking invalid code right away (if that is seen as something positive).

bug#48479: 28.0.50; Crash on `read--expression'

[Eli Zaretskii]

> Feedback-ID:mattiase@acm.or
> From: Mattias Engdegård <mattiase@acm.org>
> Date: Tue, 18 May 2021 17:00:33 +0200
> Cc: 48479@debbugs.gnu.org
> 
> 18 maj 2021 kl. 13.29 skrev Eli Zaretskii <eliz@gnu.org>:
> 
> > What would be the advantage of making it an error?
> 
> Detecting mistakes, early.

That doesn't sound to me like a significant advantage, not in Emacs
Lisp.

bug#48479: 28.0.50; Crash on `read--expression'

[Mattias Engdegård]

18 maj 2021 kl. 17.09 skrev Eli Zaretskii <eliz@gnu.org>:

>> Detecting mistakes, early.
> 
> That doesn't sound to me like a significant advantage, not in Emacs
> Lisp.

Is there any significant advantage of not detecting syntax errors?

Detection of syntax errors is something that people tend to find useful. Maybe you could tolerate it even though it would not be of value to you personally?

bug#48479: 28.0.50; Crash on `read--expression'

[Eli Zaretskii]

> From: Mattias Engdegård <mattiase@acm.org>
> Date: Tue, 18 May 2021 17:42:08 +0200
> Cc: 48479@debbugs.gnu.org
> 
> 18 maj 2021 kl. 17.09 skrev Eli Zaretskii <eliz@gnu.org>:
> 
> >> Detecting mistakes, early.
> > 
> > That doesn't sound to me like a significant advantage, not in Emacs
> > Lisp.
> 
> Is there any significant advantage of not detecting syntax errors?

I don't think I agree that this is a syntax error.

What do others think about this?  Lars? Stefan? Richard?

bug#48479: 28.0.50; Crash on `read--expression'

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

>> Is there any significant advantage of not detecting syntax errors?
> I don't think I agree that this is a syntax error.
> What do others think about this?  Lars? Stefan? Richard?

I think the condition handler of `condition-case` should be of the form
(CONDITIONS . BODY) so I think having nil counts as a syntax error, tho
it's arguable since nil ≃ (nil . nil)

I wouldn't bother signaling an error when we interpret the code, but it
would be good for the byte-compiler to warn about it.


        Stefan

bug#48479: 28.0.50; Crash on `read--expression'

[Andreas Schwab]

On Mai 18 2021, Stefan Monnier via "Bug reports for GNU Emacs, the Swiss army knife of text editors" wrote:

>>> Is there any significant advantage of not detecting syntax errors?
>> I don't think I agree that this is a syntax error.
>> What do others think about this?  Lars? Stefan? Richard?
>
> I think the condition handler of `condition-case` should be of the form
> (CONDITIONS . BODY) so I think having nil counts as a syntax error, tho
> it's arguable since nil ≃ (nil . nil)

For some reason, the loop at the top of internal_lisp_condition_case
explicitly allows for a handler to be nil.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

bug#48479: 28.0.50; Crash on `read--expression'

[Lars Ingebrigtsen]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>>> Is there any significant advantage of not detecting syntax errors?
>> I don't think I agree that this is a syntax error.
>> What do others think about this?  Lars? Stefan? Richard?

[...]

> I wouldn't bother signaling an error when we interpret the code, but it
> would be good for the byte-compiler to warn about it.

I thought we did warn about this, but some testing now seems to indicate
that we don't?  So then we definitely can't turn this into an error, but
I agree with Stefan that we should issue a warning...  probably.
(Depending on whether there's a lot of code out there on this form or
not.)

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#48479: 28.0.50; Crash on `read--expression'

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: Eli Zaretskii <eliz@gnu.org>,  Mattias Engdegård
>  <mattiase@acm.org>,
>   Richard Stallman <rms@gnu.org>,  48479@debbugs.gnu.org
> Date: Wed, 19 May 2021 16:53:38 +0200
> 
> I agree with Stefan that we should issue a warning...  probably.
> (Depending on whether there's a lot of code out there on this form or
> not.)

I'm also okay with a warning during byte-compilation.

bug#48479: 28.0.50; Crash on `read--expression'

[Mattias Engdegård]

19 maj 2021 kl. 16.53 skrev Lars Ingebrigtsen <larsi@gnus.org>:

> I thought we did warn about this, but some testing now seems to indicate
> that we don't?

 (condition-case nil t nil)

definitely elicits a warning by the byte-compiler (not about the syntax per se but about `nil` not being a condition). Or were you thinking about something else?

bug#48479: 28.0.50; Crash on `read--expression'

[Mattias Engdegård]

18 maj 2021 kl. 19.46 skrev Andreas Schwab <schwab@linux-m68k.org>:

> For some reason, the loop at the top of internal_lisp_condition_case
> explicitly allows for a handler to be nil.

Quite right! And a proper syntax check at that point would make the code no slower or more complicated.

bug#48479: 28.0.50; Crash on `read--expression'

[Lars Ingebrigtsen]

Mattias Engdegård <mattiase@acm.org> writes:

> 19 maj 2021 kl. 16.53 skrev Lars Ingebrigtsen <larsi@gnus.org>:
>
>> I thought we did warn about this, but some testing now seems to indicate
>> that we don't?
>
>  (condition-case nil t nil)
>
> definitely elicits a warning by the byte-compiler (not about the
> syntax per se but about `nil` not being a condition). Or were you
> thinking about something else?

I was thinking of the

(condition-case nil
    (foo)
  (error))

case...  (I.e., with a missing handler body.)  I'm not sure whether
that's supposed to or not.  A handler is defined as

(CONDITIONS BODY...)

but a BODY in many other circumstances are allowed to be missing.

Also note that

(condition-case nil
    (foo))

doesn't give a warning.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#48479: 28.0.50; Crash on `read--expression'

[Mattias Engdegård]

19 maj 2021 kl. 20.58 skrev Lars Ingebrigtsen <larsi@gnus.org>:

> I was thinking of the
> 
> (condition-case nil
>    (foo)
>  (error))
> 
> case...  (I.e., with a missing handler body.)  I'm not sure whether
> that's supposed to or not.

Thank you, and yes, that is valid; a body can be empty (it's an implicit progn).
I don't think an empty body in an error handler warrants a warning more than anywhere else. Do you?

> (condition-case nil
>    (foo))
> 
> doesn't give a warning.

Right, but its meaning is also well-defined and could even be useful in a macro. I'm slightly more inclined to accept a warning here, but we are drifting away from the original question: for syntactically invalid handlers, like (), can we signal an error? I think we can.

bug#48479: 28.0.50; Crash on `read--expression'

[Lars Ingebrigtsen]

Mattias Engdegård <mattiase@acm.org> writes:

> 19 maj 2021 kl. 20.58 skrev Lars Ingebrigtsen <larsi@gnus.org>:
>
>> I was thinking of the
>> 
>> (condition-case nil
>>    (foo)
>>  (error))
>> 
>> case...  (I.e., with a missing handler body.)  I'm not sure whether
>> that's supposed to or not.
>
> Thank you, and yes, that is valid; a body can be empty (it's an
> implicit progn).
> I don't think an empty body in an error handler warrants a warning
> more than anywhere else. Do you?

Not really -- it's just not explicitly stated that the empty version is
allowed, but I guess it can be reasonably inferred (and that the return
value of emptiness is nil).

>> (condition-case nil
>>    (foo))
>> 
>> doesn't give a warning.
>
> Right, but its meaning is also well-defined and could even be useful
> in a macro. I'm slightly more inclined to accept a warning here, but
> we are drifting away from the original question: for syntactically
> invalid handlers, like (), can we signal an error? I think we can.

On

(condition-case nil
   (foo)
  ())

?  A warning is nice, but I think signalling an error would be
excessive.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#48479: 28.0.50; Crash on `read--expression'

[Mattias Engdegård]

Hello Lars,

25 maj 2021 kl. 07.05 skrev Lars Ingebrigtsen <larsi@gnus.org>:

> (condition-case nil
>   (foo)
>  ())
> 
> ?  A warning is nice, but I think signalling an error would be
> excessive.

Why is an error excessive for that syntax mistake, when we signal an error for

 (condition-case
    (do-something)
   (error "failure"))

and

 (condition-case nil
    (do-something)
   error "failure")

? Wouldn't the user be best served with an error in all cases?