Byte-compilation of custom themes

[Basil L. Contovounesios]

This email is a follow-up to a question[1] I asked on the Emacs Stack
Exchange Q&A forum a couple of months ago, which received no answers in
the interim.

[1] https://emacs.stackexchange.com/q/36892/15748

I am wondering why custom themes are ordinarily left non-byte-compiled,
based on the following observations:

1. All built-in themes under etc/themes/ set the file variable
no-byte-compile to t.

2. The function custom-available-themes in lisp/custom.el considers only
*-theme.el and not *-theme.elc files under custom-theme-load-path.

3. The command load-theme in lisp/custom.el is able to load *-theme.elc
files under custom-theme-load-path, but does so only if there is no
corresponding source file in the same directory.

4. The manual nodes '(elisp) Custom Themes', '(emacs) Custom Themes' and
'(emacs) Creating Custom Themes' make no mention of byte-compilation.

Is this "aversion" to byte-compilation of custom themes intentional? If
so, what is the reasoning behind it? Should the documentation be
extended to describe it?

If not, should the functions custom-available-themes and load-theme be
made made less picky about which file extensions they consider? Should
the behaviour of the latter be likened more to the that of the load
function in src/lread.c?

FWIW, I have not noticed any issues when using my own byte-compiled
custom themes. The only minor nuisance is having to keep the source
files separate from their translations and out of
custom-theme-load-path.

Thanks,

[Stefan Monnier]

Post by Basil L. Contovounesios
I am wondering why custom themes are ordinarily left non-byte-compiled,

Good question.

Post by Basil L. Contovounesios
Is this "aversion" to byte-compilation of custom themes intentional?

I think it's due to the idea that users might download theme files from
random places without realizing that it contains arbitrary Lisp code
(contrary to normal Emacs packages where we consider that users should
know that it contains arbitrary Lisp code). So we prompt users to
confirm that they think the theme file is safe, and users can't be
expected to assess the safety of a .elc file, so we insist on using the
.el file, which the user can inspect without nearly as much pain.


Stefan

[Basil L. Contovounesios]

Post by Stefan Monnier

Post by Basil L. Contovounesios
Is this "aversion" to byte-compilation of custom themes intentional?

I think it's due to the idea that users might download theme files from
random places without realizing that it contains arbitrary Lisp code
(contrary to normal Emacs packages where we consider that users should
know that it contains arbitrary Lisp code). So we prompt users to
confirm that they think the theme file is safe, and users can't be
expected to assess the safety of a .elc file, so we insist on using the
.el file, which the user can inspect without nearly as much pain.

Ah, that makes sense. Do you think it would be worthwhile clarifying
this in the manual? Is there a clearer way of saying the following?

[Stefan Monnier]

diff --git a/doc/lispref/customize.texi b/doc/lispref/customize.texi
index 6c7ca260ab..7fea507fd0 100644
--- a/doc/lispref/customize.texi
+++ b/doc/lispref/customize.texi
@@ -1432,7 +1432,9 @@ Custom Themes
would be evaluated when loading the theme, but that is bad form.
To protect against loading themes containing malicious code, Emacs
displays the source file and asks for confirmation from the user
-before loading any non-built-in theme for the first time.
+before loading any non-built-in theme for the first time. As
+such, themes are not ordinarily byte-compiled, and source files
+always take precedence when Emacs is looking for a theme to load.
The following functions are useful for programmatically enabling and

Sounds good to me.

Why are built-in themes not exempt to this safety net, though?

They're not? There is code which tries to exempt them, so if they're
not, it's a bug in that code.

Finally, would you care to post your helpful explanation as an answer to
my Stack Exchange question, or would you rather I paraphrased this
thread?

Feel free to post my answer there,


Stefan

[Basil L. Contovounesios]

Post by Stefan Monnier

diff --git a/doc/lispref/customize.texi b/doc/lispref/customize.texi
index 6c7ca260ab..7fea507fd0 100644
--- a/doc/lispref/customize.texi
+++ b/doc/lispref/customize.texi
@@ -1432,7 +1432,9 @@ Custom Themes
would be evaluated when loading the theme, but that is bad form.
To protect against loading themes containing malicious code, Emacs
displays the source file and asks for confirmation from the user
-before loading any non-built-in theme for the first time.
+before loading any non-built-in theme for the first time. As
+such, themes are not ordinarily byte-compiled, and source files
+always take precedence when Emacs is looking for a theme to load.
The following functions are useful for programmatically enabling and

Sounds good to me.

I attach a patch with this amendment. If it is up to scratch, would
someone please push it?

Post by Stefan Monnier

Why are built-in themes not exempt to this safety net, though?

They're not? There is code which tries to exempt them, so if they're
not, it's a bug in that code.

Sorry, I should have been clearer. As documented, load-theme indeed
exempts built-in themes from the custom-theme-load-confirm check. What
I meant to ask is why built-in themes are not byte-compiled in addition
to being considered safe. Are there any arguments against doing this,
other than any performance gain being negligible?

Post by Stefan Monnier

Finally, would you care to post your helpful explanation as an answer to
my Stack Exchange question, or would you rather I paraphrased this
thread?

Feel free to post my answer there,

Done: https://emacs.stackexchange.com/a/38510/15748.

Thanks,

[Stefan Monnier]

Post by Basil L. Contovounesios
I attach a patch with this amendment.

Installed in the emacs-26 branch, thank you.

Post by Basil L. Contovounesios
What I meant to ask is why built-in themes are not byte-compiled in
addition to being considered safe. Are there any arguments against
doing this, other than any performance gain being negligible?

I can't think of any technical reason, indeed (except that we'd then
have to special case this in the code so as to load the .elc file
instead of the .el file).


Stefan

[Eli Zaretskii]

Date: Fri, 11 May 2018 21:43:42 +0100

We should at least have a comment there that we are relying on
custom-theme--load-path to do the test, and perhaps also an assertion.

Do you mean a cl-assertion, or an emulation thereof?

I meant cl-assert.

Either way, what is the benefit of barfing before directory-files does?

That you can control the text of the error message, and make it more
user-friendly. Also, an assertion clearly means we intended this not
to happen, rather than that the code failed to handle some valid
situation.

Once again, the minimum I requested was a comment; it's up to you
whether to use cl-assert. But see below.

Wouldn't a docstring for custom-theme--load-path and a comment in
custom-available-themes suffice for the reader (they do for me)?

We've seen many situations where code guidelines are violated, for
whatever reasons, so just documenting stuff is not enough. Moreover,
custom-theme--load-path might some day change and invalidate our
assumption. The way to prevent that from happening could be either
having the assertion in the caller, or by adding a test to the test
suite that makes sure the list returned by custom-theme--load-path
never includes anything but existing directories.

(dolist (dir (custom-theme--load-path))
+ ;; `custom-theme--load-path' promises DIR exists.

"promises DIR exists and is a directory", I think.

(defun custom-theme--load-path ()
+ "Expand `custom-theme-load-path' into list of directories.
+Only existing directories are included in the path returned."

I'd find this text a bit of a riddle. How about this instead:

"Expand `custom-theme-load-path' into list of directories.
Members of `custom-theme-load-path' that either don't exist or are not
directories are omitted from the expansion."

Thanks.

[Basil L. Contovounesios]

While I generally prefer to use #' where applicable, I have resisted the
temptation to use it in keymaps because I have found it leads to
"spurious" warnings more often than in other contexts (and the impact
of an invalid binding is also less serious than a call to
a non-existing function).

Good point, thanks!

Also, to me #'f means "the function bound to this symbol" whereas 'f
means "the symbol f", and in key-bindings I really want to use "the
symbol" rather than "the associated function" because `C-h m` gives poor
results when keys are bound to lambda expressions. It's a rather
"philosophical" argument, tho.

Agreed; too bad I live for philosophical discourse. ;)

[Eli Zaretskii]

Date: Thu, 10 May 2018 03:54:27 +0100
I forgot to ask in my previous email whether the duplicate
';;; Custom Themes' headings in custom.el are intentional, and if not
how they might be better shuffled around.

The first one is a pasto, and should be removed.