bug#67916: 30.0.50; No lexical-binding directive warning in -pkg.el files

bug#67916: 30.0.50; No lexical-binding directive warning in -pkg.el files

[Basil L. Contovounesios via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Severity: minor

0. cd "$(mktemp -d)"
1. HOME="${PWD}" XDG_CONFIG_HOME="${PWD}/.config" emacs
2. M-x package-install RET rbit RET
3. C-x b *Compile-Log* RET

In toplevel form:
rbit-pkg.el:1:1: Warning: file has no ‘lexical-binding’ directive on its first
    line

I wasn't sure about this one:

- OT1H the warning wouldn't crop up if byte-compile-file checked
  no-byte-compile before it checks lexical-binding, or if
  package--compile added -pkg.el files to the ignore list

- OTOH which dialect an Elisp file is written in (and our push for
  having this specified everywhere by default) is generally orthogonal
  to whether it should be byte-compiled

- OT3H -pkg.el files are arguably plain .eld files for which specifying
  lexical-binding makes little sense

WDYT?  Couple of sample patches to follow.

Thanks,
-- 
Basil

In GNU Emacs 30.0.50 (build 1, x86_64-pc-linux-gnu, X toolkit, cairo
 version 1.18.0, Xaw3d scroll bars) of 2023-12-19 built on tia
Repository revision: 7c1c2519167d51931a5d17f27529c8c8358c7c61
Repository branch: master
Windowing system distributor 'The X.Org Foundation', version 11.0.12101009
System Description: Debian GNU/Linux trixie/sid

Configured using:
 'configure 'CFLAGS=-Og -ggdb3' -C --prefix=/home/blc/.local
 --enable-checking=structs --without-native-compilation
 --with-file-notification=yes --with-x-toolkit=lucid --with-x'

Configured features:
ACL CAIRO DBUS FREETYPE GIF GLIB GMP GNUTLS GPM GSETTINGS HARFBUZZ JPEG
JSON LCMS2 LIBOTF LIBSELINUX LIBSYSTEMD LIBXML2 M17N_FLT MODULES NOTIFY
INOTIFY PDUMPER PNG RSVG SECCOMP SOUND SQLITE3 THREADS TIFF
TOOLKIT_SCROLL_BARS TREE_SITTER WEBP X11 XAW3D XDBE XIM XINPUT2 XPM
LUCID ZLIB

Important settings:
  value of $LANG: en_IE.UTF-8
  value of $XMODIFIERS: @im=ibus
  locale-coding-system: utf-8-unix

Major mode: elisp-compile

Minor modes in effect:
  tooltip-mode: t
  global-eldoc-mode: t
  show-paren-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  minibuffer-regexp-mode: t
  buffer-read-only: t
  line-number-mode: t
  indent-tabs-mode: t
  transient-mark-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t

Load-path shadows:
None found.

Features:
(shadow sort mail-extr emacsbug ert ewoc debug backtrace help-mode
find-func pcase warnings compile comint ansi-osc ansi-color ring
rbit-autoloads loaddefs-gen lisp-mnt radix-tree cus-edit pp cus-start
cus-load icons wid-edit mm-archive message sendmail yank-media dired
dired-loaddefs rfc822 mml mml-sec epa derived gnus-util
text-property-search time-date mailabbrev gmm-utils mailheader mm-decode
mm-bodies mm-encode mail-utils gnutls network-stream url-cache url-http
url-auth mail-parse rfc2231 rfc2047 rfc2045 mm-util ietf-drums
mail-prsvr url-gw nsm puny epg rfc6068 epg-config finder-inf package
browse-url url url-proxy url-privacy url-expand url-methods url-history
url-cookie generate-lisp-file url-domsuf url-util mailcap url-handlers
url-parse auth-source cl-seq eieio eieio-core cl-macs password-cache
json subr-x map byte-opt gv bytecomp byte-compile url-vars cl-loaddefs
cl-lib rmc iso-transl tooltip cconv eldoc paren electric uniquify
ediff-hook vc-hooks lisp-float-type elisp-mode mwheel term/x-win x-win
term/common-win x-dnd touch-screen tool-bar dnd fontset image regexp-opt
fringe tabulated-list replace newcomment text-mode lisp-mode prog-mode
register page tab-bar menu-bar rfn-eshadow isearch easymenu timer select
scroll-bar mouse jit-lock font-lock syntax font-core term/tty-colors
frame minibuffer nadvice seq simple cl-generic indonesian philippine
cham georgian utf-8-lang misc-lang vietnamese tibetan thai tai-viet lao
korean japanese eucjp-ms cp51932 hebrew greek romanian slovak czech
european ethiopic indian cyrillic chinese composite emoji-zwj charscript
charprop case-table epa-hook jka-cmpr-hook help abbrev obarray oclosure
cl-preloaded button loaddefs theme-loaddefs faces cus-face macroexp
files window text-properties overlay sha1 md5 base64 format env
code-pages mule custom widget keymap hashtable-print-readable backquote
threads dbusbind inotify lcms2 dynamic-setting system-font-setting
font-render-setting cairo x-toolkit xinput2 x multi-tty move-toolbar
make-network-process emacs)

Memory information:
((conses 16 136237 15260) (symbols 48 11969 6) (strings 32 44639 1179)
 (string-bytes 1 1150536) (vectors 16 21515)
 (vector-slots 8 263985 21000) (floats 8 39 77) (intervals 56 408 0)
 (buffers 992 12))

bug#67916: 30.0.50; No lexical-binding directive warning in -pkg.el files

[Basil L. Contovounesios via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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

tags 67916 + patch
quit

Basil L. Contovounesios [2023-12-19 22:49 +0100] wrote:
> WDYT?  Couple of sample patches to follow.

I attach a patch each for emacs.git and elpa.git.

Their main intention was emitting a lexical-binding cookie in -pkg.el
files, but even if we decide against that, the patches contain some
other cleanups which are hopefully welcome.

Question for Philip:

package-vc--generate-description-file currently passes:
  :kind 'vc
unquoted to define-package, which results in the -pkg.el contents:
  (define-package ... :kind vc ... :keywords '(...) ...)
Note the difference in quoting between :kind and :keywords.  Is this
intentional?  Or can/should :kind pass through macroexp-quote as well?

Questions for Stefan:

- Which version of Emacs can/does elpa-admin.el assume?
- When the FIXME in elpaa--write-pkg-file says to use
  package-generate-description-file, does that mean reuse its guts like
  I did in package-vc.el with package--write-description-file?  Or was
  the idea rather to have elpaa--write-pkg-file create synthetic
  package-desc structs for passing to the public API of
  package-generate-description-file directly?

Thanks,
-- 
Basil


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Declare-lexical-binding-in-pkg.el-files.patch --]
[-- Type: text/x-diff, Size: 18490 bytes --]

From e602e79c00f2c4515c31b3f4ae744e63b7192174 Mon Sep 17 00:00:00 2001
From: "Basil L. Contovounesios" <contovob@tcd.ie>
Date: Mon, 18 Dec 2023 13:27:39 +0100
Subject: [PATCH] Declare lexical-binding in -pkg.el files

Running package-install emits a 'has no lexical-binding directive'
warning for generated -pkg.el files.

* lisp/emacs-lisp/package.el:
(package--with-response-buffer-1): Remove redundant requires and
function declarations.
(package): Add to tools :group as per Keywords header.  Bump
:version accordingly.
(package-vc-p): Remove redundant inline-letevals.
(package--unquote): New convenience function.
(package-desc-from-define, package-desc-suffix): Use it.
(package-desc): Add individual :type and :documentation options to
slots.  Document dir and vc kinds.
(package--alist-to-plist-args): Replace nconc+mapcar with mapcan.
(package--write-description-file): New function extracted from
package-generate-description-file.  Specify lexical-binding to avoid
package-install warnings (bug#67916).  Stricter calls to prin1 and
replace-regexp-in-string with overriding arguments.  Use
macroexp-quote.
(package-generate-description-file): Refactor in terms of
package--write-description-file.

* lisp/emacs-lisp/package-vc.el: Add development and vc Keywords.
Remove redundant requires.
(package-vc): Add to vc :group and bump :version accordingly.
(package-vc--main-file): Simplify.
(package-vc--generate-description-file): Simplify and refactor in
terms of package--write-description-file.
(package-vc--unpack): Use package-vc-p.
---
 lisp/emacs-lisp/package-vc.el |  78 +++++----------
 lisp/emacs-lisp/package.el    | 182 ++++++++++++++++------------------
 2 files changed, 110 insertions(+), 150 deletions(-)

diff --git a/lisp/emacs-lisp/package-vc.el b/lisp/emacs-lisp/package-vc.el
index bef498f997c..14f3dc859bf 100644
--- a/lisp/emacs-lisp/package-vc.el
+++ b/lisp/emacs-lisp/package-vc.el
@@ -3,7 +3,7 @@
 ;; Copyright (C) 2022-2023 Free Software Foundation, Inc.
 
 ;; Author: Philip Kaludercic <philipk@posteo.net>
-;; Keywords: tools
+;; Keywords: development, tools, vc
 
 ;; This file is part of GNU Emacs.
 
@@ -44,20 +44,19 @@
 
 ;;; Code:
 
-(eval-when-compile (require 'rx))
 (eval-when-compile (require 'map))
 (eval-when-compile (require 'cl-lib))
 (require 'package)
 (require 'lisp-mnt)
 (require 'vc)
-(require 'seq)
 
 (defgroup package-vc nil
   "Manage packages from VC checkouts."
   :group 'package
+  :group 'vc
   :link '(custom-manual "(emacs) Fetching Package Sources")
   :prefix "package-vc-"
-  :version "29.1")
+  :version "30.1")
 
 (defconst package-vc--elpa-packages-version 1
   "Version number of the package specification format understood by package-vc.")
@@ -308,59 +307,32 @@ package-vc--main-file
       ;; try and find the closest match.
       (let ((distance most-positive-fixnum) (best nil))
         (dolist (alt (directory-files directory t "\\.el\\'" t))
-          (let ((sd (string-distance file alt)))
-            (when (and (not (string-match-p (rx (or (: "-autoloads.el")
-                                                    (: "-pkg.el"))
-                                                eos)
-                                            alt))
-                       (< sd distance))
+          (unless (or (string-suffix-p "-autoloads.el" alt)
+                      (string-suffix-p "-pkg.el" alt))
+            (let ((sd (string-distance file alt)))
               (when (< sd distance)
-                (setq distance (string-distance file alt)
-                      best alt)))))
+                (setq distance sd best alt)))))
         best))))
 
 (defun package-vc--generate-description-file (pkg-desc pkg-file)
   "Generate a package description file for PKG-DESC and write it to PKG-FILE."
-  (let ((name (package-desc-name pkg-desc)))
-    ;; Infer the subject if missing.
-    (unless (package-desc-summary pkg-desc)
-      (setf (package-desc-summary pkg-desc)
-            (let ((main-file (package-vc--main-file pkg-desc)))
-              (or (package-desc-summary pkg-desc)
-                  (and-let* ((pkg (cadr (assq name package-archive-contents))))
-                    (package-desc-summary pkg))
-                  (and main-file (file-exists-p main-file)
-                       (lm-summary main-file))
-                  package--default-summary))))
-    (let ((print-level nil)
-          (print-quoted t)
-          (print-length nil))
-      (write-region
-       (concat
-        ";;; Generated package description from "
-        (replace-regexp-in-string
-         "-pkg\\.el\\'" ".el"
-         (file-name-nondirectory pkg-file))
-        "  -*- no-byte-compile: t -*-\n"
-        (prin1-to-string
-         (nconc
-          (list 'define-package
-                (symbol-name name)
-                (package-vc--version pkg-desc)
-                (package-desc-summary pkg-desc)
-                (let ((requires (package-desc-reqs pkg-desc)))
-                  (list 'quote
-                        ;; Turn version lists into string form.
-                        (mapcar
-                         (lambda (elt)
-                           (list (car elt)
-                                 (package-version-join (cadr elt))))
-                         requires))))
-          (list :kind 'vc)
-          (package--alist-to-plist-args
-           (package-desc-extras pkg-desc))))
-        "\n")
-       nil pkg-file nil 'silent))))
+  ;; Infer the subject if missing.
+  (unless (package-desc-summary pkg-desc)
+    (setf (package-desc-summary pkg-desc)
+          (or (and-let* ((pkg (cadr (assq (package-desc-name pkg-desc)
+                                          package-archive-contents))))
+                (package-desc-summary pkg))
+              (and-let* ((main-file (package-vc--main-file pkg-desc))
+                         ((file-exists-p main-file)))
+                (lm-summary main-file))
+              package--default-summary)))
+  (let ((name (symbol-name (package-desc-name pkg-desc)))
+        (ver (package-vc--version pkg-desc))
+        (doc (package-desc-summary pkg-desc))
+        (reqs (package-desc-reqs pkg-desc))
+        (extras (package-desc-extras pkg-desc))
+        (props '(:kind vc)))
+    (package--write-description-file pkg-file name ver doc reqs extras props)))
 
 (defcustom package-vc-allow-build-commands nil
   "Whether to run extra build commands when installing VC packages.
@@ -674,7 +646,7 @@ package-vc--unpack
 how to fetch and build the package.  See `package-vc--archive-spec-alists'
 for details.  The optional argument REV specifies a specific revision to
 checkout.  This overrides the `:branch' attribute in PKG-SPEC."
-  (unless (eq (package-desc-kind pkg-desc) 'vc)
+  (unless (package-vc-p pkg-desc)
     (let ((copy (copy-package-desc pkg-desc)))
       (setf (package-desc-kind copy) 'vc
             pkg-desc copy)))
diff --git a/lisp/emacs-lisp/package.el b/lisp/emacs-lisp/package.el
index bed6e74c921..d1ff6e67a8a 100644
--- a/lisp/emacs-lisp/package.el
+++ b/lisp/emacs-lisp/package.el
@@ -144,20 +144,17 @@
 ;;; Code:
 
 (require 'cl-lib)
-(eval-when-compile (require 'subr-x))
 (eval-when-compile (require 'epg))      ;For setf accessors.
-(eval-when-compile (require 'inline))   ;For `define-inline'
-(require 'seq)
 
 (require 'tabulated-list)
-(require 'macroexp)
 (require 'url-handlers)
 (require 'browse-url)
 
 (defgroup package nil
   "Manager for Emacs Lisp packages."
   :group 'applications
-  :version "24.1")
+  :group 'tools
+  :version "30.1")
 
 \f
 ;;; Customization options
@@ -325,9 +322,6 @@ package-directory-list
   :risky t
   :version "24.1")
 
-(declare-function epg-find-configuration "epg-config"
-                  (protocol &optional no-cache program-alist))
-
 (defcustom package-gnupghome-dir (expand-file-name "gnupg" package-user-dir)
   "Directory containing GnuPG keyring or nil.
 This variable specifies the GnuPG home directory used by package.
@@ -457,14 +451,18 @@ package--default-summary
 
 (define-inline package-vc-p (pkg-desc)
   "Return non-nil if PKG-DESC is a VC package."
-  (inline-letevals (pkg-desc)
-    (inline-quote (eq (package-desc-kind ,pkg-desc) 'vc))))
+  (inline-quote (eq (package-desc-kind ,pkg-desc) 'vc)))
+
+(define-inline package--unquote (arg)
+  "Return ARG without its surrounding `quote', if any."
+  (inline-letevals (arg)
+    (inline-quote (if (eq (car-safe ,arg) 'quote) (cadr ,arg) ,arg))))
 
 (cl-defstruct (package-desc
                ;; Rename the default constructor from `make-package-desc'.
                (:constructor package-desc-create)
                ;; Has the same interface as the old `define-package',
-               ;; which is still used in the "foo-pkg.el" files. Extra
+               ;; which is still used in the "foo-pkg.el" files.  Extra
                ;; options can be supported by adding additional keys.
                (:constructor
                 package-desc-from-define
@@ -472,15 +470,14 @@ package-vc-p
                  &rest rest-plist
                  &aux
                  (name (intern name-string))
-                 (version (if (eq (car-safe version-string) 'vc)
-                              (version-to-list (cdr version-string))
-                            (version-to-list version-string)))
+                 (version (version-to-list
+                           (if (eq (car-safe version-string) 'vc)
+                               (cdr version-string)
+                             version-string)))
                  (reqs (mapcar (lambda (elt)
                                  (list (car elt)
                                        (version-to-list (cadr elt))))
-                               (if (eq 'quote (car requirements))
-                                   (nth 1 requirements)
-                                 requirements)))
+                               (package--unquote requirements)))
                  (kind (plist-get rest-plist :kind))
                  (archive (plist-get rest-plist :archive))
                  (extras (let (alist)
@@ -489,47 +486,42 @@ package-vc-p
                                (let ((value (cadr rest-plist)))
                                  (when value
                                    (push (cons (car rest-plist)
-                                               (if (eq (car-safe value) 'quote)
-                                                   (cadr value)
-                                                 value))
+                                               (package--unquote value))
                                          alist))))
                              (setq rest-plist (cddr rest-plist)))
                            alist)))))
-  "Structure containing information about an individual package.
-Slots:
-
-`name'	Name of the package, as a symbol.
-
-`version' Version of the package, as a version list.
-
-`summary' Short description of the package, typically taken from
-        the first line of the file.
-
-`reqs'	Requirements of the package.  A list of (PACKAGE
-        VERSION-LIST) naming the dependent package and the minimum
-        required version.
-
-`kind'	The distribution format of the package.  Currently, it is
-        either `single' or `tar'.
-
-`archive' The name of the archive (as a string) whence this
-        package came.
-
-`dir'	The directory where the package is installed (if installed),
-        `builtin' if it is built-in, or nil otherwise.
-
-`extras' Optional alist of additional keyword-value pairs.
-
-`signed' Flag to indicate that the package is signed by provider."
-  name
-  version
-  (summary package--default-summary)
-  reqs
-  kind
-  archive
-  dir
-  extras
-  signed)
+  "Structure containing information about an individual package."
+  (name
+   nil :type symbol :documentation
+   "Name of the package.")
+  (version
+   () :type list :documentation
+   "Version of the package, as a version list.")
+  (summary
+   package--default-summary :type string :documentation
+   "Short description of the package, typically taken from the first
+line of the file.")
+  (reqs
+   () :type list :documentation
+   "Requirements of the package.  A list of (PACKAGE VERSION-LIST)
+naming the dependent package and the minimum required version.")
+  (kind
+   nil :type symbol :documentation
+   "The distribution format of the package.  Currently, it is one of
+`single', `tar', `dir', or `vc'.")
+  (archive
+   nil :type string :documentation
+   "The name of the archive whence this package came.")
+  (dir
+   nil :type (or string symbol) :documentation
+   "The directory where the package is installed (if installed),
+`builtin' if it is built-in, or nil otherwise." )
+  (extras
+   () :type list :documentation
+   "Optional alist of additional keyword-value pairs.")
+  (signed
+   nil :type boolean :documentation
+   "Flag to indicate that the package is signed by provider."))
 
 (defun package--from-builtin (bi-desc)
   "Create a `package-desc' object from BI-DESC.
@@ -597,12 +589,9 @@ package-desc-suffix
 (defun package-desc--keywords (pkg-desc)
   "Return keywords of package-desc object PKG-DESC.
 These keywords come from the foo-pkg.el file, and in general
-corresponds to the keywords in the \"Keywords\" header of the
+correspond to the keywords in the \"Keywords\" header of the
 package."
-  (let ((keywords (cdr (assoc :keywords (package-desc-extras pkg-desc)))))
-    (if (eq (car-safe keywords) 'quote)
-        (nth 1 keywords)
-      keywords)))
+  (package--unquote (cdr (assq :keywords (package-desc-extras pkg-desc)))))
 
 (defun package-desc-priority (pkg-desc)
   "Return the priority of the archive of package-desc object PKG-DESC."
@@ -978,8 +967,7 @@ package-untar-buffer
 
 (defun package--alist-to-plist-args (alist)
   (mapcar #'macroexp-quote
-          (apply #'nconc
-                 (mapcar (lambda (pair) (list (car pair) (cdr pair))) alist))))
+          (mapcan (lambda (pair) (list (car pair) (cdr pair))) alist)))
 
 (defun package-unpack (pkg-desc)
   "Install the contents of the current buffer as a package."
@@ -1032,37 +1020,43 @@ package-unpack
         (package--reload-previously-loaded new-desc)))
     pkg-dir))
 
+;; Potentially also used in elpa.git.
+(defun package--write-description-file ( file name version doc reqs extras
+                                         &optional extra-props verbose)
+  "Write a `define-package' declaration to FILE.
+Absolute FILE names the -pkg.el description file.
+NAME, VERSION, and DOC are the leading, and EXTRA-PROPS the
+trailing, arguments of `define-package'.
+REQS and EXTRAS are the eponymous `package-desc' slots.
+Non-nil VERBOSE means display \"Wrote file\" message."
+  (let* ((src (replace-regexp-in-string (rx "-pkg.el" eos) ".el"
+                                        (file-name-nondirectory file) t t))
+         (def `(define-package ,name ,version ,doc
+                 ,(macroexp-quote
+                   ;; Turn requirement version lists into string form.
+                   (mapcar (lambda (elt)
+                             (list (car elt)
+                                   (package-version-join (cadr elt))))
+                           reqs))
+                 ,@extra-props
+                 ,@(package--alist-to-plist-args extras)))
+         (print-cfg '((length . nil)
+                      (level . nil)
+                      (quoted . t)))
+         (str (concat ";;; Generated package description from " src
+                      "  -*- no-byte-compile: t; lexical-binding: t -*-\n"
+                      (prin1-to-string def nil print-cfg)
+                      "\n")))
+    (write-region str nil file nil (unless verbose 'silent))))
+
 (defun package-generate-description-file (pkg-desc pkg-file)
   "Create the foo-pkg.el file PKG-FILE for single-file package PKG-DESC."
-  (let* ((name (package-desc-name pkg-desc)))
-    (let ((print-level nil)
-          (print-quoted t)
-          (print-length nil))
-      (write-region
-       (concat
-        ";;; Generated package description from "
-        (replace-regexp-in-string "-pkg\\.el\\'" ".el"
-                                  (file-name-nondirectory pkg-file))
-        "  -*- no-byte-compile: t -*-\n"
-        (prin1-to-string
-         (nconc
-          (list 'define-package
-                (symbol-name name)
-                (package-version-join (package-desc-version pkg-desc))
-                (package-desc-summary pkg-desc)
-                (let ((requires (package-desc-reqs pkg-desc)))
-                  (list 'quote
-                        ;; Turn version lists into string form.
-                        (mapcar
-                         (lambda (elt)
-                           (list (car elt)
-                                 (package-version-join (cadr elt))))
-                         requires))))
-          (package--alist-to-plist-args
-           (package-desc-extras pkg-desc))))
-        "\n")
-       nil pkg-file nil 'silent))))
-
+  (let ((name (symbol-name (package-desc-name pkg-desc)))
+        (ver (package-version-join (package-desc-version pkg-desc)))
+        (doc (package-desc-summary pkg-desc))
+        (reqs (package-desc-reqs pkg-desc))
+        (extras (package-desc-extras pkg-desc)))
+    (package--write-description-file pkg-file name ver doc reqs extras)))
 
 ;;;; Autoload
 (declare-function autoload-rubric "autoload" (file &optional type feature))
@@ -1311,11 +1305,6 @@ package--archive-file-exists-p
           (url-http-file-exists-p (concat location file)))
       (file-exists-p (expand-file-name file location)))))
 
-(declare-function epg-make-context "epg"
-                  (&optional protocol armor textmode include-certs
-                             cipher-algorithm
-                             digest-algorithm
-                             compress-algorithm))
 (declare-function epg-verify-string "epg" ( context signature
                                             &optional signed-text))
 (declare-function epg-context-result-for "epg" (context name))
@@ -1397,7 +1386,6 @@ package--with-response-buffer-1
                  url
                  (lambda (status)
                    (let ((b (current-buffer)))
-                     (require 'url-handlers)
                      (package--unless-error body
                        (when-let* ((er (plist-get status :error)))
                          (error "Error retrieving: %s %S" url er))
-- 
2.43.0


[-- Attachment #3: 0002-Declare-lexical-binding-in-pkg.el-files.patch --]
[-- Type: text/x-diff, Size: 4642 bytes --]

From 0359f04660bd23423d93f8aa9d9bce3a726d79a4 Mon Sep 17 00:00:00 2001
From: "Basil L. Contovounesios" <contovob@tcd.ie>
Date: Tue, 19 Dec 2023 19:44:43 +0100
Subject: [PATCH] Declare lexical-binding in -pkg.el files

This avoids a 'has no lexical-binding directive' warning when
byte-compiling generated -pkg.el files (bug#67916).

* elpa-admin.el (elpaa--alist-to-plist-args): Remove.  All callers
updated to call package--alist-to-plist-args instead.
(elpaa--write-pkg-file): Emit lexical-binding cookie on ;;;-heading.
Hoist metadata destructuring for reuse.  Mention new
package--write-description-file in commentary.
(elpaa-batch-generate-autoloads): Remove redundant call to require.
---
 elpa-admin.el | 56 ++++++++++++++++++++-------------------------------
 1 file changed, 22 insertions(+), 34 deletions(-)

diff --git a/elpa-admin.el b/elpa-admin.el
index 9cbc805ba4..2e4fcc82df 100644
--- a/elpa-admin.el
+++ b/elpa-admin.el
@@ -1401,16 +1401,6 @@ elpaa--metadata
      (t
       (error "Can't find main file %s file in %s" mainfile dir)))))
 
-(defun elpaa--alist-to-plist-args (alist)
-  (mapcar (lambda (x)
-            (if (and (not (consp x))
-                     (or (keywordp x)
-                         (not (symbolp x))
-                         (memq x '(nil t))))
-                x `',x))
-          (apply #'nconc
-                 (mapcar (lambda (pair) (list (car pair) (cdr pair))) alist))))
-
 (defun elpaa--plist-args-to-alist (plist)
   (let (alist)
     (while plist
@@ -1451,25 +1441,24 @@ elpaa--multi-file-package-def
     (elpaa--form-from-file-contents pkg-file)))
 
 (defun elpaa--write-pkg-file (pkg-dir name metadata &optional revision)
-  (setf (alist-get :commit (nth 4 metadata))
-        (or revision
-            ;; FIXME: Emacs-26's `vc-git-working-revision' ignores its
-            ;; arg and uses the `default-directory' to get the revision.
-            ;; Similar to the kludge in `elpaa--select-revision'.
-            (let ((default-directory pkg-dir))
-              (vc-working-revision pkg-dir))))
-  ;; FIXME: Use package-generate-description-file!
-  (let ((pkg-file (expand-file-name (format "%s-pkg.el" name) pkg-dir))
-	(print-level nil)
-        (print-quoted t)
-	(print-length nil))
-    (elpaa--temp-file pkg-file)
-    (write-region
-     (concat (format ";; Generated package description from %s.el  -*- no-byte-compile: t -*-\n"
-		     name)
-	     (prin1-to-string
-              (pcase-let ((`(,version ,desc ,requires ,extras)
-                           (cdr metadata)))
+  (pcase-let ((`(,version ,desc ,requires ,extras) (cdr metadata)))
+    (setf (alist-get :commit extras)
+          (or revision
+              ;; FIXME: Emacs-26's `vc-git-working-revision' ignores its
+              ;; arg and uses the `default-directory' to get the revision.
+              ;; Similar to the kludge in `elpaa--select-revision'.
+              (let ((default-directory pkg-dir))
+                (vc-working-revision pkg-dir))))
+    ;; FIXME: Use `package--write-description-file' (in Emacs≥30)!
+    (let ((pkg-file (expand-file-name (format "%s-pkg.el" name) pkg-dir))
+          (print-level nil)
+          (print-quoted t)
+          (print-length nil))
+      (elpaa--temp-file pkg-file)
+      (write-region
+       (concat (format ";;; Generated package description from %s.el" name)
+               "  -*- no-byte-compile: t; lexical-binding: t -*-\n"
+               (prin1-to-string
                 (nconc
                  (list 'define-package
                        (format "%s" name) ;It's been a string, historically :-(
@@ -1482,10 +1471,10 @@ elpaa--write-pkg-file
                                 (list (car elt)
                                       (package-version-join (cadr elt))))
                               requires)))
-                 (elpaa--alist-to-plist-args extras))))
-	     "\n")
-     nil
-     pkg-file)))
+                 (package--alist-to-plist-args extras)))
+               "\n")
+       nil
+       pkg-file))))
 
 (defun elpaa--write-plain-readme (pkg-dir pkg-spec)
   "Render a plain text readme from PKG-SPEC in PKG-DIR.
@@ -2904,7 +2893,6 @@ elpaa-batch-generate-autoloads
          (pkgname (file-name-nondirectory (directory-file-name dir)))
          (pkg-spec (elpaa--get-package-spec pkgname nil 'guess))
          (lisp-dir (elpaa--spec-get pkg-spec :lisp-dir)))
-    (require 'package)
     (if (null lisp-dir)
         (progn
           (cl-assert (equal alf (format "%s%s-autoloads.el"
-- 
2.43.0

bug#67916: 30.0.50; No lexical-binding directive warning in -pkg.el files

[Philip Kaludercic]

"Basil L. Contovounesios" <contovob@tcd.ie> writes:

> tags 67916 + patch
> quit
>
> Basil L. Contovounesios [2023-12-19 22:49 +0100] wrote:
>> WDYT?  Couple of sample patches to follow.
>
> I attach a patch each for emacs.git and elpa.git.
>
> Their main intention was emitting a lexical-binding cookie in -pkg.el
> files, but even if we decide against that, the patches contain some
> other cleanups which are hopefully welcome.
>
> Question for Philip:
>
> package-vc--generate-description-file currently passes:
>   :kind 'vc
> unquoted to define-package, which results in the -pkg.el contents:
>   (define-package ... :kind vc ... :keywords '(...) ...)
> Note the difference in quoting between :kind and :keywords.  Is this
> intentional?  Or can/should :kind pass through macroexp-quote as well?

This is not intentional, if anything a lucky oversight.

> Questions for Stefan:
>
> - Which version of Emacs can/does elpa-admin.el assume?

All I can say is that the ELPA build server, the main user of
elpa-admin.el, has Emacs 28.2 installed.

> - When the FIXME in elpaa--write-pkg-file says to use
>   package-generate-description-file, does that mean reuse its guts like
>   I did in package-vc.el with package--write-description-file?  Or was
>   the idea rather to have elpaa--write-pkg-file create synthetic
>   package-desc structs for passing to the public API of
>   package-generate-description-file directly?
>
> Thanks,
> -- 
> Basil
>
> From e602e79c00f2c4515c31b3f4ae744e63b7192174 Mon Sep 17 00:00:00 2001
> From: "Basil L. Contovounesios" <contovob@tcd.ie>
> Date: Mon, 18 Dec 2023 13:27:39 +0100
> Subject: [PATCH] Declare lexical-binding in -pkg.el files
>
> Running package-install emits a 'has no lexical-binding directive'
> warning for generated -pkg.el files.
>
> * lisp/emacs-lisp/package.el:
> (package--with-response-buffer-1): Remove redundant requires and
> function declarations.
> (package): Add to tools :group as per Keywords header.  Bump
> :version accordingly.
> (package-vc-p): Remove redundant inline-letevals.
> (package--unquote): New convenience function.
> (package-desc-from-define, package-desc-suffix): Use it.
> (package-desc): Add individual :type and :documentation options to
> slots.  Document dir and vc kinds.
> (package--alist-to-plist-args): Replace nconc+mapcar with mapcan.
> (package--write-description-file): New function extracted from
> package-generate-description-file.  Specify lexical-binding to avoid
> package-install warnings (bug#67916).  Stricter calls to prin1 and
> replace-regexp-in-string with overriding arguments.  Use
> macroexp-quote.
> (package-generate-description-file): Refactor in terms of
> package--write-description-file.
>
> * lisp/emacs-lisp/package-vc.el: Add development and vc Keywords.
> Remove redundant requires.
> (package-vc): Add to vc :group and bump :version accordingly.
> (package-vc--main-file): Simplify.
> (package-vc--generate-description-file): Simplify and refactor in
> terms of package--write-description-file.
> (package-vc--unpack): Use package-vc-p.
> ---
>  lisp/emacs-lisp/package-vc.el |  78 +++++----------
>  lisp/emacs-lisp/package.el    | 182 ++++++++++++++++------------------
>  2 files changed, 110 insertions(+), 150 deletions(-)
>
> diff --git a/lisp/emacs-lisp/package-vc.el b/lisp/emacs-lisp/package-vc.el
> index bef498f997c..14f3dc859bf 100644
> --- a/lisp/emacs-lisp/package-vc.el
> +++ b/lisp/emacs-lisp/package-vc.el
> @@ -3,7 +3,7 @@
>  ;; Copyright (C) 2022-2023 Free Software Foundation, Inc.
>  
>  ;; Author: Philip Kaludercic <philipk@posteo.net>
> -;; Keywords: tools
> +;; Keywords: development, tools, vc
>  
>  ;; This file is part of GNU Emacs.
>  
> @@ -44,20 +44,19 @@
>  
>  ;;; Code:
>  
> -(eval-when-compile (require 'rx))
>  (eval-when-compile (require 'map))
>  (eval-when-compile (require 'cl-lib))
>  (require 'package)
>  (require 'lisp-mnt)
>  (require 'vc)
> -(require 'seq)
>  
>  (defgroup package-vc nil
>    "Manage packages from VC checkouts."
>    :group 'package
> +  :group 'vc
>    :link '(custom-manual "(emacs) Fetching Package Sources")
>    :prefix "package-vc-"
> -  :version "29.1")
> +  :version "30.1")
>  
>  (defconst package-vc--elpa-packages-version 1
>    "Version number of the package specification format understood by package-vc.")
> @@ -308,59 +307,32 @@ package-vc--main-file
>        ;; try and find the closest match.
>        (let ((distance most-positive-fixnum) (best nil))
>          (dolist (alt (directory-files directory t "\\.el\\'" t))
> -          (let ((sd (string-distance file alt)))
> -            (when (and (not (string-match-p (rx (or (: "-autoloads.el")
> -                                                    (: "-pkg.el"))
> -                                                eos)
> -                                            alt))
> -                       (< sd distance))
> +          (unless (or (string-suffix-p "-autoloads.el" alt)
> +                      (string-suffix-p "-pkg.el" alt))
> +            (let ((sd (string-distance file alt)))
>                (when (< sd distance)
> -                (setq distance (string-distance file alt)
> -                      best alt)))))
> +                (setq distance sd best alt)))))
>          best))))
>  
>  (defun package-vc--generate-description-file (pkg-desc pkg-file)
>    "Generate a package description file for PKG-DESC and write it to PKG-FILE."
> -  (let ((name (package-desc-name pkg-desc)))
> -    ;; Infer the subject if missing.
> -    (unless (package-desc-summary pkg-desc)
> -      (setf (package-desc-summary pkg-desc)
> -            (let ((main-file (package-vc--main-file pkg-desc)))
> -              (or (package-desc-summary pkg-desc)
> -                  (and-let* ((pkg (cadr (assq name package-archive-contents))))
> -                    (package-desc-summary pkg))
> -                  (and main-file (file-exists-p main-file)
> -                       (lm-summary main-file))
> -                  package--default-summary))))
> -    (let ((print-level nil)
> -          (print-quoted t)
> -          (print-length nil))
> -      (write-region
> -       (concat
> -        ";;; Generated package description from "
> -        (replace-regexp-in-string
> -         "-pkg\\.el\\'" ".el"
> -         (file-name-nondirectory pkg-file))
> -        "  -*- no-byte-compile: t -*-\n"
> -        (prin1-to-string
> -         (nconc
> -          (list 'define-package
> -                (symbol-name name)
> -                (package-vc--version pkg-desc)
> -                (package-desc-summary pkg-desc)
> -                (let ((requires (package-desc-reqs pkg-desc)))
> -                  (list 'quote
> -                        ;; Turn version lists into string form.
> -                        (mapcar
> -                         (lambda (elt)
> -                           (list (car elt)
> -                                 (package-version-join (cadr elt))))
> -                         requires))))
> -          (list :kind 'vc)
> -          (package--alist-to-plist-args
> -           (package-desc-extras pkg-desc))))
> -        "\n")
> -       nil pkg-file nil 'silent))))
> +  ;; Infer the subject if missing.
> +  (unless (package-desc-summary pkg-desc)
> +    (setf (package-desc-summary pkg-desc)
> +          (or (and-let* ((pkg (cadr (assq (package-desc-name pkg-desc)
> +                                          package-archive-contents))))
> +                (package-desc-summary pkg))
> +              (and-let* ((main-file (package-vc--main-file pkg-desc))
> +                         ((file-exists-p main-file)))
> +                (lm-summary main-file))
> +              package--default-summary)))
> +  (let ((name (symbol-name (package-desc-name pkg-desc)))
> +        (ver (package-vc--version pkg-desc))
> +        (doc (package-desc-summary pkg-desc))
> +        (reqs (package-desc-reqs pkg-desc))
> +        (extras (package-desc-extras pkg-desc))
> +        (props '(:kind vc)))
> +    (package--write-description-file pkg-file name ver doc reqs extras props)))
>  
>  (defcustom package-vc-allow-build-commands nil
>    "Whether to run extra build commands when installing VC packages.
> @@ -674,7 +646,7 @@ package-vc--unpack
>  how to fetch and build the package.  See `package-vc--archive-spec-alists'
>  for details.  The optional argument REV specifies a specific revision to
>  checkout.  This overrides the `:branch' attribute in PKG-SPEC."
> -  (unless (eq (package-desc-kind pkg-desc) 'vc)
> +  (unless (package-vc-p pkg-desc)
>      (let ((copy (copy-package-desc pkg-desc)))
>        (setf (package-desc-kind copy) 'vc
>              pkg-desc copy)))
> diff --git a/lisp/emacs-lisp/package.el b/lisp/emacs-lisp/package.el
> index bed6e74c921..d1ff6e67a8a 100644
> --- a/lisp/emacs-lisp/package.el
> +++ b/lisp/emacs-lisp/package.el
> @@ -144,20 +144,17 @@
>  ;;; Code:
>  
>  (require 'cl-lib)
> -(eval-when-compile (require 'subr-x))
>  (eval-when-compile (require 'epg))      ;For setf accessors.
> -(eval-when-compile (require 'inline))   ;For `define-inline'
> -(require 'seq)
>  
>  (require 'tabulated-list)
> -(require 'macroexp)
>  (require 'url-handlers)
>  (require 'browse-url)
>  
>  (defgroup package nil
>    "Manager for Emacs Lisp packages."
>    :group 'applications
> -  :version "24.1")
> +  :group 'tools
> +  :version "30.1")

I am not sure if bumping :version is necessary (here and above).

>  \f
>  ;;; Customization options
> @@ -325,9 +322,6 @@ package-directory-list
>    :risky t
>    :version "24.1")
>  
> -(declare-function epg-find-configuration "epg-config"
> -                  (protocol &optional no-cache program-alist))
> -
>  (defcustom package-gnupghome-dir (expand-file-name "gnupg" package-user-dir)
>    "Directory containing GnuPG keyring or nil.
>  This variable specifies the GnuPG home directory used by package.
> @@ -457,14 +451,18 @@ package--default-summary
>  
>  (define-inline package-vc-p (pkg-desc)
>    "Return non-nil if PKG-DESC is a VC package."
> -  (inline-letevals (pkg-desc)
> -    (inline-quote (eq (package-desc-kind ,pkg-desc) 'vc))))
> +  (inline-quote (eq (package-desc-kind ,pkg-desc) 'vc)))
> +
> +(define-inline package--unquote (arg)
> +  "Return ARG without its surrounding `quote', if any."
> +  (inline-letevals (arg)
> +    (inline-quote (if (eq (car-safe ,arg) 'quote) (cadr ,arg) ,arg))))

Honestly, the usage of define-inline was probably a premature
optimisation on my part, I don't think we really need it here either.
You removed (require 'inline) anyway, or is it now preloaded?

>  (cl-defstruct (package-desc
>                 ;; Rename the default constructor from `make-package-desc'.
>                 (:constructor package-desc-create)
>                 ;; Has the same interface as the old `define-package',
> -               ;; which is still used in the "foo-pkg.el" files. Extra
> +               ;; which is still used in the "foo-pkg.el" files.  Extra
>                 ;; options can be supported by adding additional keys.
>                 (:constructor
>                  package-desc-from-define
> @@ -472,15 +470,14 @@ package-vc-p
>                   &rest rest-plist
>                   &aux
>                   (name (intern name-string))
> -                 (version (if (eq (car-safe version-string) 'vc)
> -                              (version-to-list (cdr version-string))
> -                            (version-to-list version-string)))
> +                 (version (version-to-list
> +                           (if (eq (car-safe version-string) 'vc)
> +                               (cdr version-string)
> +                             version-string)))
>                   (reqs (mapcar (lambda (elt)
>                                   (list (car elt)
>                                         (version-to-list (cadr elt))))
> -                               (if (eq 'quote (car requirements))
> -                                   (nth 1 requirements)
> -                                 requirements)))
> +                               (package--unquote requirements)))
>                   (kind (plist-get rest-plist :kind))
>                   (archive (plist-get rest-plist :archive))
>                   (extras (let (alist)
> @@ -489,47 +486,42 @@ package-vc-p
>                                 (let ((value (cadr rest-plist)))
>                                   (when value
>                                     (push (cons (car rest-plist)
> -                                               (if (eq (car-safe value) 'quote)
> -                                                   (cadr value)
> -                                                 value))
> +                                               (package--unquote value))
>                                           alist))))
>                               (setq rest-plist (cddr rest-plist)))
>                             alist)))))
> -  "Structure containing information about an individual package.
> -Slots:
> -
> -`name'	Name of the package, as a symbol.
> -
> -`version' Version of the package, as a version list.
> -
> -`summary' Short description of the package, typically taken from
> -        the first line of the file.
> -
> -`reqs'	Requirements of the package.  A list of (PACKAGE
> -        VERSION-LIST) naming the dependent package and the minimum
> -        required version.
> -
> -`kind'	The distribution format of the package.  Currently, it is
> -        either `single' or `tar'.
> -
> -`archive' The name of the archive (as a string) whence this
> -        package came.
> -
> -`dir'	The directory where the package is installed (if installed),
> -        `builtin' if it is built-in, or nil otherwise.
> -
> -`extras' Optional alist of additional keyword-value pairs.
> -
> -`signed' Flag to indicate that the package is signed by provider."
> -  name
> -  version
> -  (summary package--default-summary)
> -  reqs
> -  kind
> -  archive
> -  dir
> -  extras
> -  signed)
> +  "Structure containing information about an individual package."
> +  (name
> +   nil :type symbol :documentation
> +   "Name of the package.")
> +  (version
> +   () :type list :documentation
> +   "Version of the package, as a version list.")
> +  (summary
> +   package--default-summary :type string :documentation
> +   "Short description of the package, typically taken from the first
> +line of the file.")
> +  (reqs
> +   () :type list :documentation
> +   "Requirements of the package.  A list of (PACKAGE VERSION-LIST)
> +naming the dependent package and the minimum required version.")
> +  (kind
> +   nil :type symbol :documentation
> +   "The distribution format of the package.  Currently, it is one of
> +`single', `tar', `dir', or `vc'.")
> +  (archive
> +   nil :type string :documentation
> +   "The name of the archive whence this package came.")
> +  (dir
> +   nil :type (or string symbol) :documentation
> +   "The directory where the package is installed (if installed),
> +`builtin' if it is built-in, or nil otherwise." )
> +  (extras
> +   () :type list :documentation
> +   "Optional alist of additional keyword-value pairs.")
> +  (signed
> +   nil :type boolean :documentation
> +   "Flag to indicate that the package is signed by provider."))
>  
>  (defun package--from-builtin (bi-desc)
>    "Create a `package-desc' object from BI-DESC.
> @@ -597,12 +589,9 @@ package-desc-suffix
>  (defun package-desc--keywords (pkg-desc)
>    "Return keywords of package-desc object PKG-DESC.
>  These keywords come from the foo-pkg.el file, and in general
> -corresponds to the keywords in the \"Keywords\" header of the
> +correspond to the keywords in the \"Keywords\" header of the
>  package."
> -  (let ((keywords (cdr (assoc :keywords (package-desc-extras pkg-desc)))))
> -    (if (eq (car-safe keywords) 'quote)
> -        (nth 1 keywords)
> -      keywords)))
> +  (package--unquote (cdr (assq :keywords (package-desc-extras pkg-desc)))))
>  
>  (defun package-desc-priority (pkg-desc)
>    "Return the priority of the archive of package-desc object PKG-DESC."
> @@ -978,8 +967,7 @@ package-untar-buffer
>  
>  (defun package--alist-to-plist-args (alist)
>    (mapcar #'macroexp-quote
> -          (apply #'nconc
> -                 (mapcar (lambda (pair) (list (car pair) (cdr pair))) alist))))
> +          (mapcan (lambda (pair) (list (car pair) (cdr pair))) alist)))
>  
>  (defun package-unpack (pkg-desc)
>    "Install the contents of the current buffer as a package."
> @@ -1032,37 +1020,43 @@ package-unpack
>          (package--reload-previously-loaded new-desc)))
>      pkg-dir))
>  
> +;; Potentially also used in elpa.git.
> +(defun package--write-description-file ( file name version doc reqs extras
> +                                         &optional extra-props verbose)
> +  "Write a `define-package' declaration to FILE.
> +Absolute FILE names the -pkg.el description file.
> +NAME, VERSION, and DOC are the leading, and EXTRA-PROPS the
> +trailing, arguments of `define-package'.
> +REQS and EXTRAS are the eponymous `package-desc' slots.
> +Non-nil VERBOSE means display \"Wrote file\" message."
> +  (let* ((src (replace-regexp-in-string (rx "-pkg.el" eos) ".el"
> +                                        (file-name-nondirectory file) t t))
> +         (def `(define-package ,name ,version ,doc
> +                 ,(macroexp-quote
> +                   ;; Turn requirement version lists into string form.
> +                   (mapcar (lambda (elt)
> +                             (list (car elt)
> +                                   (package-version-join (cadr elt))))
> +                           reqs))
> +                 ,@extra-props
> +                 ,@(package--alist-to-plist-args extras)))
> +         (print-cfg '((length . nil)
> +                      (level . nil)
> +                      (quoted . t)))
> +         (str (concat ";;; Generated package description from " src
> +                      "  -*- no-byte-compile: t; lexical-binding: t -*-\n"
> +                      (prin1-to-string def nil print-cfg)
> +                      "\n")))
> +    (write-region str nil file nil (unless verbose 'silent))))

I like this, but we should really make sure that there are no hidden
edge-cases that might cause problems.

> +
>  (defun package-generate-description-file (pkg-desc pkg-file)
>    "Create the foo-pkg.el file PKG-FILE for single-file package PKG-DESC."
> -  (let* ((name (package-desc-name pkg-desc)))
> -    (let ((print-level nil)
> -          (print-quoted t)
> -          (print-length nil))
> -      (write-region
> -       (concat
> -        ";;; Generated package description from "
> -        (replace-regexp-in-string "-pkg\\.el\\'" ".el"
> -                                  (file-name-nondirectory pkg-file))
> -        "  -*- no-byte-compile: t -*-\n"
> -        (prin1-to-string
> -         (nconc
> -          (list 'define-package
> -                (symbol-name name)
> -                (package-version-join (package-desc-version pkg-desc))
> -                (package-desc-summary pkg-desc)
> -                (let ((requires (package-desc-reqs pkg-desc)))
> -                  (list 'quote
> -                        ;; Turn version lists into string form.
> -                        (mapcar
> -                         (lambda (elt)
> -                           (list (car elt)
> -                                 (package-version-join (cadr elt))))
> -                         requires))))
> -          (package--alist-to-plist-args
> -           (package-desc-extras pkg-desc))))
> -        "\n")
> -       nil pkg-file nil 'silent))))
> -
> +  (let ((name (symbol-name (package-desc-name pkg-desc)))
> +        (ver (package-version-join (package-desc-version pkg-desc)))
> +        (doc (package-desc-summary pkg-desc))
> +        (reqs (package-desc-reqs pkg-desc))
> +        (extras (package-desc-extras pkg-desc)))
> +    (package--write-description-file pkg-file name ver doc reqs extras)))
>  
>  ;;;; Autoload
>  (declare-function autoload-rubric "autoload" (file &optional type feature))
> @@ -1311,11 +1305,6 @@ package--archive-file-exists-p
>            (url-http-file-exists-p (concat location file)))
>        (file-exists-p (expand-file-name file location)))))
>  
> -(declare-function epg-make-context "epg"
> -                  (&optional protocol armor textmode include-certs
> -                             cipher-algorithm
> -                             digest-algorithm
> -                             compress-algorithm))
>  (declare-function epg-verify-string "epg" ( context signature
>                                              &optional signed-text))
>  (declare-function epg-context-result-for "epg" (context name))
> @@ -1397,7 +1386,6 @@ package--with-response-buffer-1
>                   url
>                   (lambda (status)
>                     (let ((b (current-buffer)))
> -                     (require 'url-handlers)
>                       (package--unless-error body
>                         (when-let* ((er (plist-get status :error)))
>                           (error "Error retrieving: %s %S" url er))

bug#67916: 30.0.50; No lexical-binding directive warning in -pkg.el files

[Basil L. Contovounesios via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Philip Kaludercic [2023-12-20 15:14 +0000] wrote:

> "Basil L. Contovounesios" <contovob@tcd.ie> writes:
>
>> package-vc--generate-description-file currently passes:
>>   :kind 'vc
>> unquoted to define-package, which results in the -pkg.el contents:
>>   (define-package ... :kind vc ... :keywords '(...) ...)
>> Note the difference in quoting between :kind and :keywords.  Is this
>> intentional?  Or can/should :kind pass through macroexp-quote as well?
>
> This is not intentional, if anything a lucky oversight.

Lucky in the sense that it's preferable this way, or just an accident? ;)

>> Questions for Stefan:
>>
>> - Which version of Emacs can/does elpa-admin.el assume?
>
> All I can say is that the ELPA build server, the main user of
> elpa-admin.el, has Emacs 28.2 installed.

I'm wondering because elpa-admin.el seems to contain some compatibility
code for Emacs 26 (elpaa--select-revision, elpaa--write-pkg-file) and
Emacs versions <28 (elpaa--get-section, elpaa--html-build-doc).

>>  (defgroup package nil
>>    "Manager for Emacs Lisp packages."
>>    :group 'applications
>> -  :version "24.1")
>> +  :group 'tools
>> +  :version "30.1")
>
> I am not sure if bumping :version is necessary (here and above).

I think it's unnecessary in the sense that I don't know of any place
where this information is displayed, but otherwise I thought it was good
form to do this since any change in :groups is user-visible.

>>  (define-inline package-vc-p (pkg-desc)
>>    "Return non-nil if PKG-DESC is a VC package."
>> -  (inline-letevals (pkg-desc)
>> -    (inline-quote (eq (package-desc-kind ,pkg-desc) 'vc))))
>> +  (inline-quote (eq (package-desc-kind ,pkg-desc) 'vc)))
>> +
>> +(define-inline package--unquote (arg)
>> +  "Return ARG without its surrounding `quote', if any."
>> +  (inline-letevals (arg)
>> +    (inline-quote (if (eq (car-safe ,arg) 'quote) (cadr ,arg) ,arg))))
>
> Honestly, the usage of define-inline was probably a premature
> optimisation on my part, I don't think we really need it here either.

You mean, you prefer package--unquote being a plain function?

[ To be honest, I'm slightly inclined to add this to macroexp.el
  instead, since it's a somewhat common operation. ]

> You removed (require 'inline) anyway, or is it now preloaded?

define-inline is autoloaded, and there are no other in-tree occurrences
of (require 'inline).

>> +;; Potentially also used in elpa.git.
>> +(defun package--write-description-file ( file name version doc reqs extras
>> +                                         &optional extra-props verbose)
>> +  "Write a `define-package' declaration to FILE.
>> +Absolute FILE names the -pkg.el description file.
>> +NAME, VERSION, and DOC are the leading, and EXTRA-PROPS the
>> +trailing, arguments of `define-package'.
>> +REQS and EXTRAS are the eponymous `package-desc' slots.
>> +Non-nil VERBOSE means display \"Wrote file\" message."
>> +  (let* ((src (replace-regexp-in-string (rx "-pkg.el" eos) ".el"
>> +                                        (file-name-nondirectory file) t t))
>> +         (def `(define-package ,name ,version ,doc
>> +                 ,(macroexp-quote
>> +                   ;; Turn requirement version lists into string form.
>> +                   (mapcar (lambda (elt)
>> +                             (list (car elt)
>> +                                   (package-version-join (cadr elt))))
>> +                           reqs))
>> +                 ,@extra-props
>> +                 ,@(package--alist-to-plist-args extras)))
>> +         (print-cfg '((length . nil)
>> +                      (level . nil)
>> +                      (quoted . t)))
>> +         (str (concat ";;; Generated package description from " src
>> +                      "  -*- no-byte-compile: t; lexical-binding: t -*-\n"
>> +                      (prin1-to-string def nil print-cfg)
>> +                      "\n")))
>> +    (write-region str nil file nil (unless verbose 'silent))))
>
> I like this, but we should really make sure that there are no hidden
> edge-cases that might cause problems.

How do we find them if they're hidden? ;)
Did you have something specific in mind?

Thanks,
-- 
Basil

bug#67916: 30.0.50; No lexical-binding directive warning in -pkg.el files

[Philip Kaludercic]

"Basil L. Contovounesios" <contovob@tcd.ie> writes:

> Philip Kaludercic [2023-12-20 15:14 +0000] wrote:
>
>> "Basil L. Contovounesios" <contovob@tcd.ie> writes:
>>
>>> package-vc--generate-description-file currently passes:
>>>   :kind 'vc
>>> unquoted to define-package, which results in the -pkg.el contents:
>>>   (define-package ... :kind vc ... :keywords '(...) ...)
>>> Note the difference in quoting between :kind and :keywords.  Is this
>>> intentional?  Or can/should :kind pass through macroexp-quote as well?
>>
>> This is not intentional, if anything a lucky oversight.
>
> Lucky in the sense that it's preferable this way, or just an accident? ;)

The latter.

>>> Questions for Stefan:
>>>
>>> - Which version of Emacs can/does elpa-admin.el assume?
>>
>> All I can say is that the ELPA build server, the main user of
>> elpa-admin.el, has Emacs 28.2 installed.
>
> I'm wondering because elpa-admin.el seems to contain some compatibility
> code for Emacs 26 (elpaa--select-revision, elpaa--write-pkg-file) and
> Emacs versions <28 (elpaa--get-section, elpaa--html-build-doc).

My guess is that it was just not removed, but I'll just let Stefan
explain that.

>>>  (defgroup package nil
>>>    "Manager for Emacs Lisp packages."
>>>    :group 'applications
>>> -  :version "24.1")
>>> +  :group 'tools
>>> +  :version "30.1")
>>
>> I am not sure if bumping :version is necessary (here and above).
>
> I think it's unnecessary in the sense that I don't know of any place
> where this information is displayed, but otherwise I thought it was good
> form to do this since any change in :groups is user-visible.

My understanding was that this information is supposed to indicate when
the group was added, while each member of a group that has since been
changed would have a newer :version tag.

>>>  (define-inline package-vc-p (pkg-desc)
>>>    "Return non-nil if PKG-DESC is a VC package."
>>> -  (inline-letevals (pkg-desc)
>>> -    (inline-quote (eq (package-desc-kind ,pkg-desc) 'vc))))
>>> +  (inline-quote (eq (package-desc-kind ,pkg-desc) 'vc)))
>>> +
>>> +(define-inline package--unquote (arg)
>>> +  "Return ARG without its surrounding `quote', if any."
>>> +  (inline-letevals (arg)
>>> +    (inline-quote (if (eq (car-safe ,arg) 'quote) (cadr ,arg) ,arg))))
>>
>> Honestly, the usage of define-inline was probably a premature
>> optimisation on my part, I don't think we really need it here either.
>
> You mean, you prefer package--unquote being a plain function?

Yes.

> [ To be honest, I'm slightly inclined to add this to macroexp.el
>   instead, since it's a somewhat common operation. ]

My only concern is that this would slightly complicate the initiative of
adding package.el to GNU ELPA, if that is to proceed at all.

>> You removed (require 'inline) anyway, or is it now preloaded?
>
> define-inline is autoloaded, and there are no other in-tree occurrences
> of (require 'inline).

Nevermind then.

>>> +;; Potentially also used in elpa.git.
>>> +(defun package--write-description-file ( file name version doc reqs extras
>>> +                                         &optional extra-props verbose)
>>> +  "Write a `define-package' declaration to FILE.
>>> +Absolute FILE names the -pkg.el description file.
>>> +NAME, VERSION, and DOC are the leading, and EXTRA-PROPS the
>>> +trailing, arguments of `define-package'.
>>> +REQS and EXTRAS are the eponymous `package-desc' slots.
>>> +Non-nil VERBOSE means display \"Wrote file\" message."
>>> +  (let* ((src (replace-regexp-in-string (rx "-pkg.el" eos) ".el"
>>> +                                        (file-name-nondirectory file) t t))
>>> +         (def `(define-package ,name ,version ,doc
>>> +                 ,(macroexp-quote
>>> +                   ;; Turn requirement version lists into string form.
>>> +                   (mapcar (lambda (elt)
>>> +                             (list (car elt)
>>> +                                   (package-version-join (cadr elt))))
>>> +                           reqs))
>>> +                 ,@extra-props
>>> +                 ,@(package--alist-to-plist-args extras)))
>>> +         (print-cfg '((length . nil)
>>> +                      (level . nil)
>>> +                      (quoted . t)))
>>> +         (str (concat ";;; Generated package description from " src
>>> +                      "  -*- no-byte-compile: t; lexical-binding: t -*-\n"
>>> +                      (prin1-to-string def nil print-cfg)
>>> +                      "\n")))
>>> +    (write-region str nil file nil (unless verbose 'silent))))
>>
>> I like this, but we should really make sure that there are no hidden
>> edge-cases that might cause problems.
>
> How do we find them if they're hidden? ;)
> Did you have something specific in mind?

IIRC there were some bugs related to the generation of -pkg.el files,
but I can't find them right now.  I'll ping you if I find anything.

> Thanks,

bug#67916: 30.0.50; No lexical-binding directive warning in -pkg.el files

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> I'm wondering because elpa-admin.el seems to contain some compatibility
> code for Emacs 26 (elpaa--select-revision, elpaa--write-pkg-file) and
> Emacs versions <28 (elpaa--get-section, elpaa--html-build-doc).

AFAIK `elpa-admin.el` is mostly used with "Emacs from Debian stable" (in
`elpa.gnu.org`) and with the bleeding edge of Emacs, so backward
compatibility is not very important beyond Emacs-28 now.  The one you
see is what was added at the time and simply hasn't been removed (yet).

> You mean, you prefer package--unquote being a plain function?
> [ To be honest, I'm slightly inclined to add this to macroexp.el
>   instead, since it's a somewhat common operation. ]

Usually in the context of macro expansion you can use `eval` for that.
Here we don't, for (arguably bogus) "security" reasons.


        Stefan