nextjournal/clerk

Analyzer crashes on specter's caching implementation (analyzing forms that create new defs)

Closed this issue · 7 comments

lvh commented

I created a full reproducer here: https://github.com/latacora/clerk-analyzer-specter-bug

The bug seems to be caused because Specter has a macro expansion that itself new vars (for local cache purposes).

For search purposes, the error looks like this:

Unhandled clojure.lang.ExceptionInfo
Hash is missing on dependency 'whatever/pathcache1234' of the form '(defn f [x] (sr/select [...])) 'in foo.bar.baz

I wonder if the analyzer could just compute the hash when this happens?

As a workaround, use select*, which is the version with no fancy caching.

lvh commented

FYI: redplanetlabs/specter#194

I don't think this works for clerk's use case: clerk analyzes tons of stuff marked :private, but at least it means specter recognizes it might do things to cooperate with tooling that isn't expecting it to do any of that.

lvh commented

(Also for me, the select call is in an external library, and I don't think specter allows controlling cache behavior globally, so "replace with the starred variant" isn't really much of an option.

mk commented

@lvh thanks for the detailed repro. I think we should at least support turning off the error via {:nextjournal.clerk/error-on-missing-vars :off}. Clerk's error display in this case is also far from good.

lvh commented

Yep, that makes sense. Do we know of any cases where erroring hard is the correct behavior? I feel like warnings are better, because IIUC the worst case when you hit this is that Clerk doesn't re-render correctly?

Since this is dynamically generated I'm not even sure you could compute this hash if you wanted to. That's kind of the point of this error: it's telling the truth, the analyzer can't get to the source code.

Clerk's error display in this case is also far from good.

I think there's a regression due to #629 which prevents to reach the intended exception with a more understandable message:

(throw (ex-info (str "The var `#'" missing-dep "` is being referenced, but Clerk can't find it in the namespace's source code. Did you remove it? This validation can fail when the namespace is mutated programmatically (e.g. using `clojure.core/intern` or side-effecting macros). You can turn off this check by adding `{:nextjournal.clerk/error-on-missing-vars :off}` to the namespace metadata.")
{:var-name missing-dep :form form :file file #_#_:defined defined}))))))))

the hash missing check should not throw, maybe just warn

(throw (ex-info (format "Hash is missing on dependency '%s' of the form '%s' in %s" dep-with-missing-hash form ns)
{:dep dep-with-missing-hash :codeblock codeblock :ns ns})))

I also bumped into this error when debugging stuff in clerk itself, and changing in nextjournal.clerk.view:

(defn doc->viewer
  ([doc] (doc->viewer {} doc))
  ([opts {:as doc :keys [ns file]}]
   (def d doc) ;; NOTE THIS LINE
   (binding [*ns* ns]
     (-> (merge doc opts) v/notebook v/present))))

I can't repro it in another namespace though

@borkdude thanks for the repro. This helps understand some of the edge cases which can trigger the exception. In this case

Hash is missing on dependency 'nextjournal.clerk.cljs-libs/prepend-required-cljs' of the form '(defn doc->viewer ([doc] (doc->viewer {} doc)) ([opts {:as doc, :keys [ns file]}] (def d :doo) (binding [*ns* ns] (-> (merge doc opts) v/notebook v/present cljs-libs/prepend-required-cljs))))' in ahoi (id: nextjournal.clerk.view/anon-expr-5dsZdRsdGo2tMPyWFe9fq2nNXNVq85, key: nextjournal.clerk.view/doc->viewer)

In the specific case the form (defn doc->viewer ... is stored in the analyzer graph with multiple different keys (since it defines more than one var) not all of which are related with edges to the shared dependency nextjournal.clerk.cljs-libs/prepend-required-cljs and hence can occur in random reciprocal order in the sorted keys.

Here only nextjournal.clerk.view/anon-expr-5dsZdRsdGo2tMPyWFe9fq2nNXNVq85 (the id of the code block) depends on nextjournal.clerk.cljs-libs/prepend-required-cljs but not nextjournal.clerk.view/doc->viewer. This is something we might want to fix.