Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clerk Analyzer doesn't strip metadata from forms when hashing #489

Closed
craig-latacora opened this issue May 17, 2023 · 1 comment · Fixed by latacora/clerk#1
Closed

Clerk Analyzer doesn't strip metadata from forms when hashing #489

craig-latacora opened this issue May 17, 2023 · 1 comment · Fixed by latacora/clerk#1

Comments

@craig-latacora
Copy link
Contributor

The nextjournal.clerk.analyzer/hash-codeblock function will attempt to print out a form to generate a hash. This will result in bad things happening if the form, when evaluated, tagged it's results with some metadata. That metadata will now be applied to the form itself. That's not much of a problem, except that metadata can be used to set the :tag of a value, and then a print-method can be defined for that tag. That print-method body now will try and print the form (a sequence) and likely blow up.

Here is a quick patch that strips metadata before hashing the form. This works, and I don't think that we need to be sensitive to changes in metadata. However, perhaps the more correct mechanism would collect some part of the metadata and include that as input to the hash.

(require '[nextjournal.clerk :as clerk])

;; This will compile and run just fine, but clerk errors when evaluating this as
;; a notebook
(defprotocol Foo
  (-foo [this]))

(defmethod print-method :foo [v ^java.io.Writer w]
  (.write w (str "FOO<" (-foo v) ">")))

;; When this is analyzed by clerk, you will get an exception:
;;
;; Unhandled java.lang.IllegalArgumentException
;; No implementation of method: :-foo of protocol: #'user/Foo found for class: clojure.lang.PersistentList

(defn gimme-a-foo []
  ^{:type :foo}
  (reify Foo
    (-foo [this]
      :baz)))

;; this should use the printer above
(gimme-a-foo)


;; evaluate this, and you will patch the
;; nextjournal.clerk.analyzer/hash-codeblock to strip metadata before printing
;; forms
;;
;; (in-ns 'nextjournal.clerk.analyzer)
;; (require 'clojure.walk)
;; (defn strip-form-meta [form]
;;     (clojure.walk/walk
;;      (fn [v]
;;        (if (or (instance? clojure.lang.IObj v)
;;                (instance? clojure.lang.IMeta v))
;;          (vary-meta v (constantly nil))
;;          v)) identity form))

;;   (defn hash-codeblock [->hash {:as codeblock :keys [hash form id deps vars]}]
;;     (when (and (seq deps) (not (ifn? ->hash)))
;;       (throw (ex-info "`->hash` must be `ifn?`" {:->hash ->hash :codeblock codeblock})))
;;     (let [hashed-deps (into #{} (map ->hash) deps)]
;;       (sha1-base58 (binding [*print-length* nil]
;;                      (pr-str (set/union (conj hashed-deps (if (strip-form-meta form) hash))
;;                                         vars))))))
@mk
Copy link
Member

mk commented May 22, 2023

Hey @craig-latacora,

Thanks for the issue. Should be able to take a closer look tomorrow or Wednesday if it's still a problem that should be fixed upstream for you. Was the closing intentional or should we reopen it?

mk pushed a commit that referenced this issue May 25, 2023
* Be resilient in face of unrecognized type tags

closes #488

* Strip metadata from forms before printing them to hash

closes #489
craig-latacora added a commit to latacora/clerk that referenced this issue Jul 4, 2023
craig-latacora added a commit to latacora/clerk that referenced this issue Aug 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants