bug#59920: [PATCH] Make checkdoc warn if not using lexical-binding

bug#59920: [PATCH] Make checkdoc warn if not using lexical-binding

[Stefan Kangas]

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

Severity: wishlist

The attached patch makes checkdoc warn if there's no lexical-binding
cookie.

[-- Attachment #2: 0001-Make-checkdoc-warn-if-not-using-lexical-binding.patch --]
[-- Type: text/x-diff, Size: 2461 bytes --]

From 2418e0aa770dbab25f73185d9a30acdbfd9d86ab Mon Sep 17 00:00:00 2001
From: Stefan Kangas <stefankangas@gmail.com>
Date: Fri, 9 Dec 2022 10:58:22 +0100
Subject: [PATCH] Make checkdoc warn if not using lexical-binding

* lisp/emacs-lisp/checkdoc.el (checkdoc-file-comments-engine):
Warn if there is no lexical-binding cookie.
---
 etc/NEWS                    |  7 +++++++
 lisp/emacs-lisp/checkdoc.el | 24 ++++++++++++++++++++++++
 2 files changed, 31 insertions(+)

diff --git a/etc/NEWS b/etc/NEWS
index 7f073c4e2d..1025f43207 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -56,6 +56,13 @@ switches for shortlogs, such as the one produced by 'C-x v L'.
 You can now configure how to display the "*buffer-selection*" buffer
 using this new option.  (Or set 'display-buffer-alist' directly.)
 
+** checkdoc
+
+---
+*** New checkdock warning if not using lexical-binding.
+Checkdoc now warns if the first line of an Emacs Lisp file does not
+end with a "-*- lexical-binding: t -*-" cookie.
+
 \f
 * New Modes and Packages in Emacs 30.1
 
diff --git a/lisp/emacs-lisp/checkdoc.el b/lisp/emacs-lisp/checkdoc.el
index 3bddb93b64..4ef6f0a212 100644
--- a/lisp/emacs-lisp/checkdoc.el
+++ b/lisp/emacs-lisp/checkdoc.el
@@ -2369,6 +2369,30 @@ checkdoc-file-comments-engine
 	      (point-min) (save-excursion (goto-char (point-min))
 					  (line-end-position))))
 	 nil))
