emacs-orgmode@gnu.org archives
 help / color / mirror / code / Atom feed
* [BUG] org-capture autoload bug? [9.5.2 (9.5.2-gfbff08 @ /home/ignacio/.emacs.d/elpa/org-9.5.2/)]
@ 2022-03-10 12:53 Ignacio Casso
  2022-03-10 16:32 ` Max Nikulin
  0 siblings, 1 reply; 22+ messages in thread
From: Ignacio Casso @ 2022-03-10 12:53 UTC (permalink / raw)
  To: emacs-orgmode

Hello,

I think I've found a bug with org-capture autoload. If the first time
you use org-capture before it's actually loaded is within a let form binding
org-capture-templates, it produces an error saying that the template was
not found.

For example, if we call this:

  (let ((org-capture-templates
         '(("d" "default" entry
            (file+headline org-default-notes-file "Tasks")
            "* %?"))))
    (org-capture nil "d")))

It produces the error:

  (error "No capture template referred to by \"d\" keys")


Furthermore, depending on where that form was evaluated or defined, the
next time you called org-capture may work or produce the following
error:

  org-capture: Symbol’s value as variable is void: org-capture-templates

In particular, if it is evaluated directly in the scratch buffer the
next call to org-capture works, and if it is part of a function
defined in my init file and evaluated in the scratch buffer, it produces
the error.

I have tried this too in Emacs 29 and the behaviour is similar, but
sometimes the error produced is

  (error "Defining as dynamic an already lexical var")

instead of

  (error "No capture template referred to by \"d\" keys")

This again depends on where the form is evaluated or defined.


Best regards,

Ignacio

P.S., I now this has an easy solution: not autoloading org-capture and having
(require 'org-capture) in my config. I'm just reporting the bug, if that
is what this is.

Emacs  : GNU Emacs 27.2 (build 1, x86_64-pc-linux-gnu, GTK+ Version 3.24.20)
 of 2022-01-16
Package: Org mode version 9.5.2 (9.5.2-gfbff08 @ /home/ignacio/.emacs.d/elpa/org-9.5.2/)


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [BUG] org-capture autoload bug? [9.5.2 (9.5.2-gfbff08 @ /home/ignacio/.emacs.d/elpa/org-9.5.2/)]
  2022-03-10 12:53 Ignacio Casso
@ 2022-03-10 16:32 ` Max Nikulin
  2022-03-10 18:00   ` Ignacio Casso
  0 siblings, 1 reply; 22+ messages in thread
From: Max Nikulin @ 2022-03-10 16:32 UTC (permalink / raw)
  To: Ignacio Casso, emacs-orgmode; +Cc: c.buhtz

On 10/03/2022 19:53, Ignacio Casso wrote:
> For example, if we call this:
> 
>    (let ((org-capture-templates
>           '(("d" "default" entry
>              (file+headline org-default-notes-file "Tasks")
>              "* %?"))))
>      (org-capture nil "d")))
> 
> It produces the error:
> 
>    (error "No capture template referred to by \"d\" keys")

Ignacio, could you, please, try the following?

     (custom-set-variables
      '(org-capture-templates
        '(("d" "default" entry
           (file "/tmp/capture-test.org")
           "* %?"))))
     (org-capture nil "d")

On 09/03/2022 23:39, c.buhtz wrote in 
https://list.orgmode.org/7395938a0e6b06139239bc70ba7c2b19@posteo.de:> 
Why using custom-set variables here? Is there something wrong with just 
doing
> (org-export-with-broken-links t)
> ? 

I do not think that something similar may happen with 
`org-export-with-broken-links', but I suppose, the example above is 
another argument to prefer setting variables through customization 
interface instead of bare `setq' in general.


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [BUG] org-capture autoload bug? [9.5.2 (9.5.2-gfbff08 @ /home/ignacio/.emacs.d/elpa/org-9.5.2/)]
  2022-03-10 16:32 ` Max Nikulin
@ 2022-03-10 18:00   ` Ignacio Casso
  2022-03-11 10:07     ` Max Nikulin
  0 siblings, 1 reply; 22+ messages in thread
From: Ignacio Casso @ 2022-03-10 18:00 UTC (permalink / raw)
  To: Max Nikulin; +Cc: c.buhtz, emacs-orgmode


Max Nikulin <manikulin@gmail.com> writes:

> On 10/03/2022 19:53, Ignacio Casso wrote:
>> For example, if we call this:
>>    (let ((org-capture-templates
>>           '(("d" "default" entry
>>              (file+headline org-default-notes-file "Tasks")
>>              "* %?"))))
>>      (org-capture nil "d")))
>> It produces the error:
>>    (error "No capture template referred to by \"d\" keys")
>
> Ignacio, could you, please, try the following?
>
>     (custom-set-variables
>      '(org-capture-templates
>        '(("d" "default" entry
>           (file "/tmp/capture-test.org")
>           "* %?"))))
>     (org-capture nil "d")
>

That works, of course, but it's not suitable to my use case. The example
I have provided has been reduced from a larger function I wrote that
relies on org-capture functionality to create new entries in an org
file, using a template specific for that function which is not relevant
for any other Elisp code calling org-capture. Thus the function binds
locally and temporarily org-capture-templates, as I've seen other Elisp
code snippets do. If org-capture is already loaded it works just fine,
and I would expect it to work too when it is not and the function
triggers the autoload, but it doesn't.


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [BUG] org-capture autoload bug? [9.5.2 (9.5.2-gfbff08 @ /home/ignacio/.emacs.d/elpa/org-9.5.2/)]
  2022-03-10 18:00   ` Ignacio Casso
@ 2022-03-11 10:07     ` Max Nikulin
  2022-03-11 10:38       ` Ignacio Casso
  0 siblings, 1 reply; 22+ messages in thread
From: Max Nikulin @ 2022-03-11 10:07 UTC (permalink / raw)
  To: Ignacio Casso; +Cc: c.buhtz, emacs-orgmode

On 11/03/2022 01:00, Ignacio Casso wrote:
> Max Nikulin writes:
> 
>> On 10/03/2022 19:53, Ignacio Casso wrote:
>>> For example, if we call this:
>>>     (let ((org-capture-templates
>>>            '(("d" "default" entry
>>>               (file+headline org-default-notes-file "Tasks")
>>>               "* %?"))))
>>>       (org-capture nil "d")))
>>> It produces the error:
>>>     (error "No capture template referred to by \"d\" keys")
>>
>> Ignacio, could you, please, try the following?
>>
>>      (custom-set-variables
>>       '(org-capture-templates
>>         '(("d" "default" entry
>>            (file "/tmp/capture-test.org")
>>            "* %?"))))
>>      (org-capture nil "d")
>>
> 
> That works, of course, but it's not suitable to my use case. The example
> I have provided has been reduced from a larger function I wrote that
> relies on org-capture functionality to create new entries in an org
> file, using a template specific for that function which is not relevant
> for any other Elisp code calling org-capture. Thus the function binds
> locally and temporarily org-capture-templates, as I've seen other Elisp
> code snippets do. If org-capture is already loaded it works just fine,
> and I would expect it to work too when it is not and the function
> triggers the autoload, but it doesn't.

Ignacio, I think, you can add (require 'org-capture) inside your 
function just before `let' and it would work almost as lazy loading. I 
am unsure if org-capture is the best approach in your case. Have you 
considered yasnippet or Org specific functions to transform and to 
generate content? Even changing arguments of the `org-capture' function 
or introducing another function that has explicit argument with a 
template might be better then marking the template list variable for 
autoloading.

I have no particular opinion concerning adding autoload cookie to 
`org-capture-templates'. Emacs has enough number of them, but Org has no 
such custom variables. Arguments why autoload should be avoided:

info "(elisp) When to Autoload" 
https://www.gnu.org/software/emacs/manual/html_node/elisp/When-to-Autoload.html
> Don’t autoload a user option just so that a user can set it.

The following thread related to Emacs core and linked from 
https://emacs.stackexchange.com/questions/32859/autoloading-defcustoms-good-practice-or-not 
:

