emacs-elsa / Elsa

Emacs Lisp Static Analyzer and gradual type system.
GNU General Public License v3.0
640 stars 26 forks source link

Is it safe to expand macros? #205

Closed Fuco1 closed 1 year ago

Fuco1 commented 1 year ago

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...

alphapapa commented 1 year ago

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.

Fuco1 commented 1 year ago

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).

alphapapa commented 1 year ago

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.

Fuco1 commented 1 year ago

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

alphapapa commented 1 year ago

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.

Fuco1 commented 1 year ago

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.

alphapapa commented 1 year ago

Haha, oops! :) Thanks for your work! Looking forward to being able to really benefit from Elsa.