Setting :arglists metadata when vararg is present

Question

consider function with no parameters


(defn aarg {:arglists '([fake])} [])
=> {:ns cljs.user,
 :name aarg,
 :file nil,
 :end-column 11,
 :column 1,
 :line 1,
 :end-line 1,
 :arglists ([fake]),
 :doc nil,
 :test nil}


All works as expected, but with the introduction of a vararg


(defn aarg {:arglists '([fake])} [& env])
(meta #'aarg)
=> {:ns cljs.user,
 :name aarg,
 :file nil,
 :end-column 11,
 :top-fn {:variadic true,
          :max-fixed-arity 0,
          :method-params [(env)],
          :arglists ([& env]),
          :arglists-meta (nil)},
 :column 1,
 :line 1,
 :end-line 1,
 :arglists ([& env]),
 :doc nil,
 :test nil}


:arglists does not get affected.

Answer 1

Comment made by: hlolli

I just submitted a patch, never used jira patch system before, that aside, the patch will make the compiler respect :arglists metadata when the user provdes one. Hope to get feedback and merge on this :)

Answer 2

Comment made by: dnolen

Thanks have you submitted your Clojure CA?

Answer 3

Comment made by: dnolen

Patch review, I don't really understand the purpose the changing arity of variadic-fn and adding this new flag. Just put the logic directly into variadic-fn no?

Answer 4

_Comment made by: hlolli_

Yes, I'll find a better solution to my patch, just that the `m` symbol in the let binding on line 3176

m (conj {:arglists (core/list 'quote (sigs fdecl))} m)

Merges the arglists and there's after this point no way to know if the :arglists came from a provided metadata or not. But I'll rename the let symbols and it's easy to avoid adding arity, I'll do another patch later today.

No haven't submitted my Clojure Contibutor Agreement, I'm filling it out now...

Answer 5

_Comment made by: hlolli_

Yesterday I signed the Clojure CA
"Clojure CA between Rich Hickey and Hlöðver Sigurðsson is Signed and Filed!"

The code:
The symbol name m (for metadata) is already not very good, but I just added a comma to differentiate between vararg-fn case and the other cases.

I found as I was testing this that multi-arity-fn arglist metadata does not work with or without specifically adding arglists. If so, I can open another jira ticket for that.

Answer 6

Reference: https://clojure.atlassian.net/browse/CLJS-2351 (reported by hlolli)

Answer 7

This also applies to multi-arity functions as well:

(defn bar {:arglists '([foo bar])}
  ([x] 1)
  ([x y] 2))

(meta #'bar)
;;=> {:ns cljs.user,
 :name bar,
 :file nil,
 :end-column 10,
 :top-fn
 {:variadic? false,
  :fixed-arity 2,
  :max-fixed-arity 2,
  :method-params ([x] [x y]),
  :arglists ([x] [x y]),
  :arglists-meta (nil nil)},
 :column 1,
 :line 1, 
 :end-line 1, 
 :arglists ([x] [x y]), 
 :doc nil, 
 :test nil}

Whereas single-arity funtions work fine:

(defn foo {:arglists '([foo])} [x] 1)
(meta #'foo)
;; => {:ns cljs.user,
 :name foo,
 :file nil,
 :end-column 10,
 :column 1,
 :line 1, 
 :end-line 1, 
 :arglists ([foo]), 
 :doc nil, 
 :test nil}

I think this could be fixed by checking if there already exists an :arglists key on the meta in the multi-arity-fn definition and the variadic-arity-fn.

I would love to see this fixed, let me know if you'd like a patch for the multi-arity-fn fix as well.

Answer 8

This ticket is lacking an explanation why this change is necessary/useful?

Messing with :arglists will directly effect the code generated by the compiler. It will break variadic invoke or even "drop" calls to certain arities if the metadata does not match the actual function definition exactly.

Comments

I notice that the defn of `eduction` overrides its :arglists metadata, in both Clojure and ClojureScript.

---

In Clojure, at least, it's fairly common to provide :arglists metadata to provide helpful hints for editors and in the REPL -- and the :arglists metadata does not affect the semantics at all: it's just to provide human readable hints.

The following works in Clojure:

dev=> (defn foo {:arglists '([foo bar])} ([x]) ([x y]))

#'dev/foo

dev=> (meta #'foo)

{:arglists ([foo bar]), :line 1, :column 1, :file "/home/seanc/.clojure/dev.clj", :name foo, :ns #object[clojure.lang.Namespace 0x34e700f4 "dev"]}

---

As I said there are a few places where the `:arglists` metadata may affect the code that is generated (eg. [CLJS-1871](https://clojure.atlassian.net/browse/CLJS-1871)).

It should be fine to use this for documentation purposes but this was not clear from the ticket description.

---

For my case it is purely for documentation purposes, and while I'm not familiar with the CLJS codebase the proposed patch makes me a bit wary. Rather than overriding the internal `arglists` variable I would just override it when assembling the metadata, where it would have no downstream effects.

---

For my case it is purely for documentation purposes. And it's rather important for that, otherwise the editor hinting shows gensyms instead of proper names.

 I'm not familiar with the CLJS codebase but the proposed patch makes me a bit wary. Rather than overriding the internal `arglists` variable I wonder if it would help to only override it when assembling the metadata, where it would have no downstream effects.

```
meta     (assoc meta
                               :top-fn
                               {:variadic? variadic?
                                :fixed-arity mfa
                                :max-fixed-arity mfa
                                :method-params (core/cond-> sigs macro? elide-implicit-macro-args)
                                ;; FIX: something like this:
                                :arglists (or (:arglists meta)
                                                       (core/cond-> arglists macro? elide-implicit-macro-args))
                                :arglists-meta (if (:arglists meta)
                                                 (doall (map meta (:arglists meta)))
                                                 (doall (map meta arglists)))})
```