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

consider using sci for macro-expansion #811

Closed
11 tasks done
borkdude opened this issue Mar 18, 2020 · 19 comments
Closed
11 tasks done

consider using sci for macro-expansion #811

borkdude opened this issue Mar 18, 2020 · 19 comments
Assignees
Labels

Comments

@borkdude
Copy link
Member

borkdude commented Mar 18, 2020

Note (23rd of June 2020):

A blog post about this feature can be found here.

Documentation for this feature can be found here.

The original text of this issue

In the macros-sci branch, we added the following config option:

{:macroexpand
                       {foo/weird-macro
                        "
(fn weird-macro [{:keys [:sexpr]}]
  (let [[[sym val opts] & body] (rest sexpr)]
    (when-not (and sym val)
      (throw (ex-info \"No sym and val provided\" {})))
    {:sexpr `(let [~sym ~val] ~@(cons opts body))}))
"}}

I.e. the :macroexpand option takes a map from fully qualified symbols to a string which represents Clojure code. The Clojure code should return a function which:

  • takes a map with :sexpr, the sexpr representing the macro invocation
  • return a map with :sexpr, the macroexpansion.
  • if the code contains a single line string that doesn't start with a paren, it's interpreted as a file relative to the .clj-kondo directory

The reason we accept and return maps is for extensibility. The reason we expose the entire sexpr and not just the macro args to the function is that the function can copy location metadata from the original macro invocation to be used in lint warnings.

An example usage can be found in corpus/macroexpand.clj.

TODO:

  • cache evaluation of macroexpand functions in config
  • preserve arity linting for macro call
  • code loading using require. this allows for multiple macros from the same library to make a small lib with helpers functions that can be re-used over multiple macros. the sci ctx will be shared over all calls. We can add .clj-kondo/src to the classpath of sci.
  • maybe this feature should not be called macroexpand but hooks, since the user could do all kinds of things with the sexpr. instead of transforming it, it could also just do some linting on the arguments and return some warnings?
  • make distinction between multiple hooks? {:hooks {:call {foo/bar ...}?
  • fix locations of numbers and keywords in transformed sexprs (since they can't carry metadata, these locations are derived from the location in the sexpr, which is off)
    solution: we're going to use rewrite-clj directly. see next.
  • I'm considering switching entirely to rewrite-clj node based transformations instead of converting them to sexprs and parse the transformed sexpr as rewrite-clj nodes again as it's quite difficult to preserve location metadata, for the "macroexpand" feature (edited)
    11:43 AM
    Still, the sexpr can be used for some validations, when people don't "macroexpand"
  • translate rumc and better-cond example to rewrite-clj
  • I might still rename the naming of {:hooks {rum/defc ...}} since this gets invoked during analysis of a call only. so maybe {:hooks {:analyze-call {rum/defc ...}}} might be better for future extension and then we could potentially add more and more hooks
  • test for expectations:
  • make it easy to returning findings either as :findings or with reg-finding!
@borkdude borkdude added the enhancement New feature or request label Mar 18, 2020
@borkdude
Copy link
Member Author

Just adding this for reference:

clojure-lsp uses a DSL to describe custom macros:

https://github.com/snoe/clojure-lsp/#macro-defs

@wilkerlucio
Copy link

@borkdude looks awesome! one question though, could the return of each entry be Clojure forms instead of a string?

@borkdude
Copy link
Member Author

@wilkerlucio Which entry/string, where? Can you clarify?

@wilkerlucio
Copy link

wilkerlucio commented May 31, 2020

I mean, I'm wondering if there could be a way to be something like:

{:macroexpand
 {foo/weird-macro
  (fn weird-macro [{:keys [:sexpr]}]
    (let [[[sym val opts] & body] (rest sexpr)]
      (when-not (and sym val)
        (throw (ex-info "No sym and val provided " {})))
      {:sexpr `(let [~sym ~val] ~@(cons opts body))}))}}

But given the macro/runtime constraints, I'm not sure if its possible, but wonder if you did consider it.

@borkdude
Copy link
Member Author

borkdude commented May 31, 2020

@wilkerlucio The reason the code is in a string is that not all code is valid EDN. This is part of the .clj-kondo/config.edn file.

@wilkerlucio
Copy link

Gotcha, makes sense, thanks for the clarification.

@borkdude
Copy link
Member Author

borkdude commented Jun 1, 2020

@wilkerlucio Your comment did get me thinking that large code strings in an EDN file aren't very ergonomic. Maybe I could allow load-file in the sci expression, so it becomes like:

{:macroexpand {foo/weird-macro "(load-file \".clj-kondo/macros/weird-macro.clj\")"}}

Thoughts?

@borkdude
Copy link
Member Author

borkdude commented Jun 1, 2020

@wilkerlucio
Copy link

Yeah, writing code on strings gets me bad memories from the past, hahaha. So yeah, I like the idea of having this being something externally loaded.

@borkdude
Copy link
Member Author

borkdude commented Jun 1, 2020

@borkdude
Copy link
Member Author

borkdude commented Jun 1, 2020

Hmm, one problem is that relative paths don't work, since the editor sometimes set the current working directory to the current file.

@borkdude
Copy link
Member Author

borkdude commented Jun 1, 2020

@wilkerlucio Solved the problem as follows: if the code is a single line and doesn't start with a paren, it's interpreted as a file path relative to the .clj-kondo directory.

@SevereOverfl0w
Copy link
Contributor

If both :macroexpand and :lint-as are defined, what is the priority?

@borkdude
Copy link
Member Author

@SevereOverfl0w Yet to be determined. What do you think?

@SevereOverfl0w
Copy link
Contributor

:macroexpand is explicit opting into the newer behavior, and is more likely to be accurate. Seems a reasonably priority to me.

@borkdude
Copy link
Member Author

borkdude commented Jun 17, 2020

Generalizing this idea to :hooks. One can transform/macroexpand the sexpr, but also just do some checking and throw errors.

Screenshot 2020-06-18 00 32 27

@borkdude
Copy link
Member Author

borkdude commented Jun 17, 2020

11:48 PM
dominicm @borkdude it would be nice to attach location info somehow

11:48 PM
borkdude one of the problems in the above example is that keywords and numbers don't carry metadata
11:49 PM
also I think rather than throwing people can return :findings with :level, :row, :col etc. (edited)
11:50 PM
The row and col come from the metadata of the sexpr elements, but in case of numbers of keywords these aren't present
11:51 PM
Of course the function could get an additional :node thing which is the raw rewrite-clj node
11:51 PM
and then you can lookup the metadata yourself
11:52 PM
using (-> node :children second meta) for example
11:56 PM
dominicm A basic thing to do is point at the argument
11:56 PM
The rewrite node is most powerful
11:57 PM
borkdude
A basic thing to do is point at the argument
what do you mean?
11:58 PM
dominicm A common use case that it would be nice to make easy.
11:59 PM
borkdude right, or a path into the sexpr, like [1 0]

@borkdude
Copy link
Member Author

@SevereOverfl0w Change::lint-as is used to determine if some macro is syntactically similar to another one. And then /that/ gets used to call the expand hook, if present.

Example: rum.core/defcs is syntactically similar to rum.core/defc. So when a hook for defc is present and defcs is linted as defc, then defcs will be linted as defc, which will then be expanded by the hook.

Example: https://github.com/borkdude/clj-kondo/blob/e6524125d09a12e5fcd56a1c3be25c0e213f06ac/corpus/macroexpand.clj#L74-L120

cc @martinklepsch

@borkdude
Copy link
Member Author

borkdude added a commit that referenced this issue Jun 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants