ClojureScript

improvements to exception messages and printing

Details

  • Type: Enhancement Enhancement
  • Status: Closed Closed
  • Priority: Blocker Blocker
  • Resolution: Completed
  • Affects Version/s: None
  • Fix Version/s: None
  • Component/s: None
  • Labels:
    None
  • Patch:
    Code and Test
  • Approval:
    Accepted

Description

Port CLJ-2373 to ClojureScript.

Activity

Hide
Ben Brinckerhoff added a comment -

https://github.com/clojure/clojurescript/commit/5f0fabc65ae7ba201b32cc513a1e5931a80a2bf7 makes CLJS spec reporting behave like CLJ, which is excellent.

However, until CLJS also ports over the error reporting code in CLJ (which will format spec errors using "explain-out"), users cannot use a third-party spec reporter e.g. expound

;; launch with 
;; clj -Srepro -Sdeps '{:deps {org.clojure/test.check {:mvn/version "0.9.0"} org.clojure/clojurescript {:mvn/version "1.10.439"}}}' --main cljs.main --repl

(require '[cljs.spec.alpha :as s])
(require '[cljs.spec.test.alpha :as st])

;; instrumenting a function
(s/fdef foobar :args (s/cat :x int?))
(defn foobar [x] x)

(st/instrument)

;; set up an example spec printer
(set! s/*explain-out* (fn [e-data] (println "SPEC ERROR!")))

(foobar "")

;; Expected: 
;; Execution error - invalid arguments to user/foobar at (REPL:1).
;; SPEC ERROR!

;; Actual:
;; #error {:message "Call to #'cljs.user/foobar did not conform to spec.", :data #:cljs.spec.alpha{:problems [{:path [:x], :pred cljs.core/int?, :val "", :via [], :in [0]}], :spec #object[cljs.spec.alpha.t_cljs$spec$alpha1379], :value (""), :fn cljs.user/foobar, :args (""), :failure :instrument}}
;;	 cljs$core$ExceptionInfo (cljs/core.cljs:11154:11)
;;	 cljs.core.ex_info.cljs$core$IFn$_invoke$arity$3 (cljs/core.cljs:11186:5)
;;	 cljs.core.ex_info.cljs$core$IFn$_invoke$arity$2 (cljs/core.cljs:11184:16)
;;	 cljs.core/ex-info (cljs/core.cljs:11181:1)
;;	 cljs$spec$test$alpha$spec_checking_fnconform_BANG_ (cljs/spec/test/alpha.cljs:104:34)
;;	 G__1767__delegate (cljs/spec/test/alpha.cljs:118:42)
;;	 G__1767 (cljs/spec/test/alpha.cljs:115:20)

;; This also effects macroexpansion error reporting e.g. try
(let [x])

For comparison, here is what happens in CLJ

;; launch with
;; clj -Srepro -Sdeps '{:deps {org.clojure/test.check {:mvn/version "0.9.0"} org.clojure/clojure {:mvn/version "1.10.0-beta8"}}}'

(require '[clojure.spec.alpha :as s])
(require '[clojure.spec.test.alpha :as st])

;; instrumenting a function
(s/fdef foobar :args (s/cat :x int?))
(defn foobar [x] x)

(st/instrument)

;; set up an example spec printer
(set! s/*explain-out* (fn [e-data] (println "SPEC ERROR!")))

(foobar "")
;; Execution error - invalid arguments to user/foobar at (REPL:1).
;; SPEC ERROR!

(let [x])
Show
Ben Brinckerhoff added a comment - https://github.com/clojure/clojurescript/commit/5f0fabc65ae7ba201b32cc513a1e5931a80a2bf7 makes CLJS spec reporting behave like CLJ, which is excellent. However, until CLJS also ports over the error reporting code in CLJ (which will format spec errors using "explain-out"), users cannot use a third-party spec reporter e.g. expound
;; launch with 
;; clj -Srepro -Sdeps '{:deps {org.clojure/test.check {:mvn/version "0.9.0"} org.clojure/clojurescript {:mvn/version "1.10.439"}}}' --main cljs.main --repl

(require '[cljs.spec.alpha :as s])
(require '[cljs.spec.test.alpha :as st])

;; instrumenting a function
(s/fdef foobar :args (s/cat :x int?))
(defn foobar [x] x)

(st/instrument)

;; set up an example spec printer
(set! s/*explain-out* (fn [e-data] (println "SPEC ERROR!")))

(foobar "")

;; Expected: 
;; Execution error - invalid arguments to user/foobar at (REPL:1).
;; SPEC ERROR!

;; Actual:
;; #error {:message "Call to #'cljs.user/foobar did not conform to spec.", :data #:cljs.spec.alpha{:problems [{:path [:x], :pred cljs.core/int?, :val "", :via [], :in [0]}], :spec #object[cljs.spec.alpha.t_cljs$spec$alpha1379], :value (""), :fn cljs.user/foobar, :args (""), :failure :instrument}}
;;	 cljs$core$ExceptionInfo (cljs/core.cljs:11154:11)
;;	 cljs.core.ex_info.cljs$core$IFn$_invoke$arity$3 (cljs/core.cljs:11186:5)
;;	 cljs.core.ex_info.cljs$core$IFn$_invoke$arity$2 (cljs/core.cljs:11184:16)
;;	 cljs.core/ex-info (cljs/core.cljs:11181:1)
;;	 cljs$spec$test$alpha$spec_checking_fnconform_BANG_ (cljs/spec/test/alpha.cljs:104:34)
;;	 G__1767__delegate (cljs/spec/test/alpha.cljs:118:42)
;;	 G__1767 (cljs/spec/test/alpha.cljs:115:20)

;; This also effects macroexpansion error reporting e.g. try
(let [x])
For comparison, here is what happens in CLJ
;; launch with
;; clj -Srepro -Sdeps '{:deps {org.clojure/test.check {:mvn/version "0.9.0"} org.clojure/clojure {:mvn/version "1.10.0-beta8"}}}'

(require '[clojure.spec.alpha :as s])
(require '[clojure.spec.test.alpha :as st])

;; instrumenting a function
(s/fdef foobar :args (s/cat :x int?))
(defn foobar [x] x)

(st/instrument)

;; set up an example spec printer
(set! s/*explain-out* (fn [e-data] (println "SPEC ERROR!")))

(foobar "")
;; Execution error - invalid arguments to user/foobar at (REPL:1).
;; SPEC ERROR!

(let [x])
Hide
Mike Fikes added a comment -

WIP in this branch towards ultimately producing a patch for review: https://github.com/mfikes/clojurescript/tree/CLJS-2913-WIP

Show
Mike Fikes added a comment - WIP in this branch towards ultimately producing a patch for review: https://github.com/mfikes/clojurescript/tree/CLJS-2913-WIP
Hide
Mike Fikes added a comment -

Much of the new source in cljs.repl is a direct copy of the relevant code from clojure.main. Why is it copied into into cljs.repl? That's where much of the other similar code copied from clojure.main already lives.

In general, the revisions amount to ensuring that exceptions have :clojure.error/phase ex-data on them. As in Clojure, many of the existing exceptions are just chained onto a new exception as the cause.

For cljs.analyzer, this is done / ensured in the wrapping-errors macro. Elsewhere in the code cljs.compiler, cljs.closure, etc, a util/compilation-error helper is used to effect the desired chaining.

As in Clojure, this results in a need to revise many tests to look at cause messages instead of the top-level exception messages, and as in Clojure a thrown-with-cause-msg? facility is copied over to ease transitioning to these kinds of tests.

There is a bit of Clojure code that depends on clojure.spec.alpha being loaded. This hard dependency is eliminated in cljs.repl by instead using resolve for now.

There is also some destructuring code that uses newer :<namespace-name>/keys syntax which is avoided to get things to compile with Clojure 1.8.

Show
Mike Fikes added a comment - Much of the new source in cljs.repl is a direct copy of the relevant code from clojure.main. Why is it copied into into cljs.repl? That's where much of the other similar code copied from clojure.main already lives. In general, the revisions amount to ensuring that exceptions have :clojure.error/phase ex-data on them. As in Clojure, many of the existing exceptions are just chained onto a new exception as the cause. For cljs.analyzer, this is done / ensured in the wrapping-errors macro. Elsewhere in the code cljs.compiler, cljs.closure, etc, a util/compilation-error helper is used to effect the desired chaining. As in Clojure, this results in a need to revise many tests to look at cause messages instead of the top-level exception messages, and as in Clojure a thrown-with-cause-msg? facility is copied over to ease transitioning to these kinds of tests. There is a bit of Clojure code that depends on clojure.spec.alpha being loaded. This hard dependency is eliminated in cljs.repl by instead using resolve for now. There is also some destructuring code that uses newer :<namespace-name>/keys syntax which is avoided to get things to compile with Clojure 1.8.
Hide
Mike Fikes added a comment -

CLJS-2913.patch passes CI

Show
Mike Fikes added a comment - CLJS-2913.patch passes CI

People

Vote (5)
Watch (3)

Dates

  • Created:
    Updated:
    Resolved: