bug#30990: Should the byte compiler warn about :type mismatches?

bug#30990: Should the byte compiler warn about :type mismatches?

[Alex Branham]

severity: wishlist

Would it be possible/desirable to make the byte-compiler warn about
:type mismatches with defcustoms? I've noticed that the default values
for quite a few packages I use (both in Emacs [Bug#30901 for example]
and not) do not match. Presumably this would be easier to catch if the
compiler warned about it.

In case I'm not being clear, writing something like this:

(defcustom my-var t
  "Docs."
  :group 'text
  :type 'string)

Would result in a warning during byte compilation along the lines of:

"Warning: Default value for 'my-var' does not match :type".

bug#30990: Should the byte compiler warn about :type mismatches?

[Glenn Morris]

Alex Branham wrote:

> Would it be possible/desirable to make the byte-compiler warn about
> :type mismatches with defcustoms?

I think that to do this in general requires evaluating the defcustom at
compile time, which probably isn't the right thing.

emacs -batch -l admin/cus-test.el -f cus-test-opts all

will find these mismatches. I think it should be converted to an
(expensive) ert test, which would solve the problem for Emacs itself.

bug#30990: Should the byte compiler warn about :type mismatches?

[Stefan Kangas]

Glenn Morris <rgm@gnu.org> writes:

> Alex Branham wrote:
>
>> Would it be possible/desirable to make the byte-compiler warn about
>> :type mismatches with defcustoms?
>
> I think that to do this in general requires evaluating the defcustom at
> compile time, which probably isn't the right thing.
>
> emacs -batch -l admin/cus-test.el -f cus-test-opts all
>
> will find these mismatches. I think it should be converted to an
> (expensive) ert test, which would solve the problem for Emacs itself.

I think that's a good idea.

Running the above command in my tree, I get:

The following options might have problems:
sql-password-wallet
tramp-default-host-alist
display-fill-column-indicator-character
bibtex-string-file-path
js-jsx-indent-level
bibtex-file-path
sql-use-indent-support
sql-postgres-login-params
winner-boring-buffers-regexp
add-log-dont-create-changelog-file
nnir-notmuch-filter-group-names-function
flymake-cc-command

Since we're getting closer to the release of Emacs 27.1, perhaps we
should sort these out.

Best regards,
Stefan Kangas

bug#30990: Should the byte compiler warn about :type mismatches?

[Eli Zaretskii]

> From: Stefan Kangas <stefan@marxist.se>
> Date: Sat, 28 Sep 2019 21:00:22 +0200
> Cc: Alex Branham <alex.branham@gmail.com>, 30990@debbugs.gnu.org
> 
> The following options might have problems:
> sql-password-wallet
> tramp-default-host-alist
> display-fill-column-indicator-character
> bibtex-string-file-path
> js-jsx-indent-level
> bibtex-file-path
> sql-use-indent-support
> sql-postgres-login-params
> winner-boring-buffers-regexp
> add-log-dont-create-changelog-file
> nnir-notmuch-filter-group-names-function
> flymake-cc-command
> 
> Since we're getting closer to the release of Emacs 27.1, perhaps we
> should sort these out.

We should, but bugs like this can be fixed even after the emacs-27
branch is cut.

bug#30990: Should the byte compiler warn about :type mismatches?

[Robert Pluim]

>>>>> On Sat, 28 Sep 2019 22:21:55 +0300, Eli Zaretskii <eliz@gnu.org> said:

    >> From: Stefan Kangas <stefan@marxist.se>
    >> Date: Sat, 28 Sep 2019 21:00:22 +0200
    >> Cc: Alex Branham <alex.branham@gmail.com>, 30990@debbugs.gnu.org
    >> 
    >> The following options might have problems:
    >> sql-password-wallet
    >> tramp-default-host-alist
    >> display-fill-column-indicator-character
    >> bibtex-string-file-path
    >> js-jsx-indent-level
    >> bibtex-file-path
    >> sql-use-indent-support
    >> sql-postgres-login-params
    >> winner-boring-buffers-regexp
    >> add-log-dont-create-changelog-file
    >> nnir-notmuch-filter-group-names-function
    >> flymake-cc-command
    >> 
    >> Since we're getting closer to the release of Emacs 27.1, perhaps we
    >> should sort these out.

    Eli> We should, but bugs like this can be fixed even after the emacs-27
    Eli> branch is cut.

I took a quick look, most of them are because the starting value is
nil, and the type is something that canʼt be nil, which means the type
should actually be something like

(choice string (const nil))

an example, where nil here means "donʼt do any checking":

diff --git a/lisp/winner.el b/lisp/winner.el
index ec3b296489..0e5bd90966 100644
--- a/lisp/winner.el
+++ b/lisp/winner.el
@@ -71,7 +71,8 @@ winner-boring-buffers
 
 (defcustom winner-boring-buffers-regexp nil
   "`winner-undo' will not restore windows with buffers matching this regexp."
-  :type 'string
+  :type '(choice (string :tag "Regexp")
+                 (const :tag "Don't check" nil))
   :version "27.1")

Iʼm not sure those are worth fixing.

I did spot what seem to be some genuine errors though (hmm, would
flymake-cc-command need a :version tag update as well? The existing
definition makes customize error out)

diff --git i/lisp/image.el w/lisp/image.el
index eaa6ed3b0e..e44330fdfa 100644
--- i/lisp/image.el
+++ w/lisp/image.el
@@ -148,7 +148,7 @@ image-use-external-converter
 support in Emacs can still be displayed if an external conversion
 program (like ImageMagick \"convert\", GraphicsMagick \"gm\"
 or \"ffmpeg\") is installed."
-  :type 'bool
+  :type 'boolean
   :version "27.1")
 
 ;; Map put into text properties on images.
diff --git i/lisp/progmodes/flymake-cc.el w/lisp/progmodes/flymake-cc.el
index ecf6e648a7..7b36fcf06d 100644
--- i/lisp/progmodes/flymake-cc.el
+++ w/lisp/progmodes/flymake-cc.el
@@ -37,7 +37,7 @@ flymake-cc-command
 input and prints the result on its standard output."
   :type '(choice
           (symbol :tag "Function")
-          ((repeat :) string))
+          (repeat :tag "Command(s)" string))
   :group 'flymake-cc)
 
 (defun flymake-cc--make-diagnostics (source)
diff --git i/lisp/progmodes/sql.el w/lisp/progmodes/sql.el
index 2d33b3130c..0e14a6e4b2 100644
--- i/lisp/progmodes/sql.el
+++ w/lisp/progmodes/sql.el
@@ -737,7 +737,7 @@ sql-use-indent-support
 The package must be available to be loaded and activated."
   :group 'SQL
   :link '(url-link "https://elpa.gnu.org/packages/sql-indent.html")
-  :type 'booleanp
+  :type 'boolean
   :version "27.1")
 
 (defun sql-indent-enable ()
diff --git i/lisp/vc/add-log.el w/lisp/vc/add-log.el
index 47a68167fb..5c27a65ea1 100644
--- i/lisp/vc/add-log.el
+++ w/lisp/vc/add-log.el
@@ -811,7 +811,7 @@ add-log-dont-create-changelog-file
   "If non-nil, don't create ChangeLog files for log entries.
 If a ChangeLog file does not already exist, a non-nil value
 means to put log entries in a suitably named buffer."
-  :type :boolean
+  :type 'boolean
   :version "27.1")
 
 (put 'add-log-dont-create-changelog-file 'safe-local-variable 'booleanp)

bug#30990: Should the byte compiler warn about :type mismatches?

[Lars Ingebrigtsen]

Robert Pluim <rpluim@gmail.com> writes:

>  (defcustom winner-boring-buffers-regexp nil
>    "`winner-undo' will not restore windows with buffers matching this regexp."
> -  :type 'string
> +  :type '(choice (string :tag "Regexp")
> +                 (const :tag "Don't check" nil))
>    :version "27.1")
>
> Iʼm not sure those are worth fixing.

