Bug with Emacs 29 setopt does not correctly honor modus defcustom types

[shipmints]

Not sure if this is an issue with modus defcustom definitions or with the implementation of setopt itself. This is FYI as more people might start using setopt and you do briefly mention setopt in the modus documentation. If setopt is broken, it might be good to report a bug before Emacs 30 goes to press. Or this could just be my misunderstanding.

Emacs 29.3, and the most recent ELPA modus-themes-20240905...

The literal example from the docstring works

  (setopt modus-themes-headings
          (quote ((1 . (variable-pitch 1.5))
                  (2 . (1.3))
                  (agenda-date . (1.3))
                  (agenda-structure . (variable-pitch light 1.8))
                  (t . (1.1)))))

Amended with level 3 works

  (setopt modus-themes-headings
          '((1 . (variable-pitch 1.5))
            (2 . (1.3))
            (3 . (variable-pitch 1.8))
            (agenda-date . (1.3))
            (agenda-structure . (variable-pitch light 1.8))
            (t . (1.1))))

But breaks with overline added

  (setopt modus-themes-headings
          '((1 . (variable-pitch 1.5))
            (2 . (1.3))
            (3 . (overline variable-pitch 1.8))
            (agenda-date . (1.3))
            (agenda-structure . (variable-pitch light 1.8))
            (t . (1.1))))

⛔ Warning (emacs): Value '((1 variable-pitch 1.5) (2 1.3) (3 overline variable-pitch 1.8) (agenda-date 1.3) (agenda-structure variable-pitch light 1.8) (t 1.1))' does not match type (alist :options ((0 #1=(set :tag Properties :greedy t (const :tag Proportionately spaced font (variable-pitch) variable-pitch) (choice :tag Font weight (must be supported by the typeface) (const :tag Unspecified (use whatever the default is) nil) (const :tag Thin thin) (const :tag Ultra-light ultralight) (const :tag Extra-light extralight) (const :tag Light light) (const :tag Semi-light semilight) (const :tag Regular regular) (const :tag Medium medium) (const :tag Semi-bold semibold) (const :tag Bold bold) (const :tag Extra-bold extrabold) (const :tag Ultra-bold ultrabold)) (radio :tag Height (float :tag Floating point to adjust height by) (cons :tag Cons cell of `(height . FLOAT)' (const :tag The `height' key (constant) height) (float :tag Floating point))))) (1 #1#) (2 #1#) (3 #1#) (4 #1#) (5 #1#) (6 #1#) (7 #1#) (8 #1#) (t #1#) (agenda-date #1#) (agenda-structure #1#)) :key-type symbol :value-type #1#)

Similarly, this works

  (setopt modus-themes-completions
          '(
            (matches . (extrabold underline))
            (selection . (semibold italic))
            )))

But breaks when text-also added

  (setopt modus-themes-completions
          '(
            (matches . (extrabold underline))
            (selection . (semibold italic text-also))
            ))

