bhb / expound

Human-optimized error messages for clojure.spec
Eclipse Public License 1.0
924 stars 24 forks source link

(cljs.spec.test.alpha/instrument) breaks expound due to wrong arity #197

Closed raymond-w-ko closed 4 years ago

raymond-w-ko commented 4 years ago

Hello, I am using expound in ClojureScript "1.10.773", with shadow-cljs "2.10.21" inside the latest Chrome browser.

When I turn instrumentation (either via cljs.spec.test.alpha/instrument) or via orchestra-cljs.spec.test/instrument my code crashes with:

... TypeError: expound.alpha.expound_str.cljs$core$IFn$_invoke$arity$3 is not a function

and when you inspect the function in the console, it indeed does not have this arity.

expound.alpha.expound_str
[& args]
ƒ (var_args)
cljs$core$IFn$_invoke$arity$variadic: 
[args]
cljs$lang$applyTo: 
[arglist]
cljs$lang$maxFixedArity: 0
arguments: null
caller: null
length: 1
name: "G__54405"
prototype: {constructor: ƒ}
__proto__: ƒ ()
[[FunctionLocation]]: <unknown>
[[Scopes]]: Scopes[3]

My working theory is that: https://github.com/bhb/expound/blob/22f592ab31c51ab5f39d9a48ac29a9a9a9b4ad13/src/expound/alpha.cljc#L1038 (s/fdef) is wrapping the original function and not generating the right aritys. I don't know if the spec code can be modified so that it generates the correct arity.

If I change mode code to have 3 args to expound.alpha/expound-str then it fails at expound.printer.print_table.cljs$core$IFn$_invoke$arity$2.

In the end, I am not sure if this is a ClojureScript bug/limitation or project bug/limitation. Any ideas?

bhb commented 4 years ago

@raymond-w-ko Interesting!

There's a few variables here (Clojurescript, Shadow, Chrome, perhaps other things in your environment), so let's see if we can get to the bottom of it 😄

On my side, I can't reproduce the issue in a Node REPL (unless I'm missing some important step in the repro):

clj -Sdeps '{:deps {org.clojure/clojurescript {:mvn/version "1.10.773"} expound {:mvn/version "0.8.5"}}}' -m cljs.main -re node

cljs.user=> (require '[expound.alpha :as expound])
nil
cljs.user=> (require '[cljs.spec.test.alpha :as st])
nil
cljs.user=> (st/instrument)
[expound.printer/indent expound.alpha/explain-result-str expound.alpha/specs expound.alpha/explain-results expound.alpha/error-message expound.alpha/custom-printer expound.alpha/explain-results-str expound.printer/print-table expound.alpha/value-in-context expound.printer/pprint-str expound.alpha/expound expound.alpha/defmsg expound.printer/summary-form expound.alpha/expound-str expound.alpha/printer expound.alpha/explain-result expound.printer/no-trailing-whitespace]
cljs.user=> (expound/expound-str string? {})
"-- Spec failed --------------------\n\n  {}\n\nshould satisfy\n\n  string?\n\n-------------------------\nDetected 1 error\n"

I also can't reproduce in a Chrome (version 84.0.4147.125) REPL:

clj -Sdeps '{:deps {org.clojure/clojurescript {:mvn/version "1.10.773"} expound {:mvn/version "0.8.5"}}}' -m cljs.main

(same REPL steps as above)

Based on those tests in the REPL, I don't think the issue is with Chrome or with the way that expound-str is specced.

I don't use Shadow, so I'm not familiar with how to get a basic setup working. I'm curious - in your setup, can you reproduce by just creating a multi-arity function (unrelated to Expound) and speccing it similarly? e.g. does this work?

;; in your namespace
(s/fdef fake-expound-str
  :args (s/cat :spec any?
               :form any?
               :opts (s/? any?))
  :ret string?)
(defn fake-expound-str
  "Some docstring"
  ([spec form]
   (fake-expound-str spec form {}))
  ([spec form opts]
   (println spec form opts)))

;; then call
;; (cljs.spec.test.alpha/instrument)
raymond-w-ko commented 4 years ago

Thank you very much for the response!

I did some more experiments and the results are maddening! I am looking at the generated JS code for the troublesome project and they look almost the same.

The key line is:

playbox.fake_expound_str.cljs$core$IFn$_invoke$arity$2((1),(2));

I tried to make a minimal reproduction, where I am using shadow-cljs to output a node.js library and it works. I paste the above line in the node.js REPL and it works.

I then tried to paste the above same line in my troublesome project (shadow-cljs targeting the browser) in the Chrome console, and it does not work because playbox.fake_expound_str.cljs$core$IFn$_invoke$arity$2 is undefined.

I tried to make sure my project options are the same, with the same deps.edn used, but couldn't get it to break.

Next steps, I'll try to get my mini-repo to target and output for the browsers. Getting that is a bit more work.

This is maddening because it is happening on my machine development build, is being built by my CI server as release, and is internally being used at work by other people's computer. I don't think it is caused by caches, or a bad machine.

Closest issue I found is this, but I'm not sure if it is even relevant. Trying a few fixes didn't seem to immediately work. https://github.com/thheller/shadow-cljs/issues/611

raymond-w-ko commented 4 years ago

Okay, I tracked it down the cause of my issues, and it is caused by [orchestra-cljs.spec.test] being required everywhere.

It is probably caused by https://github.com/jeaye/orchestra/commit/d4bb78659d66a18075ede05b7a5f984287d2dfbd They moved from a clone implementation to a monkey patch implementation. Obviously, something about the implementation is not complete.

Thanks for all your help 😃

bhb commented 4 years ago

Thanks for posting the results of your investigation here! I suspect others may run into similar issues. Good luck!