I think fixing those would be a win.

> I did spot what seem to be some genuine errors though (hmm, would
> flymake-cc-command need a :version tag update as well? The existing
> definition makes customize error out)

[...]

> -  :type 'bool
> +  :type 'boolean

etc.  Looks good to me; please push.

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

bug#30990: Should the byte compiler warn about :type mismatches?

[Robert Pluim]

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

>>>>> On Mon, 30 Sep 2019 17:45:39 +0200, Lars Ingebrigtsen <larsi@gnus.org> said:

    Lars> Robert Pluim <rpluim@gmail.com> writes:
    >> (defcustom winner-boring-buffers-regexp nil
    >> "`winner-undo' will not restore windows with buffers matching this regexp."
    >> -  :type 'string
    >> +  :type '(choice (string :tag "Regexp")
    >> +                 (const :tag "Don't check" nil))
    >> :version "27.1")
    >> 
    >> Iʼm not sure those are worth fixing.

    Lars> I think fixing those would be a win.

Hmm. Current attempt attached. Iʼm having trouble with
'sql-postgres-login-params', and 'sql-password-wallet', probably
because I donʼt know enough about custom, suggestions welcome.

    >> I did spot what seem to be some genuine errors though (hmm, would
    >> flymake-cc-command need a :version tag update as well? The existing
    >> definition makes customize error out)

    Lars> [...]

    >> -  :type 'bool
    >> +  :type 'boolean

    Lars> etc.  Looks good to me; please push.

