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

Consider supporting IDeref "interface" implementation in defrecord #401

Closed
borkdude opened this issue Sep 9, 2020 · 5 comments
Closed

Comments

@borkdude
Copy link
Collaborator

borkdude commented Sep 9, 2020

@djblue has already made a start with this, which is merged to the IDeref branch. One remaining problem is how can we simultaneously have IDeref as a real Java class in sci (probably mainly for supporting (instance? IDeref x) and also let people implement it e.g. in defrecord, like protocols in sci (which are implemented using multimethods).

We currently represent protocols as maps with a set of methods in them:

https://github.com/borkdude/babashka/blob/5d9027fe0a41ff1d1e9db124514ad257f1982fe5/src/babashka/impl/protocols.clj#L33

We could add the interface class to that protocol. So when we resolve a class name and we really want a class, but we get a map, we can look in that map and get the class out of it. In other cases where we expect the class to behave more like the protocol we can handle it like the protocol.

@djblue
Copy link
Contributor

djblue commented Sep 13, 2020

With the following code:

(defrecord Example []
  clojure.lang.IDeref
  (deref [this] :deref)

  clojure.lang.IAtom
  (reset [this new-value] :reset)

  (swap  [this f]          :swap)
  (swap  [this f a]        :swap)
  (swap  [this f a b]      :swap)
  (swap  [this f a b args] :swap)

  (compareAndSet [this oldv newv] :compare-and-set))

I get this error:

----- Context ------------------------------------------------------------------
1: (defrecord Example []
   ^--- No implementation of method: :getName of protocol: #'sci.impl.vars/HasName found for class: nil
2:   IDeref
3:   (deref [this] :deref)
4:
5:   IAtom
6:   (reset [this new-value] :reset)

----- Stack trace --------------------------------------------------------------

@borkdude
Copy link
Collaborator Author

borkdude commented Sep 13, 2020

Thanks. This example works for me:

(defrecord Example []
  clojure.lang.IDeref
  (deref [this] :deref)
  clojure.lang.IAtom
  (reset [this new-value] :reset)
  (swap  [this f]          :swap)
  #_(swap  [this f a]        :swap)
  #_(swap  [this f a b]      :swap)
  #_(swap  [this f a b args] :swap)
  #_(compareAndSet [this oldv newv] :compare-and-set))
(prn @(->Example))
(prn (reset! (->Example) 1))
(prn (swap! (->Example) inc))

but there is a problem with multi-arity implementations. Also I didn't add compareAndSet yet. I'll let you know when this is fixed.

PS: make sure to update both bb and sci and run lein clean after compilation with GraalVM. Also, just for trying you can also invoke lein bb or make a clojure alias which invokes bb from the JVM.

@borkdude
Copy link
Collaborator Author

borkdude commented Sep 14, 2020

@djblue On the latest bb /sci IDeref branch:

https://11627-201467090-gh.circle-artifacts.com/0/release/babashka-0.2.1-SNAPSHOT-macos-amd64.zip

(defrecord Example []
  clojure.lang.IDeref
  (deref [this] :deref)

  clojure.lang.IAtom
  (reset [this new-value] :reset)

  (swap  [this f]          :swap)
  (swap  [this f a]        :swap)
  (swap  [this f a b]      :swap)
  (swap  [this f a b args] :swap)

  (compareAndSet [this oldv newv] :compare-and-set))

[@(->Example)
 (reset! (->Example) 1)
 (swap! (->Example) inc)
 (swap! (->Example) + 1)
 (swap! (->Example) + 1 2)
 (swap! (->Example) + 1 2 3)
 (compare-and-set! (->Example) 1 2)]
$ ~/Downloads/bb-ideref /tmp/foo.clj
[:deref :reset :swap :swap :swap :swap :compare-and-set]

@djblue
Copy link
Contributor

djblue commented Sep 14, 2020

@borkdude, this works perfectly for me!

@borkdude
Copy link
Collaborator Author

borkdude commented Sep 16, 2020

@djblue Now also added IAtom2 to both branches. Will merge with master later today.

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

No branches or pull requests

2 participants