⛔ Warning (emacs): Value '((matches extrabold underline) (selection semibold italic text-also))' does not match type (set (cons :tag Matches (const matches) (set :tag Style of matches :greedy t #1=(choice :tag Font weight (must be supported by the typeface) (const :tag Unspecified (use whatever the default is) nil) (const :tag Thin thin) (const :tag Ultra-light ultralight) (const :tag Extra-light extralight) (const :tag Light light) (const :tag Semi-light semilight) (const :tag Regular regular) (const :tag Medium medium) (const :tag Semi-bold semibold) (const :tag Bold bold) (const :tag Extra-bold extrabold) (const :tag Ultra-bold ultrabold)) (const :tag Italic font (oblique or slanted forms) italic) (const :tag Underline underline))) (cons :tag Selection (const selection) (set :tag Style of selection :greedy t #1# (const :tag Italic font (oblique or slanted forms) italic) (const :tag Underline underline))) (cons :tag Fallback for both matches and selection (const t) (set :tag Style of both matches and selection :greedy t #1# (const :tag Italic font (oblique or slanted forms) italic) (const :tag Underline underline))))

[protesilaos]

I have encounted this issue with practically everything I have tested. setopt does not seem to honour the :type. But I did not look into it further.

Does this issue happen if you try to modify those variables through the Custom user interface?

[shipmints]

I use only programmatic elisp for better control and easier version controlling. There might be a simple adjustment to allow the custom infrastructure to work with modus-themes-completion. In the end, setopt just warns but still does what you tell it.

[protesilaos]

From: shipmints @.***> Date: Fri, 18 Oct 2024 14:13:31 -0700

I use only programmatic elisp for better control and easier version controlling.

Same here.

There might be a simple adjustment to allow the custom infrastructure to work with modus-themes-completion. In the end, setopt just warns but still does what you tell it.

I only ever use 'setq', which has no magic to it. Though I wonder when is the type checking supposed to happen. I am trying this now and it does not give me any error:

(custom-set-variables
 '(modus-themes-to-toggle "hello"))

Meanwhile, the :type of that user option does not allow a string or non-nil value:

:type `(choice (const :tag "No toggle" nil) (list :tag "Pick two themes to toggle between" (choice :tag "Theme one of two" ,@(mapcar (lambda (theme) (list 'const theme)) modus-themes-items)) (choice :tag "Theme two of two" ,@(mapcar (lambda (theme) (list 'const theme)) modus-themes-items))))

So I guess those are only meant for the Custom UI.

-- Protesilaos Stavrou https://protesilaos.com

[alphapapa]

According to the info manual for setopt:

This works the same as ‘setq’, but if the variable has any special setter functions, they will be run automatically when using ‘setopt’. You can also use ‘setopt’ on other, non-customizable variables, but this is less efficient than using ‘setq’.

It would be nice if a warning were given for a value that doesn't match the expected type, but I guess that isn't implemented yet. Might be worth mentioning on emacs-devel.

[shipmints]

Debated at length https://debbugs.gnu.org/cgi/bugreport.cgi?bug=73098

[shipmints]

I am trying this now and it does not give me any error: (custom-set-variables '(modus-themes-to-toggle "hello")) Meanwhile, the :type of that user option does not allow a string or non-nil value: :type `(choice (const :tag "No toggle" nil) (list :tag "Pick two themes to toggle between" (choice :tag "Theme one of two" ,@(mapcar (lambda (theme) (list 'const theme)) modus-themes-items)) (choice :tag "Theme two of two" ,@(mapcar (lambda (theme) (list 'const theme)) modus-themes-items)))) So I guess those are only meant for the Custom UI.

I think you might have meant (customize-set-variable which I believe uses the full customize mechanical infrastructure.

setopt is implemented on top of the customize type checking infrastructure but I do not know how robust that is vs. a set of convenience helpers to drive the customize GUI.

The question for your modus users, Prot, is that since you have setters on your options, should they have a way to call the same function manually after running their setq's?

[protesilaos]

From: shipmints @.***> Date: Sat, 19 Oct 2024 07:49:31 -0700

I am trying this now and it does not give me any error: (custom-set-variables '(modus-themes-to-toggle "hello")) Meanwhile, the :type of that user option does not allow a string or non-nil value: :type `(choice (const :tag "No toggle" nil) (list :tag "Pick two themes to toggle between" (choice :tag "Theme one of two" ,@(mapcar (lambda (theme) (list 'const theme)) modus-themes-items)) (choice :tag "Theme two of two" ,@(mapcar (lambda (theme) (list 'const theme)) modus-themes-items)))) So I guess those are only meant for the Custom UI.

I think you might have meant (customize-set-variable which I believe uses the full customize mechanical infrastructure.

I thought custom-set-variables was a wrapper for this. If I recall correctly, it is what is appended to one's init file by default if they use the Custom interface.

setopt is implemented on top of the customize type checking infrastructure but I do not know how robust that is vs. a set of convenience helpers to drive the customize GUI.

The question for your modus users, Prot, is that since you have setters on your options, should they have a way to call the same function manually after running their setq's?

The :set we have now reloads the current theme when the user option modus-themes-custom-auto-reload is non-nil (the default). Do you think we need an interactive function to do this? Or maybe you have something else in mind?

With setq we do not run any function behind the scenes. For me, this is the desired behaviour: I do not want to reload the theme each time I set a variable (I may plan to set many of those, or do some test with one of the helper functions, for example).

-- Protesilaos Stavrou https://protesilaos.com

[shipmints]

If the only purpose of the setters is to force theme reloading, then I'd say not needed programmatically.

[protesilaos]

From: shipmints @.***> Date: Sat, 19 Oct 2024 08:23:52 -0700

If the only purpose of the setters is to force theme reloading, then I'd say not needed programmatically.

Fine.

On this note, I now think we do not need the :set for all those user options. It only makes sense for those that will produce a visual change when the theme is loaded again. So modus-themes-to-toggle does not need it, for example, but modus-themes-bold-constructs does.

What do you think?

-- Protesilaos Stavrou https://protesilaos.com

[shipmints]

If a setter has no practical value for a particular user-settable option, I'd say remove it. Leaving it in might cause people who read the code itself (vs. just the documentation) to believe they need to do something special vs. merely setq. I've adopted the habit of reading the code for every user option so I can see precisely what's going on/intended vs. documentation. I would have been fooled by an erroneous setter.

[protesilaos]

From: shipmints @.***> Date: Sun, 20 Oct 2024 05:28:51 -0700

If a setter has no practical value for a particular user-settable option, I'd say remove it.

Good!

Leaving it in might cause people who read the code itself (vs. just the documentation) to believe they need to do something special vs. merely setq.

I agree.

I've adopted the habit of reading the code for every user option so I can see precisely what's going on/intended vs. documentation. I would have been fooled by an erroneous setter.

I tend to do the same. And use 'setq' because I have never encountered a case where the :set not running actually breaks the package.

-- Protesilaos Stavrou https://protesilaos.com

[protesilaos]

I just removed some of the :set that are not relevant. Most are still there though. Thank you!