Incorrect :arglists, embellishment in the README, and nonstandard `clojure.core/sigs` breaking on Babashka

[edenworky]

So, the README claims that this:

(defun say-hi
  ([:dennis] "Hi,good morning, dennis.")
  ([:catty] "Hi, catty, what time is it?")
  ([:green] "Hi,green, what a good day!")
  ([other] (str "Say hi to " other)))

Should expand to this:

(defn
 say-hi
 {:arglists '([& args])}
 [& args#]
 (clojure.core.match/match
  [(vec args#)]
  [[:dennis]]
  (do "Hi,good morning, dennis.")
  [[:catty]]
  (do "Hi, catty, what time is it?")
  [[:green]]
  (do "Hi,green, what a good day!")
  [[other]]
  (do (str "Say hi to " other))))

However, as the defun macro definition confirms, this isn't actually how the code works, and the code in fact expand to:

(def say-hi
 (fun
   ([:dennis] "Hi,good morning, dennis.")
   ([:catty] "Hi, catty, what time is it?")
   ([:green] "Hi,green, what a good day!")
   ([other] (str "Say hi to " other))))

Or more fully:

(def say-hi
 (fn fn__13414
  ([G__13416]
    (let [placeholder13415 placeholder]
      (match
        [G__13416]
        [:dennis]
        (do "Hi,good morning, dennis.")
        [:catty]
        (do "Hi, catty, what time is it?")
        [:green]
        (do "Hi,green, what a good day!")
        [other]
        (do (str "Say hi to " other)))))))

Notably, defun only wraps an (anonymous) fun, and manually runs vary-meta and sets :arglists by using a private undocumented clojure.core/sigs function. However, this sigs function, for the provided say-hi, gives an :arglists of ([:dennis] [:catty] [:green] [other]), which seems to work well enough, but isn't the [& args] described in the README, but that's good, since [& args] is too generic and lends itself poorly to static analysis and automatic currying and so forth. The appropriate :arglists in this case, I believe (as this is nonstandard and undocumented), should be simply ([other]).

So first of all and certainly, the README should be updated to reflect the state of the code, because no one should use defun and expect an [& args] :arglists value. But speaking more generally, it seems to me like the sigs we get could easily be post-processed to be more compliant quite simply, by removing all non-symbol values and unifying signatures per-arity looking for a non-'_ value if one is available. I'm not sure if this is the best approach to go with (and it does add (yet more) processing at macro-time), but I submitted a PR #25 with this post-processing included, as well as updates to the tests (which don't currently check :arglists at all), and the README, and will keep an updated version on wizard-enterprises/defun where I also updated the project dependencies and added a deps.edn file.

[killme2008]

Good catch!
Thanks! You're right, the README is outdated due to several refactors.

by removing all non-symbol values and unifying signatures per-arity looking for a non-'_ value if one is available

I don’t think that’s a better approach. The original ([:dennis] [:catty] [:green] [other]) looks clearer and matches clojure.core/sigs, as expected.

Updating the README and tests should be sufficient. Thanks.

[edenworky]

Hey, sorry for taking a moment to respond.

I'm a bit unclear on the actionables for this and #26 (tests and README aside) so just to consolidate.

The changes I made on my fork can be summed up as:

1. Interning clojure.core/sigs (and some helpers it uses) to support Babashka as detailed in Doesn't work on Babashka #26 starting from line 155 and ending in line 208 after defining a private defun.core/core--sigs which can be used by changing (@#'clojure.core/sigs body) to (core--sigs body)

2. A small wrapper on top of it which unifies and dedupes per-arity as proposed above

So to sum up my understanding of your response here and on #26, I could just take out the wrapper, keep the interned sigs (which (should) work identically to clojure.core/sigs regardless), and update the tests accordingly, and this would fit what you describe?

I can certainly do that, just send confirmation.

However I'd like to respectfully disagree with you first <3

:arglists, it seems, isn't really documented or standardised anywhere that I could find except for the (tersely written) clojure.core/sigs function, which (if you follow the source), essentially normalises over e.g. macro args and & rest-args and ^any.kinda.Tag on-an-arg (and, notably, & {:keys [destructed key params] :or {...}}) etc., and past this just kinda pulls out the args vector and returns it (normalisation aside) as is.

All of this results in misery grief and complication, as exemplified well by the linked examples from clj-kondo, and there are many nonstandard examples in the wild, but in general and to maintain compatibility with as much of the ecosystem as possible, :arglists (as expressed in metadata which is different (apparently) to the default non-metadata :arglists) should in my analysis be restricted to EDN-friendly symbol lists, one per arity. See this awkward example from before, this helper function which counts arities, this stubbing function I'm pretty sure would break, and I'm sure many others existing in the wild.

The easiest way to ensure compatibility is just to have generated arglists be a flat [& args], but this is both uninformative and not necessarily correct (e.g. functions that don't take a limited number of args). So if we want to take defun arglists and make :arglists-compatible ones out of them, then the fact of the matter is, no one expects an :arglists to have non-symbols as args, no one expects multiple argvecs of the same arity, certainly no one expects an arg to be a list with a :guard clause (which might break in environment where :arglists gets evaluated!), and unless you know something I don't, the :arglists produced by defun as of now would be (even more than usually) unexpected and irregular to any :arglists value available anywhere else.

So to my analysis, the :arglists as they stand (again, unless you know something I don't) would only be helpful if you're doing reflection on a value you already know to be a defun, and otherwise lend themselves poorly to analysis and cohesion with other clojure functions, even though the whole point of meta-specified :arglists is to clarify and specify over the defaults given by clojure.core/sigs to begin with, which is fundamentally different to core.match vector semantics and the whole thing defun comes to reconcile.

So, to that particular end, maybe we could preserve the way it currently is as :defun/arglists or :caselists or :matchlists or some such, in case anyone wants to do analysis/reflection over that, but I think it's a very misleading and confusing :arglists value to have for the hypermajority of cases. (Even with my proposed fix, this still only handles defun and not fun itself which should probably also be covered honestly, but that's an edge case on an edge case.)

All of this being said, I defer to your better judgement on the subject, and will be glad to put together a PR either (a) just with the interned sigs + tests + README, (b) the above as well as my proposed "cleaned" :arglists, or (c) the above but with the current "raw" :arglists value available under :defun/arglists or some such, just let me know :)

I'll close #26 to consolidate our discussion here since we're consolidating the PR.

[killme2008]

@edenworky
Sorry for the delayed response. Thank you for the detailed explanation.

So to sum up my understanding of your response here and on #26, I could just take out the wrapper, keep the interned sigs (which (should) work identically to clojure.core/sigs regardless), and update the tests accordingly, and this would fit what you describe?

Yes. That's what I mean.

(a) just with the interned sigs + tests + README, (b) the above as well as my proposed "cleaned" :arglists, or (c) the above but with the current "raw" :arglists value available under :defun/arglists or some such, just let me know :)

I think (c) is more appropriate. I trust your judgment in this case. I’m not paying much attention to Clojure at the moment, so I prefer to rely on the expert’s opinion.

Thanks a lot.