I attached the patch I've been trying to an "Outward function type hint propagation" JIRA: http://dev.clojure.org/jira/browse/CLJS-1997

interesting how small the patch is 🙂

Yes—it could be even smaller (but a little more aggressive) if it just dropped the inferred type directly into :ret-tag. But, yes—I was surprised the compiler is already arranged to support it.

The patch not only has potential perf benefits (eliding the truth check), but it could help catch more errors via type propagation:

cljs.user=> (defn f [] "h")
#'cljs.user/f
cljs.user=> (+ (f) 1)
            ^
WARNING: cljs.core/+, all arguments must be numbers, got [string number] instead. at line 1
"h1"

So it seems that Array.prototype.join already does work properly with nil + js/undefined:

(js* "[~{},~{},~{}].join('')" nil "foo" js/undefined)

I'd like to propose to change the str macro to:

(let [pre-cat (reduce
                      (fn [xs x]
                        ;; If this and the last is a string, we can pre-concat:
                        (if (and (string? x) (string? (last xs)))
                          (conj (pop xs) (str (last xs) x))
                          (conj xs x)))
                      []
                      args)]
        ;; If all that's left is a single string we can just emit that:
        (if (and (= 1 (count pre-cat)) (string? (first pre-cat)))
          (first pre-cat)
          (apply core/list 'js/goog.string.buildString pre-cat)))

That'd also allow people to just use (str "foo" "bar") to break up long string over multiple lines.

And use goog.string.buildString (which just calls the Array.prototype.join)

Similar for str function. There is

goog.string.makeSafe = function(obj) {
  return obj == null ? '' : String(obj);
};

@rauh there is a lot of history behind the str macro, see http://dev.clojure.org/jira/browse/CLJS-890

and FWIW the closure compiler will do the optimization you proposed (if I understand the intent correctly)

Oh boy, ok going through this ticket.

I don't see google closure compiler pre-concatenating strings unless I use js's +-operator

I did this recently http://dev.clojure.org/jira/browse/CLJS-1879 which let the closure compiler inline more str related things

it should inline ["foo", null, undefined].join("")] to just "foo"

Yeah I so that ticket. But strange that the current impl is faster than buildString. Their docstring says the opposite.

I did only this minimal change because I was afraid of the history of CLJS-890, but it provided most of the benefits

but FWIW that ticket is also ancient history and probably not relevant today

Ok I see, lots of edge cases 😕. Though: 1. The calls to the 1-arity version don't get optimized away even when passing a string literal 2. Why again .join("") call to concat the resulting strings when we already know that we have only strings. (So + should be safe to use, even for Safari)

Ie, why not:

(core/let [strs (core/->> (mapv (fn [x]
                                        (if (string? x)
                                          "~{}"
                                          "cljs.core.str.cljs$core$IFn$_invoke$arity$1(~{})"))
                                      xs)
                                (interpose "+")
                                (apply core/str))]
        (apply list 'js* (core/str "(" strs ")") xs))

GCC should concat the constants for us and we don't emit unnecessary calls to the arity$1 str function.

so if I understand correctly you want (str "foo" "bar") to emit ["foo", "bar"].join("") instead of [cljs.core.str.cljs$core$IFn$_invoke$arity$1("foo"), cljs.core.str.cljs$core$IFn$_invoke$arity$1("bar")].join("")

I'm fairly sure that closure will do that

No, note the +, I'm using + concat after we have guaranteed that we deal with strings

Nope, GCC def won't optimize the function calls away. At least not for me

eg the proposed would result in: (cljs.core.str.cljs$core$IFn$_invoke$arity$1(ns) + "/" + cljs.core.str.cljs$core$IFn$_invoke$arity$1(name))

https://closure-compiler.appspot.com/home#code%3D%252F%252F%2520%253D%253DClosureCompiler%253D%253D%250A%252F%252F%2520%2540compilation_level%2520ADVANCED_OPTIMIZATIONS%250A%252F%252F%2520%2540output_file_name%2520default.js%250A%252F%252F%2520%253D%253D%252FClosureCompiler%253D%253D%250A%250A%252F%252F%2520ADD%2520YOUR%2520CODE%2520HERE%250Afunction%2520hello(name)%2520%257B%250A%2520%2520alert('Hello%252C%2520'%2520%252B%2520name)%253B%250A%257D%250Ahello('New%2520user')%253B%250A%250A

The link doesn't work

https://closure-compiler.appspot.com/home#code%3D%252F%252F%2520%253D%253DClosureCompiler%253D%253D%250A%252F%252F%2520%2540compilation_level%2520ADVANCED_OPTIMIZATIONS%250A%252F%252F%2520%2540output_file_name%2520default.js%250A%252F%252F%2520%253D%253D%252FClosureCompiler%253D%253D%250A%250A%252F%252F%2520ADD%2520YOUR%2520CODE%2520HERE%250Afunction%2520str(x)%2520%257B%250A%2520%2520return%2520x%253B%250A%257D%250Aconsole.log(%255Bstr(%2522foo%2522)%252C%2520str(%2522bar%2522)%255D.join(%2522%2522))%253B%250A%250A

function str(x) {
  return x;
}
console.log([str("foo"), str("bar")].join(""));

produces console.log("foobar");

it optimized the functions calls and the join

Try:

function str(x) {
if((x == null)){
return "";
} else {
return [x].join("");
}
}
console.log([str("foo"), str("bar")].join(""));

ah the nil check

hmm

See the pseudo advanced compilation. It's littered with cljs.core.str.cljs$core$IFn$_invoke$arity$1("some string")

that should really be optimized by the macro then, I was sure closure took care of that

Yeah it should also be faster, since after the call we can do fast + concatenation.

I would stick to [].join("") it has basically the same performance

but avoids the valueOf history

But we don't need the join anymore, they're all guaranteed to be strings.

ah yeah

Well so silly question: Why do we need the nil check?

join will take care of that.

And then it'll also be optimized away:

`

function str(x) {
  return [x].join("");
}
console.log([str("foo"), str("bar")].join(""));

Works with nil and with old Safari.

some JS engine printed [null].join("") as "null"

Oh ok.

I still think we should stop worrying about Safari 6.0.5. Even more so now since 2+ years have passed

That safari is only on not-fully-updated OS X Mountain Lion

I have an ancient mountain lion mac and even it uses safari 6.1.x, which does not have that str bug

Agreed. This also indicates it can be safely dropped: http://caniuse.com/usage-table

@favila I don’t think that seems wise given iOS

changing the behavior of str is way low priority

Isn't that still, like iOS 6?

The only report we had was a desktop safari from a logged js stacktrace

@dnolen I don't propose changing the behavior. The macro change would result in smaller & faster code. So IMO a win win without any downside.

I just can’t see how this will matter

did you benchmark?

like across at least 3 engines?

@favila that doesn’t really tell us anything - we fixed it for Safari so we don’t get reports about older iOS

@dnolen I just tested it on ff+chrome. It looks like chrome prefers [].join(). So I'd say stick with array.join but I'd still vote for omitting the str$arity$1 call if it's already a string.

again it doesn’t seem important to me

@dnolen When I wrote CLJS-801 two years ago it was a dramatic difference, but the macro was a bit dumber then. (You also accepted it without comment)

churn where there is no measurable value isn’t something to spend time on

js engines may be better now, and emiting a direct str arity-1 call is an improvement

my goal then was to give closure more optimization chances