tools.namespace does not recognise ns deps implied by data_readers.cljc

Question

i found an interesting issue in our codebase around some tagged-literals seemingly violating the principle of least surprise

we have been creating some pure-data descriptions of processes including some defrecord objects from a custom tagged-literal reader declared in data_readers.cljc, and having those defrecord objects participate in a protocol. it's all been working just fine until i ran across this issue

someone had put one of these tagged literals into a def (which seemed like a reasonable thing to do at first blush):

(def foo #ctx/event-path [:blah])

which was ok... until c.t.n.r/refresh was called, after which point everything broke

it turned out that the defrecord object foo had a stale class (presumably because the namespace dependencies induced by the tagged-literal parsers declared in data_readers.cljc are not recognised by tools.namespace, and namespaces were therefore recompiled out of order), and so no longer participated in the protocol

should tools.namespace be able to recognise data_readers.cljc induced dependencies ? or is that a step too far for it ?

Answer 1

I think one important consideration here is that having a data_readers.cljc does not do anything to automatically load or require the reader function namespaces. Your program is still responsible for requiring the namespace that contains the reader functions before the point where you read tagged literals mapped to that reader function. This is often annoying for tagged literals in the source itself, and maybe something that should be changed, but that's just how things work right now.

Given that, the namespace containing the def implicitly depends on the reader namespace for it to be readable, and the correct thing to do is to make that dependency explicit in that namespace's ns definition. And if you do that, there is no "data_readers.cljc induced dependency", only dependencies already explicit in the namespace definitions and you will not have this issue.

Therefore, I do not think this is a feature that is needed by tools.namespace (given the current implementation of Clojure).

Comments

hmm - that's a killer point @alexmiller - which means that my understanding of why things are failing after `c.t.n.r/refresh` (but never in prod) is flawed

back to the drawing board

Answer 2

My initial reaction to this is "Well, c.t.n.r/refresh can break all sorts of things and I see beginners getting into trouble with reload/refresh workflows quite a lot..." which is why I tend to jump into every conversation where reload/refresh appears and say "Just don't do that!".

As far as I'm concerned, it's a bad solution to a problem that is caused by having a less-than-ideal REPL workflow in the first place. Don't put a band-aid on, encourage better REPL workflows.

And, yes, I know that some parts of Clojure that actually generate Java classes can make life in the REPL hard -- but c.t.n.r/refresh doesn't fix all of those cases anyway (and I think it causes people to stop even thinking about such things).

I would definitely like to see some guidance from the core team around REPL-friendly ways to deal with records and some other areas of Clojure/Java interop. I've seen some recommendations that using the generated functions instead of referencing record types directly helps, but I think there are still issues around how to organize protocol extensions of records that need better guidance?

Comments

There is an interesting repo at https://github.com/aredington/clojure-repl-sufficient-p that highlights some downsides of the current REPL implementation that could stand to include this particular case if someone were motivated to add it. I particularly like the way that this repo is organized in terms of expectations for certain activities.

---

It would be a much more useful repository if it actually _explained_ what the specific issues were and what, if anything, you can do to mitigate those issues. Both the src files and the one test file are extremely opaque in their intent.

Only inside the comp/wrapper src files does it attempt to show a REPL-friendly equivalent.

---

@sean for my case, i found a way of constructing records such that a record's protocol implementation survives the protocol-class getting reloaded out of dependency order - by using `:extend-via-metadata`.  it wasn't very elegant - using the record type to dispatch print-method to serialize the record as a tagged-literal, and metadata to implement protocols, but it worked

(unfortunately it only worked on clj, and hit inscrutable compile errors on cljs, so i've given up on the whole idea of custom tagged-literals with a full serialization round-trip for now)

@sean i'm interested in further detail  on what you describe as "less-than-ideal" and "better" repl workflows - can you link ?

---

I'm a bit disappointed at this non-answer. A far more constructive take would be "indeed there's a flaw in the lib, and it can be fixed".

If bugs in tools.namespace were fixed, we wouldn't be talking about it "breaking all sorts of things".

Repeatedly discouraging a lib is sort of a self-fulfilling prophecy - at some point not many people will want to hack on tools.namespace, guided by the FUD. Which perpetuates the bugs.

I hope someone takes on tools.namespace. I'm definitely happy with the fork I'm using. I don't happen to have the time to improve the official t.n currently.

---

That's a bit over-dramatic, vemv -- I'm not discouraging tools.namespace, I'm discouraging refresh specifically. And I think the flaw is in the workflow, not the library.

---

Dramatic or not, I remain baffled at the repeated campaigning against a particular approach (which may have a fancy name but it's nothing more than 'automation'). Generally that's not seen in our community, as reflected in https://clojure.org/community/etiquette .

---

Let's please keep the discussion to the problem at hand.