This page is not created by, affiliated with, or supported by Slack Technologies, Inc.
2018-11-29
Channels
- # adventofcode (7)
- # announcements (11)
- # beginners (76)
- # boot (3)
- # calva (139)
- # cider (24)
- # clara (2)
- # cljdoc (11)
- # cljs-dev (90)
- # cljsjs (1)
- # cljsrn (3)
- # clojure (98)
- # clojure-austin (2)
- # clojure-brasil (2)
- # clojure-dev (16)
- # clojure-europe (3)
- # clojure-italy (55)
- # clojure-nl (37)
- # clojure-sweden (11)
- # clojure-uk (40)
- # clojurebridge (1)
- # clojurescript (107)
- # core-logic (10)
- # cursive (34)
- # data-science (9)
- # datascript (19)
- # datomic (48)
- # emacs (6)
- # figwheel (13)
- # figwheel-main (3)
- # fulcro (67)
- # jobs (1)
- # juxt (4)
- # lumo (8)
- # mount (1)
- # off-topic (29)
- # onyx (1)
- # reagent (7)
- # reitit (3)
- # ring-swagger (5)
- # shadow-cljs (39)
- # spacemacs (5)
- # tools-deps (1)
@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?
🙂 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 😉
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
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 🙂
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 😉
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 apply
ret
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 maybe-setup-static-dispatch
:
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 ret
match 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 apply
on 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 apply
on 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
1) 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 f
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 ret
(loop)
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 apply
)
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.)
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
Instead of (apply ~ret ~argv)
conceptually it would first do the spec check, and then delegeate directly to f
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.)
https://github.com/clojure/clojurescript/blob/7d3b94de959cafc3e56be5b869c57977119c51f3/src/main/cljs/cljs/spec/test/alpha.cljs#L113-L114
Oh, there is another one there: https://github.com/clojure/clojurescript/blob/7d3b94de959cafc3e56be5b869c57977119c51f3/src/main/cljs/cljs/spec/test/alpha.cljs#L127
The main theme appears to involve: Direct, simple logic, even if it leads to bloat, avoiding any reliance on apply
if possible.
@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.