Clojurians Log v2

There is now a candidate patch in https://dev.clojure.org/jira/browse/CLJS-2793 that might help with proper behavior when specing variadic ClojureScript fns.

Nice! I’m impressed! It’d be super interesting to me at least to know something about the workflow you use to fix bugs as these.

I looked at the code in question since the stacktrace pointed me to it, but when I saw the code, I backed very slowly away. Too scared:)

I noticed that running the cljs tests with the cljs-test runner is much much faster than with our script: https://github.com/borkdude/speculative/blob/cljs-test-runner/deps.edn#L17 With the runner:

$ time clj -A:test:cljstests2

Testing speculative.core-test

Ran 19 tests containing 65 assertions.
0 failures, 0 errors.
clj -A:test:cljstests2  16.97s user 0.90s system 314% cpu 5.680 total

$ time clj -A:test:cljstests
WARNING: Wrong number of args (4) passed to cljs.core/range at line 191 /Users/Borkdude/Dropbox/dev/clojure/speculative/test/speculative/core_test.cljc

Testing speculative.core-test

Ran 19 tests containing 65 assertions.
0 failures, 0 errors.
clj -A:test:cljstests  100.56s user 3.30s system 348% cpu 29.830 total

Not sure why this is.

Maybe because it used Nashorn.

@bbrinck’s inteview on “The Repl” podcast is quite interesting.

yeah, I listened to it last Wednesday

Got something working for :ret and :fn checking. Not quite there yet, but it does something 🙂 https://github.com/borkdude/speculative/blob/a05836d28b056267aa487ceef5253dba7802c793/test/speculative/core_test.cljc#L11

Yeah, Node is much faster than Nashorn. The problem with Node is it is challenging to get the error code out of it when using`cljs.main`

yeah, the test runner does it properly somehow

probably something in doo

I’m excited to see so much activity on speculative - nice work!

I’ve been thinking about if/how expound could be extended to take advantages of this work

One issue that Alex noted is that many core functions intentionally work on abstract concepts like ifn? and seq? which may yield somewhat confusing error messages.

e.g. getting an error from map that says the second argument:

should satisfy

  ifn?

So the error could read something like “should be a function (or a value that can be called like a function, such as a keyword, set, or map)”

These could be defined in an optional ns within expound, or maybe a new project. I suppose they could be defined in speculative, but that had the downside of requiring a dependency on expound.

Nice! Probably the responsibility of rendering spec errors to text belongs to expound?

I agree it shouldn’t go in speculative, but not sure it should go in expound either - these messages are really a user-defined configuration for expound

Perhaps it should just be a separate library for now, but I may move it into expound later (and put it in a ns that the user can optionally load)

Yeah, a separate library might be best, since this library would depend on both speculative and expound (whereas I wouldn’t want expound to depend on speculative, and speculative shouldn’t depend on expound either)

you could always make a speculative-expound library 😉

or expound-core where you hide the name of speculative as an implementation detail 😉

@slipset I noticed you removed the “inlined” specs, but they still have value imo.

Yeah, I they serve a documentation purpose.

I was wondering about including a really.speculative ns for these kinds of specs 🙂

They can be checked in tests. You only have to be a bit careful. https://github.com/slipset/speculative/blob/e0e2f192c7da8c78f89a4d14c38ea8de11ac4ccc/test/speculative/core_test.cljc#L16

And it doesn’t hurt to instrument them, even if they are inlined.

ok, I’ll revert

reverted.

About this issue: I fixed this in the “style guide” branch: https://github.com/slipset/speculative/issues/43#event-1929317840

https://github.com/slipset/speculative/pull/50/files#diff-c33c03cf47d9105d05ca5300ca56286fR9

(under the umbrella of using meaningful names for alternative arities)

this language is crazy!

yeah 😉

