magit-todo-keywords incompatible with use-package :custom

[zhaojiangbin]

With the following minimal setup:

(use-package magit-todos
  :load-path "~/CodeWorks/emacs/magit-todos"
  :commands (magit-todos-mode)
  :hook (magit-mode . magit-todos-mode)
  :config
  (setq magit-todos-recursive t
        magit-todos-depth 100)
  :custom (magit-todos-keywords (list "TODO" "FIXME")))

Trying to run M-x magit-status for the first time after Emacs restarts resulted in error: "user-error: Please add some keywords". Re-eval the above snippet fixes it.

In comparison, using custom-set-variables within :config just works:

(use-package magit-todos
  ;; :ensure t
  :load-path "~/CodeWorks/emacs/magit-todos"
  :commands (magit-todos-mode)
  :hook (magit-mode . magit-todos-mode)
  :config
  (setq magit-todos-recursive t
        magit-todos-depth 100)
  (custom-set-variables
   '(magit-todos-keywords (list "TODO" "FIXME")))
  ;; :custom (magit-todos-keywords (list "TODO" "FIXME"))
  )

[alphapapa]

Thanks. I don't know what's going on there. It's hard to say whether it's a bug in this or in use-package (but I would never bet against John, haha). You'll have to look at the macro-expansion of the use-package form to see what it's doing. I think the :custom keyword was added relatively recently, so there could be an issue with the more complex way we're initializing custom variables. But maybe we just need to change the order of some of the defcustoms.

However, when we move the regexp creation to the scan invocations, this probably won't matter anymore. So I'd suggest leaving this until that change is made.

[zhaojiangbin]

However, when we move the regexp creation to the scan invocations, this probably won't matter anymore. So I'd suggest leaving this until that change is made.

Sounds good.

[alphapapa]

This should be "fixed" in this branch: #27

[zhaojiangbin]

Still happening in master f7c2b2d.

It appears that use-package expands a :custom keyword into a customize-set-varaible call before require 'magit-todos.

The :config keyword, in comparison, is expanded into a progn form after 'magit-todos is required. Using either custom-set-variables or customize-set-variable works here.

[alphapapa]

There are a lot of variables at play here.

magit-todos-keywords has a custom setter. If magit-todos is not loaded yet, then I guess its custom setter isn't loaded. And if customize-set-variable is called before magit-todos is loaded, I don't know exactly what happens, and I don't know what happens later when magit-todos is loaded. I don't know if this is an issue in magit-todos or in use-package. Maybe use-package needs to call custom-reevaluate-setting for each :custom setting after the package is loaded.

I guess I need to move the logic from the custom setter for magit-todos-keywords into the scanner command so it's always done dynamically.

I don't know about the other custom variables.

[zhaojiangbin]

This is what the README of 'use-package' says about :custom (emphasis mine):

NOTE: These are only for people who wish to keep customizations with their accompanying use-package declarations. Functionally, the only benefit over using setq in a :config block is that customizations might execute code when values are assigned.

It should call customize-set-variable after the library is loaded otherwise how could the library code run, right?

[alphapapa]

Here's what this:

(use-package magit-todos
  :custom ((magit-todos-depth 25)
           (magit-todos-keywords '("TODO"))))

expands to:

(progn
  (defvar use-package--warning93
    #'(lambda
        (keyword err)
        (let
            ((msg
              (format "%s/%s: %s" 'magit-todos keyword
                      (error-message-string err))))
          (display-warning 'use-package msg :error))))
  (condition-case-unless-debug err
      (progn
        (customize-set-variable 'magit-todos-depth 25 "Customized with use-package magit-todos")
        (customize-set-variable 'magit-todos-keywords
                                '("TODO")
                                "Customized with use-package magit-todos")
        (if
            (not
             (require 'magit-todos nil t))
            (display-warning 'use-package
                             (format "Cannot load %s" 'magit-todos)
                             :error)))
    (error
     (funcall use-package--warning93 :catch err))))

As you can see, it require's after doing customize-set-variable. The docstring for customize-set-variable says:

If VARIABLE has a ‘custom-set’ property, that is used for setting VARIABLE, otherwise ‘set-default’ is used.

set-default says:

set-default is a built-in function in ‘C source code’. ... Set SYMBOL’s default value to VALUE. SYMBOL and VALUE are evaluated. The default value is seen in buffers that do not have their own values for this variable.

Looking at the docstring for defcustom, as best I can tell, this means that the magit-todo-keywords custom-set property's setter will not be run, because the package has not been required yet.

However, it seems strange to me that this would be the case, because it seems like something that should have been considered when :custom was added to use-package.

[alphapapa]

Indeed, putting those calls to customize-set-variable inside with-eval-after-load seems to fix the problem:

(progn
  (defvar use-package--warning93
    #'(lambda
        (keyword err)
        (let
            ((msg
              (format "%s/%s: %s" 'magit-todos keyword
                      (error-message-string err))))
          (display-warning 'use-package msg :error))))
  (condition-case-unless-debug err
      (progn
        (with-eval-after-load 'magit-todos
          (customize-set-variable 'magit-todos-depth 25 "Customized with use-package magit-todos")
          (customize-set-variable 'magit-todos-keywords
                                  '("TODO")
                                  "Customized with use-package magit-todos"))
        (if
            (not
             (require 'magit-todos nil t))
            (display-warning 'use-package
                             (format "Cannot load %s" 'magit-todos)
                             :error)))
    (error
     (funcall use-package--warning93 :catch err))))

After doing that in an Emacs instance in which magit-todos has not yet been loaded, magit-todos-keywords is set to ("TODO").

So this appears to be a bug in use-package.

[alphapapa]

Filed as jwiegley/use-package#702

[ambihelical]

Is it really necessary to have dependent variables updated in the defcustom? If possible it would be preferable if this was done on the fly when a change is detected in magit-todos-keywords (by checking the value used when the dependent variable(s) were last updated). This way customize-set-variable is not required, setq will work in an use-package :init section, which is I think what most people expect.

[alphapapa]

This way customize-set-variable is not required, setq will work in an use-package :init section, which is I think what most people expect.

These are defined with defcustom, not defvar, and they use functionality defined in custom.el, so setq is not the correct way to set their values. Sometimes it works, but users shouldn't expect it to, because there are defined ways to set defcustom variables.

in an use-package :init section

The :init section is definitely the wrong way to set a variable defined in a package, because :init is evaluated before the package is loaded. :config can be used for defvar variables with setq, but again, setq shouldn't be expected to work with defcustoms.

[ambihelical]

You are right, all the :init setq's that I've looked in my init.el are for defcustom vars, defvars I have in :config. I suppose I should fix the defcustoms to be 100% correct. It appears that defcustom side-effect code isn't very common, otherwise I think I would have had more issues.

[alphapapa]

It appears that defcustom side-effect code isn't very common, otherwise I think I would have had more issues.

I think you're right; I don't often see it myself. It's not often necessary. But it is there when you need it.