Glenn Morris. Re: master f995fbd: * lisp/server.el (server-name): Add 
autoload cookie. (Bug#23576) Thu, 19 May 2016 12:44:49 -0400
https://lists.gnu.org/archive/html/emacs-devel/2016-05/msg00415.html

Org-specific thread:
Achim Gratz. missing autoload cookies for defcustom? Sun, 09 Oct 2011 
17:20:16 +0200 https://list.orgmode.org/87lisub39r.fsf@Rainer.invalid/T/#u

c.buhtz, I am sorry, despite `org-capture-templates' has the :set 
attribute, this is not a really convincing example why 
`custom-set-variables' should be used. The setter was added to ensure 
compatibility with older template format and this example works with 
`setq' as well.


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [BUG] org-capture autoload bug? [9.5.2 (9.5.2-gfbff08 @ /home/ignacio/.emacs.d/elpa/org-9.5.2/)]
  2022-03-11 10:07     ` Max Nikulin
@ 2022-03-11 10:38       ` Ignacio Casso
  2022-03-11 19:59         ` Tim Cross
  0 siblings, 1 reply; 22+ messages in thread
From: Ignacio Casso @ 2022-03-11 10:38 UTC (permalink / raw)
  To: Max Nikulin; +Cc: c.buhtz, emacs-orgmode


Max Nikulin <manikulin@gmail.com> writes:

> Ignacio, I think, you can add (require 'org-capture) inside your
> function just before `let' and it would work almost as lazy loading.

Thanks, I was already using (require 'org-capture) in my init file to
solve this. As I said, it's not really a problem for me, I was just
reporting it in case it was a bug. I was not sure since I don't really
know the inner workings of autoloads and custom variables, but since
this snippet works when org-capture is not yet loaded

  (setq org-capture-templates
         '(("d" "default" entry
            (file+headline org-default-notes-file "Tasks")
            "* %?")))
  (org-capture nil "d")

and this snippet also works

  (let ((my-new-var 2))
    (defcustom my-new-var 1 "New variable that did not exist before")
    (message "%s" my-new-var)) ;; This prints 2
  (message "%s" my-new-var) ;; This prints 1

I thought that this snippet should work too when org-capture is not yet
loaded, and that the fact that it doesn't could mean that there is a bug
somewhere

  (let ((org-capture-templates
         '(("d" "default" entry
            (file+headline org-default-notes-file "Tasks")
            "* %?"))))
    (org-capture nil "d")))


> I have no particular opinion concerning adding autoload cookie to
> `org-capture-templates'. Emacs has enough number of them, but Org has
> no such custom variables.

I put an autoload cookie myself and it doesn't fix it, so it's probably
not that. It's the first time I manage autoload cookies though, so I may
have done something wrong. I only tested that the autoload cookie worked
by checking that before loading org-capture, org-capture-templates
appears in the completion list for C-h v, and I can evaluate it.


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [BUG] org-capture autoload bug? [9.5.2 (9.5.2-gfbff08 @ /home/ignacio/.emacs.d/elpa/org-9.5.2/)]
  2022-03-11 10:38       ` Ignacio Casso
@ 2022-03-11 19:59         ` Tim Cross
  2022-03-14 10:42           ` Ignacio Casso
  2022-03-14 14:52           ` Max Nikulin
  0 siblings, 2 replies; 22+ messages in thread
From: Tim Cross @ 2022-03-11 19:59 UTC (permalink / raw)
  To: Ignacio Casso; +Cc: c.buhtz, Max Nikulin, emacs-orgmode


Ignacio Casso <ignaciocasso@hotmail.com> writes:

> Max Nikulin <manikulin@gmail.com> writes:
>
>> Ignacio, I think, you can add (require 'org-capture) inside your
>> function just before `let' and it would work almost as lazy loading.
>
> Thanks, I was already using (require 'org-capture) in my init file to
> solve this. As I said, it's not really a problem for me, I was just
> reporting it in case it was a bug. I was not sure since I don't really
> know the inner workings of autoloads and custom variables, but since
> this snippet works when org-capture is not yet loaded
>
>   (setq org-capture-templates
>          '(("d" "default" entry
>             (file+headline org-default-notes-file "Tasks")
>             "* %?")))
>   (org-capture nil "d")
>
> and this snippet also works
>
>   (let ((my-new-var 2))
>     (defcustom my-new-var 1 "New variable that did not exist before")
>     (message "%s" my-new-var)) ;; This prints 2
>   (message "%s" my-new-var) ;; This prints 1
>
> I thought that this snippet should work too when org-capture is not yet
> loaded, and that the fact that it doesn't could mean that there is a bug
> somewhere
>
>   (let ((org-capture-templates
>          '(("d" "default" entry
>             (file+headline org-default-notes-file "Tasks")
>             "* %?"))))
>     (org-capture nil "d")))
>
>
>> I have no particular opinion concerning adding autoload cookie to
>> `org-capture-templates'. Emacs has enough number of them, but Org has
>> no such custom variables.
>
> I put an autoload cookie myself and it doesn't fix it, so it's probably
> not that. It's the first time I manage autoload cookies though, so I may
> have done something wrong. I only tested that the autoload cookie worked
> by checking that before loading org-capture, org-capture-templates
> appears in the completion list for C-h v, and I can evaluate it.

While I don't know if this is a bug, it certainly doesn't seem to be
doing the right thing from an 'intuitive' point of view. I would expect
when a variable is bound to a value inside a let and a function is then
called which uses that variable, the initial let bound value should be
used and the result be the same regardless of whether org-capture has or
has not been loaded. It means there is a hidden side-effect here, which
isn't good and probably needs more analysis. If you had set the value
using setq rather than as a let form, it wouldn't be overridden when
org-capture is loaded, so why does it when it is a let binding? 

Might be worth asking a general question on emacs-devel? Stephan or Eli
can probably provide some clarification here (maybe this is somehow
related to the changes associated with the lexical binding stuff?)



^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [BUG] org-capture autoload bug? [9.5.2 (9.5.2-gfbff08 @ /home/ignacio/.emacs.d/elpa/org-9.5.2/)]
  2022-03-11 19:59         ` Tim Cross
@ 2022-03-14 10:42           ` Ignacio Casso
  2022-03-14 14:52           ` Max Nikulin
  1 sibling, 0 replies; 22+ messages in thread
From: Ignacio Casso @ 2022-03-14 10:42 UTC (permalink / raw)
  To: Tim Cross; +Cc: c.buhtz, Max Nikulin, emacs-orgmode


> While I don't know if this is a bug, it certainly doesn't seem to be
> doing the right thing from an 'intuitive' point of view. I would expect
> when a variable is bound to a value inside a let and a function is then
> called which uses that variable, the initial let bound value should be
> used and the result be the same regardless of whether org-capture has or
> has not been loaded. It means there is a hidden side-effect here, which
> isn't good and probably needs more analysis. If you had set the value
> using setq rather than as a let form, it wouldn't be overridden when
> org-capture is loaded, so why does it when it is a let binding? 
>

I've investigated this a little bit further and this is not specific to
org-capture. For example if you evaluate (let ((org-agenda-files
'("/tmp/foo.org"))) (org-agenda-list)) before org-agenda is laoded, the
let-binding of org-agenda-files is ignored too.

I think the reason is the following:

1) Loading the file defining those autoload functions runs defcustom for
the variable we are using.

2) defcustom calls the function F passed as argument with the :set
keyword (set-default by default) to initialize the variable. That
function receives as argument the variable and a value.

2.1) If the variable was already initialized (e.g., with setq) with a
value V, the value passed to F is V. It it was not, the value passed to
F is the value passed as STANDARD argument to defcustom. So far, so
good.

2.2) If the variable is let-bound with lexical-binding non-nil, there is
an error in Emacs 29, since at the time the variable is let-bound Emacs
does not know it is special and thus it uses lexical binding, but when
defcustom runs it finds out that it is special and therefore it should
have used dynamic binding. In Emacs 27 this check is not done, and the
let-binding is considered lexical, so it has no effect for uses of the
variable while the let body is being executed, unless they appear
textually within that body. In particular, the value passed to F is
again the STANDARD argument of defcustom.

2.2.1) An exception to this is when the variable is autoloaded, since
Emacs can know that it is special by the time is evaluates let.

2.2.2) But in general this means that the programmer should know whether
an autoload function is going to be loaded at that execution
point if he is going to do this.

2.3) If the variable is let-bound with lexical-binding nil, things then
break completely. First, the value passed to F is again the STANDARD
argument of defcustom. This makes sense since F would initialize the
variable globally and the let binding was supposed to be local. But it
means that again the let binding gets shadowed. But it's worse than
that. The initialization of the variable does not work and only has the
same scope as the let-binding, so after the let body is finished, the
variable is void. Furthermore, since the feature in question is already
provided, there is no easy way to leave that state and initialize the
variable again.

This makes me conclude that let-bindings of variables used by autoloaded
functions should be avoided unless we know for sure that the function
will be loaded at that point in execution.

> Might be worth asking a general question on emacs-devel? Stephan or Eli
> can probably provide some clarification here (maybe this is somehow
> related to the changes associated with the lexical binding stuff?)

Yes, I will email them explaining this, since I think the point 2.3 is a
bug. Maybe there is no better way to do this, but I think at least there
should be a warning if a misuse of let + autoload can leave Emacs in
what I think is a broken state.


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [BUG] org-capture autoload bug? [9.5.2 (9.5.2-gfbff08 @ /home/ignacio/.emacs.d/elpa/org-9.5.2/)]
  2022-03-11 19:59         ` Tim Cross
  2022-03-14 10:42           ` Ignacio Casso
@ 2022-03-14 14:52           ` Max Nikulin
  2022-03-14 18:42             ` Tim Cross
  1 sibling, 1 reply; 22+ messages in thread
From: Max Nikulin @ 2022-03-14 14:52 UTC (permalink / raw)
  To: emacs-orgmode

On 12/03/2022 02:59, Tim Cross wrote:
> Ignacio Casso writes:

>>    (let ((org-capture-templates
>>           '(("d" "default" entry
>>              (file+headline org-default-notes-file "Tasks")
>>              "* %?"))))
>>      (org-capture nil "d")))
>>
>> I put an autoload cookie myself and it doesn't fix it, so it's probably
>> not that. It's the first time I manage autoload cookies though, so I may
>> have done something wrong. I only tested that the autoload cookie worked
>> by checking that before loading org-capture, org-capture-templates
>> appears in the completion list for C-h v, and I can evaluate it.
> 
> While I don't know if this is a bug, it certainly doesn't seem to be
> doing the right thing from an 'intuitive' point of view. I would expect
> when a variable is bound to a value inside a let and a function is then
> called which uses that variable, the initial let bound value should be
> used and the result be the same regardless of whether org-capture has or
> has not been loaded. It means there is a hidden side-effect here, which
> isn't good and probably needs more analysis. If you had set the value
> using setq rather than as a let form, it wouldn't be overridden when
> org-capture is loaded, so why does it when it is a let binding?

For `defcustom' autoload generates no more than

     (defvar org-capture-templates nil "...")

It seems, behavior depends on whether `org-capture-templates' has the 
:set attribute. The setter receives nil instead of the let-bound value. 
I can not say I understand the tricks with bypassing lexical binding in 
`defcustom' and some checks in `custom-declare-variable'. Since emacs-26 
something has changed:

https://git.savannah.gnu.org/cgit/emacs.git/commit/?id=6309131349
> -  ;; Use defvar to set the docstring as well as the special-variable-p flag.
> -  ;; FIXME: We should reproduce more of `defvar's behavior, such as the warning
> -  ;; when the var is currently let-bound.
> -  (if (not (default-boundp symbol))

I am unsure that the setter of `defcustom' should get let-bound value in 
the case of autoloading since it might lead to fragile behavior.

I still think that explicit templates argument for `org-capture' is 
better than relying on autoloading.

Reading the code I noticed `org-capture-templates-contexts' so I wonder 
if it might help for the original use case.



^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [BUG] org-capture autoload bug? [9.5.2 (9.5.2-gfbff08 @ /home/ignacio/.emacs.d/elpa/org-9.5.2/)]
  2022-03-14 14:52           ` Max Nikulin
@ 2022-03-14 18:42             ` Tim Cross
  2022-03-14 19:43               ` Ignacio Casso
  0 siblings, 1 reply; 22+ messages in thread
From: Tim Cross @ 2022-03-14 18:42 UTC (permalink / raw)
  To: Max Nikulin; +Cc: emacs-orgmode


Max Nikulin <manikulin@gmail.com> writes:

> On 12/03/2022 02:59, Tim Cross wrote:
>> Ignacio Casso writes:
>
>>>    (let ((org-capture-templates
>>>           '(("d" "default" entry
>>>              (file+headline org-default-notes-file "Tasks")
>>>              "* %?"))))
>>>      (org-capture nil "d")))
>>>
>>> I put an autoload cookie myself and it doesn't fix it, so it's probably
>>> not that. It's the first time I manage autoload cookies though, so I may
>>> have done something wrong. I only tested that the autoload cookie worked
>>> by checking that before loading org-capture, org-capture-templates
>>> appears in the completion list for C-h v, and I can evaluate it.
>> While I don't know if this is a bug, it certainly doesn't seem to be
>> doing the right thing from an 'intuitive' point of view. I would expect
>> when a variable is bound to a value inside a let and a function is then
>> called which uses that variable, the initial let bound value should be
>> used and the result be the same regardless of whether org-capture has or
>> has not been loaded. It means there is a hidden side-effect here, which
>> isn't good and probably needs more analysis. If you had set the value
>> using setq rather than as a let form, it wouldn't be overridden when
>> org-capture is loaded, so why does it when it is a let binding?
>
> For `defcustom' autoload generates no more than
>
>     (defvar org-capture-templates nil "...")
>
> It seems, behavior depends on whether `org-capture-templates' has the :set
> attribute. The setter receives nil instead of the let-bound value. I can not say
> I understand the tricks with bypassing lexical binding in `defcustom' and some
> checks in `custom-declare-variable'. Since emacs-26 something has changed:
>
> https://git.savannah.gnu.org/cgit/emacs.git/commit/?id=6309131349
>> -  ;; Use defvar to set the docstring as well as the special-variable-p flag.
>> -  ;; FIXME: We should reproduce more of `defvar's behavior, such as the warning
>> -  ;; when the var is currently let-bound.
>> -  (if (not (default-boundp symbol))
>
> I am unsure that the setter of `defcustom' should get let-bound value in the
> case of autoloading since it might lead to fragile behavior.
>
> I still think that explicit templates argument for `org-capture' is better than
> relying on autoloading.
>
Yes, it would seem that something has changed and I suspect it may be
related to the introduction of lexical bindings. Explicit template
arguments for org-capture may or may not be better, I don't know.
However, this behaviour seems like it may be the tip of a much bigger
issue outside of org-mode and potential source of bugs for Emacs.

Bottom line is I don't think any function should behave differently
depending on when autoload runs. It could be because the setter used in
the defcustom is incorrect since the change a while back to use
lexical-binding by default or something else. It looks like the setter
on org-capture-template is used to do some form of conversion on the
template specification to force a new format. Not sure how long that has
been there or whether it is actually still required. Could be that this
setter was added to aid in transition to a new template format which
could/should be removed at some point. Personally, I've never liked
custom setters. I typically avoid the custom interface. However, exotic
custom variable setters can mean that using setq won't work correctly
and you need to use custom-set-variable etc. In fact, I'm seeing a lot
more use of custom-set-variable* in init code recently - something which
was almost never seen 20 years ago. 

Regardless, I don't think having the situation where the programmer must
know (guess) whether autoload will/could execute during the evaluation
of code they write is tenable and am beginning to suspect it may be an
Emacs bug OR a subtle bug in org-mode as a result to the transition to
lexical binding as the default. My recommendation would be to come up
with a non-org specific example which reproduces the behaviour and then
ask on emacs-devel, using the example to demonstrate the issue.

> Reading the code I noticed `org-capture-templates-contexts' so I wonder if it
> might help for the original use case.

I thought about that as well. However, from re-reading the OP's posts, I
suspect not. org-capture-templates-contexts seems to be useful when you
want to restrict access to some templates based on context. However,
from reading the OP's posts, I get the impression that what they want is
for templates to be different based on context i.e. they still want the
template associated with 'd', but want its definition to be different.

It is possible org-capture-template-contexts might provide an
alternative solution where you don't need to dynamically modify/add
templates in this manner. For example, you could have all your templates
defined, but only offer those relevant based on context. However, I
guess this would mean having to have different template keys, which
might be an issue.


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [BUG] org-capture autoload bug? [9.5.2 (9.5.2-gfbff08 @ /home/ignacio/.emacs.d/elpa/org-9.5.2/)]
  2022-03-14 18:42             ` Tim Cross
@ 2022-03-14 19:43               ` Ignacio Casso
  2022-03-14 22:54                 ` Tim Cross
  2022-03-15 12:04                 ` Max Nikulin
  0 siblings, 2 replies; 22+ messages in thread
From: Ignacio Casso @ 2022-03-14 19:43 UTC (permalink / raw)
  To: Tim Cross; +Cc: Max Nikulin, emacs-orgmode

I've also investigated the issue a little bit further and wrote and
email with my conclusions about the same time Max wrote his. I comment
inline about a few of your thoughts:

> For `defcustom' autoload generates no more than
>
>     (defvar org-capture-templates nil "...")
>
> It seems, behavior depends on whether `org-capture-templates' has the :set
> attribute.

Not really, all defcustoms have a :set attribute, be it passed
explicitly as a parameter or the default value, set-default. This issue
happens with all autoload functions that use a custom variable: if they
are called inside a let form binding that variable and the feature was
not loaded yet, the let-binding will have no effect.

> The setter receives nil instead of the let-bound value. I can not say
> I understand the tricks with bypassing lexical binding in `defcustom' and some
> checks in `custom-declare-variable'. Since emacs-26 something has changed:

> I am unsure that the setter of `defcustom' should get let-bound value in the
> case of autoloading since it might lead to fragile behavior.

See my other email where I explain what I think that defcustom of a
variable is doing when called inside a let-binding of that variable.

>> https://git.savannah.gnu.org/cgit/emacs.git/commit/?id=6309131349
>>> -  ;; Use defvar to set the docstring as well as the special-variable-p flag.
>>> -  ;; FIXME: We should reproduce more of `defvar's behavior, such as the warning
>>> -  ;; when the var is currently let-bound.
>>> -  (if (not (default-boundp symbol))

But the conclusion is that right now it does not work, so I think that warning
is needed.

> It looks like the setter on org-capture-template is used to do some
> form of conversion on the template specification to force a new
> format. Not sure how long that has been there or whether it is
> actually still required. Could be that this setter was added to aid in
> transition to a new template format which could/should be removed at
> some point.

That is right, it seems it was added for backwards compatibility when a
new template format was added. However, it's not actually needed, since
org-capture-select-template also calls that function to ensure they are
in the new format. It needs to do so in order to allow
setting org-capture-templates also with setq, or let-binding
org-capture-templates as I do. That's not the problem anyway, as I
explained above.

> However, this behaviour seems like it may be the tip of a much bigger
> issue outside of org-mode and potential source of bugs for Emacs.

> Bottom line is I don't think any function should behave differently
> depending on when autoload runs

> Regardless, I don't think having the situation where the programmer must
> know (guess) whether autoload will/could execute during the evaluation
> of code they write is tenable and am beginning to suspect it may be an
> Emacs bug OR a subtle bug in org-mode as a result to the transition to
> lexical binding as the default. My recommendation would be to come up
> with a non-org specific example which reproduces the behaviour and then
> ask on emacs-devel, using the example to demonstrate the issue.

I agree. I'm on it.

>> Reading the code I noticed `org-capture-templates-contexts' so I wonder if it
>> might help for the original use case.
>
> I thought about that as well. However, from re-reading the OP's posts, I
> suspect not. org-capture-templates-contexts seems to be useful when you
> want to restrict access to some templates based on context. However,
> from reading the OP's posts, I get the impression that what they want is
> for templates to be different based on context i.e. they still want the
> template associated with 'd', but want its definition to be different.
>
> It is possible org-capture-template-contexts might provide an
> alternative solution where you don't need to dynamically modify/add
> templates in this manner. For example, you could have all your templates
> defined, but only offer those relevant based on context. However, I
> guess this would mean having to have different template keys, which
> might be an issue.

If I recall correctly template contexts allow to redefine the template
key, so that would not be an issue. And yes, using template contexts
could probably be a solution to my use case, as it would be having an
org-capture version with template parameter. But don't worry too much
about my use case, since a simple (require 'org-capture) is enough to
keep doing what I was doing.


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [BUG] org-capture autoload bug? [9.5.2 (9.5.2-gfbff08 @ /home/ignacio/.emacs.d/elpa/org-9.5.2/)]
  2022-03-14 19:43               ` Ignacio Casso
@ 2022-03-14 22:54                 ` Tim Cross
  2022-03-15  9:02                   ` Ignacio Casso
  2022-03-15 12:04                 ` Max Nikulin
  1 sibling, 1 reply; 22+ messages in thread
From: Tim Cross @ 2022-03-14 22:54 UTC (permalink / raw)
  To: Ignacio Casso; +Cc: Max Nikulin, emacs-orgmode


Ignacio Casso <ignaciocasso@hotmail.com> writes:

>> Regardless, I don't think having the situation where the programmer must
>> know (guess) whether autoload will/could execute during the evaluation
>> of code they write is tenable and am beginning to suspect it may be an
>> Emacs bug OR a subtle bug in org-mode as a result to the transition to
>> lexical binding as the default. My recommendation would be to come up
>> with a non-org specific example which reproduces the behaviour and then
>> ask on emacs-devel, using the example to demonstrate the issue.
>
> I agree. I'm on it.
>

Excellent. I will watch with interest to see what feedback you get!



^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [BUG] org-capture autoload bug? [9.5.2 (9.5.2-gfbff08 @ /home/ignacio/.emacs.d/elpa/org-9.5.2/)]
  2022-03-14 22:54                 ` Tim Cross
@ 2022-03-15  9:02                   ` Ignacio Casso
  2022-03-15 15:59                     ` Ignacio Casso
  0 siblings, 1 reply; 22+ messages in thread
From: Ignacio Casso @ 2022-03-15  9:02 UTC (permalink / raw)
  To: Tim Cross; +Cc: Max Nikulin, emacs-orgmode

>>> My recommendation would be to come up
>>> with a non-org specific example which reproduces the behaviour and then
>>> ask on emacs-devel, using the example to demonstrate the issue.
>>
>> I agree. I'm on it.

After trying to build a simple example I have realized a part of my
analysis was wrong.

> Not really, all defcustoms have a :set attribute, be it passed
> explicitly as a parameter or the default value, set-default.

I was wrong about this: set-default is not the default setter for
defcustoms without one, it's actually set-default-toplevel-value
(although everywhere in the documentation and comments it says it is
set-default). set-default-toplevel-value works well with let bindings,
but set-default does not.

> This issue happens with all autoload functions that use a custom
> variable: if they are called inside a let form binding that variable
> and the feature was not loaded yet, the let-binding will have no
> effect.

So this is only true for defcustoms without explicit :set argument if
the let form is evaluated with lexical binding enabled.

The following table shows all the cases and what works and doesn't:

- Lexical Binding and no autoload for the variable -> The let binding is
  ignored in Emacs 27 (or more accurately, it's considered lexical), and
  an error occurs in Emacs 29

- Dynamic binding or autoload for the variable:

  - No setter (or the actual default, set-default-toplevel-value) ->
    Everything works as expected

  - Setter with set or set-default (the suggested value in the
    documentation) -> Everything breaks as discussed in previous emails

So if we remove the setter for org-capture-templates, which is not
actually needed as discussed, or use set-default-toplevel-value instead
of set, it would fix the problem when dynamic binding is used, and in
Emacs 29 it would at least produce an error with lexical binding. If an
autoload cookie was also added, it would fix the problem also when
lexical binding is used, but I understand that may come with its own
disadvantages. I'll leave that decision to you, for me this is no longer
a problem since I just require org-capture now before doing this.

I'll write now the email to emacs-devel with all these issues and
mention in this thread the corresponding debbugs thread in case anyone
wants to follow it.

Regards,

Ignacio


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [BUG] org-capture autoload bug? [9.5.2 (9.5.2-gfbff08 @ /home/ignacio/.emacs.d/elpa/org-9.5.2/)]
  2022-03-14 19:43               ` Ignacio Casso
  2022-03-14 22:54                 ` Tim Cross
@ 2022-03-15 12:04                 ` Max Nikulin
  2022-03-15 12:26                   ` Ignacio Casso
  1 sibling, 1 reply; 22+ messages in thread
From: Max Nikulin @ 2022-03-15 12:04 UTC (permalink / raw)
  To: emacs-orgmode

On 15/03/2022 02:43, Ignacio Casso wrote:
> I've also investigated the issue a little bit further and wrote and
> email with my conclusions about the same time Max wrote his. I comment
> inline about a few of your thoughts:
> 
>> For `defcustom' autoload generates no more than
>>
>>      (defvar org-capture-templates nil "...")
>>
>> It seems, behavior depends on whether `org-capture-templates' has the :set
>> attribute.
> 
> Not really, all defcustoms have a :set attribute, be it passed
> explicitly as a parameter or the default value, set-default. This issue
> happens with all autoload functions that use a custom variable: if they
> are called inside a let form binding that variable and the feature was
> not loaded yet, the let-binding will have no effect.

To avoid misunderstanding. I was testing with Emacs-26.3 (Ubuntu-20.04 
focal LTS system package) and did not explicitly set lexical binding 
cookie in an .el file. However defcustom autoloading was assumed.

;; simulate effect of ;;;###autoload
(defvar org-capture-templates nil)

(defun print-org-capture-templates ()
   (message "print-org-capture-templates")
   (pp org-capture-templates)
   (message "-----"))
(let ((org-capture-templates
        '(("d" "default" entry
	  (file "/tmp/capture-test.org")
	  "* %?"))))
   (print-org-capture-templates)
   (require 'org-capture)
   (print-org-capture-templates))

Second call of print-org-capture templates prints nil with original 
definition of `org-capture-templates' but it prints locally let-bound 
value if I comment out the :set attribute.

I am not disputing any observation from your thorough study
mid:PAXPR06MB7760A3031924BA897FD082E4C60F9@PAXPR06MB7760.eurprd06.prod.outlook.com
(it seems message delivery took several hours, so I noticed it after I 
sent my message).

Maybe it is reasonable to expect that let-binding should work with 
autoloading. I believe that loading of a file should be performed inside 
top-level scope, so it should ignore let-bound values. It is fragile 
when let-binding may affect global value of defcustom. However following 
call of just loaded function should be performed with regular stack of 
dynamic scopes. I am unsure if such behavior is feasible with elisp 
implementation.



^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [BUG] org-capture autoload bug? [9.5.2 (9.5.2-gfbff08 @ /home/ignacio/.emacs.d/elpa/org-9.5.2/)]
  2022-03-15 12:04                 ` Max Nikulin
@ 2022-03-15 12:26                   ` Ignacio Casso
  0 siblings, 0 replies; 22+ messages in thread
From: Ignacio Casso @ 2022-03-15 12:26 UTC (permalink / raw)
  To: Max Nikulin; +Cc: emacs-orgmode


>>> For `defcustom' autoload generates no more than
>>>
>>>      (defvar org-capture-templates nil "...")
>>>
>>> It seems, behavior depends on whether `org-capture-templates' has the :set
>>> attribute.
>> Not really, all defcustoms have a :set attribute, be it passed
>> explicitly as a parameter or the default value, set-default. This issue
>> happens with all autoload functions that use a custom variable: if they
>> are called inside a let form binding that variable and the feature was
>> not loaded yet, the let-binding will have no effect.
>
> To avoid misunderstanding. I was testing with Emacs-26.3 (Ubuntu-20.04
> focal LTS system package) and did not explicitly set lexical binding
> cookie in an .el file. However defcustom autoloading was assumed.
>
> ;; simulate effect of ;;;###autoload
> (defvar org-capture-templates nil)
>
> (defun print-org-capture-templates ()
>   (message "print-org-capture-templates")
>   (pp org-capture-templates)
>   (message "-----"))
> (let ((org-capture-templates
>        '(("d" "default" entry
> 	  (file "/tmp/capture-test.org")
> 	  "* %?"))))
>   (print-org-capture-templates)
>   (require 'org-capture)
>   (print-org-capture-templates))
>
> Second call of print-org-capture templates prints nil with original
> definition of `org-capture-templates' but it prints locally let-bound
> value if I comment out the :set attribute.
>
> I am not disputing any observation from your thorough study
> mid:PAXPR06MB7760A3031924BA897FD082E4C60F9@PAXPR06MB7760.eurprd06.prod.outlook.com
> (it seems message delivery took several hours, so I noticed it after I
> sent my message).

Yes, I was wrong about that. Instead of testing it without :set
argument, I had tested it with a :set argument that was just
set-default plus some messages for debugging. I assumed that it was
equivalent since the documentation says that set-default is the default
value for :set. However it turns out that the actual default value that
is used when no argument is passed is set-default-toplevel-value. With
the later it works (with dynamic binding or autoload variable), but with
the former it doesn't. I think I tested once without :set argument, but
I must have used lexical binding so it didn't work anyway, so I
concluded that it didn't work either in that case.


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [BUG] org-capture autoload bug? [9.5.2 (9.5.2-gfbff08 @ /home/ignacio/.emacs.d/elpa/org-9.5.2/)]
  2022-03-15  9:02                   ` Ignacio Casso
@ 2022-03-15 15:59                     ` Ignacio Casso
  0 siblings, 0 replies; 22+ messages in thread
From: Ignacio Casso @ 2022-03-15 15:59 UTC (permalink / raw)
  To: Tim Cross; +Cc: Max Nikulin, emacs-orgmode


> I'll write now the email to emacs-devel with all these issues and
> mention in this thread the corresponding debbugs thread in case anyone
> wants to follow it.

54399@debbugs.gnu.org


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [BUG] org-capture autoload bug? [9.5.2 (9.5.2-gfbff08 @ /home/ignacio/.emacs.d/elpa/org-9.5.2/)]
@ 2022-06-11  7:50 Ignacio Casso
  2022-06-11 13:32 ` Ihor Radchenko
  0 siblings, 1 reply; 22+ messages in thread
From: Ignacio Casso @ 2022-06-11  7:50 UTC (permalink / raw)
  To: ignaciocasso; +Cc: emacs-orgmode, Max Nikulin, Tim Cross

Hello,

The bug I reported about this to the Emacs devel mailing list
(https://debbugs.gnu.org/cgi/bugreport.cgi?bug=54399) has now been
closed, and some documentation has been updated in commit 071722e41120.

Basically, the problem is that in order for

  (let ((custom-variable local-value) ...)
      ...
      (defcustom custom-variable standard-value ...)
      ...)

to work as expected with dynamic binding, `defcustom' has to set the
variable with `set-default-toplevel-value'. This way, the code inside
the `let' form uses local-value and the default value is set to
standard-value. If `defcustom' uses `set-default' instead, it overwrites
the local value, so the let-binding is useless, and after the let form
is evaluated the variable is void.

The problem was that `set-default-toplevel-value' is used by default,
but the documentation said that the default was `set-default'. So either
whoever wrote the `org-capture-templates' setter was misguided by the
documentation and used `set-default', or more likely, that setter was
written before the introduction of `set-default-toplevel-value' in
custom.el, and was never updated.

The first thing we should do is finding all uses of `set-default' in the
org `defcustom' setters and replace them with
`set-default-toplevel-value'. This would fix this bug and similar ones
when using dynamic binding, and would not affect any other use case.

Then we should decide if we want to use autoload cookies for custom
variables to make this work also with lexical binding. Otherwise, code
like the snippet above would produce an error in Emacs 29, and in Emacs
27 the let binding would be ignored (although at least the custom setter
would work). I have no opinion regarding this last point since I don't
remember what were the disadvantages of using autoload cookies for
custom variables.

I've prepared a patch for the first point, which I attach at the end of
this email. All changes fall in one of the following cases:

- `set-default' -> `set-default-toplevel-value' (as explained)

- `set' -> `set-default-toplevel-value'

  The same, but in this cases there was another bug: If a buffer set the
  custom variable locally before the feature was autoloaded, the setter
  of the variable would not set the standard value as the default for
  other buffers, and would overwrite the buffer-local value.

- :set 'set-default -> nothing, since it would be already the default

I don't really know what most of the variables whose setter I have
changed do or whether it makes any sense to use them inside a let
binding, but I have made the change for all of them nevertheless, since
it can not harm and could potentially fix a bug. Feel free to restrict
those changes only to those variables where it makes sense, such as
`org-capture-templates', if you want to keep the patch small and simple.

Best regards,

Ignacio

[2. patch --- text/x-diff; 0001-using-set-default-toplevel-value-in-defcustom-setter.patch]
From b20e23013329fab574a4d05eb5be8cde1d43dad1 Mon Sep 17 00:00:00 2001
From: Ignacio Casso <ignaciocasso@hotmail.com>
Date: Fri, 10 Jun 2022 12:39:43 +0200
Subject: [PATCH] using `set-default-toplevel-value' in `defcustom' setters

---
 lisp/ob-lilypond.el  |  2 +-
 lisp/ob-shell.el     |  2 +-
 lisp/org-capture.el  |  2 +-
 lisp/org-clock.el    |  2 +-
 lisp/org-duration.el |  2 +-
 lisp/org-faces.el    |  2 +-
 lisp/org-footnote.el |  2 +-
 lisp/org-list.el     |  4 ++--
 lisp/org.el          | 23 +++++++++++------------
 lisp/ox-odt.el       |  2 +-
 10 files changed, 21 insertions(+), 22 deletions(-)

diff --git a/lisp/ob-lilypond.el b/lisp/ob-lilypond.el
index df128441a..dc33ebc17 100644
--- a/lisp/ob-lilypond.el
+++ b/lisp/ob-lilypond.el
@@ -107,7 +107,7 @@ you can leave the string empty on this case."
   :package-version '(Org . "8.2.7")
   :set
   (lambda (symbol value)
-    (set symbol value)
+    (set-default-toplevel-value symbol value)
     (setq
      org-babel-lilypond-ly-command   (nth 0 value)
      org-babel-lilypond-pdf-command  (nth 1 value)
diff --git a/lisp/ob-shell.el b/lisp/ob-shell.el
index c25941a44..4454e3b5d 100644
--- a/lisp/ob-shell.el
+++ b/lisp/ob-shell.el
@@ -68,7 +68,7 @@ outside the Customize interface."
   :group 'org-babel
   :type '(repeat (string :tag "Shell name: "))
   :set (lambda (symbol value)
-	 (set-default symbol value)
+	 (set-default-toplevel-value symbol value)
 	 (org-babel-shell-initialize)))
 
 (defcustom org-babel-shell-results-defaults-to-output t
diff --git a/lisp/org-capture.el b/lisp/org-capture.el
index 773234967..948eb8bc6 100644
--- a/lisp/org-capture.el
+++ b/lisp/org-capture.el
@@ -376,7 +376,7 @@ When you need to insert a literal percent sign in the template,
 you can escape ambiguous cases with a backward slash, e.g., \\%i."
   :group 'org-capture
   :package-version '(Org . "9.5")
-  :set (lambda (s v) (set s (org-capture-upgrade-templates v)))
+  :set (lambda (s v) (set-default-toplevel-value s (org-capture-upgrade-templates v)))
   :type
   (let ((file-variants '(choice :tag "Filename       "
 				(file :tag "Literal")
diff --git a/lisp/org-clock.el b/lisp/org-clock.el
index e0c40ae23..b94c79baa 100644
--- a/lisp/org-clock.el
+++ b/lisp/org-clock.el
@@ -493,7 +493,7 @@ This variable only has effect if set with \\[customize]."
          (if value
              (add-hook 'kill-emacs-query-functions #'org-clock-kill-emacs-query)
            (remove-hook 'kill-emacs-query-functions #'org-clock-kill-emacs-query))
-         (set symbol value))
+         (set-default-toplevel-value symbol value))
   :type 'boolean
   :package-version '(Org . "9.5"))
 
diff --git a/lisp/org-duration.el b/lisp/org-duration.el
index b242a6f2c..338ea11a9 100644
--- a/lisp/org-duration.el
+++ b/lisp/org-duration.el
@@ -98,7 +98,7 @@ sure to call the following command:
   :version "26.1"
   :package-version '(Org . "9.1")
   :set (lambda (var val)
-         (set-default var val)
+         (set-default-toplevel-value var val)
          ;; Avoid recursive load at startup.
 	 (when (featurep 'org-duration)
            (org-duration-set-regexps)))
diff --git a/lisp/org-faces.el b/lisp/org-faces.el
index f919a6b47..5fb6c3e07 100644
--- a/lisp/org-faces.el
+++ b/lisp/org-faces.el
@@ -338,7 +338,7 @@ determines if it is a foreground or a background color."
 
 (defvar org-tags-special-faces-re nil)
 (defun org-set-tag-faces (var value)
-  (set var value)
+  (set-default-toplevel-value var value)
   (if (not value)
       (setq org-tags-special-faces-re nil)
     (setq org-tags-special-faces-re
diff --git a/lisp/org-footnote.el b/lisp/org-footnote.el
index 0a5f994a4..8e0ac0da2 100644
--- a/lisp/org-footnote.el
+++ b/lisp/org-footnote.el
@@ -110,7 +110,7 @@ you will need to run the following command after the change:
   :group 'org-footnote
   :initialize 'custom-initialize-default
   :set (lambda (var val)
-	 (set var val)
+	 (set-default-toplevel-value var val)
 	 (when (fboundp 'org-element-cache-reset)
 	   (org-element-cache-reset 'all)))
   :type '(choice
diff --git a/lisp/org-list.el b/lisp/org-list.el
index 515763036..af560cff4 100644
--- a/lisp/org-list.el
+++ b/lisp/org-list.el
@@ -235,7 +235,7 @@ interface or run the following code after updating it:
   :type '(choice (const :tag "dot like in \"2.\"" ?.)
 		 (const :tag "paren like in \"2)\"" ?\))
 		 (const :tag "both" t))
-  :set (lambda (var val) (set var val)
+  :set (lambda (var val) (set-default-toplevel-value var val)
 	 (when (featurep 'org-element) (org-element-update-syntax))))
 
 (defcustom org-list-allow-alphabetical nil
@@ -253,7 +253,7 @@ interface or run the following code after updating it:
   :group 'org-plain-lists
   :version "24.1"
   :type 'boolean
-  :set (lambda (var val) (set var val)
+  :set (lambda (var val) (set-default-toplevel-value var val)
 	 (when (featurep 'org-element) (org-element-update-syntax))))
 
 (defcustom org-list-two-spaces-after-bullet-regexp nil
diff --git a/lisp/org.el b/lisp/org.el
index ac94fb614..c57ccf319 100644
--- a/lisp/org.el
+++ b/lisp/org.el
@@ -231,7 +231,7 @@ Stars are put in group 1 and the trimmed body in group 2.")
 ;;;###autoload
 (defun org-babel-do-load-languages (sym value)
   "Load the languages defined in `org-babel-load-languages'."
-  (set-default sym value)
+  (set-default-toplevel-value sym value)
   (dolist (pair org-babel-load-languages)
     (let ((active (cdr pair)) (lang (symbol-name (car pair))))
       (if active
@@ -706,7 +706,7 @@ defined in org-duration.el.")
 
 (defun org-set-modules (var value)
   "Set VAR to VALUE and call `org-load-modules-maybe' with the force flag."
-  (set var value)
+  (set-default-toplevel-value var value)
   (when (featurep 'org)
     (org-load-modules-maybe 'force)
     (org-element-cache-reset 'all)))
@@ -837,7 +837,7 @@ depends on, if any."
   :package-version '(Org . "9.0")
   :initialize 'custom-initialize-set
   :set (lambda (var val)
-	 (if (not (featurep 'ox)) (set-default var val)
+	 (if (not (featurep 'ox)) (set-default-toplevel-value var val)
 	   ;; Any back-end not required anymore (not present in VAL and not
 	   ;; a parent of any back-end in the new value) is removed from the
 	   ;; list of registered back-ends.
@@ -862,7 +862,7 @@ depends on, if any."
 			  backend))
 		((not (memq backend new-list)) (push backend new-list))))
 	     ;; Set VAR to that list with fixed dependencies.
-	     (set-default var new-list))))
+	     (set-default-toplevel-value var new-list))))
   :type '(set :greedy t
 	      (const :tag "   ascii       Export buffer to ASCII format" ascii)
 	      (const :tag "   beamer      Export buffer to Beamer presentation" beamer)
@@ -1815,9 +1815,9 @@ are followed by a letter in parenthesis, like TODO(t)."
   :group 'org-todo
   :set (lambda (var val)
 	 (cond
-	  ((eq var t) (set var 'auto))
-	  ((eq var 'prefix) (set var nil))
-	  (t (set var val))))
+	  ((eq var t) (set-default-toplevel-value var 'auto))
+	  ((eq var 'prefix) (set-default-toplevel-value var nil))
+	  (t (set-default-toplevel-value var val))))
   :type '(choice
 	  (const :tag "Never" nil)
 	  (const :tag "Automatically, when key letter have been defined" auto)
@@ -1899,7 +1899,7 @@ be blocked if any prior sibling is not yet done.
 Finally, if the parent is blocked because of ordered siblings of its own,
 the child will also be blocked."
   :set (lambda (var val)
-	 (set var val)
+	 (set-default-toplevel-value var val)
 	 (if val
 	     (add-hook 'org-blocker-hook
 		       'org-block-todo-from-children-or-siblings-or-parent)
@@ -1917,7 +1917,7 @@ This variable needs to be set before org.el is loaded, and you need to
 restart Emacs after a change to make the change effective.  The only way
 to change it while Emacs is running is through the customize interface."
   :set (lambda (var val)
-	 (set var val)
+	 (set-default-toplevel-value var val)
 	 (if val
 	     (add-hook 'org-blocker-hook
 		       'org-block-todo-from-checkboxes)
@@ -2368,7 +2368,6 @@ The formats are defined through the variable `org-time-stamp-custom-formats'.
 To turn this on on a per-file basis, insert anywhere in the file:
    #+STARTUP: customtime"
   :group 'org-time
-  :set 'set-default
   :type 'sexp)
 (make-variable-buffer-local 'org-display-custom-times)
 
@@ -3275,7 +3274,7 @@ header, or they will be appended."
 
 (defun org-set-packages-alist (var val)
   "Set the packages alist and make sure it has 3 elements per entry."
-  (set var (mapcar (lambda (x)
+  (set-default-toplevel-value var (mapcar (lambda (x)
 		     (if (and (consp x) (= (length x) 2))
 			 (list (car x) (nth 1 x) t)
 		       x))
@@ -3548,7 +3547,7 @@ After a match, the match groups contain these elements:
 (defvar org-emphasis-alist) ; defined just below
 (defun org-set-emph-re (var val)
   "Set variable and compute the emphasis regular expression."
-  (set var val)
+  (set-default-toplevel-value var val)
   (when (and (boundp 'org-emphasis-alist)
 	     (boundp 'org-emphasis-regexp-components)
 	     org-emphasis-alist org-emphasis-regexp-components)
diff --git a/lisp/ox-odt.el b/lisp/ox-odt.el
index aa6e90122..9b46f15b5 100644
--- a/lisp/ox-odt.el
+++ b/lisp/ox-odt.el
@@ -404,7 +404,7 @@ with GNU ELPA tar or standard Emacs distribution."
     "Set `org-odt-schema-dir'.
 Also add it to `rng-schema-locating-files'."
     (let ((schema-dir value))
-      (set var
+      (set-default-toplevel-value var
 	   (if (and
 		(file-expand-wildcards
 		 (expand-file-name "od-manifest-schema*.rnc" schema-dir))
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 22+ messages in thread

* Re: [BUG] org-capture autoload bug? [9.5.2 (9.5.2-gfbff08 @ /home/ignacio/.emacs.d/elpa/org-9.5.2/)]
  2022-06-11  7:50 [BUG] org-capture autoload bug? [9.5.2 (9.5.2-gfbff08 @ /home/ignacio/.emacs.d/elpa/org-9.5.2/)] Ignacio Casso
@ 2022-06-11 13:32 ` Ihor Radchenko
  2022-06-11 17:25   ` Ignacio Casso
  2022-06-12 12:36   ` Max Nikulin
  0 siblings, 2 replies; 22+ messages in thread
From: Ihor Radchenko @ 2022-06-11 13:32 UTC (permalink / raw)
  To: Ignacio Casso; +Cc: emacs-orgmode, Max Nikulin, Tim Cross

Ignacio Casso <ignaciocasso@hotmail.com> writes:

> Then we should decide if we want to use autoload cookies for custom
> variables to make this work also with lexical binding. Otherwise, code
> like the snippet above would produce an error in Emacs 29, and in Emacs
> 27 the let binding would be ignored (although at least the custom setter
> would work). I have no opinion regarding this last point since I don't
> remember what were the disadvantages of using autoload cookies for
> custom variables.

AFAIK, autoloading defcustoms is not discussed in the manual. I have no
idea about possible pitfalls as well.

> I've prepared a patch for the first point, which I attach at the end of
> this email. All changes fall in one of the following cases:
>
> - `set-default' -> `set-default-toplevel-value' (as explained)
>
> - `set' -> `set-default-toplevel-value'
>
>   The same, but in this cases there was another bug: If a buffer set the
>   custom variable locally before the feature was autoloaded, the setter
>   of the variable would not set the standard value as the default for
>   other buffers, and would overwrite the buffer-local value.
>
> - :set 'set-default -> nothing, since it would be already the default
>
> I don't really know what most of the variables whose setter I have
> changed do or whether it makes any sense to use them inside a let
> binding, but I have made the change for all of them nevertheless, since
> it can not harm and could potentially fix a bug. Feel free to restrict
> those changes only to those variables where it makes sense, such as
> `org-capture-templates', if you want to keep the patch small and simple.

LGTM! Unless others have objections, I am inclined to merge the patch
fully. But please add changlog entries to the commit message.

Best,
Ihor


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [BUG] org-capture autoload bug? [9.5.2 (9.5.2-gfbff08 @ /home/ignacio/.emacs.d/elpa/org-9.5.2/)]
  2022-06-11 13:32 ` Ihor Radchenko
@ 2022-06-11 17:25   ` Ignacio Casso
  2022-06-14  4:02     ` Ihor Radchenko
  2022-06-12 12:36   ` Max Nikulin
  1 sibling, 1 reply; 22+ messages in thread
From: Ignacio Casso @ 2022-06-11 17:25 UTC (permalink / raw)
  To: Ihor Radchenko; +Cc: emacs-orgmode, Max Nikulin, Tim Cross

[-- Attachment #1: Type: text/plain, Size: 193 bytes --]


> LGTM! Unless others have objections, I am inclined to merge the patch
> fully. But please add changlog entries to the commit message.

Done. I attach the patch with the new commit message.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: patch --]
[-- Type: text/x-diff, Size: 10691 bytes --]

From 26d157aedfaf1496174682a1f9033d83160a06c2 Mon Sep 17 00:00:00 2001
From: Ignacio Casso <ignaciocasso@hotmail.com>
Date: Sat, 11 Jun 2022 18:59:15 +0200
Subject: [PATCH] use `set-default-toplevel-value' in `defcustom' setters

* lisp/ob-lilypond.el (org-babel-lilypond-commands): use
`set-default-toplevel-value' instead of `set' or `set-default' in
`defcustom' :set argument.
* lisp/ob-shell.el (org-babel-shell-names): Ditto.
* lisp/org-capture.el (org-capture-templates): Ditto.
* lisp/org-clock.el (org-clock-ask-before-exiting): Ditto.
* lisp/org-duration.el (org-duration-units): Ditto.
* lisp/org-faces.el (org-set-tag-faces): Ditto.
* lisp/org-footnote.el (org-footnote-section): Ditto.
* lisp/org-list.el (org-plain-list-ordered-item-terminator): Ditto.
(org-list-allow-alphabetical): Ditto.
* lisp/org.el (org-babel-do-load-languages): Ditto.
(org-set-modules): Ditto.
(org-export-backends): Ditto.
(org-use-fast-todo-selection): Ditto.
(org-enforce-todo-dependencies): Ditto.
(org-enforce-todo-checkbox-dependencies): Ditto.
(org-display-custom-times): Ditto.
(org-set-packages-alist): Ditto.
(org-set-emph-re): Ditto.
* lisp/ox-odt.el (org-odt-schema-dir): Ditto.

This commit fixes a bug that occurred when using an autoload function
inside a let-binding for a custom variable when the feature defining
both the function and the custom variable had not been loaded yet.
---
 lisp/ob-lilypond.el  |  2 +-
 lisp/ob-shell.el     |  2 +-
 lisp/org-capture.el  |  2 +-
 lisp/org-clock.el    |  2 +-
 lisp/org-duration.el |  2 +-
 lisp/org-faces.el    |  2 +-
 lisp/org-footnote.el |  2 +-
 lisp/org-list.el     |  4 ++--
 lisp/org.el          | 23 +++++++++++------------
 lisp/ox-odt.el       |  2 +-
 10 files changed, 21 insertions(+), 22 deletions(-)

diff --git a/lisp/ob-lilypond.el b/lisp/ob-lilypond.el
index df128441a..dc33ebc17 100644
--- a/lisp/ob-lilypond.el
+++ b/lisp/ob-lilypond.el
@@ -107,7 +107,7 @@ you can leave the string empty on this case."
   :package-version '(Org . "8.2.7")
   :set
   (lambda (symbol value)
-    (set symbol value)
+    (set-default-toplevel-value symbol value)
     (setq
      org-babel-lilypond-ly-command   (nth 0 value)
      org-babel-lilypond-pdf-command  (nth 1 value)
diff --git a/lisp/ob-shell.el b/lisp/ob-shell.el
index c25941a44..4454e3b5d 100644
--- a/lisp/ob-shell.el
+++ b/lisp/ob-shell.el
@@ -68,7 +68,7 @@ outside the Customize interface."
   :group 'org-babel
   :type '(repeat (string :tag "Shell name: "))
   :set (lambda (symbol value)
-	 (set-default symbol value)
+	 (set-default-toplevel-value symbol value)
 	 (org-babel-shell-initialize)))
 
 (defcustom org-babel-shell-results-defaults-to-output t
diff --git a/lisp/org-capture.el b/lisp/org-capture.el
index 773234967..948eb8bc6 100644
--- a/lisp/org-capture.el
+++ b/lisp/org-capture.el
@@ -376,7 +376,7 @@ When you need to insert a literal percent sign in the template,
 you can escape ambiguous cases with a backward slash, e.g., \\%i."
   :group 'org-capture
   :package-version '(Org . "9.5")
-  :set (lambda (s v) (set s (org-capture-upgrade-templates v)))
+  :set (lambda (s v) (set-default-toplevel-value s (org-capture-upgrade-templates v)))
   :type
   (let ((file-variants '(choice :tag "Filename       "
 				(file :tag "Literal")
diff --git a/lisp/org-clock.el b/lisp/org-clock.el
index e0c40ae23..b94c79baa 100644
--- a/lisp/org-clock.el
+++ b/lisp/org-clock.el
@@ -493,7 +493,7 @@ This variable only has effect if set with \\[customize]."
          (if value
              (add-hook 'kill-emacs-query-functions #'org-clock-kill-emacs-query)
            (remove-hook 'kill-emacs-query-functions #'org-clock-kill-emacs-query))
-         (set symbol value))
+         (set-default-toplevel-value symbol value))
   :type 'boolean
   :package-version '(Org . "9.5"))
 
diff --git a/lisp/org-duration.el b/lisp/org-duration.el
index b242a6f2c..338ea11a9 100644
--- a/lisp/org-duration.el
+++ b/lisp/org-duration.el
@@ -98,7 +98,7 @@ sure to call the following command:
   :version "26.1"
   :package-version '(Org . "9.1")
   :set (lambda (var val)
-         (set-default var val)
+         (set-default-toplevel-value var val)
          ;; Avoid recursive load at startup.
 	 (when (featurep 'org-duration)
            (org-duration-set-regexps)))
diff --git a/lisp/org-faces.el b/lisp/org-faces.el
index f919a6b47..5fb6c3e07 100644
--- a/lisp/org-faces.el
+++ b/lisp/org-faces.el
@@ -338,7 +338,7 @@ determines if it is a foreground or a background color."
 
 (defvar org-tags-special-faces-re nil)
 (defun org-set-tag-faces (var value)
-  (set var value)
+  (set-default-toplevel-value var value)
   (if (not value)
       (setq org-tags-special-faces-re nil)
     (setq org-tags-special-faces-re
diff --git a/lisp/org-footnote.el b/lisp/org-footnote.el
index 0a5f994a4..8e0ac0da2 100644
--- a/lisp/org-footnote.el
+++ b/lisp/org-footnote.el
@@ -110,7 +110,7 @@ you will need to run the following command after the change:
   :group 'org-footnote
   :initialize 'custom-initialize-default
   :set (lambda (var val)
-	 (set var val)
+	 (set-default-toplevel-value var val)
 	 (when (fboundp 'org-element-cache-reset)
 	   (org-element-cache-reset 'all)))
   :type '(choice
diff --git a/lisp/org-list.el b/lisp/org-list.el
index 97d856fc9..872436fd6 100644
--- a/lisp/org-list.el
+++ b/lisp/org-list.el
@@ -235,7 +235,7 @@ interface or run the following code after updating it:
   :type '(choice (const :tag "dot like in \"2.\"" ?.)
 		 (const :tag "paren like in \"2)\"" ?\))
 		 (const :tag "both" t))
-  :set (lambda (var val) (set var val)
+  :set (lambda (var val) (set-default-toplevel-value var val)
 	 (when (featurep 'org-element) (org-element-update-syntax))))
 
 (defcustom org-list-allow-alphabetical nil
@@ -253,7 +253,7 @@ interface or run the following code after updating it:
   :group 'org-plain-lists
   :version "24.1"
   :type 'boolean
-  :set (lambda (var val) (set var val)
+  :set (lambda (var val) (set-default-toplevel-value var val)
 	 (when (featurep 'org-element) (org-element-update-syntax))))
 
 (defcustom org-list-two-spaces-after-bullet-regexp nil
diff --git a/lisp/org.el b/lisp/org.el
index 080962cdb..f01cd988f 100644
--- a/lisp/org.el
+++ b/lisp/org.el
@@ -231,7 +231,7 @@ Stars are put in group 1 and the trimmed body in group 2.")
 ;;;###autoload
 (defun org-babel-do-load-languages (sym value)
   "Load the languages defined in `org-babel-load-languages'."
-  (set-default sym value)
+  (set-default-toplevel-value sym value)
   (dolist (pair org-babel-load-languages)
     (let ((active (cdr pair)) (lang (symbol-name (car pair))))
       (if active
@@ -706,7 +706,7 @@ defined in org-duration.el.")
 
 (defun org-set-modules (var value)
   "Set VAR to VALUE and call `org-load-modules-maybe' with the force flag."
-  (set var value)
+  (set-default-toplevel-value var value)
   (when (featurep 'org)
     (org-load-modules-maybe 'force)
     (org-element-cache-reset 'all)))
@@ -837,7 +837,7 @@ depends on, if any."
   :package-version '(Org . "9.0")
   :initialize 'custom-initialize-set
   :set (lambda (var val)
-	 (if (not (featurep 'ox)) (set-default var val)
+	 (if (not (featurep 'ox)) (set-default-toplevel-value var val)
 	   ;; Any back-end not required anymore (not present in VAL and not
 	   ;; a parent of any back-end in the new value) is removed from the
 	   ;; list of registered back-ends.
@@ -862,7 +862,7 @@ depends on, if any."
 			  backend))
 		((not (memq backend new-list)) (push backend new-list))))
 	     ;; Set VAR to that list with fixed dependencies.
-	     (set-default var new-list))))
+	     (set-default-toplevel-value var new-list))))
   :type '(set :greedy t
 	      (const :tag "   ascii       Export buffer to ASCII format" ascii)
 	      (const :tag "   beamer      Export buffer to Beamer presentation" beamer)
@@ -1815,9 +1815,9 @@ are followed by a letter in parenthesis, like TODO(t)."
   :group 'org-todo
   :set (lambda (var val)
 	 (cond
-	  ((eq var t) (set var 'auto))
-	  ((eq var 'prefix) (set var nil))
-	  (t (set var val))))
+	  ((eq var t) (set-default-toplevel-value var 'auto))
+	  ((eq var 'prefix) (set-default-toplevel-value var nil))
+	  (t (set-default-toplevel-value var val))))
   :type '(choice
 	  (const :tag "Never" nil)
 	  (const :tag "Automatically, when key letter have been defined" auto)
@@ -1899,7 +1899,7 @@ be blocked if any prior sibling is not yet done.
 Finally, if the parent is blocked because of ordered siblings of its own,
 the child will also be blocked."
   :set (lambda (var val)
-	 (set var val)
+	 (set-default-toplevel-value var val)
 	 (if val
 	     (add-hook 'org-blocker-hook
 		       'org-block-todo-from-children-or-siblings-or-parent)
@@ -1917,7 +1917,7 @@ This variable needs to be set before org.el is loaded, and you need to
 restart Emacs after a change to make the change effective.  The only way
 to change it while Emacs is running is through the customize interface."
   :set (lambda (var val)
-	 (set var val)
+	 (set-default-toplevel-value var val)
 	 (if val
 	     (add-hook 'org-blocker-hook
 		       'org-block-todo-from-checkboxes)
@@ -2368,7 +2368,6 @@ The formats are defined through the variable `org-time-stamp-custom-formats'.
 To turn this on on a per-file basis, insert anywhere in the file:
    #+STARTUP: customtime"
   :group 'org-time
-  :set 'set-default
   :type 'sexp)
 (make-variable-buffer-local 'org-display-custom-times)
 
@@ -3275,7 +3274,7 @@ header, or they will be appended."
 
 (defun org-set-packages-alist (var val)
   "Set the packages alist and make sure it has 3 elements per entry."
-  (set var (mapcar (lambda (x)
+  (set-default-toplevel-value var (mapcar (lambda (x)
 		     (if (and (consp x) (= (length x) 2))
 			 (list (car x) (nth 1 x) t)
 		       x))
@@ -3548,7 +3547,7 @@ After a match, the match groups contain these elements:
 (defvar org-emphasis-alist) ; defined just below
 (defun org-set-emph-re (var val)
   "Set variable and compute the emphasis regular expression."
-  (set var val)
+  (set-default-toplevel-value var val)
   (when (and (boundp 'org-emphasis-alist)
 	     (boundp 'org-emphasis-regexp-components)
 	     org-emphasis-alist org-emphasis-regexp-components)
diff --git a/lisp/ox-odt.el b/lisp/ox-odt.el
index aa6e90122..9b46f15b5 100644
--- a/lisp/ox-odt.el
+++ b/lisp/ox-odt.el
@@ -404,7 +404,7 @@ with GNU ELPA tar or standard Emacs distribution."
     "Set `org-odt-schema-dir'.
 Also add it to `rng-schema-locating-files'."
     (let ((schema-dir value))
-      (set var
+      (set-default-toplevel-value var
 	   (if (and
 		(file-expand-wildcards
 		 (expand-file-name "od-manifest-schema*.rnc" schema-dir))
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 22+ messages in thread

* Re: [BUG] org-capture autoload bug? [9.5.2 (9.5.2-gfbff08 @ /home/ignacio/.emacs.d/elpa/org-9.5.2/)]
  2022-06-11 13:32 ` Ihor Radchenko
  2022-06-11 17:25   ` Ignacio Casso
@ 2022-06-12 12:36   ` Max Nikulin
  1 sibling, 0 replies; 22+ messages in thread
From: Max Nikulin @ 2022-06-12 12:36 UTC (permalink / raw)
  To: emacs-orgmode

On 11/06/2022 20:32, Ihor Radchenko wrote:
> Ignacio Casso writes:
> 
>> Then we should decide if we want to use autoload cookies for custom
>> variables to make this work also with lexical binding. Otherwise, code
>> like the snippet above would produce an error in Emacs 29, and in Emacs
>> 27 the let binding would be ignored (although at least the custom setter
>> would work). I have no opinion regarding this last point since I don't
>> remember what were the disadvantages of using autoload cookies for
>> custom variables.
> 
> AFAIK, autoloading defcustoms is not discussed in the manual. I have no
> idea about possible pitfalls as well.

I have impression that it is not encouraged, but Emacs sources have 
enough examples of such autoloading. I posted some links earlier in the 
discussion of this issue, see
Max Nikulin, Fri, 11 Mar 2022 17:07:03 +0700.
https://list.orgmode.org/c58ae4d5-dd66-0d06-a332-cfdf23e1889e@gmail.com

> LGTM! Unless others have objections, I am inclined to merge the patch
> fully.

I see clear intention of improvement, but I have no particular opinion 
if the patch is risky. I worry a bit that some code related to this 
issue has changed in the development branch of Emacs, so I am unsure 
concerning behavior in earlier versions. I do not object committing the 
patch since it can be reverted later if some problem would arise.



^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [BUG] org-capture autoload bug? [9.5.2 (9.5.2-gfbff08 @ /home/ignacio/.emacs.d/elpa/org-9.5.2/)]
  2022-06-11 17:25   ` Ignacio Casso
@ 2022-06-14  4:02     ` Ihor Radchenko
  2022-06-14  7:49       ` Ignacio Casso
  0 siblings, 1 reply; 22+ messages in thread
From: Ihor Radchenko @ 2022-06-14  4:02 UTC (permalink / raw)
  To: Ignacio Casso; +Cc: emacs-orgmode, Max Nikulin, Tim Cross

Ignacio Casso <ignaciocasso@hotmail.com> writes:

>> LGTM! Unless others have objections, I am inclined to merge the patch
>> fully. But please add changlog entries to the commit message.
>
> Done. I attach the patch with the new commit message.

Thanks! Some more comments.

> Subject: [PATCH] use `set-default-toplevel-value' in `defcustom' setters
                   ^Use

> * lisp/ob-lilypond.el (org-babel-lilypond-commands): use
                                                       ^Use
> `set-default-toplevel-value' instead of `set' or `set-default' in
> `defcustom' :set argument.
> * lisp/ob-shell.el (org-babel-shell-names): Ditto.

Please don't use ditto. (what does it even mean?)

> This commit fixes a bug that occurred when using an autoload function
> inside a let-binding for a custom variable when the feature defining
> both the function and the custom variable had not been loaded yet.

It would be helpful to reference the relevant ML discussions.

Best,
Ihor


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [BUG] org-capture autoload bug? [9.5.2 (9.5.2-gfbff08 @ /home/ignacio/.emacs.d/elpa/org-9.5.2/)]
  2022-06-14  4:02     ` Ihor Radchenko
@ 2022-06-14  7:49       ` Ignacio Casso
  2022-06-14 13:58         ` Ihor Radchenko
  0 siblings, 1 reply; 22+ messages in thread
From: Ignacio Casso @ 2022-06-14  7:49 UTC (permalink / raw)
  To: Ihor Radchenko; +Cc: emacs-orgmode, Max Nikulin, Tim Cross

[-- Attachment #1: Type: text/plain, Size: 1067 bytes --]


>> Subject: [PATCH] use `set-default-toplevel-value' in `defcustom' setters
>                    ^Use
>
>> * lisp/ob-lilypond.el (org-babel-lilypond-commands): use
>                                                        ^Use
>> `set-default-toplevel-value' instead of `set' or `set-default' in
>> `defcustom' :set argument.
>> * lisp/ob-shell.el (org-babel-shell-names): Ditto.

Done.

> Please don't use ditto. (what does it even mean?)

It means the same thing again. I saw it in the message of the Emacs
commit I referenced earlier in this thread, so I assumed it was standard
practice. I have replaced it with "The same". But if the problem is that
it makes it difficult to work later with the changelog, I can copy and
paste.


> It would be helpful to reference the relevant ML discussions.

Done. Is there a standard, concise way to do this for org-mode threads?
I know I can reference the Emacs discussion with just bug#54399, but I'm
using long lists.gnu.org/archive/... links for org-mode.


I attach the new patch (same diff, different commit message):


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: New patch with better commit message --]
[-- Type: text/x-diff, Size: 10908 bytes --]

From c38584ea4e396f56d34ca934582c43436372b102 Mon Sep 17 00:00:00 2001
From: Ignacio Casso <ignaciocasso@hotmail.com>
Date: Sat, 11 Jun 2022 18:59:15 +0200
Subject: [PATCH] Use `set-default-toplevel-value' in `defcustom' setters

* lisp/ob-lilypond.el (org-babel-lilypond-commands): Use
`set-default-toplevel-value' instead of `set' or `set-default' in
`defcustom' :set argument.
* lisp/ob-shell.el (org-babel-shell-names): The same.
* lisp/org-capture.el (org-capture-templates): The same.
* lisp/org-clock.el (org-clock-ask-before-exiting): The same.
* lisp/org-duration.el (org-duration-units): The same.
* lisp/org-faces.el (org-set-tag-faces): The same.
* lisp/org-footnote.el (org-footnote-section): The same.
* lisp/org-list.el (org-plain-list-ordered-item-terminator): The same.
(org-list-allow-alphabetical): The same.
* lisp/org.el (org-babel-do-load-languages): The same.
(org-set-modules): The same.
(org-export-backends): The same.
(org-use-fast-todo-selection): The same.
(org-enforce-todo-dependencies): The same.
(org-enforce-todo-checkbox-dependencies): The same.
(org-display-custom-times): The same.
(org-set-packages-alist): The same.
(org-set-emph-re): The same.
* lisp/ox-odt.el (org-odt-schema-dir): The same.

This commit fixes a bug that occurred when using an autoload function
inside a let-binding for a custom variable when the feature defining
both the function and the custom variable had not been loaded yet.

See bug#54399 and
https://lists.gnu.org/archive/html/emacs-orgmode/2022-03/msg00085.html,
https://lists.gnu.org/archive/html/emacs-orgmode/2022-06/msg00226.html,
---
 lisp/ob-lilypond.el  |  2 +-
 lisp/ob-shell.el     |  2 +-
 lisp/org-capture.el  |  2 +-
 lisp/org-clock.el    |  2 +-
 lisp/org-duration.el |  2 +-
 lisp/org-faces.el    |  2 +-
 lisp/org-footnote.el |  2 +-
 lisp/org-list.el     |  4 ++--
 lisp/org.el          | 23 +++++++++++------------
 lisp/ox-odt.el       |  2 +-
 10 files changed, 21 insertions(+), 22 deletions(-)

diff --git a/lisp/ob-lilypond.el b/lisp/ob-lilypond.el
index df128441a..dc33ebc17 100644
--- a/lisp/ob-lilypond.el
+++ b/lisp/ob-lilypond.el
@@ -107,7 +107,7 @@ you can leave the string empty on this case."
   :package-version '(Org . "8.2.7")
   :set
   (lambda (symbol value)
-    (set symbol value)
+    (set-default-toplevel-value symbol value)
     (setq
      org-babel-lilypond-ly-command   (nth 0 value)
      org-babel-lilypond-pdf-command  (nth 1 value)
diff --git a/lisp/ob-shell.el b/lisp/ob-shell.el
index c25941a44..4454e3b5d 100644
--- a/lisp/ob-shell.el
+++ b/lisp/ob-shell.el
@@ -68,7 +68,7 @@ outside the Customize interface."
   :group 'org-babel
   :type '(repeat (string :tag "Shell name: "))
   :set (lambda (symbol value)
-	 (set-default symbol value)
+	 (set-default-toplevel-value symbol value)
 	 (org-babel-shell-initialize)))
 
 (defcustom org-babel-shell-results-defaults-to-output t
diff --git a/lisp/org-capture.el b/lisp/org-capture.el
index 773234967..948eb8bc6 100644
--- a/lisp/org-capture.el
+++ b/lisp/org-capture.el
@@ -376,7 +376,7 @@ When you need to insert a literal percent sign in the template,
 you can escape ambiguous cases with a backward slash, e.g., \\%i."
   :group 'org-capture
   :package-version '(Org . "9.5")
-  :set (lambda (s v) (set s (org-capture-upgrade-templates v)))
+  :set (lambda (s v) (set-default-toplevel-value s (org-capture-upgrade-templates v)))
   :type
   (let ((file-variants '(choice :tag "Filename       "
 				(file :tag "Literal")
diff --git a/lisp/org-clock.el b/lisp/org-clock.el
index e0c40ae23..b94c79baa 100644
--- a/lisp/org-clock.el
+++ b/lisp/org-clock.el
@@ -493,7 +493,7 @@ This variable only has effect if set with \\[customize]."
          (if value
              (add-hook 'kill-emacs-query-functions #'org-clock-kill-emacs-query)
            (remove-hook 'kill-emacs-query-functions #'org-clock-kill-emacs-query))
-         (set symbol value))
+         (set-default-toplevel-value symbol value))
   :type 'boolean
   :package-version '(Org . "9.5"))
 
diff --git a/lisp/org-duration.el b/lisp/org-duration.el
index b242a6f2c..338ea11a9 100644
--- a/lisp/org-duration.el
+++ b/lisp/org-duration.el
@@ -98,7 +98,7 @@ sure to call the following command:
   :version "26.1"
   :package-version '(Org . "9.1")
   :set (lambda (var val)
-         (set-default var val)
+         (set-default-toplevel-value var val)
          ;; Avoid recursive load at startup.
 	 (when (featurep 'org-duration)
            (org-duration-set-regexps)))
diff --git a/lisp/org-faces.el b/lisp/org-faces.el
index f919a6b47..5fb6c3e07 100644
--- a/lisp/org-faces.el
+++ b/lisp/org-faces.el
@@ -338,7 +338,7 @@ determines if it is a foreground or a background color."
 
 (defvar org-tags-special-faces-re nil)
 (defun org-set-tag-faces (var value)
-  (set var value)
+  (set-default-toplevel-value var value)
   (if (not value)
       (setq org-tags-special-faces-re nil)
     (setq org-tags-special-faces-re
diff --git a/lisp/org-footnote.el b/lisp/org-footnote.el
index 0a5f994a4..8e0ac0da2 100644
--- a/lisp/org-footnote.el
+++ b/lisp/org-footnote.el
@@ -110,7 +110,7 @@ you will need to run the following command after the change:
   :group 'org-footnote
   :initialize 'custom-initialize-default
   :set (lambda (var val)
-	 (set var val)
+	 (set-default-toplevel-value var val)
 	 (when (fboundp 'org-element-cache-reset)
 	   (org-element-cache-reset 'all)))
   :type '(choice
diff --git a/lisp/org-list.el b/lisp/org-list.el
index 97d856fc9..872436fd6 100644
--- a/lisp/org-list.el
+++ b/lisp/org-list.el
@@ -235,7 +235,7 @@ interface or run the following code after updating it:
   :type '(choice (const :tag "dot like in \"2.\"" ?.)
 		 (const :tag "paren like in \"2)\"" ?\))
 		 (const :tag "both" t))
-  :set (lambda (var val) (set var val)
+  :set (lambda (var val) (set-default-toplevel-value var val)
 	 (when (featurep 'org-element) (org-element-update-syntax))))
 
 (defcustom org-list-allow-alphabetical nil
@@ -253,7 +253,7 @@ interface or run the following code after updating it:
   :group 'org-plain-lists
   :version "24.1"
   :type 'boolean
-  :set (lambda (var val) (set var val)
+  :set (lambda (var val) (set-default-toplevel-value var val)
 	 (when (featurep 'org-element) (org-element-update-syntax))))
 
 (defcustom org-list-two-spaces-after-bullet-regexp nil
diff --git a/lisp/org.el b/lisp/org.el
index 229435240..1417fd5f3 100644
--- a/lisp/org.el
+++ b/lisp/org.el
@@ -231,7 +231,7 @@ Stars are put in group 1 and the trimmed body in group 2.")
 ;;;###autoload
 (defun org-babel-do-load-languages (sym value)
   "Load the languages defined in `org-babel-load-languages'."
-  (set-default sym value)
+  (set-default-toplevel-value sym value)
   (dolist (pair org-babel-load-languages)
     (let ((active (cdr pair)) (lang (symbol-name (car pair))))
       (if active
@@ -706,7 +706,7 @@ defined in org-duration.el.")
 
 (defun org-set-modules (var value)
   "Set VAR to VALUE and call `org-load-modules-maybe' with the force flag."
-  (set var value)
+  (set-default-toplevel-value var value)
   (when (featurep 'org)
     (org-load-modules-maybe 'force)
     (org-element-cache-reset 'all)))
@@ -837,7 +837,7 @@ depends on, if any."
   :package-version '(Org . "9.0")
   :initialize 'custom-initialize-set
   :set (lambda (var val)
-	 (if (not (featurep 'ox)) (set-default var val)
+	 (if (not (featurep 'ox)) (set-default-toplevel-value var val)
 	   ;; Any back-end not required anymore (not present in VAL and not
 	   ;; a parent of any back-end in the new value) is removed from the
 	   ;; list of registered back-ends.
@@ -862,7 +862,7 @@ depends on, if any."
 			  backend))
 		((not (memq backend new-list)) (push backend new-list))))
 	     ;; Set VAR to that list with fixed dependencies.
-	     (set-default var new-list))))
+	     (set-default-toplevel-value var new-list))))
   :type '(set :greedy t
 	      (const :tag "   ascii       Export buffer to ASCII format" ascii)
 	      (const :tag "   beamer      Export buffer to Beamer presentation" beamer)
@@ -1815,9 +1815,9 @@ are followed by a letter in parenthesis, like TODO(t)."
   :group 'org-todo
   :set (lambda (var val)
 	 (cond
-	  ((eq var t) (set var 'auto))
-	  ((eq var 'prefix) (set var nil))
-	  (t (set var val))))
+	  ((eq var t) (set-default-toplevel-value var 'auto))
+	  ((eq var 'prefix) (set-default-toplevel-value var nil))
+	  (t (set-default-toplevel-value var val))))
   :type '(choice
 	  (const :tag "Never" nil)
 	  (const :tag "Automatically, when key letter have been defined" auto)
@@ -1899,7 +1899,7 @@ be blocked if any prior sibling is not yet done.
 Finally, if the parent is blocked because of ordered siblings of its own,
 the child will also be blocked."
   :set (lambda (var val)
-	 (set var val)
+	 (set-default-toplevel-value var val)
 	 (if val
 	     (add-hook 'org-blocker-hook
 		       'org-block-todo-from-children-or-siblings-or-parent)
@@ -1917,7 +1917,7 @@ This variable needs to be set before org.el is loaded, and you need to
 restart Emacs after a change to make the change effective.  The only way
 to change it while Emacs is running is through the customize interface."
   :set (lambda (var val)
-	 (set var val)
+	 (set-default-toplevel-value var val)
 	 (if val
 	     (add-hook 'org-blocker-hook
 		       'org-block-todo-from-checkboxes)
@@ -2368,7 +2368,6 @@ The formats are defined through the variable `org-time-stamp-custom-formats'.
 To turn this on on a per-file basis, insert anywhere in the file:
    #+STARTUP: customtime"
   :group 'org-time
-  :set 'set-default
   :type 'sexp)
 (make-variable-buffer-local 'org-display-custom-times)
 
@@ -3275,7 +3274,7 @@ header, or they will be appended."
 
 (defun org-set-packages-alist (var val)
   "Set the packages alist and make sure it has 3 elements per entry."
-  (set var (mapcar (lambda (x)
+  (set-default-toplevel-value var (mapcar (lambda (x)
 		     (if (and (consp x) (= (length x) 2))
 			 (list (car x) (nth 1 x) t)
 		       x))
@@ -3548,7 +3547,7 @@ After a match, the match groups contain these elements:
 (defvar org-emphasis-alist) ; defined just below
 (defun org-set-emph-re (var val)
   "Set variable and compute the emphasis regular expression."
-  (set var val)
+  (set-default-toplevel-value var val)
   (when (and (boundp 'org-emphasis-alist)
 	     (boundp 'org-emphasis-regexp-components)
 	     org-emphasis-alist org-emphasis-regexp-components)
diff --git a/lisp/ox-odt.el b/lisp/ox-odt.el
index aa6e90122..9b46f15b5 100644
--- a/lisp/ox-odt.el
+++ b/lisp/ox-odt.el
@@ -404,7 +404,7 @@ with GNU ELPA tar or standard Emacs distribution."
     "Set `org-odt-schema-dir'.
 Also add it to `rng-schema-locating-files'."
     (let ((schema-dir value))
-      (set var
+      (set-default-toplevel-value var
 	   (if (and
 		(file-expand-wildcards
 		 (expand-file-name "od-manifest-schema*.rnc" schema-dir))
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 22+ messages in thread

* Re: [BUG] org-capture autoload bug? [9.5.2 (9.5.2-gfbff08 @ /home/ignacio/.emacs.d/elpa/org-9.5.2/)]
  2022-06-14  7:49       ` Ignacio Casso
@ 2022-06-14 13:58         ` Ihor Radchenko
  0 siblings, 0 replies; 22+ messages in thread
From: Ihor Radchenko @ 2022-06-14 13:58 UTC (permalink / raw)
  To: Ignacio Casso; +Cc: emacs-orgmode, Max Nikulin, Tim Cross

Ignacio Casso <ignaciocasso@hotmail.com> writes:

>> Please don't use ditto. (what does it even mean?)
>
> It means the same thing again. I saw it in the message of the Emacs
> commit I referenced earlier in this thread, so I assumed it was standard
> practice. I have replaced it with "The same". But if the problem is that
> it makes it difficult to work later with the changelog, I can copy and
> paste.

Never mind.
Applied onto main via f9ea6c61e with necessary amendments to the commit
message.

What I meant is simply leaving all but last description empty:

* lisp/ob-shell.el (org-babel-shell-names):
* lisp/org-capture.el (org-capture-templates):
* lisp/org-clock.el (org-clock-ask-before-exiting):
...
* lisp/ob-lilypond.el (org-babel-lilypond-commands): Use
`set-default-toplevel-value' instead of `set' or `set-default' in
`defcustom' :set argument.

Best,
Ihor


^ permalink raw reply	[flat|nested] 22+ messages in thread

end of thread, other threads:[~2022-06-14 13:58 UTC | newest]

Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-06-11  7:50 [BUG] org-capture autoload bug? [9.5.2 (9.5.2-gfbff08 @ /home/ignacio/.emacs.d/elpa/org-9.5.2/)] Ignacio Casso
2022-06-11 13:32 ` Ihor Radchenko
2022-06-11 17:25   ` Ignacio Casso
2022-06-14  4:02     ` Ihor Radchenko
2022-06-14  7:49       ` Ignacio Casso
2022-06-14 13:58         ` Ihor Radchenko
2022-06-12 12:36   ` Max Nikulin
  -- strict thread matches above, loose matches on Subject: below --
2022-03-10 12:53 Ignacio Casso
2022-03-10 16:32 ` Max Nikulin
2022-03-10 18:00   ` Ignacio Casso
2022-03-11 10:07     ` Max Nikulin
2022-03-11 10:38       ` Ignacio Casso
2022-03-11 19:59         ` Tim Cross
2022-03-14 10:42           ` Ignacio Casso
2022-03-14 14:52           ` Max Nikulin
2022-03-14 18:42             ` Tim Cross
2022-03-14 19:43               ` Ignacio Casso
2022-03-14 22:54                 ` Tim Cross
2022-03-15  9:02                   ` Ignacio Casso
2022-03-15 15:59                     ` Ignacio Casso
2022-03-15 12:04                 ` Max Nikulin
2022-03-15 12:26                   ` Ignacio Casso

Code repositories for project(s) associated with this public inbox

	https://git.savannah.gnu.org/cgit/emacs/org-mode.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).