Re: master 5ee4209f30: cl-typep: Emit warning when using a type not known to be a type

Re: master 5ee4209f30: cl-typep: Emit warning when using a type not known to be a type

[Basil L. Contovounesios]

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

Stefan Monnier [2022-06-06 00:04 -0400] wrote:

> branch: master
> commit 5ee4209f307fdf8cde9775539c9596d29edccd6d
> Author: Stefan Monnier <monnier@iro.umontreal.ca>
> Commit: Stefan Monnier <monnier@iro.umontreal.ca>
>
>     cl-typep: Emit warning when using a type not known to be a type
>     
>     `cl-typep` has used a heuristic that if there's a `<foo>-p` function,
>     then <foo> can be used as a type.  This made sense in the past where
>     most types were not officially declared to be (cl-)types, but nowadays
>     this just encourages abuses such as using `cl-typecase` with
>     "types" like `fbound`.  It's also a problem for EIEIO objects, where
>     for historical reasons `<foo>-p` tests if the object is of type
>     exactly `<foo>` whereas (cl-typep OBJ <foo>) should instead test
>     if OBJ is a *subtype* of `<foo>`.
>     
>     So we change `cl-typep` to emit a warning whenever this "-p" heuristic
>     is used, to discourage abuses, encourage the use of explicit
>     `cl-deftype` declarations, and try and detect some misuses of
>     `<foo>-p` for EIEIO objects.

[...]

I think this change gave rise to the following 'error: success' message:

0. emacs -Q -l cl-lib
1. (cl-typep (make-process :name "" :command '("true")) 'process)
2. C-j
   -| Warning: Unknown type: process
   => t

In practice, I see this when using Magit with Forge[0]; see the attached
backtrace which I obtained by adding a call to backtrace before
macroexp-warn-and-return in cl-typep.

Is the following a sufficient fix?


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: cl-typep.diff --]
[-- Type: text/x-diff, Size: 473 bytes --]

diff --git a/lisp/emacs-lisp/cl-macs.el b/lisp/emacs-lisp/cl-macs.el
index ada4f0344d..24a4cec4e2 100644
--- a/lisp/emacs-lisp/cl-macs.el
+++ b/lisp/emacs-lisp/cl-macs.el
@@ -3423,6 +3423,7 @@ cl--macroexp-fboundp
                  (null		. null)
                  (overlay	. overlayp)
                  (real		. numberp)
+                 (process	. processp)
                  (sequence	. sequencep)
                  (subr		. subrp)
                  (string	. stringp)

[-- Attachment #3: Type: text/plain, Size: 58 bytes --]


Thanks,

-- 
Basil

[0]: https://magit.vc/manual/forge/


[-- Attachment #4: forge-backtrace.txt --]
[-- Type: text/plain, Size: 9938 bytes --]

  backtrace()
  cl-typep(#<process emacsql-sqlite> process)
  eieio--perform-slot-validation(process #<process emacsql-sqlite>)
  eieio--validate-slot-value(#s(eieio--class :name forge-database :docstring nil :parents (#s(eieio--class :name emacsql-sqlite-connection :docstring nil :parents (#s(eieio--class :name emacsql-connection :docstring nil :parents nil :slots [#s(cl-slot-descriptor :name process :initform ... :type process :props nil) #s(cl-slot-descriptor :name log-buffer :initform nil :type ... :props ...) #s(cl-slot-descriptor :name finalizer :initform ... :type t :props ...)] :index-table #<hash-table eq 3/65 0x155a21148c23> :children (emacsql-sqlite-connection) :initarg-tuples ((:process . process) (:log-buffer . log-buffer)) :class-slots [#s(cl-slot-descriptor :name types :initform nil :type t :props ...)] :class-allocation-values [nil] :default-object-cache #<emacsql-connection emacsql-connection-155a21148cd6> :options (:custom-groups nil (:documentation "A connection to a SQL database.") :abstract t)) #s(eieio--class :name emacsql-protocol-mixin :docstring nil :parents nil :slots [] :index-table #<hash-table eq 0/65 0x155a211a00e5> :children (emacsql-sqlite-connection) :initarg-tuples nil :class-slots [] :class-allocation-values [] :default-object-cache #<emacsql-protocol-mixin emacsql-protocol-mixin-155a20e3d476> :options (:custom-groups nil (:documentation "A mixin for back-ends following the EmacSQL protoc...") :abstract t))) :slots [#s(cl-slot-descriptor :name process :initform 'eieio--unbound :type process :props nil) #s(cl-slot-descriptor :name log-buffer :initform nil :type (or null buffer) :props ((:documentation . "Output log (debug)."))) #s(cl-slot-descriptor :name finalizer :initform 'eieio--unbound :type t :props ((:documentation . "Object returned from `make-finalizer'."))) #s(cl-slot-descriptor :name file :initform 'eieio--unbound :type (or null string) :props ((:documentation . "Database file name.")))] :index-table #<hash-table eq 4/65 0x155a20fd0af3> :children (forge-database) :initarg-tuples ((:process . process) (:log-buffer . log-buffer) (:file . file)) :class-slots [#s(cl-slot-descriptor :name types :initform '(... ... ... ...) :type t :props ((:documentation . "Maps EmacSQL types to SQL types.")))] :class-allocation-values [((integer "INTEGER") (float "REAL") (object "TEXT") (nil nil))] :default-object-cache #<emacsql-sqlite-connection emacsql-sqlite-connection-155a20fd0b1a> :options (:custom-groups nil (:documentation "A connection to a SQLite database."))) #s(eieio--class :name closql-database :docstring nil :parents nil :slots [] :index-table #<hash-table eq 0/65 0x155a211b80ed> :children (forge-database) :initarg-tuples nil :class-slots [#s(cl-slot-descriptor :name object-class :initform 'eieio--unbound :type t :props nil)] :class-allocation-values [eieio--unbound] :default-object-cache #<closql-database closql-database-155a20f644ac> :options (:custom-groups nil :abstract t))) :slots [#s(cl-slot-descriptor :name process :initform 'eieio--unbound :type process :props nil) #s(cl-slot-descriptor :name log-buffer :initform nil :type (or null buffer) :props ((:documentation . "Output log (debug)."))) #s(cl-slot-descriptor :name finalizer :initform 'eieio--unbound :type t :props ((:documentation . "Object returned from `make-finalizer'."))) #s(cl-slot-descriptor :name file :initform 'eieio--unbound :type (or null string) :props ((:documentation . "Database file name.")))] :index-table #<hash-table eq 4/65 0x155a21291cfb> :children nil :initarg-tuples ((:process . process) (:log-buffer . log-buffer) (:file . file)) :class-slots [#s(cl-slot-descriptor :name object-class :initform 'forge-repository :type t :props nil) #s(cl-slot-descriptor :name types :initform '((integer "INTEGER") (float "REAL") (object "TEXT") (nil nil)) :type t :props ((:documentation . "Maps EmacSQL types to SQL types.")))] :class-allocation-values [forge-repository ((integer "INTEGER") (float "REAL") (object "TEXT") (nil nil))] :default-object-cache #<forge-database forge-database-155a21291f30> :options (:custom-groups nil)) 1 #<process emacsql-sqlite> process)
  #f(compiled-function (obj slot value) "Do the work for the macro `oset'.\nFills in OBJ's SLOT with VALUE." #<bytecode -0x543acd812ff640f>)(#<forge-database forge-database-155a214261e6> process #<process emacsql-sqlite>)
  eieio-oset--closql-oset(#f(compiled-function (obj slot value) "Do the work for the macro `oset'.\nFills in OBJ's SLOT with VALUE." #<bytecode -0x543acd812ff640f>) #<forge-database forge-database-155a214261e6> process #<process emacsql-sqlite>)
  apply(eieio-oset--closql-oset #f(compiled-function (obj slot value) "Do the work for the macro `oset'.\nFills in OBJ's SLOT with VALUE." #<bytecode -0x543acd812ff640f>) (#<forge-database forge-database-155a214261e6> process #<process emacsql-sqlite>))
  eieio-oset(#<forge-database forge-database-155a214261e6> process #<process emacsql-sqlite>)
  #f(compiled-function (#<forge-database forge-database-155a214261e6> (:file "/home/blc/.emacs.d/index/forge-db.sqlite"))
  apply(#f(compiled-function 
  #f(compiled-function (&rest args) #<bytecode 0xbfd896ae1e94673>)(#<forge-database forge-database-155a214261e6> (:file "/home/blc/.emacs.d/index/forge-db.sqlite"))
  apply(#f(compiled-function (&rest args) #<bytecode 0xbfd896ae1e94673>) #<forge-database forge-database-155a214261e6> (:file "/home/blc/.emacs.d/index/forge-db.sqlite"))
  initialize-instance(#<forge-database forge-database-155a214261e6> (:file "/home/blc/.emacs.d/index/forge-db.sqlite"))
  #f(compiled-function (class &rest slots) "Default constructor for CLASS `eieio-default-superclass'.\nSLOTS are the initialization slots used by `initialize-instance'.\nThis static method is called when an object is constructed.\nIt allocates the vector used to represent an EIEIO object, and then\ncalls `initialize-instance' on that object." #<bytecode 0x1119482c872af6de>)(forge-database :file "/home/blc/.emacs.d/index/forge-db.sqlite")
  apply(#f(compiled-function (class &rest slots) "Default constructor for CLASS `eieio-default-superclass'.\nSLOTS are the initialization slots used by `initialize-instance'.\nThis static method is called when an object is constructed.\nIt allocates the vector used to represent an EIEIO object, and then\ncalls `initialize-instance' on that object." #<bytecode 0x1119482c872af6de>) forge-database (:file "/home/blc/.emacs.d/index/forge-db.sqlite"))
  make-instance(forge-database :file "/home/blc/.emacs.d/index/forge-db.sqlite")
  #f(compiled-function (class &optional variable file debug) #<bytecode 0x1cb794e473836dcf>)(forge-database forge--db-connection "/home/blc/.emacs.d/index/forge-db.sqlite" t)
  apply(#f(compiled-function (class &optional variable file debug) #<bytecode 0x1cb794e473836dcf>) forge-database (forge--db-connection "/home/blc/.emacs.d/index/forge-db.sqlite" t))
  closql-db(forge-database forge--db-connection "/home/blc/.emacs.d/index/forge-db.sqlite" t)
  forge-db()
  forge-sql([:select * :from repository :where (and (= forge $s1) (= owner $s2) (= name $s3))] "github.com" "magit" "magit")
  #f(compiled-function (&rest rest) "((host owner name) &optional remote demand)\n\nReturn the repository identified by HOST, OWNER and NAME." #<bytecode 0x108f7016f1000322>)(("github.com" "magit" "magit") "upstream" full)
  apply(#f(compiled-function (&rest rest) "((host owner name) &optional remote demand)\n\nReturn the repository identified by HOST, OWNER and NAME." #<bytecode 0x108f7016f1000322>) ("github.com" "magit" "magit") ("upstream" full))
  forge-get-repository(("github.com" "magit" "magit") "upstream" full)
  #f(compiled-function (url &optional remote demand) "Return the repository at URL." #<bytecode -0x14605caa7efa701e>)("https://github.com/magit/magit.git" "upstream" full)
  apply(#f(compiled-function (url &optional remote demand) "Return the repository at URL." #<bytecode -0x14605caa7efa701e>) "https://github.com/magit/magit.git" ("upstream" full))
  forge-get-repository("https://github.com/magit/magit.git" "upstream" full)
  #f(compiled-function (demand &optional remote) "Return the current forge repository.\n\nIf the `forge-buffer-repository' is non-nil, then return that.\nOtherwise if `forge-buffer-topic' is non-nil, then return the\nrepository for that.  Finally if both variables are nil, then\nreturn the forge repository corresponding to the current Git\nrepository, if any." #<bytecode 0x12412decadefb3e5>)(full)
  apply(#f(compiled-function (demand &optional remote) "Return the current forge repository.\n\nIf the `forge-buffer-repository' is non-nil, then return that.\nOtherwise if `forge-buffer-topic' is non-nil, then return the\nrepository for that.  Finally if both variables are nil, then\nreturn the forge repository corresponding to the current Git\nrepository, if any." #<bytecode 0x12412decadefb3e5>) full nil)
  forge-get-repository(full)
  forge-bug-reference-setup()
  run-hooks(change-major-mode-after-body-hook special-mode-hook magit-section-mode-hook magit-mode-hook magit-status-mode-hook)
  apply(run-hooks (change-major-mode-after-body-hook special-mode-hook magit-section-mode-hook magit-mode-hook magit-status-mode-hook))
  run-mode-hooks(magit-status-mode-hook)
  magit-status-mode()
  magit-setup-buffer-internal(magit-status-mode nil ((magit-buffer-diff-args ("--no-ext-diff")) (magit-buffer-diff-files nil) (magit-buffer-log-args ("-n64" "--show-signature" "-n256" "--decorate")) (magit-buffer-log-files nil)))
  magit-status-setup-buffer("~/.emacs.d/src/magit/")
  magit-project-status()
  funcall-interactively(magit-project-status)
  call-interactively(magit-project-status)
  project-switch-project("~/.emacs.d/src/magit/")
  funcall-interactively(project-switch-project "~/.emacs.d/src/magit/")
  call-interactively(project-switch-project nil nil)
  command-execute(project-switch-project)

Re: master 5ee4209f30: cl-typep: Emit warning when using a type not known to be a type

[Basil L. Contovounesios]

Basil L. Contovounesios [2022-06-11 15:30 +0300] wrote:

> Is the following a sufficient fix?
>
> diff --git a/lisp/emacs-lisp/cl-macs.el b/lisp/emacs-lisp/cl-macs.el
> index ada4f0344d..24a4cec4e2 100644
> --- a/lisp/emacs-lisp/cl-macs.el
> +++ b/lisp/emacs-lisp/cl-macs.el
> @@ -3423,6 +3423,7 @@ cl--macroexp-fboundp
>                   (null		. null)
>                   (overlay	. overlayp)
>                   (real		. numberp)
> +                 (process	. processp)
>                   (sequence	. sequencep)
>                   (subr		. subrp)
>                   (string	. stringp)

[ I'll submit the patch to move 'p' after 'r' in the English alphabet
  separately. ]

-- 
Basil

Re: master 5ee4209f30: cl-typep: Emit warning when using a type not known to be a type

[Stefan Monnier]

Basil L. Contovounesios [2022-06-11 15:49:14] wrote:
> Basil L. Contovounesios [2022-06-11 15:30 +0300] wrote:
>> Is the following a sufficient fix?

Looks good, yes.

>> diff --git a/lisp/emacs-lisp/cl-macs.el b/lisp/emacs-lisp/cl-macs.el
>> index ada4f0344d..24a4cec4e2 100644
>> --- a/lisp/emacs-lisp/cl-macs.el
>> +++ b/lisp/emacs-lisp/cl-macs.el
>> @@ -3423,6 +3423,7 @@ cl--macroexp-fboundp
>>                   (null		. null)
>>                   (overlay	. overlayp)
>>                   (real		. numberp)
>> +                 (process	. processp)
>>                   (sequence	. sequencep)
>>                   (subr		. subrp)
>>                   (string	. stringp)
>
> [ I'll submit the patch to move 'p' after 'r' in the English alphabet
>   separately. ]

That's long overdue, indeed.


        Stefan

Re: master 5ee4209f30: cl-typep: Emit warning when using a type not known to be a type

[Basil L. Contovounesios]

Stefan Monnier [2022-06-11 11:13 -0400] wrote:

> Basil L. Contovounesios [2022-06-11 15:49:14] wrote:
>> Basil L. Contovounesios [2022-06-11 15:30 +0300] wrote:
>>> Is the following a sufficient fix?
>
> Looks good, yes.

Thanks, pushed:

Recognize processes as a CL type again
e53428994e 2022-06-11 19:21:55 +0300
https://git.sv.gnu.org/cgit/emacs.git/commit/?id=e53428994e

>>> diff --git a/lisp/emacs-lisp/cl-macs.el b/lisp/emacs-lisp/cl-macs.el
>>> index ada4f0344d..24a4cec4e2 100644
>>> --- a/lisp/emacs-lisp/cl-macs.el
>>> +++ b/lisp/emacs-lisp/cl-macs.el
>>> @@ -3423,6 +3423,7 @@ cl--macroexp-fboundp
>>>                   (null		. null)
>>>                   (overlay	. overlayp)
>>>                   (real		. numberp)
>>> +                 (process	. processp)
>>>                   (sequence	. sequencep)
>>>                   (subr		. subrp)
>>>                   (string	. stringp)
>>
>> [ I'll submit the patch to move 'p' after 'r' in the English alphabet
>>   separately. ]
>
> That's long overdue, indeed.

Turns out my CA doesn't cover that, so it'll be a bit overduer.

-- 
Basil