Re: [PATCH] pcase.el: Add type pattern

[Adam Porter <adam@alphapapa.net>]

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

Michael Heerdegen <michael_heerdegen@web.de> writes:

> Adam Porter <adam@alphapapa.net> writes:
>
>> +(pcase-defmacro type (type)
>> +  "Pcase pattern that matches objects of TYPE.
>> +TYPE is a symbol or list as accepted by `cl-typep', which see."
>> +  `(pred (pcase--flip cl-typep ',type)))
>> +
>
> Is there a reason why you omit the (require 'cl-lib) call in the
> definition as suggested by Stefan?

My understanding was that adding the (require 'cl-lib) form was
suggested if the definition was left in pcase.el, and if the definition
were added to cl-macs.el instead, the (require 'cl-lib) form would not
be needed.  This made sense to me, because cl-macs.el has (require
'cl-lib) at the top of the file.  Of course, I may have misunderstood,
and there are nuances to autoloading and such that I don't fully
understand.

> With your definition in emacs -Q:
>
> (macroexpand-all '(pcase 10 ((type (integer 0 10)) t))) =>
>   (if (cl-typep 10 '(integer 0 10)) (progn t) nil)
>
> With Stefan's version:
>
> (macroexpand-all '(pcase 10 ((type (integer 0 10)) t))) =>
>   (if (and (integerp 10) (>= 10 '0) (<= 10 '10)) (progn t) nil)
>
> I.e. the type expression is treated at compile time and the dependency
> to cl-lib is only at compile time, the compiled code has no dependency.
> That should be better, no?

Of course, that would be better.  However, I was unable to reproduce
those results on my end.  Here are the steps I followed, both on my
main Emacs 26.3 installation and on a build from near master:

1.  Run emacs -Q.
2.  Evaluate this form in *scratch*:

  (pcase-defmacro type (type)
    "Pcase pattern that matches objects of TYPE.
  TYPE is a symbol or list as accepted by `cl-typep', which see."
    `(pred (pcase--flip cl-typep ',type)))

3.  Evaluate this form in *scratch*:

  (macroexpand-all '(pcase 10 ((type (integer 0 10)) t)))

Result:

  (if (and (integerp 10) (>= 10 (quote 0)) (<= 10 (quote 10))) (progn t)
  nil)

I haven't done much work on Emacs's core files before, so I may be doing
something wrong.  Please let me know.  :)

However, I have attached another version of the patch which adds an
autoload for the (pcase-defmacro type) form, which seems proper since
the (pcase-defmacro cl-struct) form in the same file also has one.

Thanks for your help and feedback.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Add pcase type pattern, etc. --]
[-- Type: text/x-diff, Size: 3141 bytes --]

From 3ab1eae0e71a495bb4e86012ad772146454aa4ff Mon Sep 17 00:00:00 2001
From: Adam Porter <adam@alphapapa.net>
Date: Mon, 9 Mar 2020 13:01:32 -0500
Subject: [PATCH] * lisp/emacs-lisp/cl-macs.el: Add type pattern

* lisp/emacs-lisp/cl-macs.el:
((pcase-defmacro type)): Add 'type' pattern.

* test/lisp/emacs-lisp/pcase-tests.el (pcase-tests-type): Add test.

* doc/lispref/control.texi (pcase Macro): Update manual.

Co-authored-by: Stefan Monnier <monnier@iro.umontreal.ca>
---
 doc/lispref/control.texi            |  7 +++++++
 etc/NEWS                            |  8 ++++++++
 lisp/emacs-lisp/cl-macs.el          |  8 ++++++++
 test/lisp/emacs-lisp/pcase-tests.el | 10 ++++++++++
 4 files changed, 33 insertions(+)

diff --git a/doc/lispref/control.texi b/doc/lispref/control.texi
index c601e3a..dc91fc1 100644
--- a/doc/lispref/control.texi
+++ b/doc/lispref/control.texi
@@ -555,6 +555,13 @@ pcase Macro
 Likewise, it makes no sense to bind keyword symbols
 (@pxref{Constant Variables}).
 
+@item (type @var{type})
+Matches if @var{expval} is of type @var{type}, which is a symbol or
+list as accepted by @ref{cl-typep,,,cl,Common Lisp Extensions}.
+
+Example: @code{(type integer)}
+Example: @code{(type (integer 0 10))}
+
 @item (pred @var{function})
 Matches if the predicate @var{function} returns non-@code{nil}
 when called on @var{expval}.
diff --git a/etc/NEWS b/etc/NEWS
index 47b87af..c8227c0 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -174,6 +174,14 @@ key             binding
 / v             package-menu-filter-by-version
 / /             package-menu-filter-clear
 
+** pcase.el
+
+*** Added Pcase 'type' pattern.
+
+The new 'type' pattern compares types using 'cl-typep', which allows
+comparing simple types like '(type integer)', as well as forms like
+'(type (integer 0 10))'.
+
 \f
 * New Modes and Packages in Emacs 28.1
 
diff --git a/lisp/emacs-lisp/cl-macs.el b/lisp/emacs-lisp/cl-macs.el
index 4c2f589..556ffb3 100644
--- a/lisp/emacs-lisp/cl-macs.el
+++ b/lisp/emacs-lisp/cl-macs.el
@@ -3403,6 +3403,14 @@ cl-struct-slot-value
 
 (run-hooks 'cl-macs-load-hook)
 
+;;; Pcase type pattern.
+
+;;;###autoload
+(pcase-defmacro type (type)
+  "Pcase pattern that matches objects of TYPE.
+TYPE is a symbol or list as accepted by `cl-typep', which see."
+  `(pred (pcase--flip cl-typep ',type)))
+
 ;; Local variables:
 ;; generated-autoload-file: "cl-loaddefs.el"
 ;; End:
diff --git a/test/lisp/emacs-lisp/pcase-tests.el b/test/lisp/emacs-lisp/pcase-tests.el
index 0b69bd9..97f3e55 100644
--- a/test/lisp/emacs-lisp/pcase-tests.el
+++ b/test/lisp/emacs-lisp/pcase-tests.el
@@ -71,6 +71,16 @@ pcase-tests-member
 (ert-deftest pcase-tests-vectors ()
   (should (equal (pcase [1 2] (`[,x] 1) (`[,x ,y] (+ x y))) 3)))
 
+(ert-deftest pcase-tests-type ()
+  (should (equal (pcase 1
+                   ((type integer) 'integer))
+                 'integer))
+  (should (equal (pcase 1
+                   ((type (integer 0 2)) 'integer-0<=n<=2))
+                 'integer-0<=n<=2))
+  (should-error (pcase 1
+                  ((type notatype) 'integer))))
+
 ;; Local Variables:
 ;; no-byte-compile: t
 ;; End:
-- 
2.7.4