I'm storing a collection of symbols in a set. The collection looks like this:

(def core-vars '#{_PLUS_, inc, ...})

#{+, inc, ..}

the problem in shadow-cljs was that it creates var cljs$cst$munged_symbol = new cljs.core.Symbol(null, "munged-symbol"); references and sometimes that ended up using the same name

so var cljs$cst$_EQ_ = new cljs.core.Symbol(null, "="); and var cljs$cst$_EQ_ = new cljs.core.Symbol(null, "_EQ_");

I guess optimize constants just does the same still

ah yes: https://github.com/clojure/clojurescript/blob/30171a3c423ee265e0aa814f2063ebb042931b75/src/main/clojure/cljs/compiler.cljc#L407-L411 I'll provide an issue + fix for it

A repro:

(ns test.project)

(def x '+)
(def y '_PLUS_)

(defn -main [& _args]
  (prn [x y])
  (prn (= x y)))

(set! *main-cli-fn* -main)

$clj -M -m cljs.main --target nodejs -O simple -c test.project
$ node .cljs_node_repl/main.js
[+ _PLUS_]
false
$ clj -M -m cljs.main --target nodejs -O advanced -c test.project
$ node .cljs_node_repl/main.js
[+ +]
true

@dnolen @thheller I've got a fix here which solves the problem in the above repro and I double-checked that the symbols are still properly shortened with advanced compilation: https://github.com/borkdude/clojurescript/commit/0ec3e6c9ead7930861f036fc24ccac9fd14be662

Re=new Qb(null,"_PLUS_","_PLUS_",-89880507,null)
Se=new Qb(null,"+","+",-740910886,null)

I would appreciate your feedback before I make a proper patch. Also I wonder if I should add a test for this or not, and if so, where/how. I could move the "index table" into the env instead of making it global, not sure if that's important.

I think that'll fail with caching? can't remember how or even if there is any cache though

a simpler option may be just md5 hashing the source name and using that as a variable name. no munging conflicts that way and :advanced will turn it into something more gzip friendly anyways

I remember proposing something like that to you too :) We could also do a hex64 encoding

it seems the compiler already uses stuff like this: (symbol (str "cljs.user.source$form$" (util/content-sha (pr-str src) 7))) in various places

(util/content-sha)

yeah only issue is that md5 or any hash is pretty hostile for gzipping, so less than ideal if :advanced is not used

dunno how likely conflicts are if you shorten to make the gzip impact smaller

@thheller how likely is it that people would use :optimize-constants without :advanced?

pretty sure its also enabled for :simple?

otherwise I see no reason why you'd disable it, shadow-cljs doesn't even allow you to disable

👍

Created the issue: https://clojure.atlassian.net/browse/CLJS-3401

I wonder if we could just use the original string length as the extra "index" since:

+ -> _PLUS_ ;;=> 1 (vs 6 munged) ==> cst$sym$1$_PLUS_ 
_PLUS_ ;;=> 6 (vs 6 munged) ==> cst$sym$6$_PLUS_

seems reasonable

I guess then we can't differentiate between +_PLUS_ and _PLUS_+

Hmm, maybe renaming _ to __ is enough:

'_PLUS_ => cst$sym$__PLUS__
'+ => cst$sym$_PLUS_

and renaming $ to $$:

'foo.bar => cst$sym$foo$bar
'foo$bar => cst$sym$foo$$bar

There aren't coming any other conflicts to my mind with that scheme so far

OK, attached a patch with that solution + tests