Am I mad or is this a bug in Clojurescript?
(do
(def foo nil)
(set! foo "value")
[foo
(str foo)
(subs foo 0)]
)
;=> ["value" "" "value"]
(str foo) returns empty string!hmm
cljs.user=> (str (fn [] (str foo)))
"function (){\nreturn (\"\");\n}"I'll have a look shortly
I got the right value, both in REPL and after compilation
Tried it again - same result
it might have to do with a string optimization: when (str ..) is compiled, it looks at the type of foo and if it's nil, you get an empty string - @schad.alexis did you try the newest CLJS?
but it should probably not make this assumption for global vars
yes ok nevermind, latest doesn't work
[foo,(""),cljs.core.subs.call(null,foo,(0))] got compiled to this
@dnolen I transferred the (typed-expr? &env x '#{clj-nil}) from the old implementation.
I think for global vars, no tag should be inferred since they can change? (see above).
https://github.com/clojure/clojurescript/commit/6cb2c4dfa971abd56b2b1acee5f1eb97fda9bac1#diff-0c7256dd86be433b0c77bfba825436d345fdbfc8cb975ff8d8a84d85d256c73bR844
can't load this up right now, but yes make a ticket and please collected relevant info there
I'll file a ticket tomorrow
I've been thinking about this one @dnolen. If you mark the var as dynamic, it will actually be correct.
cljs.user=> (def ^:dynamic foo nil)
#'cljs.user/foo
cljs.user=> (do (set! foo "foo") (str foo))
"foo"since dynamic vars gets analyzed with tag :any since they are expected to change.
but I guess in a REPL situation you can change vars with (def ..) as well and this actually also works correctly:
cljs.user=> (do (def foo "foo") (str foo))
"foo"so maybe it's just best practice to mark vars that you want to modify with set! as dynamic?
(similar to JVM Clojure)
I never use dynamic vars in Cljs since dynamic binding doesn't work well with async code. I use set! to for one-time initialization of global variables. Although this is the first report, I think this is a pretty big BC break.
I was actually working on a fix, while this popped into my mind. In JVM Clojure you can't use set! on vars, unless they are marked dynamic. The whole CLJS type inference machinery assumes vars aren't mutated with set! unless they are marked with :dynamic
let me give another example that would fail horribly right now.. (I expect it to but I haven't tested it...)
cljs.user=> (def x true)
#'cljs.user/x
cljs.user=> ((fn [] (set! x 0) (if x 1 2)))
2cljs.user=> (def ^:dynamic x true)
#'cljs.user/x
cljs.user=> ((fn [] (set! x 0) (if x 1 2)))
1So for consistency I think there are two directions:
• Turn off optimizations based on var tags everywhere in CLJS
• Warn on set! on var that is not marked dynamic
Compromise third solution: consider cljs.core stable (people won't mutate it with set! )
This is the approach I took when I was writing the fix for the above issue.
But even that won't hold when you use with-redefs of course
My in progress fix looked like this, but I think it's better wait for @dnolen to chime in on this first.
(core/let [ast (cljs.analyzer/no-warn (cljs.analyzer/analyze &env x))
op (:op ast)
tag (cljs.analyzer/infer-tag &env ast)
cljs? (core/when (= :var op)
(core/= 'cljs.core (:ns (:info ast))))
optimization-safe? (core/or (= :local op)
(= :const op)
cljs?)]
(core/cond
(core/and (core/= 'clj-nil tag)
optimization-safe?)
nil
(core/and (compatible? tag '#{boolean number
cljs.core/Keyword
cljs.core/Symbol})
optimization-safe?)
["+~{}" x]
:else
["+cljs.core.str.cljs$core$IFn$_invoke$arity$1(~{})" x]))Oh I guess we can throw local optimization out of the window too if we can't rely on type inference of user vars.
cljs.user=> (def foo nil)
#'cljs.user/foo
cljs.user=> (do (set! foo "foo") (let [foo foo] (str foo)))
:tag clj-nil
:opt-safe true
:compat false
""did you consider just throwing away the nil fast path? I don't know that just doesn't seem worth the trouble to dovetail into generalization work unless people are complaining about this is many contexts.
@dnolen sure, but that doesn't solve the fundamental problem that CLJS relies on tags inferred from vars in other places too
the whole feels very minor is all, not enough to get into a design discussion to address the immediate problem.
just drop the nil case, not that useful.
then a separate ticket about the more general issue.
The outcome of the more general issue affects the solution to this issue. But sure, I can do that.
the set! problem has existed forever yes, but it's not clear that many people are hitting it.
this one was reported because (as usual) a recent change
fix the recent change first, then worry about other stuff
👍
re: planning for next release probably only async/await
but we should resolve stuff like this before
(as for the more general problem: perhaps it's a good heuristic to only infer tags from defn, not from def which should be always any unless explicit tag is provided)
yeah, seems good
thanks!
but also please open a ticket about the more general problem - I am not disinterested, just don't want to queue that up right now
makes sense
@dnolen btw, you can close
https://clojure.atlassian.net/browse/CLJS-890#icft=CLJS-890
and https://clojure.atlassian.net/browse/CLJS-847 probably too.
t's already tested in test-cljs-3452
@dnolen fixed: https://clojure.atlassian.net/browse/CLJS-3472
general issue: https://clojure.atlassian.net/browse/CLJS-3474