Fork me on GitHub

I'm still hitting a case of "Invalid arity" probable related to the issue fixed in Trying to create a repro


Got it:

(def foo (with-meta (fn [& xs] (count xs))
           {:awesome true}))

(prn (satisfies? MetaFn foo)) ;; true

(prn (foo 1 2 3 4 5 6 7 9 10 11 12 13 14 15 16 17 18 19 20 21 22)) ;; 22 is not ISeqable
(prn (foo 1 2 3 4 5 6 7 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23)) ;; Invalid arity 22



(prn ((.-afn foo) 1 2 3 4 5 6 7 9 10 11 12 13 14 15 16 17 18 19 20 21 22)) 
(prn ((.-afn foo) 1 2 3 4 5 6 7 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23))
do work correctly


A similar problem with calling (#'foo ...)


As a workaround I'm going to do:

1294 |                                            f (or (.-afn f) f)])
 Cannot infer target type in expression (. f -afn)
but what should I put there, @thheller?


just ^js is fine. not really relevant that afn gets maybe renamed to a or whatever. gzip will make that a non issue

🙏 1

@dnolen Want me to submit an issue/patch for the above?


@borkdude yeah open an issue - you can attach a patch but that seems possibly a strange way - if so, I think that the other commit might need to be reverted


@dnolen it wasn't caused by the other commit, it was a bug that was already there


but I hoped it would've been fixed by that commit


@alexmiller re tools.reader there is one more breaking change around REPLs since the docstrings specifically says *in* can be an tools.reader instance


hard to understand the impact of that one since nesting readers while intended to be supported may be a more advanced use case


I suppose we can call it out as such, and see how much of a hassle it is for existing tooling


if we vendorize and stop declaring the dep - then there's also the problem of any tooling that assumes tools.reader is coming along for the ride

Alex Miller (Clojure team)14:05:44

well, I think those projects should declare a dep if they use it

Alex Miller (Clojure team)14:05:51

just like declaring your namespace deps for things you use

Alex Miller (Clojure team)14:05:55

you don't get to assume things about your nth-level deps

Alex Miller (Clojure team)14:05:40

but going back to in, presumably the number of tools this impacts is a handful. I'm wondering what that would look like and whether we can search for it specifically

Alex Miller (Clojure team)14:05:22

so let's think about this... in the case where you get this, you will receive an object that extends to the protocol

Alex Miller (Clojure team)14:05:48

it will presumably not extend to your copy of that protocol

☝️ 1
Alex Miller (Clojure team)14:05:09

so if you extend the copy protocol to Object, it could check whether is loadable, and if the reader object extends to that, you could then extend-protocol on the object to your protocol and create the extension dynamically

Alex Miller (Clojure team)14:05:46

for the concrete type, of which there are only a few, and this is a 1-time operation per type - after that it routes via the protocol extension


or you could AOT everything, but leave out the AOT-ed classes for tools.reader and just let that part be compiled again in user space?

Alex Miller (Clojure team)14:05:38

I think that's what David tried yesterday


oh hmm, yes

Alex Miller (Clojure team)15:05:54

I guess it's more complicated than I describe above as there are multiple protocols (Reader, IPushbackReader, IndexingReader) so you would need to install extension bridges on all of them

Alex Miller (Clojure team)15:05:47

maybe this could be done proactively rather than reactively by somewhere looking for a loadable tools.reader and doing the necessary bridging up front

Alex Miller (Clojure team)15:05:00

in the shaded tools.reader


right.. you could extend the shaded protocol to the unshaded protocol then I guess?

Alex Miller (Clojure team)15:05:17

well, extend the shaded protocol to the unshaded concrete types


but then the unshaded protocol isn't really extensible anymore in the world of cljs.reader I guess

Alex Miller (Clojure team)15:05:15

it is, but only if you used the shaded names

Alex Miller (Clojure team)15:05:32

which become part of the cljs api effectively

Alex Miller (Clojure team)15:05:27

this would not cover external concrete types that extend the original protocol, I assume those are rare

Alex Miller (Clojure team)15:05:48

those could also be caught via the dynamic extension if desired, but not sure that's worth it

Alex Miller (Clojure team)15:05:12

that search probably shows most of the projects that may be affected by this change. if they are taking in and inspecting it, that's another way tools could be broken

Alex Miller (Clojure team)15:05:38

if they are making the reader via tools.reader and passing it in, the bridges should address that case

Alex Miller (Clojure team)15:05:34

even having looked at all that, I still think this is worth doing right now. I guess unexpected issues with tools could change the balance of that. but that decision is of course, David's to make...

Alex Miller (Clojure team)15:05:03

I think transit-clj and data.json are trivial to shade and worth doing regardless


if you vendor transit-clj, should you then also vendor transit-java, messagepack, etc?

Alex Miller (Clojure team)15:05:41

they should all be deps, but I think they already are


ah, those aren't AOT-ed, so that's not a problem I guess

Alex Miller (Clojure team)15:05:55

yeah, that's all Java libs


> I still think this is worth doing right now is "this" referring to AOT of cljs, or something else?


I wondered if, considering the above, you would still find AOT worth doing, or if it was about making the bridges, etc.

Alex Miller (Clojure team)15:05:07

refers to shading and AOT of cljs clojure deps

👍 1

thanks for all this, will go over the backlog when I have time later


@alexmiller re: dynamic extension - is this actually possible for tools.reader - this seems pretty ugly since you need to just reimplement everything, but that doesn't seem possible due to mutable locals on the deftypes (they are hidden)


maybe you're saying something else and I am misunderstanding

Alex Miller (Clojure team)20:05:24

I don't think you'd need to reimplement everything, but I don't think you need to do dynamic anyways

Alex Miller (Clojure team)20:05:48

;; only do this if is on the classpath:

(defn dynamic-extend [rdr-class]
  (extend-protocol shaded/Reader rdr-class
    (read-char [reader]
      ( reader))
    (peek-char [reader]
      ( reader)))

(extend-protocol shaded/Reader Object
  (read-char [reader]
    (if (satisfies?
      (dynamic-extend (class reader))
      (throw ...unknown type...)))
  (peek-char [reader]
    (if (satsifies?
      (dynamic-extend (class reader))
      (throw ...))))

Alex Miller (Clojure team)20:05:48

it's a little tedious but it does work


and I guess this file is not AOTed

Alex Miller (Clojure team)20:05:41

probably - I think you'd want to stick it in something conditionally loaded


does any of this affect self-hosted? I guess not right, since none of that should deal with .class files


no effect, other than I think we do need to declare a dep on tools.reader for bootstrapping anyway


I'm only vendorizing, .clj tools.reader files for AOT

👍 1

@ghadi asm vendorize fu appeared to just work on tools.reader - all tests passing


so it's just the bridging left


cool? I don't know what you're talking about 🙂


tools.reader uses asm?


ok merged the vendorization - next step -