all messages for Emacs-related lists mirrored at yhetil.org
 help / color / mirror / code / Atom feed
* bug#66636: Move lexical-binding warning from checkdoc to byte-compiler
@ 2023-10-19 11:48 Mattias Engdegård
  2023-10-19 11:55 ` Eli Zaretskii
                   ` (2 more replies)
  0 siblings, 3 replies; 25+ messages in thread
From: Mattias Engdegård @ 2023-10-19 11:48 UTC (permalink / raw)
  To: 66636

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

The warning about a missing lexical-binding cookie rather belongs in the compiler than checkdoc, because it's not about documentation or style but code generation and ability to detect errors, both which are hindered by a missing cookie.

Moving the warning to the compiler also makes it more widely seen.


[-- Attachment #2: 0001-Move-lexical-binding-warning-from-checkdoc-to-byte-c.patch --]
[-- Type: application/octet-stream, Size: 11207 bytes --]

From 6887fadd1c262954303d706d0d556bef4b5148c5 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Mattias=20Engdeg=C3=A5rd?= <mattiase@acm.org>
Date: Sun, 15 Oct 2023 22:01:06 +0200
Subject: [PATCH] Move lexical-binding warning from checkdoc to byte-compiler

This warning is much more appropriate for the compiler, since lexical
binding affects what it can reason and warn about, than for checkdoc
as the warning has no bearing to documentation at all.
The move also improves the reach of the warning.

* etc/NEWS: Update.
* lisp/emacs-lisp/checkdoc.el (checkdoc-lexical-binding-flag)
(checkdoc-file-comments-engine): Move warning from here....
* lisp/emacs-lisp/bytecomp.el (byte-compile-file): ...to here.
* test/lisp/emacs-lisp/bytecomp-resources/no-byte-compile.el:
* test/lisp/emacs-lisp/bytecomp-tests.el
(bytecomp-tests--unescaped-char-literals)
(bytecomp-tests-function-put, bytecomp-tests--not-writable-directory)
(bytecomp-tests--target-file-no-directory):
Update tests.
(bytecomp-tests--log-from-compilation)
(bytecomp-tests--lexical-binding-cookie): New test.
---
 etc/NEWS                                      | 25 ++++++++----
 lisp/emacs-lisp/bytecomp.el                   |  4 ++
 lisp/emacs-lisp/checkdoc.el                   | 39 -------------------
 .../bytecomp-resources/no-byte-compile.el     |  2 +-
 test/lisp/emacs-lisp/bytecomp-tests.el        | 39 +++++++++++++++++--
 5 files changed, 57 insertions(+), 52 deletions(-)

diff --git a/etc/NEWS b/etc/NEWS
index 02b794a2964..e7f51e1c6c4 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -845,14 +845,6 @@ This can help avoid some awkward skip conditions.  For example
 '(skip-unless (not noninteractive))' can be changed to the easier
 to read '(skip-when noninteractive)'.
 
-** 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.
-
 ** URL
 
 +++
@@ -1153,6 +1145,23 @@ sexp-related motion commands.
 
 ** New or changed byte-compilation warnings
 
+---
+*** Warn about missing 'lexical-binding' directive.
+The compiler now warns if an Elisp file lacks the standard
+'-*- lexical-binding: ... -*-' cookie on the first line.
+This line typically looks something like
+
+    ;;; My little pony mode  -*- lexical-binding: t -*-
+
+It is needed to inform the compiler about which dialect of ELisp
+your code is using: the modern dialect with lexical binding or
+the old dialect with only dynamic binding.
+
+Lexical binding avoids some name conflicts and allows the compiler
+to detect more mistakes and generate more efficient code.  To adapt
+your code to lexical binding, see the "(elisp) Converting to Lexical
+Binding" node in the manual.
+
 ---
 *** Warn about empty bodies for more special forms and macros.
 The compiler now warns about an empty body argument to 'when',
diff --git a/lisp/emacs-lisp/bytecomp.el b/lisp/emacs-lisp/bytecomp.el
index 92abe6b4624..cc68db73c9f 100644
--- a/lisp/emacs-lisp/bytecomp.el
+++ b/lisp/emacs-lisp/bytecomp.el
@@ -2201,6 +2201,10 @@ byte-compile-file
               filename buffer-file-name))
       ;; Don't inherit lexical-binding from caller (bug#12938).
       (unless (local-variable-p 'lexical-binding)
+        (let ((byte-compile-current-buffer (current-buffer)))
+          (byte-compile-warn-x
+           (position-symbol 'a (point-min))
+           "file has no `lexical-binding' directive on its first line"))
         (setq-local lexical-binding nil))
       ;; Set the default directory, in case an eval-when-compile uses it.
       (setq default-directory (file-name-directory filename)))
diff --git a/lisp/emacs-lisp/checkdoc.el b/lisp/emacs-lisp/checkdoc.el
index 440e133f44b..471a2fbdf48 100644
--- a/lisp/emacs-lisp/checkdoc.el
+++ b/lisp/emacs-lisp/checkdoc.el
@@ -128,14 +128,6 @@
 ;; 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
@@ -347,12 +339,6 @@ 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 "30.1")
-
 ;; This is how you can use checkdoc to make mass fixes on the Emacs
 ;; source tree:
 ;;
