Normative: Make non-writable prototype properties not prevent assigning to instance

[littledan]

TC39 is discussing making inherited non-writable properties overridable with Set,
in #1307. We have not yet assessed the web compatibility of the change or written
tests, so this PR is not yet ready to merge, but this PR gives some concreteness
to how the specification may be done, to help move the discussion forward.

This patch implements the approach described in #1307 (comment)

[bmeck]

It looks like this would make all writable properties ignored? ownDesc could be on the receiver in which case we do want it to fail.

[littledan]

@bmeck Is the line If _existingDescriptor_.[[Writable]] is *false*, return *false*. insufficient for that check against own properties?

[littledan]

@caitp 's counters to assess the web compatibility have landed in Chrome (the initial figure seems to be invalid; the percentage can't be that high when it's in canary). These counters check how often there is a Set to an inherited, non-writable property, in strict and sloppy mode.

• Sloppy mode counter
• Strict mode counter

[caitp]

that does look like quite a high figure... @mathiasbynens is this normal?

[bmeck]

http://jsfiddle.net/ is at least one site that seems to reproduce this consistently as seen in Chrome canary (via 2609 in chrome://histograms/Blink.UseCounter.Features). Haven't managed to track down what is causing the counter yet. Seems related to the JS bundle with their editor code which is using MooTools. MooTools itself does not fire this usage counter by merely loading up the library.

[mathiasbynens]

I’m a little surprised by the UseCounter results as well.

https://jsfiddle.net/js/_dist-editor.js contains a few occurrences of configurable: false (well, configurable:!1). I don’t see any writable: false, though.

@zalun, do you know where/how JSFiddle relies on prototype properties that are writable: false for its editor?

[mathiasbynens]

I only see a single writable: false in https://jsfiddle.net/js/_dist-editor.js, and it’s an implicit one, in Babel’s output for export:

Object.defineProperty(exports, '__esModule', {
  value: true
});

Note that writable: false is the implicit default when using Object.defineProperty.

If Babel’s output is the culprit, that would certainly explain the higher-than-expected use counter results! However, I can’t find any code that later tries to write to the __esModule property in this script. 🤔

I could imagine Babel’s loose mode doing something like exports.__esModule = true instead, but not after Object.defineProperty()… @jridgewell In which circumstances would Babel do this? When re-exporting a module?

[loganfsmyth]

Re-exporting should always skip __esModule https://github.com/babel/babel/blob/806e1334737f4fbf29615c04d227076b76d7f041/packages/babel-helper-module-transforms/src/index.js#L243 so I wouldn't expect collisions from that.

[jridgewell]

I just converted jsfiddle to strict mode, and paused on every error. It's:

                        function Zr(e) {
                            return null == e ? e === r ? re : Y : Dn && Dn in tt(e) ? function(e) {
                                var t = ct.call(e, Dn)
                                  , n = e[Dn];
                                try {
                                    e[Dn] = r;
                                    var i = !0
                                } catch (e) {}
                                var o = dt.call(e);
                                return i && (t ? e[Dn] = n : delete e[Dn]),
                                o
                            }(e) : function(e) {
                                return dt.call(e)
                            }(e)
                        }

Particularly, it's e[Dn] = r, where e is a DataView and Dn is Symbol.toStringTag

[mathiasbynens]

@loganfsmyth Ah, that code seems only necessary for the loose mode code. When exports.__esModule is defined using defineProperty, the property is enumerable, and thus it doesn’t show up in Object.keys(exports). (Note that enumerable: false is the implicit default when using Object.defineProperty). And since in the loose mode code, the override mistake doesn’t apply, __esModule is likely not the issue after all.

[mathiasbynens]

@jridgewell Woah, nice catch (pun not intended)!

@zalun Could you please tell us where the code Justin pointed out originates? Is this part of some kind of library, or is it custom code?

[jridgewell]

Is this lodash? 😢

[jridgewell]

Yup, it's lodash:

• https://github.com/lodash/lodash/blob/4ea8c2ec249be046a0f4ae32539d652194caf74f/.internal/baseGetTag.js#L23-L26
• https://github.com/lodash/lodash/blob/4ea8c2ec249be046a0f4ae32539d652194caf74f/.internal/getTag.js#L25-L32

[jdalton]

@jridgewell
I'm lurking on this issue but the lodash mention caught my eye. It looks like

Particularly, it's e[Dn] = r, where e is a DataView and Dn is Symbol.toStringTag

That's code within a try- of a try-catch block though if that makes a difference.

[jridgewell]

It is within a try-catch. But unfortunately, the proposal would change the behavior of the surrounding code:

// Current semantics, overwrite will fail silently
baseGetTag(new DataView(new ArrayBuffer(1)));
// => "[object DataView]"

// With new semantics
baseGetTag(new DataView(new ArrayBuffer(1)));
// => "[object Object]"

Fortunately, these are feature tests. When one fails (as it would with the new behavior), lodash falls back onto a known implementation of getTag. So it'll be a little less optimized (it'll use a little wrapper around native Object.p.toString instead of just Object.p.toString), but everything will still work fine.

I think our counter is useless, though. 😞

[mathiasbynens]

@jdalton For full context, we’re trying to change the spec so that…

const prototype = {};
Object.defineProperty(prototype, 'property', {
  writable: false,
  value: 1,
});
const object = Object.create(prototype);
object.property = 2;
console.log(object.property);

…logs 2 (instead of logging 1 in sloppy mode, or throwing a TypeError in strict mode).

The lodash code appears to be in strict mode (the source code is a module) but is indeed within a try-catch block. Can you imagine this particular example actually breaking anything?

In the case of DataView[Symbol.toStringTag] = foo (in strict mode), this went from throwing to actually going through with the assignment. By itself, that doesn’t break anything.

Update: Woah, I clearly hadn’t refreshed when I posted that comment.

[jridgewell]

Actually, there is a breakage in lodash: isTypedArray will no longer return true on typed arrays.

The fallback getTag does not correct the typed array instances' tag, so getTag will return "[object Object]" instead of "[object Uint8Array]" and that will fail the typed-array regex.

But, the code currently fails anyway! The regex matches for "Uint8", not "Uint8Array". So it may still be ok?

[jdalton]

@jridgewell

That's in the master branch which is a bit of a mess, untested, and unreleased.
Check out a release branch like 4.17.11 when investigating.

[jridgewell]

Ok, so this will break lodash's isTypedArray.

[littledan]

Interesting! We had been hoping that the strict mode case would be "easier" to fix, but that seems to not be the case.

Crazy idea: what if we made toStringTag a getter with no setter, so it always throws to assign to it, unless you do Object.defineProperty (including sloppy mode and with this patch)?

[littledan]

Great work, @jridgewell!

[jridgewell]

@erights: This code has changed behavior.

Calling baseGetTag with a new Uint8Array, setting value[symToStringTag] = undefined used to:

• In strict mode, throw. This makes unmasked === false and result === "[object Uint8Array"
• In loose mode, nothing. This makes unmasked === true and result === "[object Uint8Array"

Now:

• In strict mode, change. This makes unmasked === true and result === "[object Object"]
• In loose mode, change. This makes unmasked === true and result === "[object Object"]

[caitp]

fwiw, lodash merged lodash/lodash@aa1d7d8 today, which eliminates the weird overriding behaviour.

[mathiasbynens]

@jdalton Could you cut a new lodash release? I imagine that'd make it easier for sites like JSFiddle to update.

[jdalton]

I can port the removal to the 4.17.12-pre branch for a 4.x release. Though if we're looking at the toothpaste out of the tube issues Lodash is used by a lot of websites (cdn hit stats) already.

[caitp]

Though if we're looking at the toothpaste out of the tube issues Lodash is used by a lot of websites (cdn hit stats) already.

Realistically, I don't expect that slight observable change to break most websites in a noticeable way (please don't prove me wrong!)

[littledan]

@caitp I bet we could stand the observable change in #1320 (comment) , but it might be really breaking if isTypedArray returns false on TypedArrays (as @jridgewell reported above).

I'm wondering if it's possible to empirically investigate further whether @gibson042's minor change would be enough, or if there are other issues.

[bmeck]

