Implement Symbol.prototype.description proposal

RESOLVED FIXED in Firefox 63

Status

()

RESOLVED FIXED
5 months ago
2 months ago

People

(Reporter: anba, Assigned: omersid)

Tracking

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

Trunk
mozilla63
dev-doc-complete, good-first-bug
Points:
---

Firefox Tracking Flags

(firefox63 fixed)

Details

(URL)

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

5 months ago
The Symbol.prototype.description proposal was moved to stage 3 in the May meeting [1]. Test262 tests are already present [2], but we first need to update our local test262 copy to have them available.

I'm setting the "good-first-bug" label, because the proposal seems to be relatively easy to implement (only a single accessor property needs to be added to Symbol.prototype). 


[1] https://github.com/tc39/tc39-notes/blob/master/es9/2018-05/may-22.md#symbolprototypedescription-for-stage-3
[2] https://github.com/tc39/test262/tree/master/test/built-ins/Symbol/prototype/description
(Reporter)

Comment 1

5 months ago
Symbol.prototype methods are declared here [1] and then implemented in the corresponding cpp-file js/src/builtin/Symbol.cpp. The implementation is added to the Symbol.prototype object by adding it to the (currently empty) |SymbolObject::properties| field [2]. For example the DataView.prototype.buffer accessor property shows how to use |JSPropertySpec|s [3,4].


[1] https://searchfox.org/mozilla-central/rev/d2966246905102b36ef5221b0e3cbccf7ea15a86/js/src/builtin/Symbol.h#50-55
[2] https://searchfox.org/mozilla-central/rev/d2966246905102b36ef5221b0e3cbccf7ea15a86/js/src/builtin/Symbol.cpp#33-35
[3] https://searchfox.org/mozilla-central/rev/d2966246905102b36ef5221b0e3cbccf7ea15a86/js/src/builtin/DataViewObject.cpp#836-849
[4] https://searchfox.org/mozilla-central/rev/d2966246905102b36ef5221b0e3cbccf7ea15a86/js/src/builtin/DataViewObject.cpp#963
Keywords: dev-doc-needed
(Assignee)

Comment 2

3 months ago
This seems like an easy change. The tests seem to be currently available in the local test262 copy. From what I understand, JS_PSG("description", toString, 0) needs to be added as a SymbolObject property in Symbol.cpp. Am I understanding correctly that this is all that needs to be done? If so, I can submit the patch.
Flags: needinfo?(andrebargull)
(Reporter)

Comment 3

3 months ago
Yes, that's almost all that's needed here. Except we can't reuse the existing |SymbolObject::toString| method, but instead need to create a new |SymbolObject::description| method (and its companion method |SymbolObject::description_impl|), because |toString| returns the 'descriptive-string' [1]. For example for |Symbol("my-string-description")|, |SymbolObject::toString| returns "Symbol(my-string-description)", whereas the new 'description' getter is supposed to just return "my-string-description". And when the Symbol was created without a description |Symbol()|, the 'description' getter needs to return |undefined|.

[1] https://searchfox.org/mozilla-central/rev/ca869724246f4230b272ed1c8b9944596e80d920/js/src/builtin/Symbol.cpp#192
Flags: needinfo?(andrebargull)
(Assignee)

Comment 4

3 months ago
Created attachment 9003842 [details] [diff] [review]
Modifies Symbol.h, Symbol.cpp to add description property
Attachment #9003842 - Flags: review?(andrebargull)
(Reporter)

Comment 5

3 months ago
Comment on attachment 9003842 [details] [diff] [review]
Modifies Symbol.h, Symbol.cpp to add description property

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

Looks almost complete! \o/

There only some minor style nits which need to be fixed and the return value when no description is present needs to be changed to `undefined`. When both issues are corrected, please upload a new patch and re-request review.


Actually there's another thing which needs to be done for this bug, namely we want to enable the tests from the ECMAScript conformance test suite <https://github.com/tc39/test262/> for `Symbol.prototype.description`. The tests are already present in the source tree, but they're currently disabled. To enable them this line [1] in the test262 update script needs to be removed and then the update script needs to be executed with:

  ~/hg/mozilla-inbound/js/src/tests$ ./test262-update.py --revision=ab436c465106be86719c4849c9cedecd7b570ff9

where ab436c465106be86719c4849c9cedecd7b570ff9 is the currently checked out revision [2]. (Or if you don't want to run the update script or if there are any problems when executing the script, it's also possible to manually edit the test files: In this directory [3], remove the line "// |reftest| skip -- Symbol.prototype.description is not supported" [4] in each file where it is present.)

[1] https://searchfox.org/mozilla-central/rev/c45b9755593ce6cc91f558b21e544f5165752de7/js/src/tests/test262-update.py#33
[2] https://searchfox.org/mozilla-central/source/js/src/tests/test262/GIT-INFO#1
[3] https://searchfox.org/mozilla-central/source/js/src/tests/test262/built-ins/Symbol/prototype/description
[4] https://searchfox.org/mozilla-central/rev/c45b9755593ce6cc91f558b21e544f5165752de7/js/src/tests/test262/built-ins/Symbol/prototype/description/description-symboldescriptivestring.js#1

::: js/src/builtin/Symbol.cpp
@@ +234,5 @@
>  
> +bool
> +SymbolObject::descriptionGetter_impl(JSContext* cx, const CallArgs& args)
> +{
> +    //Get symbol object pointer   

Nit: Please add a space character after `//`, end the sentence with a full stop `.`, and remove the trailing whitespace.

@@ +248,5 @@
> +    if(str){
> +        if(!sb.append(str))
> +            return false;    
> +    }else{
> +        if(!sb.append("undefined"))

According to the spec proposal, the `[[Description]]` field of the symbol should be return directly, which means in SpiderMonkey when `sym->description()` is the null-pointer, we need to return the JavaScript `undefined` value instead of the string value `"undefined"`.

And you can also directly return |sym->description()| instead of creating a StringBuffer, so these lines can be reduced to:

```
// Return the symbol's description if present, otherwise return undefined.
if (JSString* str = sym->description())
    args.rval().setString(str);
else
    args.rval().setUndefined();

return true;
```

@@ +259,5 @@
> +    args.rval().setString(str);
> +    return true;
> +}
> +
> +bool 

Nit: Please remove the trailing whitespace here.
Attachment #9003842 - Flags: review?(andrebargull)
(Assignee)

Comment 6

3 months ago
Ok, I'll fix it up. Thanks for the helpful comments and pointers!
(Assignee)

Comment 7

3 months ago
Created attachment 9004024 [details] [diff] [review]
Modifies Symbol.h, Symbol.cpp to add description property and update tests.

I fixed Symbol.cpp and updated the tests. I wasn't sure if it should all be in one patch or separate for the code and for the tests. Let me know if you see anything else that needs to be fixed or if I should separate it into two patches.
Attachment #9003842 - Attachment is obsolete: true
Attachment #9004024 - Flags: review?(andrebargull)
(Assignee)

Updated

3 months ago
Attachment #9004024 - Attachment description: Modifies Symbol.h, Symbol.cpp to add description property and update tests → Modifies Symbol.h, Symbol.cpp to add description property and update tests.
(Reporter)

Comment 8

3 months ago
Comment on attachment 9004024 [details] [diff] [review]
Modifies Symbol.h, Symbol.cpp to add description property and update tests.

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

(In reply to omersid from comment #7)
> I fixed Symbol.cpp and updated the tests. I wasn't sure if it should all be
> in one patch or separate for the code and for the tests. Let me know if you
> see anything else that needs to be fixed or if I should separate it into two
> patches.

It's okay to keep all changes in a single patch, because the test changes are relatively small, so in my opinion it's not necessary to split them off. There are only two small issues which need to be fixed and the commit message needs to be updated to include the reviewer [1], i.e. "r=anba" needs to added.

I've already r+'ed the patch, because the two remaining issues are only small nits which don't affect the overall review. And I've also pushed the patch to the try-server [2] to ensure all tests pass when the new property is added to `Symbol.prototype`. If the try-server push comes back green and the updated patch was uploaded, the patch can be marked as ready to be checked into the Firefox source repository. 

[1] https://developer.mozilla.org/en-US/docs/Mercurial/Using_Mercurial#Commit_Message_Conventions
[2] https://treeherder.mozilla.org/#/jobs?repo=try&revision=a3b4ef01b74a1eed429ecd8199029910bf4245f9

::: js/src/builtin/Symbol.cpp
@@ +249,5 @@
> +        args.rval().setUndefined();
> +    return true;
> +}
> +
> +bool 

Please remove the trailing whitespace after 'bool'.

::: js/src/builtin/Symbol.h
@@ +53,5 @@
>      static MOZ_MUST_USE bool valueOf_impl(JSContext* cx, const CallArgs& args);
>      static MOZ_MUST_USE bool valueOf(JSContext* cx, unsigned argc, Value* vp);
>      static MOZ_MUST_USE bool toPrimitive(JSContext* cx, unsigned argc, Value* vp);
>  
> +    // Properties defined on Symbol.protoype.

Nit: Typo in 'protoype' -> 'prototype'
Attachment #9004024 - Flags: review?(andrebargull) → review+
(Reporter)

Updated

3 months ago
Assignee: nobody → omersid
Status: NEW → ASSIGNED
(Assignee)

Comment 9

3 months ago
Created attachment 9004451 [details] [diff] [review]
Modifies Symbol.h, Symbol.cpp to add description property and update tests.

My bad for not noticing those things the last time through- they have been fixed.
Attachment #9004024 - Attachment is obsolete: true
Attachment #9004451 - Flags: review?(andrebargull)
(Reporter)

Comment 10

3 months ago
Comment on attachment 9004451 [details] [diff] [review]
Modifies Symbol.h, Symbol.cpp to add description property and update tests.

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

Looks great! Thank you! :-)
Attachment #9004451 - Flags: review?(andrebargull) → review+

Comment 12

3 months ago
Pushed by dluca@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/709590a1a24f
Added description as a property for Symbol and updated tests262. r=anba
Keywords: checkin-needed

Comment 13

3 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/709590a1a24f
Status: ASSIGNED → RESOLVED
Last Resolved: 3 months ago
status-firefox63: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in before you can comment on or make changes to this bug.