Skip to content

Fix ClojureScript protocol dispatch in Cherry#166

Merged
borkdude merged 1 commit intosquint-cljs:mainfrom
willcohen:protocol-externs
Nov 17, 2025
Merged

Fix ClojureScript protocol dispatch in Cherry#166
borkdude merged 1 commit intosquint-cljs:mainfrom
willcohen:protocol-externs

Conversation

@willcohen
Copy link
Copy Markdown
Contributor

@willcohen willcohen commented Nov 15, 2025

Adds externs file to preserve protocol names in runtime and updates compiler to generate full protocol names with cljs.core namespace prefix.

Changes:

  • externs/protocols.js: New file with all 54 ClojureScript protocols
  • shadow-cljs.edn: Add externs reference
  • src/cherry/internal/protocols.cljc: Expand core-protocols set and qualify unqualified protocols in proto-assign-impls

Fixes protocol dispatch between Cherry-compiled deftypes and ClojureScript runtime.

Please answer the following questions and leave the below in as part of your PR.

@willcohen willcohen marked this pull request as draft November 16, 2025 01:14
@willcohen
Copy link
Copy Markdown
Contributor Author

Tests are incorrect and don't conform to test suite. Need to redo.

@willcohen willcohen marked this pull request as ready for review November 16, 2025 16:01
@willcohen
Copy link
Copy Markdown
Contributor Author

Test suite reconforms.

@borkdude
Copy link
Copy Markdown
Member

Similar remark here as in the assert PR

@willcohen willcohen marked this pull request as draft November 17, 2025 17:04
@willcohen willcohen marked this pull request as ready for review November 17, 2025 17:25
@willcohen
Copy link
Copy Markdown
Contributor Author

Way simpler now. Just an externs file, adding it to shadow, and a new test under assert.

@willcohen
Copy link
Copy Markdown
Contributor Author

Branch now depends on #172 but changelog entry is in here as well.

@borkdude
Copy link
Copy Markdown
Member

Do we really need the big externs file? Why didn't I need them for ISwap etc before? Does the externs file get used by downstream users of cherry (I don't believe so?)

@willcohen
Copy link
Copy Markdown
Contributor Author

Hmm. Without the externs, the (correctly invoked) tests now fail at:

FAIL in (protocol-dispatch-test) (cherry/compiler_test.cljs:552:9)
ICounted protocol dispatch on custom deftype
expected: (= 42 (jsv! "(deftype TestBuffer []\n ICounted\n (-count [_] 42))\n(count (TestBuffer.))"))
actual: #object[Error Error: No protocol method ICounted.-count defined for type object: [object Object]]

@borkdude
Copy link
Copy Markdown
Member

Why does it work for ISwap etc then?

@willcohen
Copy link
Copy Markdown
Contributor Author

The test re-passes with the new extern file removed, with just an addition of ICounted to externs/cherry.txt: cljs$core$ICounted$_count$arity$1.

Will adjust to add the protocols in there

@willcohen
Copy link
Copy Markdown
Contributor Author

Okay -- I added as many user-testable protocols as I could figure out, with notes about all the ones I had to skip either because they were esoterically internal or had no methods.

I had to add protocol_mask$partition alongside the top-level cljs$core$IFoo$ to get the tests to work on about half of them.

Re deftype.cljc, that removal is just fixing a duplication of IPrintWithWriter in the list that I noticed when expanding core-protocols.

Comment thread src/cherry/internal/protocols.cljc Outdated
@willcohen
Copy link
Copy Markdown
Contributor Author

willcohen commented Nov 17, 2025

More broadly -- ICounted is the actual thing I'm hitting, but just slapping one additional protocol in felt like a bandaid, and since I saw the todo, I assumed that it'd be helpful to try to just go through all the protocols that are implementable. If you'd prefer for me to shrink it down to just ICounted and deal with the other ones later, that's totally appropriate too and I can shrink this down to just a few lines.

@borkdude borkdude merged commit 03011dc into squint-cljs:main Nov 17, 2025
2 checks passed
@willcohen willcohen deleted the protocol-externs branch November 18, 2025 01:14
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

Successfully merging this pull request may close these issues.

2 participants