Done. One more, which I missed because itʼs not defined from lisp. Iʼm
not so sure about mentioning U+2502 in the tag, since I have a
vague memory that nil actually means 'autodetect if the frame can
display U+2502'.

diff --git a/lisp/cus-start.el b/lisp/cus-start.el
index 15d33b43c0..6b8fa0c6c9 100644
--- a/lisp/cus-start.el
+++ b/lisp/cus-start.el
@@ -671,9 +671,11 @@ minibuffer-prompt-properties--setter
               :safe (lambda (value) (or (booleanp value) (integerp value))))
              (display-fill-column-indicator-character
               display-fill-column-indicator
-              character
+              (choice
+               (const :tag "Default (U+2502)" nil)
+               (character :tag "Character" ))
               "27.1"
-              :safe characterp)
+              :safe (lambda (value) (or (characterp value) (null value))))
 	     ;; xfaces.c
 	     (scalable-fonts-allowed display boolean "22.1")
 	     ;; xfns.c



[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Correct-some-more-custom-type-specs.patch --]
[-- Type: text/x-patch, Size: 3901 bytes --]

From 067250e0d5addcd8b7ee9072b3eae66c0c53d7a4 Mon Sep 17 00:00:00 2001
From: Robert Pluim <rpluim@gmail.com>
Date: Wed, 9 Oct 2019 15:28:47 +0200
Subject: [PATCH] Correct some more custom type specs
To: emacs-devel@gnu.org

* lisp/winner.el (winner-boring-buffers-regexp):
* lisp/progmodes/js.el (js-jsx-indent-level):
* lisp/image-dired.el (image-dired-external-viewer):
* lisp/gnus/nnir.el (nnir-notmuch-filter-group-names-function):
Correct custom type specification.

* lisp/textmodes/bibtex.el (bibtex-string-file-path):
(bibtex-file-path): Correct custom type specification and document
source of initial value.
---
 lisp/gnus/nnir.el        |  2 +-
 lisp/image-dired.el      |  4 +++-
 lisp/progmodes/js.el     |  3 ++-
 lisp/textmodes/bibtex.el | 14 ++++++++++----
 lisp/winner.el           |  3 ++-
 5 files changed, 18 insertions(+), 8 deletions(-)

diff --git a/lisp/gnus/nnir.el b/lisp/gnus/nnir.el
index 1041373a05..7cb2d1615a 100644
--- a/lisp/gnus/nnir.el
+++ b/lisp/gnus/nnir.el
@@ -535,7 +535,7 @@ nnir-notmuch-filter-group-names-function
    (lambda (g) (replace-regexp-in-string \"\\\\.\" \"/\" g)))"
   :version "27.1"
   :type '(choice function
-		 nil))
+		 (const :tag "No" nil)))
 
 ;;; Developer Extension Variable:
 
diff --git a/lisp/image-dired.el b/lisp/image-dired.el
index c1c767ba78..85bc924794 100644
--- a/lisp/image-dired.el
+++ b/lisp/image-dired.el
@@ -538,7 +538,9 @@ image-dired-external-viewer
   "Name of external viewer.
 Including parameters.  Used when displaying original image from
 `image-dired-thumbnail-mode'."
-  :type 'string
+  :version "27.1"
+  :type '(choice string
+                 (const :tag "Not Set" nil))
   :group 'image-dired)
 
 (defcustom image-dired-main-image-directory "~/pics/"
diff --git a/lisp/progmodes/js.el b/lisp/progmodes/js.el
index 3050e8f1a7..8d457a28ec 100644
--- a/lisp/progmodes/js.el
+++ b/lisp/progmodes/js.el
@@ -650,7 +650,8 @@ js-jsx-indent-level
       </element>
   )"
   :version "27.1"
-  :type 'integer
+  :type '(choice integer
+                 (const :tag "Not Set" nil))
   :safe (lambda (x) (or (null x) (integerp x)))
   :group 'js)
 ;; This is how indentation behaved out-of-the-box until Emacs 27.  JSX
diff --git a/lisp/textmodes/bibtex.el b/lisp/textmodes/bibtex.el
index 7e99032084..c29e963180 100644
--- a/lisp/textmodes/bibtex.el
+++ b/lisp/textmodes/bibtex.el
@@ -933,9 +933,12 @@ bibtex-string-files
   :type '(repeat file))
 
 (defcustom bibtex-string-file-path (getenv "BIBINPUTS")
-  "Colon-separated list of paths to search for `bibtex-string-files'."
+  "Colon-separated list of paths to search for `bibtex-string-files'.
+Initialized from the BIBINPUTS environment variable."
   :group 'bibtex
-  :type 'string)
+  :version "27.1"
+  :type '(choice string
+                 (const :tag "Not Set" nil)))
 
 (defcustom bibtex-files nil
   "List of BibTeX files that are searched for entry keys.
@@ -949,9 +952,12 @@ bibtex-files
                          directory file)))
 
 (defcustom bibtex-file-path (getenv "BIBINPUTS")
-  "Colon separated list of paths to search for `bibtex-files'."
+  "Colon separated list of paths to search for `bibtex-files'.
+Initialized from the BIBINPUTS environment variable."
   :group 'bibtex
-  :type 'string)
+  :version "27.1"
+  :type '(choice string
+                 (const :tag "Not Set" nil)))
 
 (defcustom bibtex-search-entry-globally nil
   "If non-nil, interactive calls of `bibtex-search-entry' search globally.
diff --git a/lisp/winner.el b/lisp/winner.el
index dc8bde5331..fea5211d90 100644
--- a/lisp/winner.el
+++ b/lisp/winner.el
@@ -61,7 +61,8 @@ winner-boring-buffers
 
 (defcustom winner-boring-buffers-regexp nil
   "`winner-undo' will not restore windows with buffers matching this regexp."
-  :type 'string
+  :type '(choice (string :tag "Regexp")
+                 (const :tag "Not Set" nil))
   :version "27.1")
 
 \f
-- 
2.23.0

bug#30990: Should the byte compiler warn about :type mismatches?

[Lars Ingebrigtsen]

Robert Pluim <rpluim@gmail.com> writes:

> Hmm. Current attempt attached.

Looks good to me.

> Iʼm having trouble with
> 'sql-postgres-login-params', and 'sql-password-wallet', probably
> because I donʼt know enough about custom, suggestions welcome.

The first one should probably not be a defcustom at all, but perhaps
just...

:type 'sexp

The second looks like a bug:

