Test ns fails to load defrecord with "Could not resolve symbol" error for a protocol's method name

[logseq-cldwalker]

Hi @borkdude. As discussed in slack, here's a repro that passes for cljs but fails for nbb + sci

version

0.8.41

platform

osx 12 with node

problem

After building a custom version of nbb with sci configs for this datascript fork, the nbb build is unable to load this datascript test ns

repro

• Clone this project's branch - https://github.com/logseq/nbb-features/tree/logseq/storage-test-failing-repro
• Build nbb with datascript sci configs - bb release
• Run nbb tests with bb test and see:

----- Error --------------------------------------
Message:  Could not resolve symbol: -store
Location: /Users/me/code/work/nbb-features/test/libraries/datascript/test/datascript/test/storage.cljs:8:1
Phase:    analysis

----- Context ------------------------------------
4:    [clojure.test :as t :refer [is are deftest testing]]
5:    [datascript.core :as d]
6:    [datascript.storage :as storage]))
7:
8: (defrecord Storage [*disk *reads *writes *deletes]
    ^--- Could not resolve symbol: -store
9:   storage/IStorage
10:   (-store [_ addr+data-seq]
11:     (doseq [[addr data] addr+data-seq]
12:       (vswap! *disk assoc addr (pr-str data))
13:       (vswap! *writes conj add

If it's helpful, https://github.com/logseq/datascript/blob/logseq/test-storage/src/datascript/storage.cljs is the source for datascript.storage and was copied into sci with sci/copy-ns

expected behavior

I expected to see the 2 unit tests pass like they do in cljs. Run the same test ns with cljs using: cd test/libraries/datascript && yarn install && clj -M:shadow-cljs:test compile test && node target/datascript.js

This repro could be more minimal if all the deftests and defns are removed from the test ns. I left them in order to see correct behavior for the tests

Sponsor - I think so. We/logseq sponsor babashka and indirectly via Clojurists Together

[borkdude]

Is it possible to make a smaller repro?

[borkdude]

@logseq-cldwalker Perhaps you're able to make a reproduction using the sci.configs playground where datascript is available:

https://babashka.org/sci.configs/

[logseq-cldwalker]

Sure. Taking a look shortly

[logseq-cldwalker]

@borkdude I created repro at https://github.com/logseq-cldwalker/sci.configs/tree/storage-test-failing-repro. When I copy in the defprotocol and eval in the playground, I get the same result:

To repro locally:

• Clone the branch https://github.com/logseq-cldwalker/sci.configs/tree/storage-test-failing-repro
• cd playground && bb build
• Copy in the following defprotocol into the playground to eval:

(defrecord Storage [*disk *reads *writes *deletes]
  d.storage/IStorage
  (-store [_ addr+data-seq]
    (doseq [[addr data] addr+data-seq]
      (vswap! *disk assoc addr (pr-str data))
      (vswap! *writes conj addr)))

  (-restore [_ addr]
    (vswap! *reads conj addr)
    (-> @*disk (get addr) edn/read-string)))

[borkdude]

I think (sci/copy-ns ...) just doesn't copy protocol vars by default. You can verify this by running (ns-publics 'datascript.storage) within the playground.

Another issue is that SCI protocols are implemented different (using multimethods) than protocols in CLJS.

[logseq-cldwalker]

I think (sci/copy-ns ...) just doesn't copy protocol vars by default.

I'm seeing IStorage and -storage vars in the output. Are you referring to some other var?

I see I'm able to load a reified version of the protocol e.g.:

(reify d.storage/IStorage
      (-store [_ addr+data-seq]
        (let [data (map
                    (fn [[addr data]]
                      #js {:$addr addr
                           :$content (pr-str data)})
                    addr+data-seq)]
          (identity data)))

      (-restore [_ addr]
        (identity addr)))

[borkdude]

Alright, I'll try locally

[borkdude]

There is something wonky going on with a :ns missing on the protocol. When I compensate for that in a local SCI, I eventually get:

ERROR: No protocol method IMultiFn.-add-method defined for type function: function datascript$storage$_store(_,addr_PLUS_data_seq){
if((((!((_ == null)))) && ((!((_.datascript$storage$IStorage$_store$arity$2 == null)))))){
return _.datascript$storage$IStorage$_store$arity$2(_,addr_PLUS_data_seq);
} else {
return datascript$storage$IStorage$_store$dyn_74803(_,addr_PLUS_data_seq);
}

but this just boils down to the problem stated before:

Another issue is that SCI protocols are implemented different (using multimethods) than protocols in CLJS.

Protocols in SCI are implemented using multimethods. You can see how this works in SCI itself for IDeref for example.

Related issue: #639

[borkdude]

@logseq-cldwalker I pushed a partial solution here:

https://github.com/babashka/sci.configs/tree/storage-test-failing-repro

The latest commit shows how to expose an existing protocol using a wrapper.

Note that any function that calls -store and -restore needs to be patched to be made aware of these functions.

A more proper solution would be to let SCI alter existing CLJS protocols, but perhaps this will work for you in the meanwhile?

I know that @phronmophobic has found a way to do this with CLJ protocols here: https://github.com/phronmophobic/scify
Perhaps a similar approach would work with SCI. Either: altering the host protocol to take the SCI protocol into account, or: letting SCI mutate the CLJS protocol directly by adding implementations to it.

[logseq-cldwalker]

Thanks for finding the cause and a workaround!
I'm able to see that code load and then see an expected failure when a parent fn calls a protocol method e.g. No protocol method IStorage.-store defined for type object with this playground code:

(defrecord Storage [*disk *reads *writes *deletes]
  d.storage/IStorage
  (-store [_ addr+data-seq]
    (doseq [[addr data] addr+data-seq]
      (vswap! *disk assoc addr (pr-str data))
      (vswap! *writes conj addr)))

  (-restore [_ addr]
    (vswap! *reads conj addr)
    (-> @*disk (get addr) edn/read-string)))

(defn make-storage [& [opts]]
  (map->Storage
   {:*disk    (volatile! {})
    :*reads   (volatile! [])
    :*writes  (volatile! [])
    :*deletes (volatile! [])}))
  
(let [db (d/empty-db)
      storage (make-storage)]
  (d/store db storage))

Note that any function that calls -store and -restore needs to be patched to be made aware of these functions

How would I do that?

A more proper solution would be to let SCI alter existing CLJS protocols, but perhaps this will work for you in the meanwhile?

This should work as we're running a datascript fork for now. Eventually we'd like to be contribute back our changes and use standard datascript

[borkdude]

I'll try it out and push a change.

[borkdude]

Pushed a patch upstream: https://github.com/babashka/sci.configs/tree/storage-test-failing-repro

Note that I patched datascript's -store and -restore functions to use the SCI version, which in turn calls the old definition.

[logseq-cldwalker]

Thanks! The last code snippet loads and works correctly. I'm also trying to use reify with this protocol and when I pass it to another datascript fn, I get ERROR: Cannot read properties of null (reading 'g') in the playground. Full snippet:

(def s1
  (reify d.storage/IStorage
      (-store [_ addr+data-seq]
        (let [data (map
                    (fn [[addr data]]
                      #js {:$addr addr
                           :$content (pr-str data)})
                    addr+data-seq)]
          (identity data)))

      (-restore [_ addr]
        (identity addr)))) 

(d/restore-conn s1)

Is there another workaround with protocols that I'd need to do?

A more proper solution would be to let SCI alter existing CLJS protocols, but perhaps this will work for you in the meanwhile?

I didn't realize the patch was just in sci. I take back what I said and think this could be a long term solution as the protocols don't change much and wouldn't effect an upstream contribution to datascript

[borkdude]

For reify another workaround is possible I believe. Let's see...

[borkdude]

Fixed

[borkdude]

For next issues, could you please paste a full repro including the ns form? I spent some time restoring a fully working example previous attempts so this would save me some time. Thanks in advance.

[logseq-cldwalker]

For next issues, could you please paste a full repro including the ns form?

Sure. Sorry. Will do

Datascript storage tests and nbb logseq tests passing with the last patch. Thanks! Happy to add some docs next week on protocol patching if it's helpful. I could inline examples or refer to the ones of the sci.configs branch

[borkdude]

The protocol patching relies on quite a bit of implementation details which perhaps can be made public.