| 19:15 | <bakkot> | TabAtkins: the way you've written https://webidl.spec.whatwg.org/#es-map-iterator implies that (new Map)[Symbol.iterator]().next.call(webIdlMapLike) would work, which is... maybe not a thing you intended? |
| 19:15 | <bakkot> | I don't know where webidl map-likes are used though |
| 19:16 | <TabAtkins> | Hmmmmm, I dunno if that's a problem? |
| 19:16 | <TabAtkins> | (For an example usage, see https://tabatkins.github.io/css-toggle/#dom) |
| 19:18 | <bakkot> | I think that is... probably not desirable, at least for implementations? |
| 19:18 | <TabAtkins> | .next() isn't usable directly on a Map, right? You'd still have to actually extract an iterator, which has .next() from its proto. |
| 19:19 | <bakkot> | oh, yeah, so (new Map)[Symbol.iterator]().next.call(webIdlMapLike[Symbol.iterator]()) |
| 19:19 | <bakkot> | I think you probably do not want to require that that work |
| 19:19 | <TabAtkins> | Why not? Making it not work would presumably require us to not use MapIteratorPrototype at all, right? |
| 19:20 | <bakkot> | oh, I missed that you are intending to use MapIteratorPrototype |
| 19:20 | <TabAtkins> | (That was already required before my edits, btw.) |
| 19:21 | <TabAtkins> | Yeah we're manually reproducing Map's proto methods by hand, but there's no reason for us to do the same for its iterator. |
| 19:22 | <bakkot> | it's conceptually kind of weird? a "real" map iterator uses the [[MapData]] internal slot |
| 19:23 | <bakkot> | I don't think it's going to cause problems for users (or even be noticed), but it might trip up implementations |
| 19:23 | <bakkot> | when we wrote https://github.com/tc39/ecma262/pull/2045 the thinking was that brands would be single-use |
| 19:24 | <TabAtkins> | If that's problematic, the spec shouldn't be written in a way that implies it's pluggable. ^_^ |
| 19:24 | <bakkot> | the spec is not an API |
| 19:24 | <TabAtkins> | what's this about brands? |
| 19:25 | <bakkot> | the string argument to CreateIteratorFromClosure is a brand |
| 19:25 | <bakkot> | which is intended to prevent users from using .next from one kind of iterator with a different kind of iterator |
| 19:26 | <TabAtkins> | Oh, if that's the issue we can change that. |
| 19:26 | <TabAtkins> | It didn't look like something I needed to change, is all. |
| 19:26 | <bakkot> | if you change it you'll also need a new prototype, though |
| 19:27 | <bakkot> | the way it works is, CreateIteratorFromClosure specifies a brand, which is stored on the iterator, and then the various .next methods check that brand |
| 19:27 | <ljharb> | please do not reuse MapIteratorPrototype on something that's not a Map, that's not right |
| 19:28 | <bakkot> | so CreateMapIterator and %MapIteratorPrototype%.next ( ) both specify the "%MapIteratorPrototype%" brand, which is how we achieve that only the MapIterator .next works with map iterators |
| 19:29 | <ljharb> | and it would be a bug if that brand check passed on something that wasn't a Map instance |
| 19:29 | <bakkot> | ljharb: the brand check is only for Map iterators, not Map instances |
| 19:30 | <ljharb> | oh right sorry |
| 19:30 | <ljharb> | ok, Map iterator instance then :-p |
| 19:30 | <ljharb> | (obv that's way less important to brand check) |
| 19:31 | <bakkot> | TabAtkins: anyway, if you want to avoid re-implementing stuff, while also not pretending to be an actual Map iterator, I think the simplest thing would be to pretend to be a userland generator - i.e. inherit from Generator rather than MapIterator, and use ~empty~ as the brand |
| 19:31 | <bakkot> | this has the bonus that it would allow implementations to implement the iterator as a JS generator |
| 19:32 | <TabAtkins> | Hm, that would mean that future things put on MapIteratorPrototype wouldn't apply then, right? |
| 19:33 | <bakkot> | well, so |
| 19:33 | <bakkot> | as long as MapIteratorPrototype is written as a spec generator, it can't really have more things put on it - the state is all internal to the closure created in CreateMapIterator |
| 19:34 | <bakkot> | so if we wanted to add more stuff, we'd have to revert the change from https://github.com/tc39/ecma262/pull/2045 anyway |
| 19:34 | <bakkot> | and go back to using specialized slots on the iterator instance to store the state of the iterator |
| 19:34 | <bakkot> | which would break this part of webidl anyway |
| 19:35 | <ljharb> | why would MapIteratorPrototype ever get something added to it? iterator helpers would be on a superclass, and generator-made iterators would have those too, i think |
| 19:35 | <bakkot> | java's iterators have a remove method which is occasionally handy, e.g. |
| 19:35 | <bakkot> | I don't think we'd want to add it but we conceptually could |
| 19:36 | <bakkot> | for iterators for collections in particular, and not for arbitrary generators/iterators |
| 19:36 | <TabAtkins> | Right, I was thinking about iterator helpers. But if they'll all show up on something that all userland generators get too, then I suppose my concern is moot. |
| 19:36 | <ljharb> | it.remove() as a shorthand for "get the key and remove it directly from the collection"? |
| 19:36 | <bakkot> | yup, iterator helpers add things to the root iterator prototype, which is shared by MapIterator and also the iterators produced by userland generators |
| 19:37 | <bakkot> | ljharb: in java it means "remove the last thing yielded" |
| 19:37 | <ljharb> | weird, ok |
| 19:37 | <TabAtkins> | def weird |
| 19:38 | <bakkot> | TabAtkins: actually wait, forget my suggestion about inheriting from generator, though |
| 19:38 | <bakkot> | that would have other consequences which are bad |
| 19:38 | <bakkot> | because generators also have .return which injects a return completion at the current yeild, which is... not a thing you want, almost certainly |
| 19:40 | <TabAtkins> | I mean, userland generators have to deal with that too, right? Is there a simple way I can, like, intercept and either ignore it or bail or something? |
| 19:40 | <TabAtkins> | (I'm willing to do a lot if it means (a) I don't have to redefine the entire interator infrastructure from ES and (b) iterator helpers will still work.) |
| 19:41 | <ljharb> | i think you probably should just make your own iterator thing that inherits from Iterator.prototype (which will make helpers work), just like map/string/set/etc are their own thing |
| 19:41 | <bakkot> | userland generators do have to deal with it but the problem is that you have to think about it at all |
| 19:41 | <bakkot> | .return just doesn't exist on map iterators |
| 19:41 | <bakkot> | anyway yeah, I think I would just add a new iterator prototype, like MapIteratorPrototype. that's what the language would do at least. |
| 19:42 | <bakkot> | you can copy-paste the whole of the MapIteratorPrototype clause; it's actually quite short now |
| 19:42 | <TabAtkins> | yeah, https://tc39.es/ecma262/#sec-%mapiteratorprototype%-object is doable |
| 19:42 | <TabAtkins> | okay, sigh, thought i was done with this edit but back into the breach |
| 19:45 | <bakkot> | apologies |
| 19:52 | <TabAtkins> | So just to be clear, the overriding issue here is that, in practice, the second and third arguments to CreateIteratorFromClosure (giving the brand and prototype) should be unique per call site, more or less? |
| 20:05 | <bakkot> | I would put it a different way: if the second and third arguments are not different, you are saying that these two things are actually the same thing, even though they have different behavior, and you should be really sure you want that |