(defcustom sql-password-wallet
  (let (wallet w)
    (dolist (ext '(".json.gpg" ".gpg" ".json" "") wallet)
      (unless wallet
        (setq w (locate-user-emacs-file (concat "sql-wallet" ext)
                                        (concat ".sql-wallet" ext)))
        (when (file-exists-p w)
          (setq wallet w)))))

It will always be nil, won't it?  My guess would be that the intention
was that this should be

(defcustom sql-password-wallet
  (let (wallet w)
    (dolist (ext '(".json.gpg" ".gpg" ".json" "") wallet)
      (unless wallet
        (setq w (locate-user-emacs-file (concat "sql-wallet" ext)
                                        (concat ".sql-wallet" ext)))
        (when (file-exists-p w)
          (setq wallet w))))
    wallet)

but I'm not familiar with this at all.  But if that's the case the type
should be a choice between nil and `file'.

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

bug#30990: Should the byte compiler warn about :type mismatches?

[Mattias Engdegård]

>- :type 'string
>+ :type '(choice (string :tag "Regexp")
>+                (const :tag "Not Set" nil))

Perhaps the type `regexp' would be better than `string' here?

bug#30990: Should the byte compiler warn about :type mismatches?

[Lars Ingebrigtsen]

Oh, I missed this bit in the sql code;

    (dolist (ext '(".json.gpg" ".gpg" ".json" "") wallet)

So `wallet' is returned and the code is fine...

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

bug#30990: Should the byte compiler warn about :type mismatches?

[Robert Pluim]

>>>>> On Wed, 09 Oct 2019 22:05:33 +0200, Lars Ingebrigtsen <larsi@gnus.org> said:

    Lars> Oh, I missed this bit in the sql code;
    Lars>     (dolist (ext '(".json.gpg" ".gpg" ".json" "") wallet)

    Lars> So `wallet' is returned and the code is fine...

Except that the underlying type is '(repeat ....) so I wonder if this:

    (dolist (ext '(".json.gpg" ".gpg" ".json" "") wallet)
      (unless wallet
        (setq w (locate-user-emacs-file (concat "sql-wallet" ext)
                                        (concat ".sql-wallet" ext)))
        (when (file-exists-p w)
          (setq wallet w))))) ; <= this returns a string
          
should use (setq wallet (list w)) instead

Robert

bug#30990: Should the byte compiler warn about :type mismatches?

[Lars Ingebrigtsen]

Robert Pluim <rpluim@gmail.com> writes:

> Except that the underlying type is '(repeat ....) so I wonder if this:
>
>     (dolist (ext '(".json.gpg" ".gpg" ".json" "") wallet)
>       (unless wallet
>         (setq w (locate-user-emacs-file (concat "sql-wallet" ext)
>                                         (concat ".sql-wallet" ext)))
>         (when (file-exists-p w)
>           (setq wallet w))))) ; <= this returns a string
>
> should use (setq wallet (list w)) instead

Indeed.  It looks like this is where it's used:

(defun sql-auth-source-search-wallet (wallet product user server database port)

[...]

    (let* ((auth-sources wallet)

and auth-source-search assumes that auth-sources is a list of
... things, not a string:

(cl-defun auth-source-search (&rest spec
                              &key max require create delete
                              &allow-other-keys)

[...]

  (let* ((backends (mapcar #'auth-source-backend-parse auth-sources))


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

bug#30990: Should the byte compiler warn about :type mismatches?

[Robert Pluim]

>>>>> On Wed, 9 Oct 2019 20:44:04 +0200, Mattias Engdegård <mattiase@acm.org> said:

    >> - :type 'string
    >> + :type '(choice (string :tag "Regexp")
    >> +                (const :tag "Not Set" nil))

    Mattias> Perhaps the type `regexp' would be better than `string' here?

Yes, it would. Fixed.

Robert

bug#30990: Should the byte compiler warn about :type mismatches?

[Lars Ingebrigtsen]

Glenn Morris <rgm@gnu.org> writes:

> I think that to do this in general requires evaluating the defcustom at
> compile time, which probably isn't the right thing.
>
> emacs -batch -l admin/cus-test.el -f cus-test-opts all
>
> will find these mismatches. I think it should be converted to an
> (expensive) ert test, which would solve the problem for Emacs itself.

I've now done this.

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

bug#30990: Should the byte compiler warn about :type mismatches?

[Lars Ingebrigtsen]

Robert Pluim <rpluim@gmail.com> writes:

> Except that the underlying type is '(repeat ....) so I wonder if this:
>
>     (dolist (ext '(".json.gpg" ".gpg" ".json" "") wallet)
>       (unless wallet
>         (setq w (locate-user-emacs-file (concat "sql-wallet" ext)
>                                         (concat ".sql-wallet" ext)))
>         (when (file-exists-p w)
>           (setq wallet w))))) ; <= this returns a string
>
> should use (setq wallet (list w)) instead

I went ahead and fixed this along with a bunch of other warnings
revealed by the test (that has snuck in after Rober fixed the last batch,
I think).

There's some that are rather intractable, though, like
sql-postgres-login-params.

These remain:

whitespace-indentation-regexp
whitespace-space-after-tab-regexp
sql-postgres-login-params
gdb-display-source-buffer-action

I don't quite understand what's wrong here:

(defcustom whitespace-indentation-regexp
  '("^\t*\\(\\( \\{%d\\}\\)+\\)[^\n\t]"
    . "^ *\\(\t+\\)[^\n]")
[...]
  :type '(cons (regexp :tag "Indentation SPACEs")
	       (regexp :tag "Indentation TABs"))
  :group 'whitespace)

Isn't that how you specify a cons of two values?

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

bug#30990: Should the byte compiler warn about :type mismatches?

[Lars Ingebrigtsen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> I don't quite understand what's wrong here:
>
> (defcustom whitespace-indentation-regexp
>   '("^\t*\\(\\( \\{%d\\}\\)+\\)[^\n\t]"
>     . "^ *\\(\t+\\)[^\n]")
> [...]
>   :type '(cons (regexp :tag "Indentation SPACEs")
> 	       (regexp :tag "Indentation TABs"))
>   :group 'whitespace)
>
> Isn't that how you specify a cons of two values?

That wasn't the problem -- the first bit here isn't really a regexp,
it's a string passed to format to yield a regexp.

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

bug#30990: Should the byte compiler warn about :type mismatches?

[Lars Ingebrigtsen]

OK, all fixed now.

But I guess the answer to "Should the byte compiler warn about :type
mismatches?" is...  Perhaps it's best to leave that to the linter?  So
I'm closing this bug report.

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