Is it safe to expand macros?
Fuco1 opened this issue · 7 comments
They can have side-effects, so in theory they can cause harm when called during analysis.
On the other hand, the Emacs byte compiler and many checks, such as flycheck elisp checker, run byte compilation which expands macros.
We can try to sandbox the expansion to the current package by having a opt-in flag in the Elsafile. But this would then make Elsa expand also macros in dependencies, which again can be dangerous.
Maybe we can expand Emacs built-in macros since those should be safe...
I'm not very knowledgeable about this subject, but I don't understand how it could be possible to provide accurate analysis (or maybe even useful output) without expanding macros. For example, I'm getting lots of "free variable" errors for variables that are bound by macros. There are so many false positives that I can't find true ones in the output. e.g. using Elsa on Ement.el produces:
Analysis finished with 3369 errors, 2519 warnings and 24 notices after 19.827 seconds
And it looks like 95% of them are "free variable" and the like, all of them false positives.
I have no great solution for this, maybe we can special case some macros to be considered safe... but I really would like to avoid loading anything into the runtime if possible.
Having macros expand would also mean loading defuns because macros often defer to functions. This would lead to some crazy spiral of loading a lot of code which might not be safe. Also analyzing any files with side effect would suddenly be impossible. Imagine you have a script which deletes some files and every time elsa runs it would load it and delete your sutff. This is just a complete no-go.
I'm sure it could be done by cherry picking the declarations though and elsa already loads "base emacs" plus its dependencies, so for example expanding cl or dash macros should be safe (elsa itself depends on cl and dash so it loads the definitions anyway).
I'd be willing to run some of that risk, I guess, because if any macro in my code (or any library that I use) did that, I'd throw a fit and fix it or stop using it. (IOW it sounds like a "Doctor, it hurts when I..." problem. :) And I generally don't like macros that delegate to functions, but sometimes it may be necessary.
Anyway, if I as a developer could set an option to enable macro expansion on my own projects, and if it solved the noted problems, I'd be glad. Otherwise, I'm getting so many false positives, that Elsa's output just doesn't seem usable.
Anyway, if I as a developer could set an option to enable macro expansion on my own projects
Yes, I had something like this in mind as well. I just really don't want to destroy people's computers because someone publishes either maliciously or by accident some bad code somewhere... although I get it that right now people just pull in literally whatever garbage comes out of melpa, so :D
I feel your pain, as well. I'm sometimes amazed that something bad hasn't happened yet, AFAIK.
I'd be fine with some kind of requirement to, e.g. write a file named .elsa-expand-macros in the project directory (or maybe outside of it, so it couldn't be in the git repo), to enable the feature.
Well, I've basically implemented the macro expansion... but the reporting is now garbage because the positions obviously don't align with the original :D That's going to be a tad bit tricky to work out.
Haha, oops! :) Thanks for your work! Looking forward to being able to really benefit from Elsa.