search

plumatic / schema

Clojure(Script) library for declarative data description and validation
Other
2.4k stars 256 forks source link

schema.core/protocol is incompatible with metadata-based protocol implementation #424

Open vemv opened 4 years ago

vemv commented 4 years ago

s/protocol uses clojure.core/satisfies?:

https://github.com/plumatic/schema/blob/ddb54c87dea6926c6d73542e517b53685cc82a37/src/cljx/schema/core.cljx#L347

In turn, satisfies? does not honor metadata-based protocol implementation: https://dev.clojure.org/jira/browse/CLJ-2426

metadata-based protocol implementation is a fine tool that can solve a variety of problems. Having s/protocol fail on it is very inconvenient.

You may find inspiration for a drop-in replacement for satisfies? here.

Thanks - V

w01fe commented 4 years ago

Thanks for the pointer. That said, I think my opinion is that it's not this library's job to work around bugs in Clojure (especially since users are free to write their own protocol schema). For example, I would be worried that any implementation we add could be broken by a new Clojure(Script) release. Is there a reason I'm not seeing why the workaround would need to live here?

vemv commented 4 years ago

Hi @w01fe , thanks for the reply!

especially since users are free to write their own protocol schema

This is not practical, since there are 3rd party libraries written in Schema. I cannot control whether they use s/protocol or my/protocol.

not this library's job to work around bugs in Clojure

That's one way to conceptualize it. Another is that s/protocol currently uses satisfies? as an implementation detail. Coupling protocol and satisfies? behavior 1:1 is a choice; one is free to base protocol in satisfies? and extra logic.

For example, I would be worried that any implementation we add could be broken by a new Clojure(Script) release.

I have made the effort of trying to imagine such an scenario and fail to see it possible. The worst thing that can possibly happen is that the extra check becomes redundant because in a future clojure.core/satisfies? would be fixed.

There's the possibility that https://dev.clojure.org/jira/browse/CLJ-2426 gets closed in the opposite direction (i.e. "satisfies? will never work on metadata-based extensions"), in that case nothing would change for existing Schema users, and one would remain able to use metadata-based extensions (at the cost of differing opinions, luckily protocol is not called satisfies? or such)

w01fe commented 4 years ago

That's one way to conceptualize it. Another is that s/protocol currently uses satisfies? as an implementation detail. Coupling protocol and satisfies? behavior 1:1 is a choice; one is free to base protocol in satisfies? and extra logic.

https://clojure.org/reference/protocols says "Protocols are fully reified and support reflective capabilities via extends? , extenders , and satisfies?." Is there documentation of another public interface that can be used to detect satisfaction?

I have made the effort of trying to imagine such an scenario and fail to see it possible

Well, for instance your implementation relies on method-builders and I can't find a reference to that implementation detail anywhere in the reference materials for defprotocol or https://clojure.org/reference/protocols, so it seems like that could be subject to change? I was also worried about the addition of new ways to satisfy but I guess since you call through into the underlying method that's probably safe.