I'm still hitting a case of "Invalid arity" probable related to the issue fixed in https://github.com/clojure/clojurescript/commit/8b3ce24a6e7e1863802b09618985a0f3a604a0c9 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

whereas:

(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))

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)

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

@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 https://github.com/nrepl/piggieback/blob/master/project.clj

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

just like declaring your namespace deps for things you use

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

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

like https://github.com/Engelberg/instaparse/blob/master/src/instaparse/cfg.cljc#L200-L211 ?

https://grep.app/search?q=cljs.tools.reader&filter[lang][0]=Clojure&filter[path][0]=src/

so let's think about this... in the case where you get this, you will receive an object that extends to the protocol cljs.tools.reader.reader-types.Reader

it will presumably not extend to your copy of that protocol

so if you extend the copy protocol to Object, it could check whether cljs.tools.reader 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

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?

I think that's what David tried yesterday

where you encounter the https://clojure.atlassian.net/browse/CLJ-1544 problem

oh hmm, yes

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

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

in the shaded tools.reader

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

yep

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

it is, but only if you used the shaded names

which become part of the cljs api effectively

right

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

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

I looked through all of these https://grep.app/search?q=cljs.tools.reader.reader-types&filter[lang][0]=Clojure&filter[path][0]=src/ and did not find anyone doing custom extensions

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

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

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...

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?

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

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.

refers to shading and AOT of cljs clojure deps

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

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

;; only do this if clojure.tools.reader.reader-types is on the classpath:

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

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

it's a little tedious but it does work

and I guess this file is not AOTed

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

@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?

@ghadi I just copied your sed I know nothing about sed 🙂 https://github.com/clojure/clojure/commit/148a0c9aa83b12efaa43c72e5ee39baff0948f9a

ok merged the vendorization - next step - https://clojure.atlassian.net/browse/CLJS-3375