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

How do I add schema annotations to defprotocol? #117

Closed
scramjet opened this issue Jun 24, 2014 · 13 comments · Fixed by #432
Closed

How do I add schema annotations to defprotocol? #117

scramjet opened this issue Jun 24, 2014 · 13 comments · Fixed by #432

Comments

@scramjet
Copy link

I'm translating a bunch of doc comments into Schema, but immediately hit the issue: how do I annotate our (copious) protocol definitions?

I'd like something like:

(s/defprotocol ICodec
  (encode-inquiry [this property :- Property]) :- [Byte]))

Is this possible some other way? Or maybe it's in the too hard basket? (I can see this wouldn't necessarily be easy, possibly requiring interfacing with the Clojure compiler).

@w01fe
Copy link
Member

w01fe commented Jun 24, 2014

There's no built-in support currently -- I think in principle it should be possible because Clojure effectively emits wrapper fns that could be s/fn, but digging into the implementation this would require copy-pasting or re-implementing a bunch of code from clojure.core that would make me feel pretty dirty.

The easy hack for now is to define your protocol methods like encode-inquiry* and then s/defn an encode-inquiry wrapper that just calls through to encode-inquiry*.

@scramjet
Copy link
Author

Thanks. I might have a quick poke around to get an idea of what's involved in the defprotocol implementation. Even if I could have a s/defprotocol that accepts the syntax and spits out a standard clojure.core/defprotocol, that would be a start.

The easy hack for now is to define your protocol methods like encode-inquiry* and then s/defn an encode-inquiry wrapper that just calls through to encode-inquiry*.

I don't really want to change the protocol interfaces and break a bunch of places that implement them.

@expez
Copy link

expez commented May 3, 2015

How about just creating a shim so we can add schemas for documentation purposes and then let the next maintainer worry about validations? :D

@w01fe
Copy link
Member

w01fe commented May 3, 2015

@expez you can always put the schemas in the protocol docstring :/. I worry that allowing annotations that behave differently than in all other schema constructs would cause more harm than good.

@dmitrig01
Copy link

Another way would be to do it in defrecord – since you may be using a protocol from somewhere else, which doesn't have validation defined.

If you were dto do it in defprotocol, you'd have to add the validation in defrecord anyway, since that's where the functions themselves reside. This does something vaguely similar – redefines some functions inside defrecord to allow varargs: https://gist.github.com/sunng87/372a0a45a07357569eb6. We may be able to do something similar here.

@dmitrig01
Copy link

Ok – I have a code snippet which does it – I don't know where the best place to put it in the schema codebase is though...

(defn remove-doc [arg-lists]
  (if (string? (last arg-lists)) (butlast arg-lists) arg-lists))

(defn doc [arg-lists]
  (if (string? (last arg-lists)) [(last arg-lists)] []))

(defn strip-types [arglist]
  (loop [acc []
         arglist arglist]
    (if (empty? arglist)
      acc
      (if (= (first arglist) :-)
    (recur acc (rest (rest arglist)))
    (recur (conj acc (first arglist)) (rest arglist))))))

(defn make-defn [[name & arglists]]
  `(s/defn ~name
     ~@(doc arglists)
     ~@(map (ƒ [arglist] `(~arglist (~(symbol (str "." name)) ~@(strip-types arglist)))) (remove-doc arglists))))

(defmacro defprotocol [name & sigs]
  `(do
     (clojure.core/defprotocol ~name ~@(map (ƒ [[name & arg-lists]]
                                              (concat [name] (map strip-types (remove-doc arg-lists)))) sigs))
     ~@(map make-defn sigs)
     ~name))

@w01fe
Copy link
Member

w01fe commented May 28, 2015

Thanks -- can you explain a bit more what this does? If it just strips types, as I mentioned above we'd be very hesitant to introduce a single schema macro that has different validation syntax than all the existing macros. That said, we welcome third-party schema extension projects, which we think are a great way to prove out ideas which might ultimately make it into core.

@dmitrig01
Copy link

sorry – my bad

(macroexpand-1 '(defprotocol ABCD (foo [_] [_ a :- s/Int] [_ a :- s/Int b :- s/Int])))
; =>
(do
   (clojure.core/defprotocol ABCD
     (foo [_] [_ a] [_ a b]))
   (schema.core/defn foo
      ([_] (.foo _))
      ([_ a :- s/Int] (.foo _ a))
      ([_ a :- s/Int b :- s/Int] (.foo _ a b)))
   ABCD)

@dmitrig01
Copy link

basically, pretty much exactly what you described above :-)

@w01fe
Copy link
Member

w01fe commented May 28, 2015

Ah, I see. But unfortunately, I think by overwriting foo you lose the "protocol nature" of foo -- I'm pretty sure it will only work on types that implement the interface ABCD directly, and not types that are extend-ed to it later. Does that make sense?

@dmitrig01
Copy link

Ahh, intersting. That would explain why what I was trying was working – lemme run some tests and see...

I know, though, if you do (.foo XXX), it won't validate – you hvae to do (namespace-of-protocol/foo XXX)

@SevereOverfl0w
Copy link

Worth noting that core.typed has this feature, no idea if that would help as a source of inspiration for implementation.

@djha-skin
Copy link

+1, I'd love to see this happen :) great work people

This was referenced Nov 9, 2021
frenchy64 added a commit that referenced this issue Aug 17, 2022
Close #117 

Implementation details were needed from both platforms to make this work in practice, but none of the actual dispatch logic was copied here, so `s/defprotocol` gracefully inherits the semantics of whichever Clojure/Script version is used.

Luckily these implementation details seem to apply to all tested versions of Clojure/Script. Once merged, we can test against dev CLJS releases via https://github.com/cljs-oss/canary (and we already test future CLJ versions). EDIT: canary seems dead, I'll look into alternatives.

I thought it was important to be able to completely opt-out of instrumentation either at expansion or evaluation time. For example, if you pull in a lib that AOT compiles a `s/defprotocol` form and there's an instrumentation-related problem, you can opt out by setting it at eval time. I ensured that `s/fn-schema` works with and without instrumentation.

Some of the more interesting issues encountered:

In Clojure:
- added `:inline` metadata to method vars to prevent the compiler from inlining the method (and skipping schema checks)
- instrument method builders, which are used to reset methods after `extend`
- propagating `.__methodImplCache` without introducing (more) data races required a lock (see `clj-protocol-cache-test` for propagation tests)

In CLJS:
- Some tricky wrapping is needed for multi-arity protocol methods to avoid infinite loops.

Squashed commits:
* Add s/defprotocol

* make test repeatable by clearing impls

* more faithful clearing of impls

* fix protocol reference from another ns

* fixed in clojure 1.7

* support declare-class-schema! for protocol methods in bb

* fix

* support bb

* explain __methodImplCache

* typo

* move simple-symbol? to a place where we can delete it later

* ws

* add example in readme

* doc
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.

6 participants