user> (get nil nil nil)
;; => nil
user> (get #{'foo} 'foo)
;; => foo
user> (associative? #{'foo})
;; => false
user>

I guess it isn’t an associative because you cannot (assoc #{} 0)

exactly, but given that sets are basically just maps of keys onto themselves, one could argue that

(assoc #{} 'foo 'foo) should work

agree (although I remember some discussion about this, of which I cannot remember the exact points)

I think it had to do with the semantics of some stuff where you get could in trouble if this was the case

(sorry for the vagueness)

I already changed stuff to ifn? and seqable? in the style guide branch as well, as I made it part of the proposal

(referring to https://github.com/slipset/speculative/issues/46)

Nice!

I think a couple of fns that would be interesting to have specs on is the stuff in clojure.set which fails in strange ways if you pass eg vectors instead of sets.

agreed

What kind’a language is this 🙂

user> (get '(1) 0)
;; => nil
user> 

user=> (get 1 1 )
nil
user=> (get "string" 1 )
\t
user=>

basically the spec for get is useless.

@slipset would you mind having a look at the style-guide branch that also has some code I wanna merge.

Will do

AFAIK nothing controversial in it anymore. 🙂

I’ll try to rebase it on master now

Rebase done

@slipset about get: I think there’s a lot of frog area in there (undefined, not important to spec, like (get 1 1))

put another way: I think it’s fine to write a spec that doesn’t allow 1 as a collection to get from

If you look at the source for get in ClojureScript, you’ll see that there is a cond checking for different types, with an :else that returns nil

@mfikes anything you disagree about the style-guide before I merge it? we can continue to change it always of course 😉

oh, so it is actually non-frog behavior then

cljs.user=> (extend-type number
       #_=>  ILookup
       #_=>  (-lookup [_ _] 42))
nil
cljs.user=> (get 1 17)
42

so it has to be something like (s/cat :thing any? ...)

Yeah, any?

extensible implies any?

I don’t see any issues with the style guide. Use of s/alt is new to me. I’ll read up on it.

it came from Alex.

I think it’s because arguments always talk about a (s/cat ...) and you use s/alt in s/cat

but maybe @alexmiller can enlighten us a bit

s/or might be more reserved for logical or cases

although the docstring of both are equal

Nice work on the style-guide and the resulting code, Michiel.

Feel free to merge 🙂

Regarding functions like (s/* any?) => (s/* any), maybe we should mark them in popular.md as not really worth implementing?

Problem is that it’s hard to figure out if anything that wide/generic without actually implementing the spec, and then it sort of is there.

what’s a function that has this “type”? str for example has the lhs, but not the rhs?

I agree on the general direction of most important/interesting specs first

get seems to be this kind of function

(get any? any? any?) => any? hmm yes

actually, (get any? any? any?) => any?)

yes with s/? for the last one 🙂

also a bit doubtful wrt if, when, or, and, not. But then since the first is a special form, and the next three are macros, we can probably leave them be.

agreed, same with ->, etc.

just something to consider: it’s possible there may be multiple useful specs for a function. stu mentioned that if you add a spec and a working program starts failing, then that’s not a useful spec (“spec should work for you, you shouldn’t work for the spec”) but OTOH, I imagine some cases (esp. beginners) where a more restrictive spec would be useful to avoid likely errors

or maybe -> has something like (-> any? (s/* (s/or list? ifn?)))…

not sure if that’s something speculative could support via different namespaces, or if that’s too complex

@bbrinck we had a conversation before what the rationale of speculative should be. who do we target?

personally I hope to get something out of it for my own dev experience

but i can certainly imagine an existing project opting into the most backwards compatible specs, but using “tighter” specs that avoid more more bugs as time goes on

if you make specs tighter for beginners (than actually is the case in core) it’s hard to foresee what programs they won’t be able to write anymore

My initial thoughts for speculative was two: 1) provide better error messages through spec. 2) Make something that might be included in clojure.core if they want it.

But I hadn’t made up my mind which of the two was more important, because I didn’t realize that they’d class so early/often.

Right, there’s sort of a tension here

(I totally agree with saying “no” to certain use cases, just pointing out that I’m unclear on what I’d choose right now)

So, I wrote a spec for / which didn’t allow for division by zero.

e.g. for get - is (get 1 1) useful? Does anyone really use that? Technically it’s valid

Which makes sense for 1), because it gives a better error message for the user, but violates 2)

@bbrinck https://clojurians.slack.com/archives/CDJGJ3QVA/p1540577835042100

extensibility implies any?

agreed

So, at least for now, I tend to listen whenever @alexmiller chimes in with some advice 🙂

My point is that I can see at least two valid goals for a set of specs

if you have a closed world like maria.cloud it’s a different story, but then I can imagine you instrument different specs depending on the level of the “game”

1. should be backwards compatible with existing behavior, even if behavior is a bit strange 2. help all users avoid common mistakes when using functions (see surprising behaviors with using set ns)

totally valid to pick 1 over 2!

I guess one could imagine that speculative.core contains stuff that aligns with clojure.core, and that another namespace really.speculative could contain stricter specs.

another vector is perf, I can at least imagine a spec that is more correct, but more expensive to verify (especially ones that validate anonymous functions)

or speculative.orly?

so I can imagine a user opting into the more correct, but more expensive specs (e.g. the second arg is map isn’t really ifn?, it’s a function with specific properties)

but for most users, the cheaper, ifn? check may be plenty

perf really is affected even with the small amount of specs now in speculative. I tried it in a big app which becomes significantly slower (backend + frontend)

yeah

anyway, love what you all are doing, and I’m not trying to say which is the right priority 🙂

I can imagine a more clever workflow with not having instrumentation on all the time. Only in cases like: what the heck went wrong here. Let’s turn it on.

ifn? is actually the only correct thing for map:

user=> (fn? :key)
false
user=>

just noting that if we know the goals of speculative, someone else could potentially fork and write a different set of specs with different goals 🙂

@bbrinck I really appreciate the discussions/feedback

> what the heck went wrong here yeah. if it were safe to retry functions, it’d be interesting to run without instrumentation by default, then re-run on error. not easy to do though …

Also, wrt forking, I’d really appreciate a discussion prior to forking 🙂

I know ghostwheel does some analysis of which functions may be pure and tries to warn, but then you get into a whole other set of conventions and tooling that is likely way out of the scope of the discussion 🙂

@slipset Will do. In all reality, I won’t have time to do that anytime soon

And maybe, as you noted, there is space for multiple ns with different specs w different tradeoffs, only some of which will ever go into core

I mean, I haven’t at all decided on the priorities, and I’d hope that we could share work. I really don’t care if the project is forked, it’s just a shame if work is duplicated for no reason.

The biggest upside for me wrt this project is the stuff that I’ve learned and the people I work with.

@slipset Re: ifn?, I only mean that technically, that spec is not tight enough … map doesn’t take any ifn?, it takes an ifn with certain conditions on params and return value

maybe reduce is a better example here actually … the second arg to reduce has a certain signature, not just fn?

@bbrink true, there should be a way of checkin arities for ifns

yeah, you’d need to dive into fspec IIRC, but that uses test.check for validation, so more expensive

https://github.com/slipset/speculative/blob/1f82ca26e726d09c9867ae2321aa78eda72dd04b/src/speculative/core.cljc#L17

But as you said, validation for that was just insanely expensive

yep, that makes sense, test.check is generating examples there … and most examples likely aren’t very useful there because any? will generate a lot of not useful data in this case

Merged something that checks our :ret + :fn specs

Arg specs are describing syntax, which is an ideal case for regex spec. alt is the regex choice spec.

This is a long standing debate and there is a ticket for it

I haven’t looked at the specifics of this ticket, but in general for questions like this, I wonder if each project could make a decision and include the appropriate spec

as opposed to waiting for a single canonical spec for each case, which requires a lot of effort to get right across every core function

The checking works pretty good. I didn’t manage to make

(check `= [0 0])

ERROR in (=-test) (Error:NaN:NaN)
expected: (check (quote cljs.core/=) [1])
  actual: #error {:message "Call to #'cljs.core/= did not conform to spec:\nIn: [1] val: :cljs.spec.alpha/invalid fails at: [:args] predicate: any?\n:cljs.spec.alpha/spec  #object[cljs.spec.alpha.t_cljs$spec$alpha4961]\n:cljs.spec.alpha/value  ([1] :cljs.spec.alpha/invalid)\n:cljs.spec.alpha/args  ([1] :cljs.spec.alpha/invalid)\n:cljs.spec.alpha/failure  :instrument\n", :data {:cljs.spec.alpha/problems [{:path [:args], :pred cljs.core/any?, :val :cljs.spec.alpha/invalid, :via [], :in [1]}], :cljs.spec.alpha/spec #object[cljs.spec.alpha.t_cljs$spec$alpha4961], :cljs.spec.alpha/value ([1] :cljs.spec.alpha/invalid), :cljs.spec.alpha/args ([1] :cljs.spec.alpha/invalid), :cljs.spec.alpha/failure :instrument}}

Will check if https://dev.clojure.org/jira/browse/CLJS-2793 fixes ^ when back to computer

I'm getting this with ClojureScript 1.10.339

cljs.user=> (check `= [0 0])
true