Fork me on GitHub
Slack Archive

This page is not created by, affiliated with, or supported by Slack Technologies, Inc.

2022-04-06

Channels

#cljs-dev
<
2022-04-06
>
borkdude09:04:04

I'm having troubles in self-hosted ClojureScript with multi-arity macros. Are there any known issues about this? The only output I'm getting is: 

Execution error (Error) at (<cljs repl>:1).
Invalid arity: 0
without any line numbers. The macros work perfectly in "normal" CLJS

borkdude09:04:58

This issue macros it hard to expand an existing macro with extra options

borkdude09:04:26

Here is a smaller repro: 

(ns sci.repro
  #?(:cljs (:require-macros [sci.repro :refer [ma-macro]])))

(defmacro ma-macro
  ([x] `(ma-macro ~x 1))
  ([x y] `(+ ~x ~y)))

(ma-macro 1)
cljs.user=> (require '[sci.repro])
Execution error (Error) at (<cljs repl>:1).
Invalid arity: -1

borkdude09:04:00

It seems to work correctly when I move the macro to a .clj file, I'll just do that

borkdude09:04:44

The problem seemed mainly a "execute the macro on top level" problem

Bob B14:04:19

how do you feel about if we move the actual impl to a multi-arity function and make the macro body an apply call? 

(defn ma-macro*
  ([x] (ma-macro* x 1))
  ([x y] `(+ ~x ~y)))

(defmacro ma-macro
  ([x & args] (apply ma-macro* x args )))

borkdude14:04:37

Not a fan of that. Also not sure if what would work since there's a reason why things are macros. If it could have been done using a function we probably would have already

borkdude15:04:48

@U013JFLRFS8 But your suggestion does seem to work in general: Using: 

[sym ns & [opts]]
as the arg vector also fixes the problem!

borkdude15:04:05

while keeping the macro single-arity

borkdude15:04:06

so I suggest we go with that solution

Bob B15:04:40

cool - I'm bouncing back and forth between this and making it look like I'm doing my job, so it might be a little while before I update the PR (you might already be done for the night because timezones)

borkdude16:04:18

Due to CircleCI outage, I ported the SCI build to github actions ;)

borkdude16:04:11

I wonder if we should treat :name as a compile time constant, similar to the first argument: 

(sci/copy-var inc nil {:name 'foo})
vs: 
(sci/copy-var inc nil {:name foo})

borkdude16:04:49

Posted it in the PR, let's discuss there next time you have time

thheller11:04:03

just a wild guess but could be that ma-macro is just resolved to the local sci.repro/ma-macro instead of the sci.repro$macros/ma-macro. In shadow-cljs I made regular defmacro not emit anything when not in "macros" compilation mode. That at least prevented confusing error messages such as this one.

thheller11:04:39

sci.repro/ma-macro will exist in the analyzer data as a "regular" defn

thheller11:04:39

by moving it to a separate file you basically remove sci.repro/ma-macro and only sci.repro$macros/ma-macro will exist

thheller11:04:09

could also just be the problem with the top level call. since both the macro variant and the normal variant will call this. it one of those versions it won't exist

borkdude12:04:23

@thheller I hoped to avoid that by using macros/deftime (borrowed from @cgrand’s macrovich) https://github.com/babashka/sci/commit/e73377fc37972d0661509a68678961926ba7acf5#diff-faebb48a23c0728ab5401da8d8a73473dfb546b9c04cf37f0913fc731f7a725eL50 but it seems it doesn't work for multi-arity macros somehow

borkdude12:04:05

But this both prints true even when I wrap that macro: 

#?(:cljs (prn (exists? repro.ma-macro)))
#?(:cljs (prn (exists? repro$macros.ma-macro)))

borkdude12:04:36

This is also surprising: 

(ns repro
  #?(:cljs (:require-macros [repro :refer [ma-macro]])))

(defmacro ma-macro
  #_([x] `(ma-macro ~x 1))
  ([x y] `(+ ~x ~y)))

(prn (ma-macro 1 2)) ;;=> (cljs.core/+ nil nil)

thheller12:04:39

if this is called as a function as I suspect thats not surprising at all

borkdude12:04:51

Unwrapping the list around the arg vec prints 2 times: 

(cljs.core/+ nil nil)
3

thheller12:04:16

helps to look at the generated JS

borkdude13:04:02

what's the connection?

mkvlr13:04:46

should have read more carefully, first. Last time we tried self-hosted an ran into issues with with getting the reagent/with-let macro to work with selfhost we ended up filing • https://clojure.atlassian.net/browse/CLJS-3286 • https://clojure.atlassian.net/browse/CLJS-3287 • https://clojure.atlassian.net/browse/CLJS-3288 of which only CLJS-3287 is fixed on main if I read things correctly. So I thought there might be a connection but your problem does look different.

borkdude13:04:26

I've ran into this problem before and then I decided to just split the macro into a separate macro for each arity...

borkdude13:04:04

In a recent release I forgot to check compatibility with self-hosted but someone filed an issue about this, since they used self-hosted to test their lib + SCI

mkvlr13:04:08

SCI + self-hosted?

borkdude13:04:53

it works again in 0.3.4

borkdude13:04:08

but now we're trying to add an option map to an existing macro and running into this stuff again

borkdude13:04:33

moving the macro to a .clj file "fixed" it for now

dnolen15:04:46

@mkvlr it wasn't on my radar there were updated patches for those, trying them out now

🙏 1
dnolen15:04:29

@mkvlr still something wrong w/ the patch for 3286

mkvlr16:04:35

oh, already out for the day, I'll take a look probably tomorrow, thank you!

dnolen16:04:32

@quoll finally looked over the math stuff, generally looks ok - I think the only thing is that we have communicate this will only work in JS environments w/ typed array support - for most folks not a big deal

quoll16:04:23

We discussed this a few months ago. I just don't have any way to get from bits back into a double without using a typed array. Luckily, they're reasonably ubiquitous now. A note on the release makes sense

Adam Kalisz09:04:50

I am working on improving random-uuid and need typed arrays for that too. Should this be perhaps a runtime check and produce some kind of warning?

quoll15:04:16

I'm in favor of this, though this is exactly the sort of thing that is up to David. It need only Show up when requiring namespaces that need this, right? So, ideally, anyone trying to use ClojureScript on a system without typed arrays would not see a warning until they tried to load the ns

dnolen16:04:11

one thing I noticed is that there is still references to Double copied from the Clojure docstrings - this is not a big deal either - could merge and let people kick it around and could take a patch to fix that stuff up

quoll16:04:50

Oops. I could PR against that branch? Or would you rather leave it for now?

dnolen16:04:24

sure go for it, happy to get this pushed through since I've stalled on it for so long 🙂

Adam Kalisz09:04:21

Perhaps the uuid regex could be improved: 

(def hex-regex
  "Regex matching to a UUID in hexadecimal form, e.g., 0713d2bf-8e70-41e2-8b81-c8ca3ef4a8cd."
  #"^[0-9A-Fa-f]{8}-[0-9A-Fa-f]{4}-[4][0-9A-Fa-f]{3}-[89ABab][0-9A-Fa-f]{3}-[0-9A-Fa-f]{12}$")
Matching other letters than A-F and a-f doesn't make any sense in my opinion. Not using the multiplicative brackets makes the whole thing really verbose. Do I miss something? We use it at OrgPad just fine.

quoll14:04:09

I have a version where I used the multiplicative braces, so I don’t know why I checked that in. I need to check what I did when I can

quoll18:04:38

I believe so, yes

quoll18:04:50

it was just waiting on a review

dnolen18:04:54

that one seems straightforward

dnolen18:04:17

I think after these can cut a proper 1.11 release, feel free to propose something else that needs review

dnolen20:04:41

@quoll all your patches applied I think?

🎉 3
quoll20:04:52

Drat, I was distracted. Did I do the PR against "master" instead of against "math"?