@@ -2391,31 +2377,6 @@ 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
diff --git a/test/lisp/emacs-lisp/bytecomp-resources/no-byte-compile.el b/test/lisp/emacs-lisp/bytecomp-resources/no-byte-compile.el
index 00ad1947507..1de5cf66b66 100644
--- a/test/lisp/emacs-lisp/bytecomp-resources/no-byte-compile.el
+++ b/test/lisp/emacs-lisp/bytecomp-resources/no-byte-compile.el
@@ -1 +1 @@
-;; -*- no-byte-compile: t; -*-
+;; -*- no-byte-compile: t; lexical-binding: t; -*-
diff --git a/test/lisp/emacs-lisp/bytecomp-tests.el b/test/lisp/emacs-lisp/bytecomp-tests.el
index e644417c3d4..4aa555f1e92 100644
--- a/test/lisp/emacs-lisp/bytecomp-tests.el
+++ b/test/lisp/emacs-lisp/bytecomp-tests.el
@@ -1302,6 +1302,30 @@ bytecomp-tests--with-temp-file
        (let ((elc (concat ,file-name-var ".elc")))
          (if (file-exists-p elc) (delete-file elc))))))
 
+(defun bytecomp-tests--log-from-compilation (source)
+  "Compile the string SOURCE and return the compilation log output."
+  (let ((text-quoting-style 'grave)
+        (byte-compile-log-buffer (generate-new-buffer " *Compile-Log*")))
+    (with-current-buffer byte-compile-log-buffer
+      (let ((inhibit-read-only t)) (erase-buffer)))
+    (bytecomp-tests--with-temp-file el-file
+      (write-region source nil el-file)
+      (byte-compile-file el-file))
+    (with-current-buffer byte-compile-log-buffer
+      (buffer-string))))
+
+(ert-deftest bytecomp-tests--lexical-binding-cookie ()
+  (cl-flet ((cookie-warning (source)
+              (string-search
+               "file has no `lexical-binding' directive on its first line"
+               (bytecomp-tests--log-from-compilation source))))
+    (let ((some-code "(defun my-fun () 12)\n"))
+      (should-not (cookie-warning
+                   (concat ";;; -*-lexical-binding:t-*-\n" some-code)))
+      (should-not (cookie-warning
+                   (concat ";;; -*-lexical-binding:nil-*-\n" some-code)))
+      (should (cookie-warning some-code)))))
+
 (ert-deftest bytecomp-tests--unescaped-char-literals ()
   "Check that byte compiling warns about unescaped character
 literals (Bug#20852)."
@@ -1310,7 +1334,9 @@ bytecomp-tests--unescaped-char-literals
         (byte-compile-debug t)
         (text-quoting-style 'grave))
     (bytecomp-tests--with-temp-file source
-      (write-region "(list ?) ?( ?; ?\" ?[ ?])" nil source)
+      (write-region (concat ";;; -*-lexical-binding:t-*-\n"
+                            "(list ?) ?( ?; ?\" ?[ ?])")
+                    nil source)
       (bytecomp-tests--with-temp-file destination
         (let* ((byte-compile-dest-file-function (lambda (_) destination))
                (err (should-error (byte-compile-file source))))
@@ -1322,7 +1348,9 @@ bytecomp-tests--unescaped-char-literals
                                     "`?\\]' expected!")))))))
     ;; But don't warn in subsequent compilations (Bug#36068).
     (bytecomp-tests--with-temp-file source
-      (write-region "(list 1 2 3)" nil source)
+      (write-region (concat ";;; -*-lexical-binding:t-*-\n"
+                            "(list 1 2 3)")
+                    nil source)
       (bytecomp-tests--with-temp-file destination
         (let ((byte-compile-dest-file-function (lambda (_) destination)))
           (should (byte-compile-file source)))))))
