Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Problem with generated autoloads #13

Closed
cruegge opened this issue Apr 4, 2017 · 10 comments
Closed

Problem with generated autoloads #13

cruegge opened this issue Apr 4, 2017 · 10 comments

Comments

@cruegge
Copy link

cruegge commented Apr 4, 2017

The autoloads.el generated for this package appears to be a bit problematic: it first defines an autoload for the helm-org-rifle-define-command macro and then directly uses that macro to define the various commands inline. Depending on the order in which the autoloads file is loaded and the load-path is set up, that either directly pulls in the package and its dependencies, or fails to define the commands, both of which are missing the point of an autoload file.

There does not seem to be a way of telling the the autoload cookie mechanism about new command-defining top-level forms -- it just special-cases some forms like defun and copies the rest verbatim. So the only way I see around this problem would be to add explicit autoloads, i.e. instead of

;;;###autoload
(helm-org-rifle-define-command "current-buffer" ()

one should probably write

;;;###autoload
(autoload 'helm-org-rifle-current-buffer "helm-org-rifle" nil t)
(helm-org-rifle-define-command "current-buffer" ()

and remove the autoload on the macro altogether.

I can prepare a PR for this if you agree.

@alphapapa
Copy link
Owner

Hi, thank you very much for reporting this. I recently converted most of the commands to be defined with macros, and I don't have a firm grasp of how autoloading works, especially when macros are involved.

Looking at the elisp info pages, I found this:

   If you write a function definition with an unusual macro that is not
one of the known and recognized function definition methods, use of an
ordinary magic autoload comment would copy the whole definition into
`loaddefs.el'.  That is not desirable.  You can put the desired
`autoload' call into `loaddefs.el' instead by writing this:

     ;;;###autoload (autoload 'foo "myfile")
     (mydefunmacro foo
       ...)

IIUC, that means that cookies like that should be before each of the helm-org-rifle-define-command forms--but I'm not sure IIUC. :)

I'm also not sure about the cookies on the macros themselves. The info page says:

   The same magic comment can copy any kind of form into `loaddefs.el'.
The form following the magic comment is copied verbatim, _except_ if it
is one of the forms which the autoload facility handles specially
(e.g., by conversion into an `autoload' call).  The forms which are not
copied verbatim are the following:

Definitions for function or function-like objects:
     `defun' and `defmacro'; also `cl-defun' and `cl-defmacro' (*note
     Argument Lists: (cl)Argument Lists.), and
     `define-overloadable-function' (see the commentary in
     `mode-local.el').

That seems to imply that the cookies should be on the macro definitions, but again I'm not sure.

I'll take a look at some bigger packages and see how they do it, see if I can figure out the proper way. Please let me know if you find out anything more also.

By the way, for future reference, how did you discover this problem? I would like to be able to reproduce it on my end. :)

Thanks!

@alphapapa
Copy link
Owner

alphapapa commented Apr 5, 2017

Googling for this was surprisingly difficult. I think the closest I've come to finding a plain answer is this: http://stackoverflow.com/questions/38779350/create-autoloaded-function-from-macro

What's really confusing about that, though, is this answer from who appears to be Stefan Monnier:

The code that picks up the ;;;###autoload cookies does expand the macros before looking at the code, so you should be able to just place a ;;;###autoload cookie right before a (align-by ...) expression and get the right autoload placed in the -autoloads.el file.

The problem, tho is that your macro is probably not going to be defined at the time the autoloads are created, so the macro expansion will not actually happen. Maybe a M-x report-emacs-bug is in order.

That answer was posted on 6 Aug 2016. It just seems very strange to me that someone like him would be recognizing this as a bug in Emacs...in 2016! Isn't defining functions with macros a fairly common thing to do? This doesn't make sense to me, so I feel like I'm missing something obvious. I know I've seen other packages do it, but I can't seem to find a good example at the moment. (I feel like such a noob right now.)

@cruegge
Copy link
Author

cruegge commented Apr 5, 2017

First of all, thanks for the quick and extensive reply.

IIUC, that means that cookies like that should be before each of the helm-org-rifle-define-command forms--but I'm not sure IIUC. :)

That's pretty much how I understand it, too.

That seems to imply that the cookies should be on the macro definitions, but again I'm not sure.

I think it just says that cookies are possible on macro definitions if you want to autoload the macro itself. It does not apply to functions defined by invocations of the macro.

What's really confusing about that, though, is this answer from who appears to be Stefan Monnier:

I also find this confusing, as it seems to contradict what the manual says. I looked into the implementation to see if I could figure it out. The main function is autoload-generate-file-autoloads, which essentially walks through all sexps in the file and looks for the generate-autoload-cookie (which is just the string ;;;###autload) and calls autoload--print-cookie-text on each result. That function reads a single sexp using (read (current-buffer)) (without expansion) and passes it to make-autoload, which finally generates the autoload. Now that function calls macroexpand in exactly one place, namely when the car of the parsed expression is one of the special forms defun, defmacro etc. So it seems IIUC as if no general macroexpansion happens, contrary to Stefan's claim.

By the way, for future reference, how did you discover this problem? I would like to be able to reproduce it on my end. :)

I'm not sure how package.el handles things, but I've tried switching to borg after it was mentioned a couple days ago on reddit. Borg first processes the autoload file and then sets up the load path, so the error was pretty easy to spot: it produced a warning when borg tried loading the file, and failed to define the autoloads, rendering the package unusuable out of the box.

@alphapapa
Copy link
Owner

First of all, thanks for the quick and extensive reply.

And to you. :)

I think it just says that cookies are possible on macro definitions if you want to autoload the macro itself. It does not apply to functions defined by invocations of the macro.

This is confusing to me, because what about byte-compiled packages? I guess this means that, for byte-compiled packages, it's not necessary to put the cookie on the macro definition, because it will be expanded when the package is byte-compiled. ... I guess it also means that, at least for a macro like this that isn't intended to be used outside this package, the macro doesn't need a cookie at all, regardless of byte-compiling.

I also find this confusing, as it seems to contradict what the manual says. I looked into the implementation to see if I could figure it out.

Wow, thanks for digging into that. Maybe he was right when he said that a bug report should be filed. But what I can hardly fathom is that, after all these years, we would be nearly the first to notice and report this as a bug--so I still feel like I'm just misunderstanding something.

I'm not sure how package.el handles things, but I've tried switching to borg after it was mentioned a couple days ago on reddit.

Yeah, Borg looks really interesting! I think I would give it a try if it weren't for its using git submodules. I just haven't wrapped my head around them, and I'm not quite comfortable with them. As it is I keep my /elpa directory in git anyway, so I'm not sure I would gain much by using it.

Anyway, could this be a bug in Borg? It seems like it ought to do things basically the same way that package.el does. If it is an issue with Borg, I'm not sure if we should change anything here. But I guess it could also be a bug in both, and just luck that it works with package.el.

Hmm...are you brave enough to bring this up on emacs-devel? ;)

@cruegge
Copy link
Author

cruegge commented Apr 5, 2017

I guess this means that, for byte-compiled packages, it's not necessary to put the cookie on the macro definition, because it will be expanded when the package is byte-compiled. ... I guess it also means that, at least for a macro like this that isn't intended to be used outside this package, the macro doesn't need a cookie at all, regardless of byte-compiling.

I think so, too, either it is expanded at compile time, or defined when loading the file from source. In any case, autoloads are not necessary for internal macros.

Anyway, could this be a bug in Borg? It seems like it ought to do things basically the same way that package.el does.

Hmm, it basically does things the same way as package.el: assuming that autoload files do not trigger loading of further files, the order of things should not matter. It's this assumption that is not fulfilled by the generated autoloads. Plus, in case package.el sets up the load path first, I'd assume it will load org and helm directly, i.e. it's also broken there, only in a different way. To check that, maybe you could try and eval (package-initialize) from emacs -Q and then see whether (featurep 'org) is t (in case you actually have the package installed via package.el, of course).

Hmm...are you brave enough to bring this up on emacs-devel? ;)

Not sure, maybe we could as in /r/emacs first, there are a lot of knowledgeable people there, and the barrier is a bit lower for something that might just be a misunderstanding.

@alphapapa
Copy link
Owner

alphapapa commented Apr 7, 2017

Not sure, maybe we could as in /r/emacs first, there are a lot of knowledgeable people there, and the barrier is a bit lower for something that might just be a misunderstanding.

Excellent idea, and I just saw that you posted it, so thanks for that. :)

BTW, do you have any Emacs packages? You seem quite knowledgeable!

@cruegge
Copy link
Author

cruegge commented Apr 7, 2017

Yeah, but not very successful as of yet :-/

No, I don't have any packages. I like poking around, but I'm generally to lazy for larger stuff :)

@alphapapa
Copy link
Owner

Hmm, let me try something...

@tarsius Sorry to bug you, but I wonder if you could teach us the way of the autoload... :)

@tarsius
Copy link

tarsius commented Apr 8, 2017

It hardly ever makes sense to autoload a macro, less so for a "definer" macro. You don't have to do so just to make those commands/funtions/variables autoloadable that were defined using it. Just autoload those definitions using something like ;;;###autoload (autoload 'bar-some-command "bar" nil t).

If some other file foo.el uses the bar-define-command macro, which is defined in bar.el, then you do have to (require 'bar). However you might still be able to avoid doing so when the package is loaded using (eval-when-compile (require 'bar)).

The macro doesn't have to be autoloaded for this to work. If bar-define-command's expansion contains symbols from bar then you might have to autoload those, depending on when the respective part of the expansion is executed. If it is at load-time, then you can just go back up to the require form and remove the eval-when-compile; you simply cannot avoid loading bar. If those symbols from bar only come into play when the command is called by the user, then you have load at least some of those symbols, to make sure bar is loaded.

@alphapapa
Copy link
Owner

Thanks a lot for explaining that! I'll be fairly busy for the next few weeks, but I'll dig into this again soon.

andersjohansson added a commit to andersjohansson/helm-org-rifle that referenced this issue Jan 14, 2018
Additionally delete autoloads for the definitions of the macros.

Fixes alphapapa#13
alphapapa pushed a commit that referenced this issue Jan 15, 2018
Also delete autoloads for the macro definitions.  This prevents
helm-org-rifle from causing org to load unnecessarily.

Fixes #13.  Closes #20.  Thanks to Anders Johansson (@andersjohansson)
and Chris Ruegge (@cruegge).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants