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

Add support for deftype #345

Closed
cldwalker opened this issue Jun 11, 2020 · 12 comments
Closed

Add support for deftype #345

cldwalker opened this issue Jun 11, 2020 · 12 comments

Comments

@cldwalker
Copy link

cldwalker commented Jun 11, 2020

Hi. Thanks again for defprotocol landing! Similar to #279, I figured I'd start a list of libraries that are dependent on deftype. Known libraries dependent on deftype:


Is your feature request related to a problem? Please describe.
Would like to use libraries that depend on deftype

Describe the solution you'd like
Deftype compatibility for sci and thus babashka

Describe alternatives you've considered
n/a

Additional context
n/a

@cldwalker
Copy link
Author

Updated list above

@borkdude
Copy link
Collaborator

@GreshamDanielStephens Feedback on #547

  • :sci.impl/type: I would call this :sci.impl/deftype to not conflict with other usages of this keyword
  • The dot-sym-fn I would handle in the interop code. Records are also handled there, so (Foo. x y) works also for records
  • We need to add support for field access like (.-x (Foo. 1)) although you could access the fields through a protocol as well
  • Start adding unit tests. E.g. (sci/eval-string "(defprotocol GetX (get-x [this])) (deftype Foo [x] GetX (get-x [this] x)) (def f (Foo. 1)) (get-x f)") already works, could capture this in a test.
  • Add support for mutable fields

@arichiardi
Copy link

arichiardi commented Apr 10, 2021

Hi there, I have run into this as well while trying to port our queries that are using Hugsql to babashka. The deftypes are here:

https://github.com/layerware/hugsql/search?q=deftype&type=

@logseq-cldwalker
Copy link

Was looking at using datascript from nbb. After fixing a couple minor things on a fork, I ran into this formidable issue again.
@borkdude Am I understanding correctly that with 1 interface defrecord approach you floated, this deftype would be doable but this one wouldn't?

@borkdude
Copy link
Collaborator

borkdude commented Apr 1, 2022

@logseq-cldwalker Protocols aren't really a problem, it's the Java interfaces that are the problem since you can't dynamically create types in a graalvm binary. But for the above examples to work, we would have to expose ILookup etc as protocols to "scripts". So far this was done by a protocol -> multimethod mapping and then patching all functions that use the protocol to make use of this multimethod instead. But maybe we can revise how we do this so we are able to expose the protocols, e.g. ILookup without the need for a multimethod and patching.

@borkdude
Copy link
Collaborator

borkdude commented Apr 1, 2022

Alternatively we could make optional bindings for nbb, like we have for babashka, so you can make a home-built version of nbb with datascript.

@borkdude
Copy link
Collaborator

borkdude commented Apr 1, 2022

I already created an issue for the first option: #639

@logseq-cldwalker
Copy link

@borkdude Gotcha. Both options sound intriguing. The first option sounds potentially more powerful and reusable for others

@borkdude
Copy link
Collaborator

I added very basic support for deftype: it only supports immutable fields and basically behaves the same as records in SCI.

Will follow up with more issues for:

  • supporting 1 Java interfaces on deftype and defrecord, similar to the approach taken in deftype using reify. #547
  • support for mutable fields and set!

@logseq-cldwalker
Copy link

Exciting! 🎉

@borkdude
Copy link
Collaborator

borkdude commented Jul 3, 2022

@logseq-cldwalker Deftype is now supported, similar to defrecord (with the same restriction that you can't add Java interfaces currently). It'd be interesting to see which of the libs are now supported by this changes in the original post.

@borkdude
Copy link
Collaborator

borkdude commented Jul 3, 2022

I went through the list: unfortunately many of these libraries implement their own data structures using deftype which isn't yet support due to the Java interface restriction. Perhaps we can "special cased" support for that use case in the future.

But I found that https://github.com/ont-app/igraph now works (at least the require doesn't crash), but due to a weird metadata usage in the library, I had to make one change in SCI / bb. So the library should now work with bb master (and if you include spec). I think we should also make spec a built-in.

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

4 participants