Incorrect behaviour of (str obj) when obj has valueOf method

Question

Example

(str #js {"toString" (fn [] "hello") "valueOf" (fn [] 42)}) ; => "42"

The problem in the fact that ClojureScript uses concatenation to convert values to strings and that doesn't work well with objects which have valueOf() method overriden.

Example in js:

`
var obj = {

toString: function() { return 'hello'; },
valueOf: function() { return 42; }

};
console.log(String(obj)); => 'hello'
console.log(obj.toString()); => 'hello'
console.log('' + obj); => '42'
`

Potential solution might be to use String() function. Using toString() won't work as described in this issue: http://dev.clojure.org/jira/browse/CLJS-847

Answer 1

Comment made by: neatonk

Is there a valid use case where toString and valueOf are not in sync? E.g.

`(not= (.toString x) (js/String (.valueOf x))`

If not, is it "incorrect" for the two methods to be out of sync?

Answer 2

Comment made by: nbeloglazov

Here is an example of such use case: https://github.com/processing-js/processing-js/blob/master/src/Objects/Char.js
That's how I found this bug.

Answer 3

Comment made by: neatonk

Thanks for the link. I see what you mean.

Answer 4

Comment made by: dnolen

The problem with going with {{String}} appears to be a massive performance hit to printing http://jsperf.com/string-vs-tostring2/6.

Unless a brilliant idea is proposed this seems best solved / worked around in user code.

Answer 5

Comment made by: nbeloglazov

Append performs better on strings and numbers, but it performs worse on objects so it is not a clear performance hit. If I heavily work with objects and use (str) to convert them into strings then I actually lose on performance with current implementation.
Anyway current implementation of str is incorrect as it doesn't honor toString method. And this is what str function supposed to do. I believe a compiler should be correct first and then worry about performance.

Answer 6

Comment made by: dnolen

Sorry going back over this I believe the issue is really that we simply need to backout CLJS-801.

Answer 7

Comment made by: dnolen

reverted CLJS-801 in master

Answer 8

Comment made by: favila

CLJS-801 only deals with the str macro. Aren't we still going to have str function problem because of CLJS-847? https://github.com/clojure/clojurescript/commit/08b4b1585cf0ef739e903985ee4c6b7fc6c47642 (also Yet if we use toString there, Safari 6.0.5 blows up. Maybe we need {{[o].join('')}}? Depending on where the bug is this may be wrong in Safari 6.0.5 too.

What we need to do very specifically is somehow get the return value of the (in ECMASCRIPT-ese) {{ToString}} abstract operation on the object (or the underlying {{ToPrimitive}} abstract operation with the String hint). String concat with the add operator

Options as I see it are:

• {{x.toString()}} : Bad because of CLJS-847
• {{[x].join('')}} : Should work (and does right thing for null/undefined), but I think we should test in Safari 6.0.5. Also very slow.
• {{String(x)}}
• {{String.prototype.concat(x)}}
• {{String.prototype.slice(x,0)}} {{String.prototype.substring(x,0)}} {{String.prototype.substr(x, 0)}}
• {{x.toString()}} normally, but {{String(x)}} if we detect that we'll trigger CLJS-847. (Can specialize on startup.)

Answer 9

Comment made by: dnolen

Is there any evidence that higher usage of str is actually problematic?

Answer 10

Comment made by: favila

String concat using the addition operator uses an un-hinted {{ToPrimitive}} abstract call (which will try {{x.valueOf()}} first then {{x.toString()}}, usually) and then {{ToString}}s the result of that, so it's not an option unless we are concating primitive values.

Details:
ToString http://www.ecma-international.org/ecma-262/5.1/#sec-9.8
ToPrimitive http://www.ecma-international.org/ecma-262/5.1/#sec-9.1
* DefaultValue (called by ToPrimitive for Objects) http://www.ecma-international.org/ecma-262/5.1/#sec-8.12.8

Answer 11

Comment made by: dnolen

I'm not really all that concerned about the specification, only if it matters in the wild. If this doesn't affect Safari 6.05 we don't care.

Answer 12

Comment made by: favila

bq. Is there any evidence that higher usage of str is actually problematic?

Kevin Neaton, who opened CLJS-847, was using a patch in production which only addressed the higher order case and he said the patch fixed the issue for them. He was unaffected by the str macro case because it either used ''+x already (with CLJS-801 applied) or it used {{[x].join('')}} (which hasn't been tested with Safari 6.0.5 yet, but probably works).

So if we had a problem using ''+x with the str macro, we will certainly have a problem with ''+x with a string function as long as CLJS-847 is applied.

I haven't pulled down master yet, but here is a test case which I bet will fail with the CLJS-847 patch:

(def tricky-obj #js {"toString" (fn [] "hello") "valueOf" (fn [] 42)}) (assert (= (apply str tricky-obj) "hello")) ;; will get "42"

Answer 13

Comment made by: favila

bq. I'm not really all that concerned about the specification, only if it matters in the wild. If this doesn't affect Safari 6.05 we don't care.

To be clear, there are two issues here:

CLJS-847: x.toString() fails on Safari 6.0.5. Workaround is ''+x (only done in str macro case).
CLJS-890: ''+x doesn't give expected results for objects which define valueOf. Expectation is that x.toString() is called, instead x.valueOf().toString(). Fix is to use array join instead of string concat in str macro, but it doesn't address the ''+x workaround from CLJS-847.

To make matters worse, it looks like the toString() error on Safari may only be triggered at certain JIT levels!

Answer 14

Comment made by: favila

bq. Workaround is ''+x (only done in str macro case).

I mean "Workaround is ''+x (only done in str function case)."

Answer 15

Comment made by: nbeloglazov

Can this bug be reopened meanwhile? If I understand correctly the fix should affect https://github.com/clojure/clojurescript/commit/08b4b1585cf0ef739e903985ee4c6b7fc6c47642 but this code still present in HEAD.