1058396 - String(aSymbol) now returns the symbol’s description in ES6 draft rev 27

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[ziyunfei]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_9_3) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/39.0.2135.0 Safari/537.36

Steps to reproduce:

es-discuss thread: http://esdiscuss.org/topic/string-symbol

js> String(Symbol("1"))
typein:4:0 TypeError: can't convert symbol to string  // ×! should be "Symbol(1)"
js> Symbol("1") + ""
typein:6:0 TypeError: can't convert symbol to string  // √, still throw

Comment 1

[ziyunfei]

Attachment: bug1058396.diff

Comment 2

[ziyunfei]

Attachment: bug1058396 v2.diff

Added testcase `String(Object(sym))`

Comment 3

[Till Schneidereit [:till]]

Comment on attachment 8478870 [details] [diff] [review]
bug1058396 v2.diff

Review of attachment 8478870 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for report and the patch. I think it makes more sense to fix this in js::ToStringSlow instead of js_String, though. If you look at line 4393 of jsstr.cpp[1], you can see that symbols are already handled there. You just have to change the way they're handled.

Also note that symbol->description() returns an atom, which is a string already. So I think all you have to do is replace the content of that block with `str = v.toSymbol()->description();`.

And finally, your patch is missing a bit of information we need to land it. Please check [2] for how to set up your machine and export the patch so we can land it.

Once you have that, please request a review. by editing the attachment's details and setting the "review" flag. Then, enter the email address of the person you want to ask for review. In this case, any core developer will be fine, so you can ask me if you want.

[1]: http://mxr.mozilla.org/mozilla-central/source/js/src/jsstr.cpp?rev=e8e520884505#4393
[2]: https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

Comment 4

[Jeff Walden [:Waldo]]

We can't change js::ToStringSlow, because that's designed to be the non-inlined kernel of JS::ToString, which we want to correspond exactly to the ToString operation in ES6, which continues to throw for symbols.  (Why exactly they're breaking the ToString <=> String correspondence is beyond me.)

Comment 5

[ziyunfei]

Comment on attachment 8478870 [details] [diff] [review]
bug1058396 v2.diff

Review of attachment 8478870 [details] [diff] [review]:
-----------------------------------------------------------------

Hi Jason, could you give me some comments on my patch.

Comment 6

[Jason Orendorff [:jorendorff]]

Comment on attachment 8478870 [details] [diff] [review]
bug1058396 v2.diff

Review of attachment 8478870 [details] [diff] [review]:
-----------------------------------------------------------------

Thank you for the patch! Please make the following changes and post an updated patch with a new review request.

::: js/src/jsstr.cpp
@@ +4211,4 @@
>  
>      RootedString str(cx);
>      if (args.length() > 0) {
> +        if(!args.isConstructing() && args[0].isSymbol()){

Style nit: add spaces after 'if' and before '{':

    if (...) {

@@ +4221,5 @@
> +                    return false;
> +            }
> +            if (!sb.append(')'))
> +                return false;
> +            str = sb.finishString();

Instead of copying and pasting code, please factor out this code and the identical code in SymbolObject::toString_impl into a new common function named js::SymbolDescriptiveString.

The new function should be declared inside the `namespace js` block in js/src/vm/Symbol.h, and the definition would go at the end of js/src/vm/Symbol.cpp.

@@ +4222,5 @@
> +            }
> +            if (!sb.append(')'))
> +                return false;
> +            str = sb.finishString();
> +        }else{

Style nit: Please add spaces here as well:

    } else {

Comment 7

[ziyunfei]

Attachment: bug1058396 v3.patch

Comment 8

[ziyunfei]

Attachment: Removed the trailing spaces

Comment 9

[ziyunfei]

Attachment: Added spaces after //

Comment 10

[Jason Orendorff [:jorendorff]]

Comment on attachment 8481758 [details] [diff] [review]
Added spaces after //

Review of attachment 8481758 [details] [diff] [review]:
-----------------------------------------------------------------

All right! r=me. I will fix the remaining nits and check it in.

Thanks for the patch!

::: js/src/jsstr.cpp
@@ +4215,5 @@
> +        if (!args.isConstructing() && args[0].isSymbol()) {
> +            return js::SymbolDescriptiveString(cx, args, args[0].toSymbol());
> +        } else {
> +            str = ToString<CanGC>(cx, args[0]);
> +        }

Style micro-nit: For check-in, I'm going to change this to not have an "else" after that return statement:

    if (!args.isConstructing() && args[0].isSymbol())
        return js::SymbolDescriptiveString(cx, args, args[0].toSymbol());

    str = ToString<CanGC>(cx, args[0]);

Else-after-return is strongly banned throughout the mozilla-central codebase. Not sure why.

::: js/src/vm/Symbol.cpp
@@ +113,5 @@
>      }
>  }
> +
> +bool
> +js::SymbolDescriptiveString(JSContext *cx, CallArgs args, JS::Symbol *sym)

I'm also going to change this from CallArgs to MutableHandleValue, and put it last (out parameters typically come last in SM style).

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3a7a4cb3a619

Comment 12

[Florian Scholz (Open Web Docs)]

Docs:
https://developer.mozilla.org/en-US/Firefox/Releases/35#JavaScript
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Symbol#Symbol_type_conversions

Thanks ziyunfei!

Comment 13

[Jason Orendorff [:jorendorff]]

Thanks again for the patch!