This page is not created by, affiliated with, or supported by Slack Technologies, Inc.
- # adventofcode (7)
- # announcements (11)
- # beginners (71)
- # boot (3)
- # cider (25)
- # clara (2)
- # cljdoc (23)
- # cljs-dev (89)
- # cljsjs (1)
- # cljsrn (6)
- # clojure (97)
- # clojure-austin (2)
- # clojure-brasil (5)
- # clojure-dev (15)
- # clojure-italy (55)
- # clojure-nl (37)
- # clojure-russia (2)
- # clojure-spec (4)
- # clojure-sweden (5)
- # clojure-uk (40)
- # clojurebridge (1)
- # clojurescript (108)
- # core-logic (10)
- # cursive (33)
- # data-science (6)
- # datascript (19)
- # datomic (44)
- # emacs (9)
- # figwheel (13)
- # fulcro (67)
- # jobs (1)
- # juxt (4)
- # lumo (8)
- # mount (1)
- # off-topic (31)
- # onyx (1)
- # pedestal (1)
- # reagent (7)
- # reitit (3)
- # ring-swagger (5)
- # shadow-cljs (40)
- # spacemacs (16)
- # tools-deps (4)
- # vim (12)
- # calva-dev (139)
- # clojure-europe (3)
- # figwheel-main (4)
@dnolen would the priority of https://dev.clojure.org/jira/browse/CLJS-2995 change if it wasn’t about spec’ing
range but about fns in general?
:slightly_smiling_face: In general, features that work in
none should probably keep working under
advanced. Also, we want to keep parity with supported features in clojure spec?
some features just can't work in
:advanced. I'm honestly surprised spec instrument works at all :wink:
Right. One recent example is the inability to get the name of a spec'd function under advanced. But that's for a good reason.
@borkdude not going to change the priority for now -
apply appears in that stack trace
I suspect it's related to the fact that
apply can't be instrumented itself, and there's some kind of loop here
it’s also not happening without optimizations advanced :slightly_smiling_face: but noted…
just to be clear: one of the goals of cljs is to make instrument work under advanced? if not, I won’t spend more time doing more research
I’m alluding to what @thheller said:
> some features just can’t work in
:advanced. I’m honestly surprised spec instrument works at all
just making sure :slightly_smiling_face:
I’d be happy to, but only if I’m sure this is supposed to work, and I think you confirmed this.
given the consistent train of issues - it may turn out that
instrument is simply not practical under advanced and it has to be testing time thing only
@borkdude quick thing to check to narrow that ticket to the actually problem, as I was walking to get coffee - I was thinking this probably about the self-call case
well, I didn’t figure it was about self-call in general yet. I have to walk for a coffee too probably :wink:
because in advanced you have direct invokes and we have some annoying machinery to make this "just work"
it turns out it only happens with a self-calling multi-arity fn. I’ll update the issue
@mfikes in your
maybe-setup-static-dispatch I note that you modify
ret and then
also arity-0 to arity-2 throws. so the problems seems to be with arity-0 to another one. arity-n -> arity-0 is also problematic.
@dnolen Here is what I was thinking when I wrote
I wanted to fundamentally endow
ret with all of the same static dispatch properties that
f had (so that existing callsites still work). (I think our previous approach with using goog to extend things had the same goal in mind.) But, I wanted to do it “by hand” to gain more control over the dispatch logic, and in particular to make the static dispatch properties on
f to high fidelity.
Now having said that, whenever callsites invoke on
ret, I wanted any call to be routed to the variadic arity that exists on
ret, because that’s where the spec-checking occurs. If a static dispatch on
ret were implemented to apply directly on
f, my first reaction is to wonder if spec checking might be skipped.
Thinking about the possibility of infinite recursion: I was thinking that if you invoke arity 2 on
ret, that implementation would do an
ret, and my hope was that this would then land control right in the spec-checking code. But… (I haven’t looked), perhaps
apply itself then looks for the invoke arity 2 on ret and then instead of going to the variadic place where I wanted it, you end up back in the invoke arity 2 on
ret and you are then in an infinite loop.
This could very well be happening (I haven’t looked yet). I also still think that doing an
f might fix it if that’s the case, but it might also lead to skipping the intrumentation check.
I think we should rewind here a bit - after looking at the code - this is what I think
ret is for when we don't have arity info - just in case we get invoked via
.call for some reason - it checks then defers to
2) the individual arity methods on
ret are only for advanced compilation - to provide a matching interface, but currently we call
apply (slow) and then re-enter
3) instead of doing 2) we could do 1) - do not re-enter
ret instead just check and invoke
f (since we have arity info we can skip
I agree. For step 3, it sounds like you are suggesting inlining (or perhaps making a call to shared) checking code in each static dispatch, and then after that, perhaps doing the same static dispatch on
f. Critically, avoiding any call to
apply which could lead to infinite loops (and generally difficult-to-reason-about logic, given that
apply is another function altogether.)
Yeah, it avoids the bits that I claim I am uncertain about above. :slightly_smiling_face:
Right here is the critical piece that would change. https://github.com/clojure/clojurescript/blob/7d3b94de959cafc3e56be5b869c57977119c51f3/src/main/cljs/cljs/spec/test/alpha.cljc#L297
(apply ~ret ~argv) conceptually it would first do the spec check, and then delegeate directly to
might also be a good time to write at least the basic test cases around fn specs - we're definitely lacking that right now
If we did this, this might be the only remaining use of
apply, the 2nd line here. Maybe there is some way to avoid it as well and just use the 1st line unconditionally if we have all of the static dispatch cases covered. (This is not completely clear to me yet, but avoiding that
apply as well would be cool, if we could pull it off.)
Yeah, I suspect we are closer to killing this problematic bit of code :slightly_smiling_face:
The main theme appears to involve: Direct, simple logic, even if it leads to bloat, avoiding any reliance on
apply if possible.
this might also solve the (unimportant) issue of being unable to instrument
@mfikes I’d be happy to implement, but I would need some time to get familiar with this code. Some instructions in the issue would help.
Yeah, it might not hurt, given your involvement with Speculative. You’d be in a better position to see some of the subtleties of whether a new candidate implementation holds water.
I have a branch here: https://github.com/clojure/clojurescript/compare/master...borkdude:CLJS-2995
- [x] The CLJS tests pass.
- [x] The test for CLJS-2995 issue passes
- [x] The self-parity tests pass
Feedback welcome! I’d like to get rid of the
apply calls in
spec-checking-fn, but that’s a nice bonus. Not sure how to do that yet.