+      (setq
+       err
+       ;; Lexical binding cookie.
+       (if (not (save-excursion
+                  (save-restriction
+                    (goto-char (point-min))
+                    (narrow-to-region (point) (pos-eol))
+                    (re-search-forward
+                     (rx "-*-" (* (* nonl) ";")
+                         (* space) "lexical-binding:" (* space) "t" (* space)
+                         (* ";" (* nonl))
+                         "-*-")
+                     nil t))))
+           (let ((pos (save-excursion (goto-char (point-min))
+                                      (goto-char (pos-eol))
+                                      (point))))
+            (if (checkdoc-y-or-n-p "There is no lexical-binding cookie!  Add one?")
+                (progn
+                  (goto-char pos)
+                  (insert "  -*- lexical-binding: t -*-"))
+              (checkdoc-create-error
+               "The first line should end with \"-*- lexical-binding: t -*-\""
+               pos (1+ pos) t)))
+         nil))
       (setq
        err
        (or
-- 
2.35.1

bug#59920: [PATCH] Make checkdoc warn if not using lexical-binding

[Eli Zaretskii]

> From: Stefan Kangas <stefankangas@gmail.com>
> Date: Fri, 9 Dec 2022 02:10:16 -0800
> 
> The attached patch makes checkdoc warn if there's no lexical-binding
> cookie.

Thanks, but I think this should be optional behavior.  checkdoc is not
only used by us.

bug#59920: [PATCH] Make checkdoc warn if not using lexical-binding

[Stefan Kangas]

Eli Zaretskii <eliz@gnu.org> writes:

> Thanks, but I think this should be optional behavior.  checkdoc is not
> only used by us.

The idea here is to encourage others to upgrade to lexical-binding, as
we are already done (at least in emacs.git, if not yet in GNU ELPA).

I'm actually more concerned that this check will not reach enough users.
So while I think an option is a good idea, the best default is IMO to
have it enabled.

bug#59920: [PATCH] Make checkdoc warn if not using lexical-binding

[Eli Zaretskii]

> From: Stefan Kangas <stefankangas@gmail.com>
> Date: Fri, 9 Dec 2022 04:42:00 -0800
> Cc: 59920@debbugs.gnu.org
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > Thanks, but I think this should be optional behavior.  checkdoc is not
> > only used by us.
> 
> The idea here is to encourage others to upgrade to lexical-binding, as
> we are already done (at least in emacs.git, if not yet in GNU ELPA).
> 
> I'm actually more concerned that this check will not reach enough users.
> So while I think an option is a good idea, the best default is IMO to
> have it enabled.

I said nothing about defaults.  I just think we should allow turning
off this warning.

bug#59920: [PATCH] Make checkdoc warn if not using lexical-binding

[Stefan Kangas]

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

Eli Zaretskii <eliz@gnu.org> writes:

> I just think we should allow turning off this warning.

Agreed, thanks.  I've added that in the attached.

[-- Attachment #2: 0001-Make-checkdoc-warn-if-not-using-lexical-binding.patch --]
[-- Type: text/x-diff, Size: 3656 bytes --]

From 1a90d2b6f3243274af347d3cf296f32293f68cc7 Mon Sep 17 00:00:00 2001
From: Stefan Kangas <stefankangas@gmail.com>
Date: Fri, 9 Dec 2022 10:58:22 +0100
Subject: [PATCH] Make checkdoc warn if not using lexical-binding

* lisp/emacs-lisp/checkdoc.el (checkdoc-file-comments-engine):
Warn if there is no lexical-binding cookie.
---
 etc/NEWS                    |  8 ++++++++
 lisp/emacs-lisp/checkdoc.el | 39 +++++++++++++++++++++++++++++++++++++
 2 files changed, 47 insertions(+)

diff --git a/etc/NEWS b/etc/NEWS
index 0c1fdfc454..1ed6a31197 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -76,6 +76,14 @@ using this new option.  (Or set 'display-buffer-alist' directly.)
 After manually editing 'eshell-aliases-file', you can use
 'M-x eshell-read-aliases-list' to load the edited aliases.
 
+** checkdoc
+
+---
+*** New checkdock warning if not using lexical-binding.
+Checkdoc now warns if the first line of an Emacs Lisp file does not
+end with a "-*- lexical-binding: t -*-" cookie.  Customize the user
+option 'checkdoc-lexical-binding-flag' to nil to disable this warning.
+
 \f
 * New Modes and Packages in Emacs 30.1
 
diff --git a/lisp/emacs-lisp/checkdoc.el b/lisp/emacs-lisp/checkdoc.el
index 3bddb93b64..0a2d9680fe 100644
--- a/lisp/emacs-lisp/checkdoc.el
+++ b/lisp/emacs-lisp/checkdoc.el
@@ -120,6 +120,14 @@
 ;; simple style rules to follow which checkdoc will auto-fix for you.
 ;; `y-or-n-p' and `yes-or-no-p' should also end in "?".
 ;;
+;; Lexical binding:
+;;
+;;   We recommend always using lexical binding in new code, and
+;; converting old code to use it.  Checkdoc warns if you don't have
+;; the recommended string "-*- lexical-binding: t -*-" at the top of
+;; the file.  You can disable this check with the user option
+;; `checkdoc-lexical-binding-flag'.
+;;
 ;; Adding your own checks:
 ;;
 ;;   You can experiment with adding your own checks by setting the
@@ -330,6 +338,12 @@ checkdoc-column-zero-backslash-before-paren
   :type 'boolean
   :version "28.1")
 
+(defcustom checkdoc-lexical-binding-flag t
+  "Non-nil means generate warnings if file is not using lexical binding.
+See Info node `(elisp) Converting to Lexical Binding' for more."
+  :type 'boolean
+  :version "29.1")
+
 ;; This is how you can use checkdoc to make mass fixes on the Emacs
 ;; source tree:
 ;;
@@ -2369,6 +2383,31 @@ checkdoc-file-comments-engine
 	      (point-min) (save-excursion (goto-char (point-min))
 					  (line-end-position))))
 	 nil))
