[Feature]: lint elpaca macro BODY

[elizagamedev]

Elpaca Version

Elpaca a216ca8 HEAD -> master, origin/master, origin/HEAD
installer:      0.5
emacs-version:  GNU Emacs 29.0.92 (build 1, x86_64-pc-linux-gnu, GTK+ Version 3.24.38, cairo version 1.16.0)
git --version:  git version 2.40.1

Operating System

Linux, nixOS

Description

To reproduce, just enable flymake in an elisp buffer on a recent-ish version of Emacs and write some nonsense forms in an (elpaca ...) macro call. The documentation isn't really clear when elisp-flymake-byte-compile was added so I'm not sure of the specific version.

The issue seems to be that the form isn't compiled at compile time, which makes sense, given that the package might not exist at compile time. The only way I think this could be fixed is if the elpaca macro detected that it's being evaluated in the context of elisp-flymake-byte-compile and the entire macro short-circuited into something like...

(require foo nil t)
(body...)

...which would introduce the misconception to the byte compiler that that package is now available past that (elpaca ...) call, but it's better than nothing.

I'm not sure if this is out of the scope of elpaca or not, or if it should be considered a bug report or a feature request. I feel like perhaps I'm a minority for using flymake with elisp instead of flycheck. I might also try fixing this myself, but I wanted to at least report a bug with my assumptions in case I'm not alone here.

[progfolio]

Hi. Thanks for taking the time to fill out a bug report.

The issue seems to be that the form isn't compiled at compile time, which makes
sense, given that the package might not exist at compile time.

The BODY of the elpaca macro expansion was being stored as a quoted list.
This prevents the byte-compiler from looking at it as anything more than data.
I've changed the strategy by which the forms are stored, which should resolve the issue.

I feel like perhaps I'm a minority for using flymake with elisp instead of flycheck.

I use flymake, but I believe either tool would've run into the same issue.

You should be able to update Elpaca via M-x elpaca-update elpaca followed by M-x restart-emacs. Testing is appreciated. Thank you.

[elizagamedev]

Thank you for the really quick fix! I only got to try it out just today, but it seems like it's working so far. There does seem to be a remaining issue with trying to access autoloads without an explicit, redundant require statement. For example,