Are we thinking of investigating how that change would affect the web / are there ways we can check compatibility of what happens since lodash is in userland instead of at the vm level?

[littledan]

OK, due to the very high counters in #1320 (comment) , and the practical cases traced in this thread, it seems that we don't have much hope of changing semantics here in either strict or sloppy mode. Great teamwork in figuring this out, @bmeck @caitp @jridgewell.

[dead-claudia]

How many of those uses would actually break after this? Like, how many are legitimately expecting an error, rather than just trying to assign and ignoring errors if it fails?

[erights]

@bmeck from our last verbal conversation, it seemed there was still hope here. Still? If so, please reopen. Thanks.

[lll000111]

Does this discussion include frozen prototype objects?

Here is an example: Moddable (run JS compile to bytecode on small embedded systems) freezes all prototype objects to allow their compiler to put such unmodifiable objects into ROM.

Here is an example where that breaks: The not uncommon pattern to assign to name of Error objects fails - silently - because name exists in the frozen prototype.

Moddable-OpenSource/moddable#144

To me, that was quite unexpected and illogical. The "freeze" semantic seems to me to be orthogonal, finding that it also has this side-effect was unexpected. The prototype was frozen, not the instance, the extension of the prototype-freeze to prevent assignment to the instance object was a surprise. Things like this are not even mentioned on common pages like the MDN page for Object.freeze, one has to find this by chance or by reading the spec, which few people do (I only did and do it when it in a targeted way, but this issue here was unexpected so I did not know I should have read the spec). I would have expected the example code above to work (same for a frozen prototype object).

By the way, I still don't understand this inconsistency (from a logic PoV): Direct assignment fails, but if I do a Object.defineProperty() on the instance object I actually can override the non-writable prototype property (see linked issue).

[bakkot]

@lll000111, yes it does, and Moddable (which is on TC39) has presented that use case to the rest of the committee.

Unfortunately, we can't make this change, however weird the current behavior might be, unless we're confident it won't break the web.

[lll000111]

@bakkot

Unfortunately, we can't make this change

Well, sure, but in the example I gave code that was supposed to work and is in common broke as a consequence of this poorly documented effect, so I'm not sure that "breaking" doesn't already happen. Especially when the failure to assign to an instance property does not result in an error but is silent.

[erights]

I suggest that we move the existing behavior into annex b, where legacy crap that browsers dare not change gathers, and then proceed to fix this in the main spec.

[erights]

That way, non browser engines that already drop some annex b stuff can drop this as well and remain conformant.

[littledan]

I'm not a big fan of moving things from the main specification to Annex B; I'd rather us make a decision one way or another on these things. Otherwise, I worry that the JavaScript ecosystem could become forked and incompatible.

[guybedford]

I was quite surprised to see this closed I must admit - I was really hoping there might still be a spec path forward here. Are we really sure we've exhausted all the options? So that the best thing to do at this point would be to define a new form of Object.freeze or similar to get frozen intrinsics? I haven't dug into the details on this, but would gladly do so if there might still be hope here.

[erights]

A different form of freezing may indeed be the way to overcome this. See https://github.com/Agoric/harden which is what the SES shim uses to freeze the intrinsics. It would be very natural to propose a harden that freezes things in this special way. Were a proposal written up, I would happily champion it.

I haven't dug into the details on this, but would gladly do so if there might still be hope here.

Please dig; thanks!!!

[billiegoose]

Just stumbling across this to say - wow yeah I just discovered that Object.freeze on prototypes is next to useless 😭 , so we'll need something else. Looking forward to harden or whatever helps make immutable intrinsics possible! 🚀

[Jamesernator]

@erights I've written a strawman gist for an Object.harden which allows non-writable properties to optionally be overridable. If this something you would be interesting in championing I would be happy to write up a full proposal with spec text.

Strawman here: https://gist.github.com/Jamesernator/dc9831f9c13d17ac9a75d1fe25a2e015

[erights]

Yes! I'd love to work with you on advancing such a proposal. Please!

See some of my thoughts on this at https://gist.github.com/Jamesernator/dc9831f9c13d17ac9a75d1fe25a2e015#gistcomment-3462493

Thanks!