bug#51368: 29.0.50; `cl-case' should error on incorrect use

bug#51368: 29.0.50; `cl-case' should error on incorrect use

[Philipp Stephani]

This form demonstrates a few incorrect uses of `cl-case':

(cl-case a
  (nil 0)   ; doesn't match anything
  (t 1)     ; matches everything, but too early
  ('foo 2)) ; matches `quote' in addition to `foo'

It would be nice if `cl-case' would signal an error or at least warn
about these at macroexpansion time, since they are somewhat subtle and
easy to get wrong.


In GNU Emacs 29.0.50 (build 3, x86_64-pc-linux-gnu, GTK+ Version 3.24.30, cairo version 1.16.0)
 of 2021-10-24
Repository revision: aea4af5119fdf130f1df7190478a23c6777f92a2
Repository branch: master
Windowing system distributor 'The X.Org Foundation', version 11.0.12011000
System Description: Debian GNU/Linux rodete

Configured using:
 'configure --enable-gcc-warnings=warn-only
 --enable-gtk-deprecation-warnings --without-pop --with-mailutils
 --enable-checking=all --enable-check-lisp-object-type --with-modules
 'CFLAGS=-O0 -ggdb3''

Configured features:
CAIRO DBUS FREETYPE GIF GLIB GMP GNUTLS GSETTINGS HARFBUZZ JPEG JSON
LIBSELINUX LIBSYSTEMD LIBXML2 MODULES NOTIFY INOTIFY PDUMPER PNG SECCOMP
SOUND THREADS TIFF TOOLKIT_SCROLL_BARS X11 XDBE XIM XPM GTK3 ZLIB

Important settings:
  value of $LC_TIME: en_DK.utf8
  value of $LANG: en_US.utf8
  value of $XMODIFIERS: @im=ibus
  locale-coding-system: utf-8-unix

Major mode: Lisp Interaction

Minor modes in effect:
  tooltip-mode: t
  global-eldoc-mode: t
  eldoc-mode: t
  show-paren-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
  indent-tabs-mode: t
  transient-mark-mode: t

Load-path shadows:
None found.

Features:
(shadow sort mail-extr emacsbug message rmc dired dired-loaddefs rfc822
mml mml-sec epa epg rfc6068 epg-config gnus-util rmail rmail-loaddefs
time-date mm-decode mm-bodies mm-encode mail-parse rfc2231 mailabbrev
gmm-utils mailheader sendmail rfc2047 rfc2045 ietf-drums mm-util
mail-prsvr mail-utils phst skeleton derived pcase ffap thingatpt url
url-proxy url-privacy url-expand url-methods url-history url-cookie
url-domsuf url-util url-parse auth-source cl-seq eieio eieio-core
cl-macs eieio-loaddefs password-cache json map url-vars mailcap rx
gnutls puny dbus xml seq gv subr-x byte-opt bytecomp byte-compile cconv
compile text-property-search comint ansi-color ring cl-loaddefs cl-lib
iso-transl tooltip eldoc paren electric uniquify ediff-hook vc-hooks
lisp-float-type elisp-mode 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 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 emoji-zwj 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 dynamic-setting system-font-setting font-render-setting cairo
move-toolbar gtk x-toolkit x multi-tty make-network-process emacs)

Memory information:
((conses 16 68529 4717)
 (symbols 48 8401 3)
 (strings 32 24306 1951)
 (string-bytes 1 786068)
 (vectors 16 15413)
 (vector-slots 8 204610 8730)
 (floats 8 26 36)
 (intervals 56 225 0)
 (buffers 992 11))

-- 
Google Germany GmbH
Erika-Mann-Straße 33
80636 München

Geschäftsführer: Paul Manicle, Halimah DeLaine Prado
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg

Diese E-Mail ist vertraulich.  Falls Sie diese fälschlicherweise erhalten haben
sollten, leiten Sie diese bitte nicht an jemand anderes weiter, löschen Sie
alle Kopien und Anhänge davon und lassen Sie mich bitte wissen, dass die E-Mail
an die falsche Person gesendet wurde.

This e-mail is confidential.  If you received this communication by mistake,
please don’t forward it to anyone else, please erase all copies and
attachments, and please let me know that it has gone to the wrong person.

bug#51368: 29.0.50; `cl-case' should error on incorrect use

