Closed Bug 918828 Opened 11 years ago Closed 10 years ago

Use @@iterator symbol instead of placeholder string

Categories

(Core :: JavaScript Engine, defect)

Product:
Core
Component:
JavaScript Engine
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla36

People

(Reporter: wingo, Assigned: jorendorff)

References

Details

(Keywords: addon-compat, dev-doc-complete, site-compat, Whiteboard: [DocArea=JS] {done: true})

Attachments

(3 files, 5 obsolete files)

bug-918828-part-3-decompiler-v2.patch
(deleted), patch
Waldo
: review+
Details | Diff | Splinter Review
bug-918828-part-4-baseline-v2.patch
(deleted), patch
nbp
: review+
Details | Diff | Splinter Review
part 1 - Change iteration code to call iterable[Symbol.iterator]() rather than iterable["@@iterator"](). try: -b d -p linux64,linux64-st-an,macosx64 -u all[10.8,x64] -t none
(deleted), patch
Waldo
: review+
Details | Diff | Splinter Review
Once symbols are implemented, the @@iterator key currently being implemented by a hard-to-type string should change to be a symbol. Specific places can be found by grepping the source tree for @@iterator and also for this bug number. Don't forget dom/bindings/Codegen.py!
Assignee: general → jorendorff
Attached patch WIP 1 (obsolete) (deleted) — — Splinter Review
Work in progress. This breaks an xpcshell test somewhere (I forget which) but I think it should be straightforward to debug. This works by adding a JSOP_SYMBOL bytecode to push Symbol.iterator to the operand stack. The method call at the top of a for-of look now looks like this: symbol 0 ;; obj Symbol.iterator callelem ;; obj obj[Symbol.iterator] swap ;; obj[Symbol.iterator] obj call 0 ;; iterator WIP 1 implements JSOP_SYMBOL in the interpreter only. I'll also patch Baseline and Ion in this bug, so we don't regress for-of. I'm not 100% sure of the approach though. It'll be fine in the interpreter and in Ion. I'm ignorant about our Baseline GetElem/CallElem IC. Need to read the code.
Keywords: dev-doc-needed
Whiteboard: [DocArea=JS]
Would be nice to have this fixed for Moz33, i.e. on the same release as symbols :)
OS: Linux → All
Hardware: x86_64 → All
My plan here is to have a JSOP_SYMBOL instruction that just pushes a well-known symbol, so that for (x of y) will do something like getlocal y symbol 0 /*==SymbolCode::iterator*/ callelem call 0 ... Filed bug 1038859 to make that JSOP_CALLELEM fast.
Attached patch bug-918828-part-1-jsapi-v1.patch (obsolete) (deleted) — — Splinter Review
Part 1 of 2: Switch the whole JSAPI so that "@@iterator" means the well-known symbol and not the string "@@iterator". An alternative would be: change only the APIs that involve JSPropertySpec/JSFunctionSpec, and have callers use a special macro (JS_FS_SYMBOL or whatever) rather than magically recognizing strings starting with "@@". I'm happy with either one. Your call.
Attachment #8437151 - Attachment is obsolete: true
Attachment #8459263 - Flags: review?(jwalden+bmo)
Attached patch bug-918828-part-2-iteration-v1.patch (obsolete) (deleted) — — Splinter Review
Part 2: Change all iteration (for-of loop and everything else) to use the well-known symbol rather than "@@iterator". Part 1 changes the JSAPI, which takes care of many *providers* of iterator methods; part 2 changes the remaining @@iterator method providers (the ones written in JS) and all consumers of iterator methods. (Part 1 alone would break stuff that is fixed by part 2, so the two parts need to land together. In fact I'll squash them into a single changeset when I land.)
Attachment #8459264 - Flags: review?(nicolas.b.pierron)
So with these patches, JS_GetProperty and JS_GetPropertyById (with the id produced by manually interning a string) will have different behavior if the string happens to start with "@@", right? That seems like a pretty backwards-incompatible change. The fact that the binding InitIds() needed to be changed similarly is worrysome: right now I'm pretty sure the strings coming through there have to be IDL identifiers, but people keep asking to enlarge the value space there to all string property names. As an API consumer, I'd vastly prefer changing on JSProperty/FunctionSpec with explicit "use a symbol" bits, plus ways to get the jsid for a symbol than I can then use as needed.
Comment on attachment 8459264 [details] [diff] [review] bug-918828-part-2-iteration-v1.patch Review of attachment 8459264 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/frontend/BytecodeEmitter.cpp @@ +4436,5 @@ > if (Emit1(cx, bce, JSOP_DUP) < 0) // OBJ OBJ > return false; > + if (Emit2(cx, bce, JSOP_SYMBOL, jsbytecode(JS::SymbolCode::iterator)) < 0) // OBJ OBJ @@ITERATOR > + return false; > + if (!EmitElemOpBase(cx, bce, JSOP_CALLELEM)) // FN OBJ I am not sure to understand the comment about the stack layout. I would expect: OBJ JSOP_DUP OBJ OBJ JSOP_SYMBOL OBJ OBJ @@ITERATOR JSOP_CALLELEM > OBJ FN < JSOP_CALL Knowing that JSOP_CALLELEM pops the 2 first and push back its result on the stack. Why should we have > FN OBJ < instead? ::: js/src/jsapi.cpp @@ +2161,5 @@ > > JSAtom *atom; > if (name[0] != '@' || name[1] != '@') { > atom = Atomize(cx, name, strlen(name)); > } else if (strcmp(name, "@@iterator") == 0) { (CharsToId) Isn't this case going to be deprecated, should we output a warning and open a bug for removing this case? @@ +5711,5 @@ > return symbol->code(); > } > > JS_PUBLIC_API(JS::Symbol *) > JS::GetWellKnownSymbol(JSContext *cx, JS::SymbolCode which) note: This cx can be a ThreadSafeContext.
Comment on attachment 8459263 [details] [diff] [review] bug-918828-part-1-jsapi-v1.patch Review of attachment 8459263 [details] [diff] [review]: ----------------------------------------------------------------- Retracting this. bz is right, it's the wrong approach for sure.
Attachment #8459263 - Flags: review?(jwalden+bmo)
Attachment #8459264 - Flags: review?(nicolas.b.pierron)
Attached patch bug-918828-part-1-jsapi-v2.patch (obsolete) (deleted) — — Splinter Review
Attachment #8459263 - Attachment is obsolete: true
Attachment #8486771 - Flags: review?(jwalden+bmo)
(In reply to Nicolas B. Pierron [:nbp] from comment #8) > ::: js/src/frontend/BytecodeEmitter.cpp > @@ +4436,5 @@ > > if (Emit1(cx, bce, JSOP_DUP) < 0) // OBJ OBJ > > return false; > > + if (Emit2(cx, bce, JSOP_SYMBOL, jsbytecode(JS::SymbolCode::iterator)) < 0) // OBJ OBJ @@ITERATOR > > + return false; > > + if (!EmitElemOpBase(cx, bce, JSOP_CALLELEM)) // FN OBJ > > I am not sure to understand the comment about the stack layout. I would > expect: > > OBJ JSOP_DUP > OBJ OBJ JSOP_SYMBOL > OBJ OBJ @@ITERATOR JSOP_CALLELEM > > OBJ FN < JSOP_CALL > > Knowing that JSOP_CALLELEM pops the 2 first and push back its result on the > stack. Why should we have > FN OBJ < instead? EmitElemOpBase emits two instructions: JSOP_CALLELEM followed by JSOP_SWAP.
Attached patch bug-918828-part-2-iteration-v2.patch (obsolete) (deleted) — — Splinter Review
Attachment #8459264 - Attachment is obsolete: true
Attachment #8486772 - Flags: review?(nicolas.b.pierron)
Attached patch bug-918828-part-3-decompiler-v2.patch (deleted) — — Splinter Review
Attachment #8486793 - Flags: review?(jwalden+bmo)
Attached patch bug-918828-part-4-baseline-v2.patch (deleted) — — Splinter Review
Attachment #8486794 - Flags: review?(nicolas.b.pierron)
Is this intended to be resolved in Fx 33? Currently, Fx 33 Beta defines `Symbol.iterator`, but defining a method named [Symbol.iterator] on an object doesn't make it iterable, which is problematic. So, in case this bug is not resolved for version 33, `Symbol.iterator` should be removed.
Comment on attachment 8486772 [details] [diff] [review] bug-918828-part-2-iteration-v2.patch Review of attachment 8486772 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/customizableui/CustomizableUI.jsm @@ -2663,3 @@ > */ > - windows: { > - "@@iterator": function*() { nit: Any reason to not replace it by windows: { [Symbol.iterator]: function*() { … } }, ? @@ +3497,5 @@ > }, > }; > + > +// Add CustomizableUI.windows[@@iterator] method > +this.CustomizableUI.windows[Symbol.iterator] = function*() { instead of moving it out-side? ::: js/src/vm/PIC.cpp @@ +43,5 @@ > // Shortcut returns below means Array for-of will never be optimizable, > // do set disabled_ now, and clear it later when we succeed. > disabled_ = true; > > + // Look up Array.prototype[@@iterator], ensure it's a slotful shape. nit: Replace @@iterator by Symbol.iterator in vm/PIC.{cpp,h}
Attachment #8486772 - Flags: review?(nicolas.b.pierron) → review+
Attachment #8486794 - Flags: review?(nicolas.b.pierron) → review+
(In reply to Claude Pache from comment #15) > Is this intended to be resolved in Fx 33? No. (But read on.) > Currently, Fx 33 Beta defines `Symbol.iterator`, but defining a method named > [Symbol.iterator] on an object doesn't make it iterable, which is > problematic. So, in case this bug is not resolved for version 33, > `Symbol.iterator` should be removed. Agreed. We've already disabled symbols in the mozilla-beta repository (see bug 1041631). If they're enabled in the current FF33 beta, that just means we shipped it before that patch landed; the next beta drop will be right (building now locally, to make totally sure this is the case).
> (building now locally, to make totally sure this is the case). Confirmed.
(In reply to Nicolas B. Pierron [:nbp] from comment #16) > [...] instead of moving it out-side? Much better! I wrote the patch before computed property names and method syntax landed. With the suggested change, the change is a nice improvement: windows: { - "@@iterator": function*() { + *[Symbol.iterator]() { ... } }, > nit: Replace @@iterator by Symbol.iterator in vm/PIC.{cpp,h} I think @@iterator is still a little better. That's how the ES6 specification refers to this symbol.
Comment on attachment 8486771 [details] [diff] [review] bug-918828-part-1-jsapi-v2.patch Review of attachment 8486771 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jsapi.cpp @@ +3020,5 @@ > RootedAtom getterNameAtom(cx, Atomize(cx, getterName, strlen(getterName))); > if (!getterNameAtom) > return false; > > + RootedAtom name(cx, IdToFunctionName(cx, id)); Seems like MOZ_ASSERT(!JSID_IS_SYMBOL(id), "self-hosted property names must not be symbols") is reasonable here. I don't see a good reason to allow self-hosted names to be symbols, and much clarity reason to forbid it. @@ +4118,5 @@ > > RootedAtom shName(cx, Atomize(cx, fs->selfHostedName, strlen(fs->selfHostedName))); > if (!shName) > return false; > + RootedAtom name(cx, IdToFunctionName(cx, id)); Same MOZ_ASSERT(!JSID_IS_SYMBOL(id)) here. ::: js/src/jsapi.h @@ +4465,5 @@ > +/* > + * Return true if the given JSPropertySpec::name or JSFunctionSpec::name value > + * is actually a symbol code and not a string. See JS_SYM_FN. > + */ > +inline bool PropertySpecNameIsSymbol(const char *name) { inline bool PropertySpecNameIsSymbol(const char *name) { ::: js/src/jsfun.cpp @@ +2072,5 @@ > + * Function names are always strings. If id is the well-known @@iterator > + * symbol, this returns "[Symbol.iterator]". > + */ > +JSAtom * > +js::IdToFunctionName(JSContext *cx, HandleId id) This method kind of shouldn't exist, right? Looks like var obj = { [Symbol.iterator]() { } }; produces an obj[Symbol.iterator].name === Symbol.iterator. But in the short run we can't do that easily, so this deviation is acceptable as a temporary matter. ::: js/src/jsfun.h @@ +488,5 @@ > return kind; > } > }; > > +JSString * I get that |extern| is the default and all, but why remove it? EIBTI, seems to me. I'd rather we consistently used it in headers and prohibited it outside of them, myself.
Attachment #8486771 - Flags: review?(jwalden+bmo) → review+
Attachment #8486793 - Flags: review?(jwalden+bmo) → review+
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #20) > > + RootedAtom name(cx, IdToFunctionName(cx, id)); > > Seems like MOZ_ASSERT(!JSID_IS_SYMBOL(id), "self-hosted property names must > not be symbols") is reasonable here. I don't see a good reason to allow > self-hosted names to be symbols, and much clarity reason to forbid it. I'm a little wary of this assertion because if I added exactly the same assertion in JS_DefineFunctions where it deals with self-hosted functions, it would fail. Symbol-keyed methods are the point of the patch, after all. The IdToFunctionName stuff below is related to my thinking here. > ::: js/src/jsapi.h > > +inline bool PropertySpecNameIsSymbol(const char *name) { yup, fixed > ::: js/src/jsfun.cpp > > +js::IdToFunctionName(JSContext *cx, HandleId id) > > This method kind of shouldn't exist, right? No, I think it has to exist; it implements step 4 of the SetFunctionName algorithm: http://people.mozilla.org/~jorendorff/es6-draft.html#sec-setfunctionname I've added a comment to that effect (and fixed the silly bug where I was appending "[Symbol." even though the algorithm clearly only calls for "["). > Looks like > > var obj = { [Symbol.iterator]() { } }; > > produces an obj[Symbol.iterator].name === Symbol.iterator. I get this: js> var obj = { [Symbol.iterator]() { } }; js> obj[Symbol.iterator].name === Symbol.iterator false js> obj[Symbol.iterator].name "" But that's a bug, bug 1054599. The method's .name should be the string "[Symbol.iterator]". See: http://people.mozilla.org/~jorendorff/es6-draft.html#sec-setfunctionname which in this code is called from: http://people.mozilla.org/~jorendorff/es6-draft.html#sec-object-initializer-runtime-semantics-propertydefinitionevaluation > ::: js/src/jsfun.h > I get that |extern| is the default and all, but why remove it? EIBTI, seems > to me. I'd rather we consistently used it in headers and prohibited it > outside of them, myself. Reverted for now. There is no prevailing style, unfortunately. Trying to fix that on IRC...
Two issues here. 1. The damned thing just didn't work. Lots of assertions in various test suites. I've fixed two rather straightforward bugs so far. Building and testing again now. 2. Jeff reminded me that even though I will be trying to enable symbols in the current release cycle, the patches in this bug should still support building without JS_HAS_SYMBOLS -- just in case they have to be disabled again. So that means sprinkling a few #ifdefs here and there, forking the XDR version number, and (which is 80% of the work) revisiting all the tests. All that is done, but will need fresh reviews.
Blocks: 1066322
Depends on: 1071177
I think I have found the issue here that caused all the failures I couldn't reproduce locally. I have added the following four-character fix to the first patch in the stack. We'll see if this fixes it. >diff --git a/js/src/jsapi.cpp b/js/src/jsapi.cpp >--- a/js/src/jsapi.cpp >+++ b/js/src/jsapi.cpp >@@ -3248,15 +3248,15 @@ JS_DefineConstIntegers(JSContext *cx, Ha > static bool > PropertySpecNameToId(JSContext *cx, const char *name, MutableHandleId id, > js::InternBehavior ib = js::DoNotInternAtom) > { > if (JS::PropertySpecNameIsSymbol(name)) { > uintptr_t u = reinterpret_cast<uintptr_t>(name); > id.set(SYMBOL_TO_JSID(cx->wellKnownSymbols().get(u - 1))); > } else { >- JSAtom *atom = Atomize(cx, name, strlen(name)); >+ JSAtom *atom = Atomize(cx, name, strlen(name), ib); > if (!atom) > return false; > id.set(AtomToId(atom)); > } > return true; > } Coincidentally, when I found this, I made a number of to-the-point four-character remarks on the issue, which I won't reproduce here.
Attachment #8486771 - Attachment is obsolete: true
Attachment #8486772 - Attachment is obsolete: true
Functionally this is the same patch as the now-obsolete "part 2" (which nbp reviewed earlier). The changes to tests here are significant enough to warrant a new review.
Blocks: 1083470
That changeset is just a test or two. The actual work here still has not landed.
Keywords: leave-open
Comment on attachment 8504930 [details] [diff] [review] part 1 - Change iteration code to call iterable[Symbol.iterator]() rather than iterable["@@iterator"](). try: -b d -p linux64,linux64-st-an,macosx64 -u all[10.8,x64] -t none Review of attachment 8504930 [details] [diff] [review]: ----------------------------------------------------------------- The bytecode emitter changes look seriously wrong, but I imagine that's been fixt in subsequent try-pushes and you just haven't posted a new patch/r?. ::: addon-sdk/source/lib/sdk/util/iteration.js @@ +2,1 @@ > * License, v. 2.0. If a copy of the MPL was not distributed with this The addon SDK is maintained in a github repository, so this needs to be upstreamed to them. Per KWierso, "would be nice to file a dependency of your patch's bug in the add-on sdk product to sync up the changes to github." CC peoples, get the ball rolling. ::: js/src/frontend/BytecodeEmitter.cpp @@ +4667,5 @@ > return false; > +#ifdef JS_HAS_SYMBOLS > + if (Emit2(cx, bce, JSOP_SYMBOL, jsbytecode(JS::SymbolCode::iterator)) < 0) // OBJ OBJ @@ITERATOR > + return false; > + if (!EmitElemOpBase(cx, bce, JSOP_CALLELEM)) // FN OBJ I understand OBJ OBJ @@ITERATOR. But doesn't JSOP_CALLELEM pop [obj, val] and push obj[val]? So how is this not OBJ ITERFN (which seems like a better name to use in the !JS_HAS_SYMBOLS case than @@ITERATOR, now that @@iterator is an actual thing)? And wouldn't we then need the same fall-through-to-swap that happens in the other arm? ::: js/src/tests/ecma_6/Array/from_errors.js @@ -139,1 @@ > } ...so this method was returning |undefined| before, as this bracing triggered a labeled arrow function that was itself useless? But that happened to cause the same observable behavior as returning an objects whose .next property when called returned a primitive? Sheesh. ::: js/src/vm/PIC.cpp @@ +20,5 @@ > > +#ifdef JS_HAS_SYMBOLS > +#define STD_ITERATOR_ID SYMBOL_TO_JSID(cx->wellKnownSymbols().iterator) > +#else > +#define STD_ITERATOR_ID (cx->names().std_iterator) A little weird that STD_ITERATOR_ID has different *types* in the two cases, but if it works... ::: js/xpconnect/tests/chrome/test_xrayToJS.xul @@ +505,1 @@ > "set", "copyWithin"]; Guh. I think this is copypasta I used when writing this method, to ensure the rest of it was correct/sensible wrt those properties, that I forgot to remove before landing. Please remove this.
Attachment #8504930 - Flags: review?(jwalden+bmo) → review+
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #48) > ::: addon-sdk/source/lib/sdk/util/iteration.js > @@ +2,1 @@ > > * License, v. 2.0. If a copy of the MPL was not distributed with this > > The addon SDK is maintained in a github repository, so this needs to be > upstreamed to them. Per KWierso, "would be nice to file a dependency of > your patch's bug in the add-on sdk product to sync up the changes to > github." CC peoples, get the ball rolling. Thanks for noticing this! Will file one. > ::: js/src/frontend/BytecodeEmitter.cpp > @@ +4667,5 @@ > > return false; > > +#ifdef JS_HAS_SYMBOLS > > + if (Emit2(cx, bce, JSOP_SYMBOL, jsbytecode(JS::SymbolCode::iterator)) < 0) // OBJ OBJ @@ITERATOR > > + return false; > > + if (!EmitElemOpBase(cx, bce, JSOP_CALLELEM)) // FN OBJ > > I understand OBJ OBJ @@ITERATOR. But doesn't JSOP_CALLELEM pop [obj, val] > and push obj[val]? So how is this not OBJ ITERFN (which seems like a better > name to use in the !JS_HAS_SYMBOLS case than @@ITERATOR, now that @@iterator > is an actual thing)? And wouldn't we then need the same > fall-through-to-swap that happens in the other arm? No, because EmitElemOpBase emits a JSOP_SWAP. Assuming you find that horrible, you're not the first. I've filed (and taken) follow-up bug 1089758 to make it clearer what's going on here. > ::: js/src/tests/ecma_6/Array/from_errors.js > ...so this method was returning |undefined| before, as this bracing > triggered a labeled arrow function that was itself useless? But that > happened to cause the same observable behavior as returning an objects whose > .next property when called returned a primitive? Sheesh. Yeah, it's my fault, I wrote that code. Horrible^2. > ::: js/src/vm/PIC.cpp > > +#ifdef JS_HAS_SYMBOLS > > +#define STD_ITERATOR_ID SYMBOL_TO_JSID(cx->wellKnownSymbols().iterator) > > +#else > > +#define STD_ITERATOR_ID (cx->names().std_iterator) > > A little weird that STD_ITERATOR_ID has different *types* in the two cases, > but if it works... I don't feel great about this; I'll try it with #define STD_ITERATOR_ID jsid(cx->names().std_iterator) which should be just as good and makes the two expressions the same type (though you never know with C++ --- much closer, anyway). > Guh. I think this is copypasta I used when writing this method, to ensure > the rest of it was correct/sensible wrt those properties, that I forgot to > remove before landing. Please remove this. Done.
NameToId(), not jsid(), is the right thing; that's what NativeObject::lookup(cx, name) uses for the conversion.
Whiteboard: [DocArea=JS] → [DocArea=JS] {done: true}
> '@@iterator' in StyleSheetList.prototype; // Firefox 31 > '@@iterator' in CSSRuleList.prototype; // Firefox 32 Now both returns false. It that correct?
Yes. Symbol.iterator in StyleSheetList.prototype returns true now.
Why is this bug still open? Are there any remaining works?
Oh, I see. Then this may need a site compat doc.
Keywords: site-compat
Depends on: 1092625
Was this change announced anywhere? It broke a bunch of Thunderbird code I was working on, and it took me a while to figure out what changed. If I just missed the announcement, I want to be sure I subscribe to the appropriate group so I don't have have any unpleasant surprises like this in the future.
This bug apparently broke most of the add-on SDK. Can we consider backing it out until we can get things fixed?
Blocks: 1092231
Flags: needinfo?(jorendorff)
Keywords: addon-compat
(In reply to Jim Porter (:squib) from comment #76) > Was this change announced anywhere? No. I apologize for that. Announcements go to dev.platform. I just blew it.
Flags: needinfo?(jorendorff)
You didn't answer comment 77.
Flags: needinfo?(jorendorff)
(In reply to Dave Townsend [:mossop] from comment #77) > This bug apparently broke most of the add-on SDK. Can we consider backing it > out until we can get things fixed? Sorry for the rough landing. Some of this could have been avoided with a few announcements and some coordination. I've been talking about this with :zombie, and the bottom line is: <zombie> jorendorff: so since JP tests are hidden, we are in bad shape.. will try to see if we can fix in next ~12 hours, and if not, ask you to backout.. sounds reasonable? I recognize it's not good for jetpack tests to be failing for this long, and it wasn't my intent to spring the changes on y'all like this. :( I hope the fixes will be fairly straightforward, and I'm happy to help out. See you in #jetpack...
Flags: needinfo?(jorendorff)
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
I think the [leave-open] keyword prevented the script to set the release version. I've removed the keyword and manually set the release to Mozilla 36. If somebody could double check, it would be nice.
Keywords: leave-open
Target Milestone: --- → mozilla36
Excellent. Thanks for the doc additions!
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: