Formatter defined with `set-formatter!` does not gets executed on hook

vincentbernat commented 1 year ago

I confirm that...

[X] I have searched the issue tracker, documentation, FAQ, Discourse, and Google, in case this issue has already been reported/resolved.
[X] I have read "How to Debug Issues", and will use it to provide as much information about this issue as possible.
[X] The issue can be reproduced on the latest available commit of Doom.
[X] The issue can be reproduced on a stable release of Emacs, such as 27 or 28. (Doom does not support development builds like 29+ or any version ending in .50 or .9x)

Expected behavior

When using set-formatter!, the provided formatter should be executed when saving on the provided mode. For example:

(set-formatter! 'nixpkgs-fmt "nixpkgs-fmt" :modes '(nix-mode))

Current behavior

Invoking format-all-buffer--from-hook does not invoke the formatter because no executable is associated with it. editor/format/config.el contains:

;; Don't pop up imposing warnings about missing formatters, but still log it in
;; to *Messages*.
(defadvice! +format--all-buffer-from-hook-a (fn &rest args)
  :around #'format-all-buffer--from-hook
  (letf! (defun format-all-buffer--with (formatter mode-result)
           (when (or (eq formatter 'lsp)
                     (eq formatter 'eglot)
                     (condition-case-unless-debug e
                         (format-all--formatter-executable formatter)
                       (error
                        (message "Warning: cannot reformat buffer because %S isn't installed"
                                 (gethash formatter format-all--executable-table))
                        nil)))
             (funcall format-all-buffer--with formatter mode-result)))
    (apply fn args)))

(format-all--formatter-executable 'nixpkgs-fmt) returns nil. I don't see anywhere in the code of set-formatter! a way to bind an executable (despite the documentation using a :executable keyword for one of the examples).

Steps to reproduce

In config.el:

(after! format-all
(set-formatter! 'nixpkgs-fmt "nixpkgs-fmt" :modes '(nix-mode)))

Open a nix file
Try to format it M-: (format-all-buffer--from-hook)

System Information

https://pastebin.com/drwxxyDX

vincentbernat commented 1 year ago

To confirm the issue, it works when I use:

(after! nix-mode
  (set-formatter! 'nixpkgs-fmt "nixpkgs-fmt" :modes '(nix-mode))
  (puthash 'nixpkgs-fmt "nixpkgs-fmt" format-all--executable-table))

I can propose a patch for this, but modules/editor is on the no-PR list.

BenediktBroich commented 1 year ago

Same error when using (set-formatter! 'alejandra "alejandra" :modes '(nix-mode)).

SPC m p in nix-mode-map should not invoke (nix-format-buffer) when a different formatter is set. Since nix-format-buffer from nix-format.el can only use nixfmt.

Here is the same fix for alejandra. So it is more easily discoverable if someone else is searching for it.

(after! nix-mode
  (set-formatter! 'alejandra "alejandra" :modes '(nix-mode))
  (puthash 'alejandra "alejandra" format-all--executable-table))

chairfield commented 1 year ago

The current implementation of Doom's +format--all-buffer-from-hook-a function is failing to format ledger-mode or bibtex-mode out of the box, even without a set-formatter! call. The root cause appears to be the same; Doom's advice function requires the existence of a formatter executable, even though some formatters don't require one.

Example in upstream lassik/emacs-format-all-the-code/format-all.el:

(define-format-all-formatter ledger-mode
  (:executable)
  (:install)
  (:modes ledger-mode)
  (:format
   (format-all--buffer-native 'ledger-mode 'ledger-mode-clean-buffer)))

Rather than further hack around the existing hack (Doom's editor/format/config.el has this function under the ;;; Hacks heading), I've disabled +format--all-buffer-from-hook-a altogether. I think I'll be okay having format-all-mode warn me about missing formatters.

In my config.el:

(advice-remove 'format-all-buffer--from-hook '+format--all-buffer-from-hook-a)

asymmetric commented 1 year ago

I added

(after! nix-mode
  (set-formatter! 'nixpkgs-fmt "nixpkgs-fmt" :modes '(nix-mode))
  (puthash 'nixpkgs-fmt "nixpkgs-fmt" format-all--executable-table))

to my config.el, but when I type SPC m p I still get `Could not locate "nixfmt". Any idea why?

peterhoeg commented 1 year ago

I think you're dealing with a race issue here. set-formatter! does (after! format-all which I think is what you need to do with the puthash bit:

(set-formatter! 'nixpkgs-fmt "nixpkgs-fmt" :modes '(nix-mode))
(after! format-all
  (puthash 'nixpkgs-fmt "nixpkgs-fmt" format-all--executable-table))

I simply gave up fighting format-all and merged https://github.com/doomemacs/doomemacs/pull/6369 to my doom fork and it works wonderfully.

panchoh commented 6 months ago

Hi, @asymmetric.

In case this is still causing you trouble, I think I can shed some light on the issue. It seems that nix-mode expects nixfmt by default. You can tweak that by adding to your config.el:

(use-package! nix-mode
  :custom (nix-nixfmt-bin "nixpkgs-fmt"))

There are currently three different interfaces for the formatter: nix-mode, apheleia and lsp-nix, it begs for some unification). In my config you have a workable solution (albeit tweaked for alejandra).

Hope that it helps.

hlissner commented 1 month ago

The :editor format module replaced format-all with Apheleia in 4ecd616cd8f60640d6b0be2ffc3d762e88099bb4, so I will close this issue as it's obsolete (and likely resolved). Thanks for bringing it to my attention, in any case!

doomemacs / doomemacs