Check order of protocol impls #378

borkdude · 2020-08-12T21:10:58Z

Protocol implementations are checked first for direct definitions (defrecord, deftype, reify), then metadata definitions, then external extensions (extend, extend-type, extend-protocol).

https://clojure.org/reference/protocols#_extend_via_metadata

Check if this holds true in sci.

We should make test cases for each of these:

Protocol implementations are checked first for direct definitions (defrecord, deftype, reify), then metadata definitions, then external extensions (extend, extend-type, extend-protocol).

Failing example:

user=> (sci/eval-string "(defprotocol Foo :extend-via-metadata true  (foo [this])) (extend-protocol Foo Object (foo [this] :object)) (foo (with-meta {} {`foo (fn [_] :meta)}))")
:object
user=> (defprotocol Foo :extend-via-metadata true  (foo [this])) (extend-protocol Foo Object (foo [this] :object)) (foo (with-meta {} {`foo (fn [_] :meta)}))
Foo
nil
:meta

Expected: :meta

The text was updated successfully, but these errors were encountered:

grzm · 2021-09-30T19:24:20Z

It does look like the order is different in sci. It looks like extend-protocol has a higher priority than metadata definitions:

(System/getProperty "babashka.version")
;; => "0.6.1"

(defprotocol Bar
  :extend-via-metadata true
  (bar [_]))

(def bar-map (with-meta {} {`bar (fn [_] {:impl :with-meta})}))

;; confirm extend-via-metadata does in fact work
(bar bar-map) ;; => {:impl :with-meta}

(defprotocol Foo
  :extend-via-metadata true
  (foo [_]))

(extend-protocol Foo
  Object
  (foo [_] {:impl :extend-protocol}))

(defrecord Fooer []
  Foo
  (foo [_] {:impl :defrecord}))

(def fooer (->Fooer))

(foo fooer);; => {:impl :defrecord}