@@ -1330,6 +1358,7 @@ bytecomp-tests--unescaped-char-literals
 (ert-deftest bytecomp-tests-function-put ()
   "Check `function-put' operates during compilation."
   (bytecomp-tests--with-temp-file source
+    (insert  ";;; -*-lexical-binding:t-*-\n")
     (dolist (form '((function-put 'bytecomp-tests--foo 'foo 1)
                     (function-put 'bytecomp-tests--foo 'bar 2)
                     (defmacro bytecomp-tests--foobar ()
@@ -1636,7 +1665,8 @@ bytecomp-tests--not-writable-directory
            (byte-compile-error-on-warn t))
       (unwind-protect
           (progn
-            (write-region "" nil input-file nil nil nil 'excl)
+            (write-region ";;; -*-lexical-binding:t-*-\n"
+                          nil input-file nil nil nil 'excl)
             (write-region "" nil output-file nil nil nil 'excl)
             (set-file-modes input-file #o400)
             (set-file-modes output-file #o200)
@@ -1700,7 +1730,8 @@ bytecomp-tests--target-file-no-directory
     (let* ((default-directory directory)
            (byte-compile-dest-file-function (lambda (_) "test.elc"))
            (byte-compile-error-on-warn t))
-      (write-region "" nil "test.el" nil nil nil 'excl)
+      (write-region  ";;; -*-lexical-binding:t-*-\n"
+                     nil "test.el" nil nil nil 'excl)
       (should (byte-compile-file "test.el"))
       (should (file-regular-p "test.elc"))
       (should (cl-plusp (file-attribute-size
-- 
2.32.0 (Apple Git-132)


^ permalink raw reply related	[flat|nested] 25+ messages in thread

end of thread, other threads:[~2023-10-22  5:28 UTC | newest]

Thread overview: 25+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-10-19 11:48 bug#66636: Move lexical-binding warning from checkdoc to byte-compiler Mattias Engdegård
2023-10-19 11:55 ` Eli Zaretskii
2023-10-19 14:12   ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
2023-10-19 17:40 ` Stefan Kangas
2023-10-20 10:15   ` Mattias Engdegård
2023-10-20 20:55     ` Stefan Kangas
2023-10-20  6:09 ` Po Lu via Bug reports for GNU Emacs, the Swiss army knife of text editors
2023-10-20  7:01   ` Eli Zaretskii
2023-10-20  7:13     ` Po Lu via Bug reports for GNU Emacs, the Swiss army knife of text editors
2023-10-20  7:21       ` Eli Zaretskii
2023-10-20 19:44         ` Stefan Kangas
2023-10-20 13:38   ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
2023-10-20 17:40     ` Mattias Engdegård
2023-10-20 19:27       ` Eli Zaretskii
2023-10-21  9:53         ` Mattias Engdegård
2023-10-21 11:17           ` Eli Zaretskii
2023-10-21 13:17             ` Mattias Engdegård
2023-10-21 14:42             ` Drew Adams
2023-10-21 15:44             ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
2023-10-21 16:08               ` Eli Zaretskii
2023-10-22  0:15               ` Po Lu via Bug reports for GNU Emacs, the Swiss army knife of text editors
2023-10-22  4:12                 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
2023-10-22  5:16                   ` Eli Zaretskii
2023-10-22  5:28                     ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
2023-10-20 16:27   ` Drew Adams

Code repositories for project(s) associated with this external index

	https://git.savannah.gnu.org/cgit/emacs.git
	https://git.savannah.gnu.org/cgit/emacs/org-mode.git

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.