[Andreas Schwab]

On Okt 24 2021, Philipp Stephani wrote:

> This form demonstrates a few incorrect uses of `cl-case':
>
> (cl-case a
>   (nil 0)   ; doesn't match anything
>   (t 1)     ; matches everything, but too early
>   ('foo 2)) ; matches `quote' in addition to `foo'
>
> It would be nice if `cl-case' would signal an error or at least warn
> about these at macroexpansion time, since they are somewhat subtle and
> easy to get wrong.

clisp errors out on the misplaced `t' clause, but is otherwise silent.

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#51368: 29.0.50; `cl-case' should error on incorrect use

[Stefan Kangas]

Andreas Schwab <schwab@linux-m68k.org> writes:

> On Okt 24 2021, Philipp Stephani wrote:
>
>> This form demonstrates a few incorrect uses of `cl-case':
>>
>> (cl-case a
>>   (nil 0)   ; doesn't match anything
>>   (t 1)     ; matches everything, but too early
>>   ('foo 2)) ; matches `quote' in addition to `foo'
>>
>> It would be nice if `cl-case' would signal an error or at least warn
>> about these at macroexpansion time, since they are somewhat subtle and
>> easy to get wrong.
>
> clisp errors out on the misplaced `t' clause, but is otherwise silent.

I think it would be good if we could error out on the misplaced t.

bug#51368: 29.0.50; `cl-case' should error on incorrect use

[Lars Ingebrigtsen]

Philipp Stephani <p.stephani2@gmail.com> writes:

> This form demonstrates a few incorrect uses of `cl-case':
>
> (cl-case a
>   (nil 0)   ; doesn't match anything
>   (t 1)     ; matches everything, but too early
>   ('foo 2)) ; matches `quote' in addition to `foo'
>
> It would be nice if `cl-case' would signal an error or at least warn
> about these at macroexpansion time, since they are somewhat subtle and
> easy to get wrong.

An error from the second case would be nice, and a warning on the first
case, but the third case:

(macroexpand
 '(cl-case a
    ('foo (message "foo"))))
=> (cond ((cl-member a ''foo) (message "foo")))

and

(equal ''foo (list 'quote 'foo))

which, sure, whatever.  But I guess the question is whether we can
actually warn about that, because to the reader, the two forms are
equivalent? 

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

bug#51368: 29.0.50; `cl-case' should error on incorrect use

[Philipp Stephani]

Am So., 24. Okt. 2021 um 19:48 Uhr schrieb Lars Ingebrigtsen <larsi@gnus.org>:
>
> Philipp Stephani <p.stephani2@gmail.com> writes:
>
> > This form demonstrates a few incorrect uses of `cl-case':
> >
> > (cl-case a
> >   (nil 0)   ; doesn't match anything
> >   (t 1)     ; matches everything, but too early
> >   ('foo 2)) ; matches `quote' in addition to `foo'
> >
> > It would be nice if `cl-case' would signal an error or at least warn
> > about these at macroexpansion time, since they are somewhat subtle and
> > easy to get wrong.
>
> An error from the second case would be nice, and a warning on the first
> case, but the third case:
>
> (macroexpand
>  '(cl-case a
>     ('foo (message "foo"))))
> => (cond ((cl-member a ''foo) (message "foo")))
>
> and
>
> (equal ''foo (list 'quote 'foo))
>
> which, sure, whatever.  But I guess the question is whether we can
> actually warn about that, because to the reader, the two forms are
> equivalent?

Yes, but the problem only arises if the user wants to match the symbol
`quote' plus exactly one other value. That should already be
exceedingly rare, and can be trivially rewritten by swapping the two
values (i.e. write (foo quote) instead of (quote foo)). So I think
issuing a warning or error for that case is worth it.
Assuming we'd want to prevent similar bugs with other reader
constructs, I think the only real problematic case is matching exactly
two of the symbols `function', `quote', `,', `,@', and `\`'. Maybe for
those cases we should just instruct people to write (cond (memq ...))
or similar.

bug#51368: 29.0.50; `cl-case' should error on incorrect use