(def foo-map (with-meta {} {`foo (fn [_] {:impl :with-meta})}))
;; confirm metadata

(meta foo-map)
;; => {user/foo
;;     #object[sci.impl.fns$fun$arity_1__1208 0x3eac374a "sci.impl.fns$fun$arity_1__1208@3eac374a"]}

;; XXX expected :impl :with-meta, but extend-protocol for Object instead.
(foo foo-map)
;; => {:impl :extend-protocol}

(def fooer-vary-meta (vary-meta (->Fooer) assoc `foo (fn [_] {:impl :with-meta})))

(foo fooer-vary-meta);; => {:impl :defrecord}
(meta fooer-vary-meta)
;; => {:sci.impl/record true,
;;     :type user/Fooer,
;;     user/foo
;;     #object[sci.impl.fns$fun$arity_1__1208 0x27dc294a "sci.impl.fns$fun$arity_1__1208@27dc294a"]}

;; note that using with-meta will overwrite the sci metadata for the defprotocol implementation
(def fooer-with-meta (with-meta (->Fooer) {`foo (fn [_] {:impl :with-meta})}))
(foo fooer-with-meta);; => {:impl :extend-protocol}
(meta fooer-with-meta)
;; => {user/foo
;;     #object[sci.impl.fns$fun$arity_1__1208 0x437834e1 "sci.impl.fns$fun$arity_1__1208@437834e1"]}

(defrecord Barrer [] Bar (bar [_] {:impl :defrecord}))

(def barrer-vary-meta (vary-meta (->Barrer) assoc `bar (fn [_] {:impl :with-meta})))
(bar barrer-vary-meta);; => {:impl :defrecord}


(def barrer-with-meta (with-meta (->Barrer) {`bar (fn [_] {:impl :with-meta})}))
(bar barrer-with-meta);; => {:impl :with-meta}

borkdude · 2021-10-01T09:42:19Z

@grzm The first fix in 8f5d84b. I think it would be feasible to add the rest in a similar manner.

grzm · 2021-10-01T13:34:28Z

@grzm The first fix in 8f5d84b.

Looks good to me!

(System/getProperty "babashka.version");; => "0.6.2-SNAPSHOT"

(defprotocol Bar :extend-via-metadata true (bar [_]))

(def bar-map (with-meta {} {`bar (fn [_] :with-meta)}))

;; confirm extend-via-metadata does in fact work
(bar bar-map);; => :with-meta

(defrecord Barrer [] Bar (bar [_] :defrecord))

(def barrer-vary-meta (vary-meta (->Barrer) assoc `bar (fn [_] :vary-meta)))
(bar barrer-vary-meta);; => :defrecord

;; with-meta unsuprisingly overwrites the sci metadata that powers protocols
(def barrer-with-meta (with-meta (->Barrer) {`bar (fn [_] :with-meta)}))
(bar barrer-with-meta);; => :with-meta

(defprotocol Foo
  :extend-via-metadata true
  (foo [_]))

(extend-protocol Foo
  Object
  (foo [_] :extend-protocol))

(foo {});; => :extend-protocol

(defrecord Fooer []
  Foo
  (foo [_] :defrecord))

(foo (->Fooer));; => :defrecord

(def foo-map (with-meta {} {`foo (fn [_] :with-meta)}))

(foo foo-map);; => :with-meta

(def fooer-vary-meta (vary-meta (->Fooer) assoc `foo (fn [_] :vary-meta)))

;; works as expected: defrecord direct definition has priority over metadata
(foo fooer-vary-meta);; => :defrecord

I think it would be feasible to add the rest in a similar manner.

What other edge cases are there?

borkdude · 2021-10-01T13:46:45Z

@grzm I would expect extend and extend-type to not behave correctly yet with respect to metadata. If you want, you can make a PR with failing tests for these and I'll try to fill in the implementation (but it would be ok if you tried that too). No obligations of course.

grzm · 2021-10-01T13:53:50Z

Ah, gotcha. I use extend and extend-type so infrequently I had forgotten about them.

(defprotocol Baz (baz [_]))
(extend Object Baz {:baz (fn [_] :extend)})

;; OK
(baz {});; => :extend

;; XXX should be vary-meta
(baz (vary-meta {} assoc `baz (fn [_] :vary-meta)));; => :extend

(defprotocol Quux (quux [_]))
(extend-type Object Quux (quux [_] :extend-type))

;; OK
(quux {});; => :extend-type
;; XXX should be :vary-meta
(quux (vary-meta {} assoc `quux (fn [_] :vary-meta)));; => :extend-type

I'll see if I can't squeeze in a PR, though I'm at Strange Loop today and tomorrow, so don't wait on me if you're feeling the itch.

borkdude · 2021-10-01T14:06:58Z

@grzm No worries, those examples are sufficient for me to create tests. Have fun at STL!

borkdude · 2021-10-01T19:54:14Z

@grzm Should be fixed now. Also updated bb master.

grzm · 2021-10-02T15:05:13Z

Awesome! Thanks, @borkdude 🚀

borkdude · 2021-10-02T15:09:15Z

Shouldn't hurt to test the newest bb to double check :)

grzm · 2021-10-02T15:17:03Z

Oh, I did 😛 : Not in anger, (it's the weekend), but I tested against snapshot and newish clojure.

(ns com.grzm.sci.protocols-test
  (:require [clojure.test :refer [deftest is testing] :as test]))

(defprotocol Bar
  :extend-via-metadata true
  (bar [_]))

(defrecord Barrer [] Bar (bar [_] :defrecord))

(deftest extend-via-metadata-vs-direct
  (testing "object with meta"
    (is (= :meta (bar (with-meta {} {`bar (constantly :meta)})))))
  (testing "record with meta prefers direct implementation"
    (is (= :defrecord (bar (vary-meta (->Barrer) assoc `bar (constantly :meta)))))))

(defprotocol Foo
  :extend-via-metadata true
  (foo [_]))

(extend-protocol Foo
  Object
  (foo [_] :extend-protocol))

(defrecord Fooer []
  Foo
  (foo [_] :defrecord))

(deftest extend-via-metadata-vs-extend-protocol
  (testing "record with implementation prefers direct implementation"
    (is (= :defrecord (foo (->Fooer)))))
  (testing "object without metadata defaults to extend-protocol implementation"
    (is (= :extend-protocol (foo {}))))
  (testing "object with metadata prefers metadata implementation"
    (is (= :meta (foo (with-meta {} {`foo (constantly :meta)})))))
  (testing "record with metadata prefers direct implementation"
    (is (= :defrecord (foo (vary-meta (->Fooer) assoc `foo (constantly :meta)))))))

(defprotocol Baz (baz [_]))
(extend Object Baz {:baz (fn [_] :extend)})

(deftest extend-via-metadata-vs-extend
  (testing "object defaults to extend implementation"
    (is (= :extend (baz {}))))
  (testing "object with metadata prefers metadata implementation"
    (is (= :extend (baz (vary-meta {} assoc `baz (constantly :meta)))))))

(defprotocol Quux (quux [_]))
(extend-type Object Quux (quux [_] :extend-type))

(deftest extend-via-metadata-vs-extend-type
  (testing "object defaults to extend-type implementation"
    (is (= :extend-type (quux {}))))
  (testing "object with metadata prefers metadata implementation"
    (is (= :extend-type (quux (vary-meta {} assoc `quux (constantly :meta)))))))

(test/test-ns *ns*)

$ clojure --version
Clojure CLI version 1.10.3.905
grzm@koke dev % clojure sci-protocols.clj       
WARNING: Implicit use of clojure.main with options is deprecated, use -M

Testing com.grzm.sci.protocols-test

$ ~/Downloads/bb --version                 
babashka v0.6.2-SNAPSHOT
$ ~/Downloads/bb sci-protocols.clj

Testing com.grzm.sci.protocols-test
{:test 4, :pass 10, :fail 0, :error 0}

borkdude · 2021-10-02T15:27:07Z

You elided the number of passed results from Clojure, but I assume they were the same :)

grzm · 2021-10-02T15:33:56Z

Actually, I didn't elide it, I just didn't print it. 😢 I think that's because babashka prints the output of the last expression, and clojure doesn't unless you explicitly tell it to (using println or the like). (The "Testing" gets printed because it's explicitly printing to stderr/stdout IIRC). Here's with the added println:

$ tail sci-protocols.clj 
(defprotocol Quux (quux [_]))
(extend-type Object Quux (quux [_] :extend-type))

(deftest extend-via-metadata-vs-extend-type
  (testing "object defaults to extend-type implementation"
    (is (= :extend-type (quux {}))))
  (testing "object with metadata prefers metadata implementation"
    (is (= :extend-type (quux (vary-meta {} assoc `quux (constantly :meta)))))))

(println (test/test-ns *ns*))

$ clojure sci-protocols.clj       
WARNING: Implicit use of clojure.main with options is deprecated, use -M

Testing com.grzm.sci.protocols-test
{:test 4, :pass 10, :fail 0, :error 0}

grzm · 2021-10-02T15:35:05Z

(I also didn't test the multi-arity versions, but it was enough to confirm behavior to my satisfaction)

Thanks again!

borkdude · 2021-10-02T15:37:03Z

Oh I see, yeah that's actually a disputable behavior of bb, but nobody complained hard enough about it yet...

I actually had to introduce more complicated changes to fix a regression for multi-arity but the tests all seem to work there as well in SCI. Thanks for bringing this to my attention.

How is/was Strangeloop?

grzm · 2021-10-02T15:54:06Z

Oh I see, yeah that's actually a disputable behavior of bb, but nobody complained hard enough about it yet...

I think the existing behavior is useful for bb! I wouldn't ask for it to be changed :)

I actually had to introduce more complicated changes to fix a regression for multi-arity but the tests all seem to work there as well in SCI. Thanks for bringing this to my attention.

Yeah, I saw that. Thanks for digging in!

How is/was Strangeloop?
Moving channels ;)

borkdude added the bug Something isn't working label Sep 30, 2021

borkdude added a commit that referenced this issue Oct 1, 2021

[#378] Prefer metadata override over extend-protocol

9249a53

borkdude added a commit that referenced this issue Oct 1, 2021

[#378] Prefer metadata override over extend-protocol (#623)

8f5d84b

borkdude added a commit that referenced this issue Oct 1, 2021

[#378] Prefer meta impl in extend

0a84b0f

borkdude added a commit that referenced this issue Oct 1, 2021

[#378] More fixes wrt/ multi-arity

9b55569

borkdude added a commit that referenced this issue Oct 1, 2021

[#378] More tests

9cd4b8b

borkdude closed this as completed Oct 1, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Check order of protocol impls #378

Check order of protocol impls #378

borkdude commented Aug 12, 2020 •

edited

Loading

grzm commented Sep 30, 2021 •

edited

Loading

borkdude commented Oct 1, 2021 •

edited

Loading

grzm commented Oct 1, 2021

borkdude commented Oct 1, 2021

grzm commented Oct 1, 2021 •

edited

Loading

borkdude commented Oct 1, 2021

borkdude commented Oct 1, 2021

grzm commented Oct 2, 2021

borkdude commented Oct 2, 2021

grzm commented Oct 2, 2021

borkdude commented Oct 2, 2021

grzm commented Oct 2, 2021

grzm commented Oct 2, 2021

borkdude commented Oct 2, 2021

grzm commented Oct 2, 2021

Check order of protocol impls #378

Check order of protocol impls #378

Comments

borkdude commented Aug 12, 2020 • edited Loading

grzm commented Sep 30, 2021 • edited Loading

borkdude commented Oct 1, 2021 • edited Loading

grzm commented Oct 1, 2021

borkdude commented Oct 1, 2021

grzm commented Oct 1, 2021 • edited Loading

borkdude commented Oct 1, 2021

borkdude commented Oct 1, 2021

grzm commented Oct 2, 2021

borkdude commented Oct 2, 2021

grzm commented Oct 2, 2021

borkdude commented Oct 2, 2021

grzm commented Oct 2, 2021

grzm commented Oct 2, 2021

borkdude commented Oct 2, 2021

grzm commented Oct 2, 2021

borkdude commented Aug 12, 2020 •

edited

Loading

grzm commented Sep 30, 2021 •

edited

Loading

borkdude commented Oct 1, 2021 •

edited

Loading

grzm commented Oct 1, 2021 •

edited

Loading