+      (when checkdoc-lexical-binding-flag
+        (setq
+         err
+         ;; Lexical binding cookie.
+         (if (not (save-excursion
+                    (save-restriction
+                      (goto-char (point-min))
+                      (narrow-to-region (point) (pos-eol))
+                      (re-search-forward
+                       (rx "-*-" (* (* nonl) ";")
+                           (* space) "lexical-binding:" (* space) "t" (* space)
+                           (* ";" (* nonl))
+                           "-*-")
+                       nil t))))
+             (let ((pos (save-excursion (goto-char (point-min))
+                                        (goto-char (pos-eol))
+                                        (point))))
+               (if (checkdoc-y-or-n-p "There is no lexical-binding cookie!  Add one?")
+                   (progn
+                     (goto-char pos)
+                     (insert "  -*- lexical-binding: t -*-"))
+                 (checkdoc-create-error
+                  "The first line should end with \"-*- lexical-binding: t -*-\""
+                  pos (1+ pos) t)))
+           nil)))
       (setq
        err
        (or
-- 
2.35.1

bug#59920: [PATCH] Make checkdoc warn if not using lexical-binding

[Eli Zaretskii]

> From: Stefan Kangas <stefankangas@gmail.com>
> Date: Sat, 10 Dec 2022 15:26:03 -0800
> Cc: 59920@debbugs.gnu.org
> 
> +(defcustom checkdoc-lexical-binding-flag t
> +  "Non-nil means generate warnings if file is not using lexical binding.
> +See Info node `(elisp) Converting to Lexical Binding' for more."
> +  :type 'boolean
> +  :version "29.1")

Should be "30.1", I guess?

> +                 (checkdoc-create-error
> +                  "The first line should end with \"-*- lexical-binding: t -*-\""

Is this accurate?  IOW, does the lexical-binding cookie have to be the
last on the first line?

Otherwise, LGTM, thanks.

bug#59920: [PATCH] Make checkdoc warn if not using lexical-binding

[Stefan Kangas]

Eli Zaretskii <eliz@gnu.org> writes:

>> +(defcustom checkdoc-lexical-binding-flag t
>> +  "Non-nil means generate warnings if file is not using lexical binding.
>> +See Info node `(elisp) Converting to Lexical Binding' for more."
>> +  :type 'boolean
>> +  :version "29.1")
>
> Should be "30.1", I guess?

Yes, well spotted.

>> +                 (checkdoc-create-error
>> +                  "The first line should end with \"-*- lexical-binding: t -*-\""
>
> Is this accurate?  IOW, does the lexical-binding cookie have to be the
> last on the first line?

You're right, it's not accurate.  I changed it to say:

    The first line should contain "-*- lexical-binding: t -*-"

> Otherwise, LGTM, thanks.

Thanks.

bug#59920: [PATCH] Make checkdoc warn if not using lexical-binding

[Stefan Kangas]

Version: 30.1

Stefan Kangas <stefankangas@gmail.com> writes:

> Eli Zaretskii <eliz@gnu.org> writes:
>
> >> +(defcustom checkdoc-lexical-binding-flag t
> >> +  "Non-nil means generate warnings if file is not using lexical binding.
> >> +See Info node `(elisp) Converting to Lexical Binding' for more."
> >> +  :type 'boolean
> >> +  :version "29.1")
> >
> > Should be "30.1", I guess?
>
> Yes, well spotted.
>
> >> +                 (checkdoc-create-error
> >> +                  "The first line should end with \"-*- lexical-binding: t -*-\""
> >
> > Is this accurate?  IOW, does the lexical-binding cookie have to be the
> > last on the first line?
>
> You're right, it's not accurate.  I changed it to say:
>
>     The first line should contain "-*- lexical-binding: t -*-"
>
> > Otherwise, LGTM, thanks.
>
> Thanks.

Pushed to master as commit 7d60d1652fc.