Upgrade ExceptionInfo to proper deftype

Question

Currently ExceptionInfo is implemented as a raw constructor function which inherits from js/Error with some ad-hoc javascript-level patches to satisfy a tiny subset of deftype functionality (mainly for printing).

Unfortunately this does not play well with cljs-devtools(link: 1). This problem surfaced when I started playing with ExceptionInfo and cljs-devtools v0.8 which newly supports printing deftypes(link: 2). ExceptionInfo does not contain getBasis, cljs$lang$type, cljs$lang$ctorStr and similar machinery.

My proposed patch implements ExceptionInfo as a proper deftype and does some patch-work to provide backward compatibility. I'm pretty sure we must not break current contract of ExceptionInfo constructor accepting 3 args and synthesizing other fields on the fly in constructor.

Implementation details:
1) first we define ExceptionInfo as normal deftype (to get a template)
2) then we remember reference to ExceptionInfo in ExceptionInfoTypeTemplate
3) then we redefine ExceptionInfo with raw constructor function which should mimic original behaviour (by scraping newly created js/Error instance, but calling ExceptionInfoTypeTemplate to do proper deftype initialization)
4) then we copy keys from ExceptionInfoTypeTemplate over ExceptionInfo
5) then we set ExceptionInfo's prototype to be ExceptionInfoTypeTemplate's prototype
6) then we fix ExceptionInfo's prototype's constructor to point to our re-defined constructor function
7) then we patch ExceptionInfo's prototype to inherit from js/Error (note this clobbers ExceptionInfoTypeTemplate as well - but we don't care about it)

This effectively gives us properly working ExceptionInfo deftype with redefined constructor function wrapping deftype's constructor for backwards compatibility.
We also patch ExceptionInfo's prototype to inherit from js/Error the same was as original code did.

Note: With working deftype, we can move IPrintWithWriter and toString implementation to the deftype itself.

(link: 1) https://github.com/binaryage/cljs-devtools/issues/23
(link: 2) https://github.com/binaryage/cljs-devtools/releases/tag/v0.8.0

Answer 1

Comment made by: thheller

Why not just add the missing {{getBasis}}, {{cljs$lang$type}}, {{cljs$lang$ctorStr}} bits per set!?

The patch looks like it would mess up advanced compilation although that is just an instinct not something I verified, did you?

Answer 2

Comment made by: darwin

I ran clojurescript tests and I assumed they run also against advanced-mode build. During development when my tests were failing I saw error messages about minified names.

This may seem as a hacky solution, but IMO the original code was also a hack. My hack will stay up-to-date with future changes to deftype implementation. I can imagine people would forget to update this part when touching deftype.

btw. there is another patch coming related to discrepancies between deftype and defrecord. That could have been avoided if defrecord shared common implementation with deftype.

Answer 3

Comment made by: thheller

Closure is usually very strict about re-defining stuff but I guess my instincts were wrong, the tests should cover advanced.

My issue with this is that {{deftype}} is for defining Clojure-specific types. ExceptionInfo is not since it inherits from Error, just like you can't have a superclass in Clojure you can't in CLJS. So IF we were to change {{deftype}} in the future we might break things in unexpected ways that just re-use {{deftype}} but aren't actually {{deftype}}.

Yes, you have to do some house-keeping but you can't enforce the rules of {{deftype}} when dealing with inheritance.

Just my 2 cents, it has advantages to re-use {{deftype}} too (as you suggested).

Answer 4

Comment made by: darwin

Unfortunately I was unable to look up any comments or docs explaining the reasoning why we do that js/Error inheritance there.

My first attempt to "fix" ExceptionInfo was to simply implement it as an ordinary deftype. And that worked just fine (for my tests). Then I tried to re-implement original behaviours on top just to make it 100% compatible.

Answer 5

Comment made by: darwin

Just adding a motivational screenshot:

https://box.binaryage.com/CLJS-1722-example.png

Those yellow warnings are listing which properties are getting copied by gobject/extend call.
The expanded log item is new implementation logged via cljs-devtools v0.8.0.
The last log item is the old implementation logged via cljs-devtools v0.8.0 (cljs-devtools does not recognise ExceptionInfo as CLJS type, but detects IPrintWithWriter and uses it to present the value)

Answer 6

Comment made by: mfikes

CLJS-1722.patch added to Patch Tender (i)

Answer 7

Comment made by: mfikes

CLJS-1722.patch passes CI and Canary (/)

Answer 8

Reference: https://clojure.atlassian.net/browse/CLJS-1722 (reported by darwin)