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

RESOLVED FIXED in mozilla35

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: 446240525, Assigned: 446240525)

Tracking

(Blocks 1 bug, {dev-doc-complete})

Trunk
mozilla35
x86
macOS
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [DocArea=JS])

Attachments

(1 attachment, 4 obsolete attachments)

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
Component: General → JavaScript Engine
Posted patch bug1058396.diff (obsolete) — Splinter Review
Posted patch bug1058396 v2.diff (obsolete) — Splinter Review
Added testcase `String(Object(sym))`
Attachment #8478820 - Attachment is obsolete: true
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
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 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.
Attachment #8478870 - Flags: review?(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 {
Attachment #8478870 - Flags: review?(jorendorff)
Posted patch bug1058396 v3.patch (obsolete) — Splinter Review
Attachment #8478870 - Attachment is obsolete: true
Attachment #8481754 - Flags: review?(jorendorff)
Posted patch Removed the trailing spaces (obsolete) — Splinter Review
Attachment #8481754 - Attachment is obsolete: true
Attachment #8481754 - Flags: review?(jorendorff)
Attachment #8481755 - Flags: review?(jorendorff)
Attachment #8481755 - Attachment is obsolete: true
Attachment #8481755 - Flags: review?(jorendorff)
Attachment #8481758 - Flags: review?(jorendorff)
Keywords: dev-doc-needed
Whiteboard: [DocArea=JS]
Blocks: es6
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).
Attachment #8481758 - Flags: review?(jorendorff) → review+
https://hg.mozilla.org/mozilla-central/rev/3a7a4cb3a619
Assignee: nobody → 446240525
Status: UNCONFIRMED → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Thanks again for the patch!
You need to log in before you can comment on or make changes to this bug.