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

satisfies? fails to find protocols that don't have methods #785

Closed
lilactown opened this issue Jul 27, 2022 · 9 comments · Fixed by #786
Closed

satisfies? fails to find protocols that don't have methods #785

lilactown opened this issue Jul 27, 2022 · 9 comments · Fixed by #786

Comments

@lilactown
Copy link
Contributor

version

0.3.32 and main

platform

macOS 12.4 / Nodejs v18.3 / Clojure 1.11

problem

satisfies? does not return true for protocols that have not methods.

repro

in Clojure:

(defprotocol IFoo)
(extend-type String IFoo)
(satisfies? IFoo "foo")

in ClojureScript:

(defprotocol IFoo)
(extend-type js/String IFoo)
(satisfies? IFoo "foo")

expected behavior

The above snippets should return true.

actual behavior

They return false.

@lilactown
Copy link
Contributor Author

lilactown commented Jul 29, 2022

There's some discrepancy between what I see in SCI and the behavior in babashka:

(sci/eval-string "
(defprotocol Marker)

(extend-type String Marker)
(satisfies? Marker \"\")
" {:classes '{java.lang.String java.lang.String} })
;; => true

However, running in bb:

Babashka v0.8.157 REPL.
Use :repl/quit or :repl/exit to quit the REPL.
Clojure rocks, Bash reaches.

user=> (defprotocol Marker)
Marker
user=> (extend-type String Marker)
{:ns #object[sci.lang.Namespace 0x6a776731 "user"], :name user/Marker, :methods #{}, :var #'user/Marker, :sigs {}, :satisfies #{class java.lang.String}}
user=> (satisfies? Marker "")
false

If I elide the {:classes ,,,} map above, it returns false too

@borkdude
Copy link
Collaborator

@lilactown Good find! I'm going to take a look

@borkdude
Copy link
Collaborator

borkdude commented Jul 29, 2022

@lilactown It has to do with how imports work. This is how it's configured in bb:

 (sci/eval-string "
(defprotocol Marker)

(extend-type String Marker)
(satisfies? Marker \"\")
" {:classes {'java.lang.String java.lang.String} :imports {'String 'java.lang.String}})
false

@lilactown
Copy link
Contributor Author

I'll see if I can begin to understand how the classes/imports stuff works

@borkdude
Copy link
Collaborator

borkdude commented Jul 30, 2022

So guess what, I did have a test for this (clj only):

#?(:clj (is (true? (sci/binding [sci/out *out*](sci/eval-string "
(defprotocol Marker)

(extend-type java.lang.Long Marker)
(satisfies? Marker 1)
" {:classes '{java.lang.Long java.lang.Long}})))))

but there is a mistake in the test: the classes config should be {'java.lang.Long java.lang.Long} instead of completely quoted.

@borkdude
Copy link
Collaborator

So we should fix that test and then update the implementation accordingly.

@borkdude
Copy link
Collaborator

borkdude commented Aug 2, 2022

@lilactown Whoops, after I merged #786, I noticed that some tests with babashka + schema.core are failing. Need to investigate why, for now I reverted the entire commit on master but pushed both SCI and bb branches named satisfies-marker with #786 applied.

@borkdude
Copy link
Collaborator

borkdude commented Aug 2, 2022

Could have been just a local thing, I'm running it in CI here now:

https://app.circleci.com/pipelines/github/babashka/babashka?branch=satisfies-marker

Will check back tomorrow.

@borkdude
Copy link
Collaborator

borkdude commented Aug 2, 2022

OK, all fine on CI.

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