taoensso / encore

Core utils library for Clojure/Script
https://www.taoensso.com/encore
Eclipse Public License 1.0
274 stars 52 forks source link

Truss aliases fail in CLJS #64

Closed p-himik closed 1 year ago

p-himik commented 1 year ago

The commit https://github.com/ptaoussanis/encore/commit/6d7d7af94e05e3322311252904d16f3edebc6391 attempts to introduce some aliases in CLJS. However, those don't allow for the CLJS code to be built:

------ ERROR -------------------------------------------------------------------
 File: jar:file:/home/p-himik/.m2/repository/com/taoensso/encore/3.35.0/encore-3.35.0.jar!/taoensso/encore.cljc:472:3
--------------------------------------------------------------------------------
 469 | ;;;; Truss aliases (for back compatibility, convenience)
 470 | 
 471 | (do
 472 |   (defalias taoensso.truss/have)
---------^----------------------------------------------------------------------
null
Unable to resolve var: have in this context at line 472 taoensso/encore.cljc
--------------------------------------------------------------------------------
 473 |   (defalias taoensso.truss/have!)
 474 |   (defalias taoensso.truss/have?)
 475 |   (defalias taoensso.truss/have!?)
 476 |   (defalias taoensso.truss/get-data)
--------------------------------------------------------------------------------

I suspect the reason is that in compiled CLJS, macros don't exist. They exist only in self-hosted CLJS. So you can't reference taoensso.truss/have or any other macro like that.

ptaoussanis commented 1 year ago

@p-himik Hi Eugene, thanks for pinging about this!

I've unfortunately been unable to reproduce the problem you're seeing. Could you please confirm the following?

Thanks!

p-himik commented 1 year ago

Ah, I completely misread the code, sorry. I can't reproduce it with just CLJS itself as well, but it's reproducible with shadow-cljs: https://github.com/p-himik/ptaoussanis-encore-issues-64

I've asked Thomas about it.

ptaoussanis commented 1 year ago

No worries at all, it's good to know if there's a potential issue.

I'm not so familiar with shadow-cljs, so let's see what Thomas says - otherwise I'll try debug using your example project. Will make sure we arrive at some solution 👍

Cheers! :-)

thheller commented 1 year ago

@p-himik your assessment is correct. Macros do not exist in CLJS, and this is the problem here.

The difference here is that shadow-cljs ignores defmacro when compiling .cljc files in CLJS mode. cljs.main does not. As such it compiles the macro as a regular function in CLJS. It'll never be called, but the var will exist.

I consider this an error, as it will produce invalid code that cannot ever be actually used. And in this case will even give you a callable taoensso.encore/have function, which can also never be used in a meaningful way.

@ptaoussanis also as an aside the CLJS variant of defalias macro is less than ideal. var in CLJS emits a whole bunch of boilerplate and just deref'ing it directly will leave all that code "alive" in :advanced. Leading to a lot of "bloat" and actually unused code. I would suggest just (def ~sym ~src), which will eliminate all that code, and yield the same result in the runtime.

ptaoussanis commented 1 year ago

@thheller Thanks for the insight Thomas, that's super helpful!

[...] cljs.main does not. As such it compiles the macro as a regular function in CLJS. It'll never be called, but the var will exist. I consider this an error, as it will produce invalid code that cannot ever be actually used. And in this case will even give you a callable taoensso.encore/have function, which can also never be used in a meaningful way.

This is really interesting. I'd never actually looked into this, and had just presumed that defmacros in .cljc files would be ignored when compiling in CLJS. That's a pretty fundamental misunderstanding I've had then. Do you have any idea why they'd be compiled as a function? Is there some utility in that?

[...] also as an aside the CLJS variant of defalias macro is less than ideal.

I appreciate the feedback on this! I've actually never been happy with the defalias behaviour in Cljs, since the intention/hope was to also duplicate metadata like docstrings, etc. - but I never managed to get that to work correctly, and didn't get around yet to investigating further.

Do you happen to have any idea off-hand if there'd be some way to do this successfully?

In the meantime, I'll definitely proceed with your suggested improvements - cheers!

ptaoussanis commented 1 year ago

@p-himik Hi Eugene, I just pushed [com.taoensso/encore "3.37.1"] to Clojars which removes all macros from ClojureScript - this should hopefully resolve your issue?

Thanks again for pinging about this, and apologies for the trouble!

p-himik commented 1 year ago

That seems to work just fine, thanks! And it was no trouble at all.

thheller commented 1 year ago

Metadata on vars doesn't really exist in the CLJS/JS runtime. It is only kept in the cljs.analyzer metadata "map". So, technically you can copy it out of there and copy it to the new var.


(defmacro defalias
  "Defines an alias for qualified source symbol, preserving its metadata (clj only):
    (defalias my-map-alias clojure.core/map)
  Cannot alias Cljs macros.
  Changes to source are not automatically applied to alias."
  ;; TODO Any way to reliably preserve cljs metadata? See #53, commit 2a63a29, etc.

  ([src] `(defalias ~(symbol (name src)) ~src nil))
  ([sym src] `(defalias ~sym ~src nil))
  ([sym src attrs]
   (let [attrs (if (string? attrs) {:doc attrs} attrs) ; Back compatibility
         link? (:link? attrs) ; Currently undocumented
         attrs (dissoc attrs :link?)]

     (if (:ns &env)
       (let [var (cljs.analyzer/resolve-var &env src)]
         `(def ~(vary-meta sym merge (:meta var)) ~src))
       `(let [attrs# (conj (-alias-meta (var ~src)) ~attrs)]
          (alter-meta! (def ~sym @(var ~src)) conj attrs#)
          (when ~link? (-link-var (var ~sym) (var ~src)))
          (var ~sym))))))

Something like that should do the trick. Although you may wanna add conditional requires or something, so you don't introduce a hard dependency on CLJS when using CLJ.

Given the limited usefulness of metadata in CLJS in the first place I doubt this is all worth it?

ptaoussanis commented 1 year ago

Thanks a lot for the example Thomas, that was very helpful! Cheers :-)