[Lars Ingebrigtsen]

Philipp Stephani <p.stephani2@gmail.com> writes:

> Yes, but the problem only arises if the user wants to match the symbol
> `quote' plus exactly one other value. That should already be
> exceedingly rare, and can be trivially rewritten by swapping the two
> values (i.e. write (foo quote) instead of (quote foo)). So I think
> issuing a warning or error for that case is worth it.

I think we're getting into slightly muddy waters, but I think I agree --
a warning here is (much more) likely to be helpful than not.

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

bug#51368: 29.0.50; `cl-case' should error on incorrect use

[Philipp Stephani]

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

Am Mo., 1. Nov. 2021 um 14:31 Uhr schrieb Lars Ingebrigtsen <larsi@gnus.org>:
>
> Philipp Stephani <p.stephani2@gmail.com> writes:
>
> > Yes, but the problem only arises if the user wants to match the symbol
> > `quote' plus exactly one other value. That should already be
> > exceedingly rare, and can be trivially rewritten by swapping the two
> > values (i.e. write (foo quote) instead of (quote foo)). So I think
> > issuing a warning or error for that case is worth it.
>
> I think we're getting into slightly muddy waters, but I think I agree --
> a warning here is (much more) likely to be helpful than not.
>

Ok, I've attached two patches that implement these suggestions.

[-- Attachment #2: 0001-Signal-an-error-if-a-fallback-case-is-misplaced-Bug-.patch --]
[-- Type: application/octet-stream, Size: 2463 bytes --]

From 9c629b85cecfd2845585d03c3bd7438b69686af9 Mon Sep 17 00:00:00 2001
From: Philipp Stephani <phst@google.com>
Date: Fri, 12 Nov 2021 20:26:06 +0100
Subject: [PATCH 1/2] Signal an error if a fallback case is misplaced
 (Bug#51368).

* lisp/emacs-lisp/cl-macs.el (cl-case): Signal an error if a t or
'otherwise' key doesn't come last.

* test/lisp/emacs-lisp/cl-macs-tests.el (cl-case-error): New unit
test.
---
 lisp/emacs-lisp/cl-macs.el            |  9 +++++++--
 test/lisp/emacs-lisp/cl-macs-tests.el | 11 +++++++++++
 2 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/lisp/emacs-lisp/cl-macs.el b/lisp/emacs-lisp/cl-macs.el
index 1852471bcb..61caa09009 100644
--- a/lisp/emacs-lisp/cl-macs.el
+++ b/lisp/emacs-lisp/cl-macs.el
@@ -776,11 +776,16 @@ cl-case
 \(fn EXPR (KEYLIST BODY...)...)"
   (declare (indent 1) (debug (form &rest (sexp body))))
   (macroexp-let2 macroexp-copyable-p temp expr
-    (let* ((head-list nil))
+    (let* ((head-list nil)
+           (has-otherwise nil))
       `(cond
         ,@(mapcar
            (lambda (c)
-             (cons (cond ((memq (car c) '(t otherwise)) t)
+             (cons (cond (has-otherwise
+                          (error "Misplaced t or `otherwise' clause"))
+                         ((memq (car c) '(t otherwise))
+                          (setq has-otherwise t)
+                          t)
                          ((eq (car c) 'cl--ecase-error-flag)
                           `(error "cl-ecase failed: %s, %s"
                                   ,temp ',(reverse head-list)))
diff --git a/test/lisp/emacs-lisp/cl-macs-tests.el b/test/lisp/emacs-lisp/cl-macs-tests.el
index 033764a7f9..68e29b32d9 100644
--- a/test/lisp/emacs-lisp/cl-macs-tests.el
+++ b/test/lisp/emacs-lisp/cl-macs-tests.el
@@ -673,4 +673,15 @@ cl-macs--progv
   (should (equal (cl-progv '(test1 test2) '(1 2) (list test1 test2))
                  '(1 2))))
 
+(ert-deftest cl-case-error ()
+  "Test that `cl-case' and `cl-ecase' signal an error if a t or
+`otherwise' key is misplaced."
+  (dolist (form '((cl-case val (t 1) (123 2))
+                  (cl-ecase val (t 1) (123 2))
+                  (cl-ecase val (123 2) (t 1))))
+    (ert-info ((prin1-to-string form) :prefix "Form: ")
+      (let ((error (should-error (macroexpand form))))
+        (should (equal (cdr error)
+                       '("Misplaced t or `otherwise' clause")))))))
+
 ;;; cl-macs-tests.el ends here
-- 
2.30.1 (Apple Git-130)


[-- Attachment #3: 0002-Have-cl-case-warn-about-suspicious-cases-Bug-51368.patch --]
[-- Type: application/octet-stream, Size: 4304 bytes --]

From 31c8a6d98222de7f7790f822e9ae22fe66addece Mon Sep 17 00:00:00 2001
From: Philipp Stephani <phst@google.com>
Date: Fri, 12 Nov 2021 20:28:23 +0100
Subject: [PATCH 2/2] Have 'cl-case' warn about suspicious cases (Bug#51368).

* lisp/emacs-lisp/cl-macs.el (cl-case): Warn if the user passes a nil
key list (which would never match).  Warn about quoted symbols that
should probably be unquoted.

* test/lisp/emacs-lisp/cl-macs-tests.el (cl-case-warning): New unit
test.
---
 lisp/emacs-lisp/cl-macs.el            | 15 +++++++++++++
 test/lisp/emacs-lisp/cl-macs-tests.el | 32 +++++++++++++++++++++++++++
 2 files changed, 47 insertions(+)

diff --git a/lisp/emacs-lisp/cl-macs.el b/lisp/emacs-lisp/cl-macs.el
index 61caa09009..c1681f37c5 100644
--- a/lisp/emacs-lisp/cl-macs.el
+++ b/lisp/emacs-lisp/cl-macs.el
@@ -789,6 +789,21 @@ cl-case
                          ((eq (car c) 'cl--ecase-error-flag)
                           `(error "cl-ecase failed: %s, %s"
                                   ,temp ',(reverse head-list)))
+                         ((null (car c))
+                          (macroexp-warn-and-return
+                           "Case nil will never match"
+                           nil 'suspicious))
+                         ((and (consp (car c)) (not (cddar c))
+                               (memq (caar c) '(quote function)))
+                          (macroexp-warn-and-return
+                           (format-message
+                            (concat "Case %s will match `%s'.  If "
+                                    "that's intended, write %s "
+                                    "instead.  Otherwise, don't "
+                                    "quote `%s'.")
+                            (car c) (caar c) (list (cadar c) (caar c))
+                            (cadar c))
+                           `(cl-member ,temp ',(car c)) 'suspicious))
                          ((listp (car c))
                           (setq head-list (append (car c) head-list))
                           `(cl-member ,temp ',(car c)))
diff --git a/test/lisp/emacs-lisp/cl-macs-tests.el b/test/lisp/emacs-lisp/cl-macs-tests.el
index 68e29b32d9..69d828709a 100644
--- a/test/lisp/emacs-lisp/cl-macs-tests.el
+++ b/test/lisp/emacs-lisp/cl-macs-tests.el
@@ -24,6 +24,8 @@
 (require 'cl-lib)
 (require 'cl-macs)
 (require 'ert)
+(require 'ert-x)
+(require 'pcase)
 
 \f
 ;;;; cl-loop tests -- many adapted from Steele's CLtL2
@@ -684,4 +686,34 @@ cl-case-error
         (should (equal (cdr error)
                        '("Misplaced t or `otherwise' clause")))))))
 
+(ert-deftest cl-case-warning ()
+  "Test that `cl-case' and `cl-ecase' warn about suspicious
+constructs."
+  (pcase-dolist (`(,case . ,message)
+                 `((nil . "Case nil will never match")
+                   ('nil . ,(concat "Case 'nil will match `quote'.  "
+                                    "If that's intended, write "
+                                    "(nil quote) instead.  "
+                                    "Otherwise, don't quote `nil'."))
+                   ('t . ,(concat "Case 't will match `quote'.  "
+                                  "If that's intended, write "
+                                  "(t quote) instead.  "
+                                  "Otherwise, don't quote `t'."))
+                   ('foo . ,(concat "Case 'foo will match `quote'.  "
+                                    "If that's intended, write "
+                                    "(foo quote) instead.  "
+                                    "Otherwise, don't quote `foo'."))
+                   (#'foo . ,(concat "Case #'foo will match "
+                                     "`function'.  If that's "
+                                     "intended, write (foo function) "
+                                     "instead.  Otherwise, don't "
+                                     "quote `foo'."))))
+    (dolist (macro '(cl-case cl-ecase))
+      (let ((form `(,macro val (,case 1))))
+        (ert-info ((prin1-to-string form) :prefix "Form: ")
+          (ert-with-message-capture messages
+            (macroexpand form)
+            (should (equal messages
+                           (concat "Warning: " message "\n")))))))))
+
 ;;; cl-macs-tests.el ends here
-- 
2.30.1 (Apple Git-130)

bug#51368: 29.0.50; `cl-case' should error on incorrect use

[Lars Ingebrigtsen]

Philipp Stephani <p.stephani2@gmail.com> writes:

> Ok, I've attached two patches that implement these suggestions.
>
> From 9c629b85cecfd2845585d03c3bd7438b69686af9 Mon Sep 17 00:00:00 2001
> From: Philipp Stephani <phst@google.com>
> Date: Fri, 12 Nov 2021 20:26:06 +0100
> Subject: [PATCH 1/2] Signal an error if a fallback case is misplaced
>  (Bug#51368).

Looks good to me -- I've added Stefan to the CCs; perhaps he has some
comments.

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

bug#51368: 29.0.50; `cl-case' should error on incorrect use

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

> Ok, I've attached two patches that implement these suggestions.

BTW, there's a third issue which is that

    (cl-case EXP
      ((a b) FOO)
      ((a c) BAR))

won't warn about the duplicate `a`.

After fixing this, many/most uses of `quote` will already emit
a warning, so maybe it would make the quote/function part of your second
patch unnecessary.

Personally, I just recommend the use of `pcase` over `cl-case` ;-)

BTW, I just noticed the use of the `suspicious` category of
warning messages.  Doesn't this category apply to all warnings?

Oh, and for the `nil` case and the presence of branches after
`otherwise`, the warnings could use the same text saying the branch
is unreachable.


        Stefan

bug#51368: 29.0.50; `cl-case' should error on incorrect use

[Lars Ingebrigtsen]

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

> BTW, there's a third issue which is that
>
>     (cl-case EXP
>       ((a b) FOO)
>       ((a c) BAR))
>
> won't warn about the duplicate `a`.
>
> After fixing this, many/most uses of `quote` will already emit
> a warning, so maybe it would make the quote/function part of your second
> patch unnecessary.

I think it's nice to have an explicit error case for the quote case so
that the warning can be better, though, so I've pushed Philipps patches
to Emacs 29.

But this test case should also emit a warning.  Philipp, you don't
happen to have a fix for that one, too?  😀

bug#51368: 29.0.50; `cl-case' should error on incorrect use

[Lars Ingebrigtsen]

I was surprised that these new tests didn't yield any new warnings.  :-)
But I'm doing a nativecomp bootstrap now, and:

../lisp/dnd.el: Warning: Case 'copy will match ‘quote’.  If that’s intended, write (copy quote) instead.  Otherwise, don’t quote ‘copy’.
../lisp/dnd.el: Warning: Case 'move will match ‘quote’.  If that’s intended, write (move quote) instead.  Otherwise, don’t quote ‘move’.
../lisp/dnd.el: Warning: Case 'copy will match ‘quote’.  If that’s intended, write (copy quote) instead.  Otherwise, don’t quote ‘copy’.
../lisp/dnd.el: Warning: Case 'move will match ‘quote’.  If that’s intended, write (move quote) instead.  Otherwise, don’t quote ‘move’.
../lisp/dnd.el: Warning: Case 'link will match ‘quote’.  If that’s intended, write (link quote) instead.  Otherwise, don’t quote ‘link’.
../lisp/dnd.el: Warning: Case 'copy will match ‘quote’.  If that’s intended, write (copy quote) instead.  Otherwise, don’t quote ‘copy’.
../lisp/dnd.el: Warning: Case 'move will match ‘quote’.  If that’s intended, write (move quote) instead.  Otherwise, don’t quote ‘move’.
../lisp/dnd.el: Warning: Case 'link will match ‘quote’.  If that’s intended, write (link quote) instead.  Otherwise, don’t quote ‘link’.
../lisp/progmodes/compile.el: Warning: Case 'if-location-known will match ‘quote’.  If that’s intended, write (if-location-known quote) instead.  Otherwise, don’t quote ‘if-location-known’.
../lisp/progmodes/compile.el: Warning: Case 'first-known will match
‘quote’.  If that’s intended, write

bug#51368: 29.0.50; `cl-case' should error on incorrect use

[Lars Ingebrigtsen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> I was surprised that these new tests didn't yield any new warnings.  :-)

Ah, nevermind -- I was testing the bootstrap on the wrong (un-updated)
machine.  Now I'm seeing the warnings in a non-nativecomp build, too.

bug#51368: 29.0.50; `cl-case' should error on incorrect use

[Göktuğ Kayaalp]

Hello,

I noticed the change associated with this bug
(6d8f5161ead689b7a2e44a7de0a695f0ab4c833b) today building Emacs and
testing it against my packages; it seems that it’s broken a few
packages, at least org-checklist and pdf-tools[1].

I usually go thru the diff of NEWS and the new commits when I build
Emacs but I doubt there’s a NEWS entry for this breaking change.

Is it possible that (i) a relevant NEWS entry is added and/or (ii) maybe
this error is made non-fatal, at least for a while?

[1] I’ll probably send patches to these packages, but I’m not sure if
org-checklist is maintained by anyone.

-- 
İ. Göktuğ Kayaalp / @cadadr / <https://www.gkayaalp.com/>
pgp:   024C 30DD 597D 142B 49AC 40EB 465C D949 B101 2427

bug#51368: 29.0.50; `cl-case' should error on incorrect use

[Stefan Kangas]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Stefan Monnier <monnier@iro.umontreal.ca> writes:
>
>> BTW, there's a third issue which is that
>>
>>     (cl-case EXP
>>       ((a b) FOO)
>>       ((a c) BAR))
>>
>> won't warn about the duplicate `a`.
>>
>> After fixing this, many/most uses of `quote` will already emit
>> a warning, so maybe it would make the quote/function part of your second
>> patch unnecessary.
>
> I think it's nice to have an explicit error case for the quote case so
> that the warning can be better, though, so I've pushed Philipps patches
> to Emacs 29.
>
> But this test case should also emit a warning.  Philipp, you don't
> happen to have a fix for that one, too?  😀

If I'm reading this right, it seems like the patches here were pushed to
master, but the bug was left open.  I'm therefore closing this bug report.

If this conclusion is incorrect, please reply to this email (use "Reply
to all" in your email client) and we can reopen the bug report.

bug#51368: 29.0.50; `cl-case' should error on incorrect use

[Mattias Engdegård]

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

>      (cl-case EXP
>        ((a b) FOO)
>        ((a c) BAR))
> 
>  won't warn about the duplicate `a`.

I was curious how common this could be so I locally added a warning to the back-end in bytecomp that generates switches (attached), and bootstrapped Emacs. Results:

----------------
In gnus-cloud-decode-data:
lisp/gnus/gnus-cloud.el:154:6: Warning: Duplicated value in ‘cond’: base64-gzip

In netrc-parse:
lisp/obsolete/netrc.el:106:22: Warning: Duplicated value in ‘cond’: "macdef"

In org-read-date-analyze:
lisp/org/org.el:14041:15: Warning: Duplicated value in ‘cond’: ""

In org-html-latex-fragment:
lisp/org/ox-html.el:3099:8: Warning: Duplicated value in ‘cond’: t
-----------------

None of these are from `cl-case`. (`pcase` doesn't typically warn either but just drops the duplicate silently.)

I probably won't keep the warning because it's in the back-end and was very much a hack.


[-- Attachment #2: bytecomp-cond-switch-dup-warn.diff --]
[-- Type: application/octet-stream, Size: 2094 bytes --]

diff --git a/lisp/emacs-lisp/bytecomp.el b/lisp/emacs-lisp/bytecomp.el
index 7feaf118b86..6bb95e7865b 100644
--- a/lisp/emacs-lisp/bytecomp.el
+++ b/lisp/emacs-lisp/bytecomp.el
@@ -4621,7 +4621,10 @@ byte-compile--cond-switch-prefix
                        (setq switch-var var)
                        (setq switch-test
                              (byte-compile--common-test switch-test fn))
-                       (unless (member value keys)
+                       (if (member value keys)
+                           (byte-compile-warn-x
+                            (car clauses)
+                            "Duplicated value in `cond': %S" value)
                          (push value keys)
                          (push (cons (list value) (or body '(t))) cases))
                        t))))
@@ -4632,7 +4635,10 @@ byte-compile--cond-switch-prefix
                      (setq switch-var var)
                      (setq switch-test
                            (byte-compile--common-test switch-test 'eq))
-                     (unless (memq nil keys)
+                     (if (memq nil keys)
+                         (byte-compile-warn-x
+                          (car clauses)
+                          "Duplicated value in `cond': nil")
                        (push nil keys)
                        (push (cons (list nil) (or body '(t))) cases))
                      t)))
@@ -4655,7 +4661,10 @@ byte-compile--cond-switch-prefix
                                                    (member . equal))))))
                             (let ((vals nil))
                               (dolist (elem value)
-                                (unless (funcall fn elem keys)
+                                (if (funcall fn elem keys)
+                                    (byte-compile-warn-x
+                                     (car clauses)
+                                     "Duplicated value in `cond': %S" elem)
                                   (push elem vals)))
                               (when vals
                                 (setq keys (append vals keys))

bug#51368: 29.0.50; `cl-case' should error on incorrect use

[Stefan Kangas]

Mattias Engdegård <mattias.engdegard@gmail.com> writes:

>>      (cl-case EXP
>>        ((a b) FOO)
>>        ((a c) BAR))
>>
>>  won't warn about the duplicate `a`.
>
> I was curious how common this could be so I locally added a warning to the back-end in bytecomp that generates switches (attached), and bootstrapped Emacs. Results:
>
> ----------------
> In gnus-cloud-decode-data:
> lisp/gnus/gnus-cloud.el:154:6: Warning: Duplicated value in ‘cond’: base64-gzip
>
> In netrc-parse:
> lisp/obsolete/netrc.el:106:22: Warning: Duplicated value in ‘cond’: "macdef"
>
> In org-read-date-analyze:
> lisp/org/org.el:14041:15: Warning: Duplicated value in ‘cond’: ""
>
> In org-html-latex-fragment:
> lisp/org/ox-html.el:3099:8: Warning: Duplicated value in ‘cond’: t
> -----------------

Thanks, I added FIXMEs for them on master.