nextjournal / clerk

⚡️ Moldable Live Programming for Clojure
https://clerk.vision
ISC License
1.78k stars 77 forks source link

Blows up when using `potemkin/def-map-type` #247

Closed pfeodrippe closed 1 year ago

pfeodrippe commented 1 year ago

potemkin/def-map-type (https://github.com/clj-commons/potemkin#def-map-type) blows up for Clerk. deftype seems to work fine though.

The var `XXX` exists at runtime,
   but Clerk cannot find it in the namespace. Did you remove it?

Is there a way that I can say that Clerk shouldn't worry about this for a namespace or some extension point where, maybe, we could leverage clj-kondo information about this?

Using declare seems to workaround this.

FelipeCortez commented 1 year ago

got the same error for hugsql's query vars

mk commented 1 year ago

@pfeodrippe Thanks a lot for the report. When a Clojure file starts with an ns form, Clerk is checking if it has seems all dependency vars so it can warn folks when a var still exists at runtime but has been removed from the file.

This is going wrong here and #248 is a similar failure.

You should be able to opt out of this by having a first form that’s something other than a ns (e.g. an empty map) until we figure out if we can fix this or should provide an opt-out.

@FelipeCortez thanks for the pointer to the hugsql failure!

zampino commented 1 year ago

Trying to have a look at this and it seems more complicated than expected. There's some stateful step that potemkin is performing while macroexpanding which messes up Clerk analysis.

In a simple example like this

(ns scratch.potemkin
  (:require [nextjournal.clerk :as clerk]
            [potemkin.types]
            [potemkin.collections :as c]))

(c/def-map-type M [m]
  (get [_ k dv] (clojure.core/get m k dv))
  (assoc [_ k v] (M. (assoc m k v)))
  (dissoc [_ k] (M. (dissoc m k)))
  (keys [_] (keys m)))

(->M {:a 1 :b 2})

the very first time Clerk shows this ns, analysis and eval goes fine. But not a second time 😕: potemkin macro deftype+ (see linked code) won't emit any deftype code and our analyzer won't be aware of the ->M name, hence the warning:

Execution error (ExceptionInfo) at nextjournal.clerk.analyzer/throw-if-dep-is-missing (analyzer.clj:252).
The var `#'scratch.potemkin/->M` exists at runtime, but Clerk cannot find it in the namespace. Did you remove it?

Now after a

(reset! potemkin.types/type-bodies {})

Clerk can show the notebook again.

I also don't think it's a similar issue as in #248 as it's not fixed by using clojure.tools.analyzer.jvm/analyze+eval instead of just analyze in nextjournal.clerk.analyzer.

For scenarios like this I think we could opt-out of the runtime warning for undefined deps as suggested by @pfeodrippe.

mk commented 1 year ago

For scenarios like this I think we could opt-out of the runtime warning for undefined deps as suggested by @pfeodrippe.

I'd prefer we only do that as a last resource. Would much prefer we figure out a solution without folks needing to opt out.

zampino commented 1 year ago

@FelipeCortez could you point at a simple example to reproduce the issue with HugSQL you found?

FelipeCortez commented 1 year ago

@zampino sure! here you go: https://github.com/FelipeCortez/clerk-hugsql-repro

zampino commented 1 year ago

Collecting analyzer failures in a separate repo https://github.com/nextjournal/clerk-analyzer-issues.

ikappaki commented 1 year ago

Hi, there's a similar error with https://github.com/redplanetlabs/specter filed with https://github.com/nextjournal/clerk/issues/286 (and marked as duplicate of this), another thing to add in https://github.com/nextjournal/clerk-analyzer-issues perhaps?

mk commented 1 year ago

@ikappaki yes, was going to suggest that, a PR adding that is very welcome.

Until we've added an opt-out or come up with a fix, here's a way to disable it globally:

(alter-var-root #'nextjournal.clerk.analyzer/throw-if-dep-is-missing (constantly (fn [_ _ _])))
ikappaki commented 1 year ago

@ikappaki yes, was going to suggest that, a PR adding that is very welcome.

Done, https://github.com/nextjournal/clerk-analyzer-issues/pull/1

Until we've added an opt-out or come up with a fix, here's a way to disable it globally:

(alter-var-root #'nextjournal.clerk.analyzer/throw-if-dep-is-missing (constantly (fn [_ _ _])))

Noted, thanks

mk commented 1 year ago

We should handle the libpython-clj and hugsql cases now. For pathological cases which we don't handle properly yet like Potemkin, it's now possible to opt out using {:nextjournal.clerk/error-on-missing-vars :off} on the ns metadata.

The error should also point folks in the right direction now. Please let us know if this works for you now.

pfeodrippe commented 1 year ago

Thank you, @mk and Clerk's team \o/