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? finds protocols that have no methods #786

Merged
merged 9 commits into from
Aug 2, 2022

Conversation

lilactown
Copy link
Contributor

Please answer the following questions and leave the below in as part of your PR.


Fixes #785

The crux of the issue is that the way that satisfies? currently turns a type into a symbol,

(let [t (cond
#?(:clj (class? t))
#?(:clj (symbol (.getName ^Class t)))
(instance? sci.lang.Type t)
(symbol (str t))
:else t)]
(contains? sats t)))))

does not match the way that extend-type and extend-protocol do when adding the metadata to the protocol

(clojure.core/alter-var-root
(var ~proto) update :satisfies (fnil conj #{})
(symbol (str ~atype)))
~@(process-methods ctx atype meths pns extend-via-metadata)))) proto+meths))))

This PR attempts to unify the way we coerce the type to a symbol between extend-type, extend-protocol and satisfies?.

In addition, it also exposes a new var clojure.core/type->str. The rationale for it is we need to use the same machinery in two different contexts: a macro context in extend-type and extend-protocol, and at runtime in satisfies?. I could emit the code directly in the macro context instead, but I thought it would be better for maintenance purposes to have everything refer to a single function.

There also already exists a type->str function in CLJS, so the only maybe-surprising thing for consumers is that there now exists a clojure.core/type->symbol function in SCI that doesn't exist in JVM Clojure.

@borkdude
Copy link
Collaborator

so the only maybe-surprising thing for consumers is that there now exists a clojure.core/type->symbol function in SCI that doesn't exist in JVM Clojure.

Perhaps move this to a "private" namespace. There are some of those in namespaces, perhaps there's already a protocols-specific one.

@lilactown
Copy link
Contributor Author

@borkdude I've update this PR to use just type->str without converting to a symbol when extending a protocol and checking for satisfies?, as well as exposed the type->str function via a private sci.impl.protocols SCI ns rather than from clojure.core

@borkdude borkdude merged commit 10dc558 into babashka:master Aug 2, 2022
@borkdude
Copy link
Collaborator

borkdude commented Aug 2, 2022

@lilactown Excellent!

borkdude added a commit that referenced this pull request Aug 2, 2022
borkdude added a commit that referenced this pull request Aug 2, 2022
borkdude added a commit that referenced this pull request Aug 2, 2022
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 this pull request may close these issues.

satisfies? fails to find protocols that don't have methods
2 participants