Add s/defprotocol

[frenchy64]

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.

[w01fe]

😲 whoa.

Thanks for taking this on, it's great that you've found a way to do this that's stable, and tested it well.

This level of complexity and coupling to implementation details is a bit above what I would feel comfortable accepting if I had to maintain it. That said, I think it does fill a major gap in functionality, and I can't see another way that doesn't compromise on ergonomics. So if you think it strikes the right balance and are willing to maintain this in the future, it's fine with me.

I have to confess I don't know the internals of protocols well enough to really review this properly, but I don't see any obvious issues and it looks like you've tested pretty comprehensively, so I'll approve.

[frenchy64]

Happy to own this feature and maintain it. Perhaps one way to lower the implementation burden is to factor out this approach into its own library so others can use it and be more invested in its maintenance.

I will flesh out all the details in comments (especially before I forget them myself!).

[ikitommi]

A separate library would be great, could use this also with malli#555.

[frenchy64]

@ikitommi working on pulling it out. However, it occurred to me that instrumenting protocols at some arbitrary point after the defprotocol has been evaluated will not always work, because methods may have been inlined (in Clojure) by the compiler in the interim.

The reason s/defprotocol always works is that we instrument the protocol directly after it has been created. I will try and create some utilities that can work in both cases.

[frenchy64]

Similar workarounds are discussed briefly here.

[sundbry]

Hi @frenchy64, thank you for providing this feature! I was surprised to find it was even possible. I gave it a test, there is a bug refering to the protocol methods from outside the namespace it was defined in. See the minimum reproducible case below:

user=> (require 'schema.core)
nil
user=> (schema.core/defprotocol Toaster (toast [this bread]))
{:on user.Toaster, :on-interface user.Toaster, :sigs {:toast {:name toast, :arglists ([this bread]), :doc nil}}, :var #'user/Toaster, :method-map {:toast :toast}, :method-builders {#'user/toast #object[schema.macros$_instrument_protocol_method$method_builder__2401 0x73115744 "schema.macros$_instrument_protocol_method$method_builder__2401@73115744"]}}
user=>
user=> (toast nil 1)
Execution error (IllegalArgumentException) at user/eval81414$fn$G (REPL:1).
No implementation of method: :toast of protocol: #'user/Toaster found for class: nil
user=> (ns user2)
nil
user2=> (user/toast nil 1)
Syntax error compiling at (REPL:1:1).
No such var: user2/toast
user2=> user/toast
#object[user$eval81433$fn__81434$toast__81403__81447 0x354d2df5 "user$eval81433$fn__81434$toast__81403__81447@354d2df5"]
user2=> (meta user/toast)
{:schema (=> Any Any Any)}

[frenchy64]

@sundbry nice, thanks! Should be fixed now.

[sundbry]

@frenchy64 Thanks, patch verified. This is just great!

[frenchy64]

Need to think about how to support in bb (it's probably easier than Clojure).

[frenchy64]

@ikitommi I got stuck striking a balance between perf and abstraction when making a shared library. I'll send a PR to malli with these ideas.