Closed Bug 1472170 Opened 6 years ago Closed 6 years ago

Implement Symbol.prototype.description proposal

Categories

(Core :: JavaScript: Standard Library, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: anba, Assigned: omersid)

References

(Blocks 1 open bug, )

Details

(Keywords: dev-doc-complete, good-first-bug)

Attachments

(1 file, 2 obsolete files)

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
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
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)
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)
Attachment #9003842 - Flags: review?(andrebargull)
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)
Ok, I'll fix it up. Thanks for the helpful comments and pointers!
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)
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.
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+
Assignee: nobody → omersid
Status: NEW → ASSIGNED
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)
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+
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
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: