camsaul / methodical

Functional and flexible multimethods for Clojure. Nondestructive multimethod construction, CLOS-style aux methods and method combinations, partial-default dispatch, easy next-method invocation, helpful debugging tools, and more.
Eclipse Public License 2.0
295 stars 19 forks source link

Effective dispatch value is incorrectly cached for `nil` #112

Closed camsaul closed 2 years ago

camsaul commented 2 years ago

This leads to a stack overflow:

(let [mf* (atom nil)
      mf  (-> (m/multifn
               (m/standard-multifn-impl
                (m/thread-last-method-combination)
                (m/multi-default-dispatcher identity)
                (m/standard-method-table)))
              (m/add-primary-method :default (fn [_next-method m] m))
              (m/add-primary-method nil (fn [_next-method m]
                                          (@mf* :default))))]
  (reset! mf* mf)
  (try
    (mf nil)
    (catch Throwable e
      (pprint/pprint (update-vals (deref (.. mf impl cache atomm)) meta))
      (throw e))))

;; => {:default {:dispatch-value :default}, nil {:dispatch-value :default}}
;; => StackOverflowError

The nil method is incorrectly getting cached for both nil and :default. This is a race condition/caching bug, since the same issue doesn't happen if you use an uncached multifn.

If you invoke (mf :default) before you invoke (mf nil), the :default method gets cached for both nil and :default. This means there is no stack overflow, but the results are still wrong.

This is probably an issue specifically with handling falsey dispatch values.

camsaul commented 2 years ago

Failing test

(deftest nil-dispatch-values-test
  (doseq [order [[nil :default]
                 [:default nil]]]
    (testing (pr-str order)
      (let [mf* (atom nil)
            mf  (-> (m/multifn
                     (m/standard-multifn-impl
                      (m/thread-last-method-combination)
                      (m/multi-default-dispatcher identity)
                      (m/standard-method-table)))
                    (m/add-primary-method :default (fn [_next-method m] :default))
                    (m/add-primary-method nil (fn [_next-method m]
                                                (@mf* :default)
                                                nil)))]
        (reset! mf* mf)
        (doseq [x order]
          (is (= x
                 (mf x))))))))