(require 'page-break-lines-autoloads nil t)
(elpaca page-break-lines
  (global-page-break-lines-mode 1))

I should also mention that I've been advising elisp-flymake-byte-compile so that it picks up my elpaca-installed packages. (Maybe not the safest advice?)

(define-advice elisp-flymake-byte-compile
    (:around (orig-fun &rest args))
  (let ((elisp-flymake-byte-compile-load-path load-path))
    (apply orig-fun args)))

I tweaked the elpaca macro to insert that redundant (require *-autoloads nil t) if it was being evaluated during compile time. In order to fully satisfy the byte compiler, it had to be at the top of a (progn) outside of the (let).

(defmacro elpaca (order &rest body)
  "Queue ORDER for installation/activation, defer execution of BODY.
If ORDER is `nil`, defer BODY until orders have been processed."
  (declare (indent 1) (debug t))
  (let ((o (gensym "order-")) (item (gensym "item-")) (q (gensym "q-")))
    `(progn
       ,@(if (and order (bound-and-true-p byte-compile-current-file))
             `((require (quote ,(intern (concat (symbol-name (elpaca--first order))
                                                "-autoloads")))
                        nil t)))
       (let* ((,o ,@(if (memq (car-safe order) '(quote \`)) `(,order) `(',order)))
              (,item (elpaca--first ,o))
              (,q (or (and after-init-time (elpaca--q (elpaca-get ,item))) (car elpaca--queues))))
         ,@(when body
             `((if ,item
                   (setf (alist-get ,item (elpaca-q<-forms ,q)) (lambda () ,@body))
                 ;;@FIX: nil semantics not good for multiple deferred...
                 (push (cons ,item (lambda () ,@body)) (elpaca-q<-forms ,q)))))
         (when ,o (elpaca--queue ,o ,q))
         (when after-init-time
           (when-let ((e (elpaca-get ,item)))
             (elpaca--maybe-log)
             (elpaca--unprocess e)
             (push 'queued (elpaca<-statuses e)))
           (when (member this-command '(eval-last-sexp eval-defun)) (elpaca-process-queues))
           (when (member this-command '(eval-region eval-buffer org-ctrl-c-ctrl-c))
             (when elpaca--interactive-timer (cancel-timer elpaca--interactive-timer))
             (run-at-time elpaca-interactive-interval nil #'elpaca-process-queues)))
         nil))))

For the first time ever, flymake is giving me useful feedback for my init.el. :)

Patch below, though it modifies basically the entire function due to the changed indentation.

diff --git a/elpaca.el b/elpaca.el
index 28a5503..d3f88bb 100644
--- a/elpaca.el
+++ b/elpaca.el
@@ -1457,25 +1457,30 @@ When MESSAGE is non-nil, message the list of dependents."
 If ORDER is `nil`, defer BODY until orders have been processed."
   (declare (indent 1) (debug t))
   (let ((o (gensym "order-")) (item (gensym "item-")) (q (gensym "q-")))
-    `(let* ((,o ,@(if (memq (car-safe order) '(quote \`)) `(,order) `(',order)))
-            (,item (elpaca--first ,o))
-            (,q (or (and after-init-time (elpaca--q (elpaca-get ,item))) (car elpaca--queues))))
-       ,@(when body
-           `((if ,item
-                 (setf (alist-get ,item (elpaca-q<-forms ,q)) (lambda () ,@body))
-               ;;@FIX: nil semantics not good for multiple deferred...
-               (push (cons ,item (lambda () ,@body)) (elpaca-q<-forms ,q)))))
-       (when ,o (elpaca--queue ,o ,q))
-       (when after-init-time
-         (when-let ((e (elpaca-get ,item)))
-           (elpaca--maybe-log)
-           (elpaca--unprocess e)
-           (push 'queued (elpaca<-statuses e)))
-         (when (member this-command '(eval-last-sexp eval-defun)) (elpaca-process-queues))
-         (when (member this-command '(eval-region eval-buffer org-ctrl-c-ctrl-c))
-           (when elpaca--interactive-timer (cancel-timer elpaca--interactive-timer))
-           (run-at-time elpaca-interactive-interval nil #'elpaca-process-queues)))
-       nil)))
+    `(progn
+       ,@(if (and order (bound-and-true-p byte-compile-current-file))
+             `((require (quote ,(intern (concat (symbol-name (elpaca--first order))
+                                                "-autoloads")))
+                        nil t)))
+       (let* ((,o ,@(if (memq (car-safe order) '(quote \`)) `(,order) `(',order)))
+              (,item (elpaca--first ,o))
+              (,q (or (and after-init-time (elpaca--q (elpaca-get ,item))) (car elpaca--queues))))
+         ,@(when body
+             `((if ,item
+                   (setf (alist-get ,item (elpaca-q<-forms ,q)) (lambda () ,@body))
+                 ;;@FIX: nil semantics not good for multiple deferred...
+                 (push (cons ,item (lambda () ,@body)) (elpaca-q<-forms ,q)))))
+         (when ,o (elpaca--queue ,o ,q))
+         (when after-init-time
+           (when-let ((e (elpaca-get ,item)))
+             (elpaca--maybe-log)
+             (elpaca--unprocess e)
+             (push 'queued (elpaca<-statuses e)))
+           (when (member this-command '(eval-last-sexp eval-defun)) (elpaca-process-queues))
+           (when (member this-command '(eval-region eval-buffer org-ctrl-c-ctrl-c))
+             (when elpaca--interactive-timer (cancel-timer elpaca--interactive-timer))
+             (run-at-time elpaca-interactive-interval nil #'elpaca-process-queues)))
+         nil))))
 
 (defcustom elpaca-wait-interval 0.01 "Seconds between `elpaca-wait' status checks."
   :type 'number)

EDIT: For the sake of completeness I should mention that there are very obvious legitimate issues suppressed by this change, for example

(elpaca foo)

(foo-some-function)

but the difference here is that flymake is mostly useless, rather than mostly correct.

[elizagamedev]

Hmm, actually, it seems like that change introduces big issues, like breaking flymake diagnostics for obviously nonexistent functions. I'll keep messing around with it.

[progfolio]

Thank you for the really quick fix! I only got to try it out just today, but it
seems like it's working so far.

You're welcome. Thanks for testing.

There does seem to be a remaining issue with
trying to access autoloads without an explicit, redundant require statement. For
example,

(require 'page-break-lines-autoloads nil t)
(elpaca page-break-lines
  (global-page-break-lines-mode 1))

The byte compiler doesn't normally know about autoloads.
You'd either need to explicitly require the feature as in your example, or declare the symbols using declare-function or defvar. e.g.

(reuqire 'elpaca)

(elpaca page-break-lines
  (declare-function global-page-break-lines-mode "page-break-lines")
  (global-page-break-lines-mode 1))

I tweaked the elpaca macro to insert that...

That is more in the realm of a configuration macro than the package manager itself. For example, use-package offers the :defines and :functions keywords for this purpose:

https://github.com/jwiegley/use-package#byte-compiling-your-emacs

I don't plan on adding anything to the elpaca macro in that regard.

I should also mention that I've been advising elisp-flymake-byte-compile so that it picks up my elpaca-installed packages. (Maybe not the safest advice?)

Rather than advice, you should be able to set elisp-flymake-byte-compile-load-path.
e.g.

(setq elisp-flymake-byte-compile-load-path
      (nthcdr 2 (directory-files elpaca-builds-directory 'full ".*")))

It looks like the original issue is taken care of, so I'll consider this closed. If you think anything else needs to be addressed, please let me know. Thanks.

[progfolio]

I'll have to see if I can find another solution for this. Eager macro expansion can cause breakage when loading an init file with the fix I applied.