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/)
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.
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.
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.
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.
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?)
> 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.
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.
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.
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.
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!
>>> 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
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.
>>> 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.
> 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
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
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
[-- 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
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.
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
[-- 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
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