make @@toStringTag and constructor prototype properties funky accessors

[michaelficarra]

Fixes #286. This includes the @@toStringTag change from #213 and also applies the same accessor strategy to "constructor". This effectively reverts #214.

We're hoping to not have to do this, but if we cannot resolve the web compatibility issue through social mechanisms in a timely manner, this will resolve it through (gross) technical mechanisms.

[Jack-Works]

oh no... although ses (they're also freezing intrinsics) also use getter/setter to fix web compatibility problems, I never expected it would appear in the spec. I hope we can fix the web compat problem, or at least do the rename like IterableIterator instead of this

[bakkot]

spec text LGTM if we have to go this route

[bakkot]

at least do the rename like IterableIterator instead of this

Renaming would be a much more annoying change for users. No normal programmer will ever notice this change, as inelegant as it appears, whereas everyone will notice a worse name.

[ljharb]

oof, the set stuff is very weird

[ljharb]

if we're really going to do this it should be an AO that many things can share, since the only things that potentially change are Iterator.prototype and the property name

[michaelficarra]

that can be an editorial thing we address in the PR to 262

[bakkot]

Per request of @erights this should be an error, to emulate strict rather than sloppy assignment.

[michaelficarra]

Please bring the off-topic discussion elsewhere. Discussion here should be about this PR.

[bathos]

I’m a bit confused by throwing when the assigned value [edit: nm it’s the receiver, which is decidedly less mysterious lol] is %Iterator.prototype%. I assume that’s motivated by the specific web compat issue, but it’s mysterious looking especially given it pivots on the identity of an unbranded object. More broadly, is there a summary somewhere of what the overall behavior is achieving vis a vis the web compat issue?

[michaelficarra]

@bathos The throw in step 2a is simulating an assignment to a non-writable property in strict mode. Before the recent change to make it throw, it just was just a noop, which is more like an assignment to a non-writable property in sloppy mode. This specific behaviour was requested at the last presentation to TC39.

edit:

is there a summary somewhere of what the overall behavior is achieving vis a vis the web compat issue?

Firstly, see https://github.com/tc39/how-we-work/blob/main/terminology.md#override-mistake. The web-compat issue is adding a non-writable property in an existing object's prototype chain. Assignments to that property that were assuming the property didn't exist in the prototype will begin to throw instead of creating the property. The correct thing to have written to get the behaviour they wanted would've been a call to Object.defineProperty instead of assignment. So we add an accessor pair to the prototype instead of a non-writable data property to detect such a situation and do the thing they wanted. The rest of the complexity in the setter is just around edge cases like "well what if they are doing the assignment against the prototype object directly?" and "well what if they extract the accessor and .call it on some other object?" to ensure we keep it behaving as much like a non-writable data property as we can.

[bathos]

@michaelficarra Thanks! I was able to follow it better this time around — though the own-unwritable-"constructor" branch remains opaque to me; I couldn’t figure out for sure why that one’s a special case.

It’s a bit sad that it’s gotta emulate strict-mode assignment behavior specifically, and I think that’s part of what threw me off reading it the first time for whatever reason, but I think I understand the trade offs being made: the alternatives would have been (a) the setter having magical knowing-if-we’re-in-sloppy-mode powers which couldn’t be explained in terms of / implemented using ES code or (b) exoticism, which would actually be explainable in ES code terms and would actually get both sloppy and strict behaviors “right” (so to speak) but which, well ... I imagine nobody wants to go there for this lol.

[michaelficarra]

having magical knowing-if-we’re-in-sloppy-mode powers which couldn’t be explained in terms of / implemented using ES code

Exactly. So the better choice is to just behave like strict mode in all cases, since we want the environment to be most sane for strict mode code.

edit:

though the own-unwritable-"constructor" branch remains opaque to me; I couldn’t figure out for sure why that one’s a special case

The setter's behaviour can be described as "assign to the constructor property of the this value as if it had no constructor property in its prototype chain", so we need to first make sure the constructor own-property, if it exists already, is non-writable. I guess technically we don't trigger an own accessor. Do you think we should?

[bathos]

The setter's behaviour can be described as "assign to the constructor property of the this value as if it had no constructor property in its prototype chain", so we need to first make sure the constructor own-property, if it exists already, is non-writable.

Oh, oops!! I was failing on basic reading comprehension there again and got confused because I’d forgotten there are two setters here and ended up mistaking that unwritable "constructor" step for being part of the the @@toStringTag setter!

I guess technically we don't trigger an own accessor. Do you think we should?

No, that makes sense the way it is, I think; plenty of stuff calls [[GOP]] and not [[Get]], right? (e.g. HasOwnProperty). However if I’m understanding right (fingers crossed) ... shouldn’t it also care about [[Configurable]]: false? Not necessarily with an explicit step, but more broadly by not ignoring the return value of the [[DefineOwnProperty]] call? Right now it looks like it’ll just return undefined if that returns false (as it [typically] would in the [[Configurable]]: false case).

[ljharb]

I thought the sentiment of plenary was to simply remove constructor and toStringTag rather than ship them with these weird semantics?

[michaelficarra]

shouldn’t it also care about [[Configurable]]: false

Not really, since we shouldn't be trying to change the configurability. But you're right that, at the moment, we may be. Also, we should be throwing when we attempt to create a property on a non-extensible object, and that one would be solved by checking the return value of [[DefineOwnProperty]]. Looks like I need to make some updates.

I thought the sentiment of plenary was to simply remove constructor and toStringTag rather than ship them with these weird semantics?

That was briefly brought up as an option, yes. We haven't decided what approach we will take if we end up needing to take action (we still hope we won't have to), but we want this one to be ready to go in case we opt for it.

[michaelficarra]

@bathos @bakkot I've rewritten the setter. It now matches assignment better when pulled off and .call-ed, and re-uses existing AOs more. I think it also fixed a bug where the property was being created non-enumerable before. Please review for correctness.