Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Should we allow the wrapper object to have additional symbol properties #137

Closed
rricard opened this issue Jun 30, 2020 · 47 comments
Closed
Milestone

Comments

@rricard
Copy link
Member

rricard commented Jun 30, 2020

While writing spec text we found out that we could either allow writing new arbitrary props to the exotic record boxing object or simply disallow them. The champion group would like to do the latter but if there are objections, we're interested to have them here.

Here is an example that we would want to disallow:

var o = Object(#{});
o.foo = true;
o.foo // => true

The main rationale against disallowing this would be that this is acceptable with a String for instance:

var o = Object("a");
o.foo = true;
o.foo // => true

This should be decided before Stage 3.

cc. @ljharb

@ljharb
Copy link
Member

ljharb commented Jun 30, 2020

Why would you prefer to disallow it?

Primitives can be as exotic as desired, but it seems preferable to minimize the ways in which objects - even boxed primitives - are exotic.

@ljharb
Copy link
Member

ljharb commented Jun 30, 2020

Additionally, if I can't set Symbols on a boxed Record object, then I can't opt them into any protocols, which is pretty important.

@rricard
Copy link
Member Author

rricard commented Jun 30, 2020

To me it's in order to avoid any mistake of having a boxed record and starting to see I can mutate it. I don't see many people will wrap their Record into their boxing objects though and passing them so honestly I'm not against going back and changing this. I'd still like to wait next week to hear @littledan's opinion on this however.

@rricard
Copy link
Member Author

rricard commented Jun 30, 2020

Additionally, if I can't set Symbols on a boxed Record object, then I can't opt them into any protocols, which is pretty important.

That might do it to reverse that decision, still invoking @rickbutton for a second champion opinion

@rickbutton
Copy link
Member

I don't really have a strong opinion on this either way. I can see the argument against allowing adding string properties to a boxed Record, for the same reason that boxed Records have a null prototype - that we don't want to allow Records to surface string-properties that don't come from the [[RecordData]] itself. However, attaching symbols to boxed records seems like a reasonable desire to me.

There was some previous discussion about these ideas in this thread: #71 (comment) (although, not for new-properties on a boxed Record, but instead for the prototype).

@littledan
Copy link
Member

Personally, I'm pretty opposed to permitting additional properties on Record wrappers. The domain of Records is their data properties, and it would be rather broken to have to think about whether the property is on the Record or its wrapper. Being frozen is not very exotic, and it corresponds to the normal mental model around Records and Tuples, so it's unsurprising.

@ljharb Could you explain the part about presence in protocols? It's very hard for me to understand how this would be useful, given that the underlying Record will not have the symbol properties, and the Record wrapper will not act like Records in key ways (e.g., === is broken).

@ljharb
Copy link
Member

ljharb commented Jul 8, 2020

That's a fair point that it'd be awkward to make a wrapper object just to install Symbols, and I just realized, and filed #142, about the seeming current inability to extract a primitive Record from a boxed Record object - which would be strictly necessary, both for "boxing the primitive for the purposes of protocol participation, and in general. I think it might make sense to hold off further discussion on this issue until that's resolved, since I'm somewhat convinced that the only outcome of it is Record.prototype, which would require exoticness so that string properties on boxed objects weren't possible, and then adding custom Symbols to a boxed object or to Record.prototype would seemingly Just Work.

@littledan
Copy link
Member

and then adding custom Symbols to a boxed object or to Record.prototype would seemingly Just Work.

I think this comment is mixing together two different things. One question is whether Record and Tuple wrappers are frozen, and another thing is whether we defer to Record.prototype on access of Symbol-keyed properties. Mechanically, they're unrelated. I think it's important that Record and Tuple wrappers are frozen, but I'm open to Record property access deferring to Record.prototype for Symbol properties.

@ljharb
Copy link
Member

ljharb commented Jul 14, 2020

If Record.prototype is a mutable object, then the use cases for mutating an individual wrapper are certainly fewer - but i don't see why these should be the only primitive wrapper objects that are frozen, and there still are use cases where I'd want to opt a wrapper into a protocol without opting in every wrapper object.

@rickbutton
Copy link
Member

It is still possible to opt-in an immutable wrapped Record into a protocol with a Proxy.

@ljharb
Copy link
Member

ljharb commented Jul 15, 2020

No, it’s not, because the Proxy wouldn’t have the internal slots of a Record, so unboxing would fail.

@littledan
Copy link
Member

That's true, it wouldn't have the internal slots of a Record. But a Record wrapper that you could add other properties to wouldn't act like a Record, e.g., with ===. Overall, I'm still having trouble understanding when you'd want to use this construct.

@ljharb
Copy link
Member

ljharb commented Jul 20, 2020

Right, but wrapped[Symbol.toPrimitive]() is a cheap call, and would work with ===, while wrapped could still contain extra data via regular properties.

@littledan
Copy link
Member

Could you give an example of how this would use in practice? I can imagine a program using this corner of the language, but I can't understand why anyone would want to write it.

If you're going to explicitly unwrap it for some operations, it's unclear to me what the problem is with Proxy-wrapping.

@ljharb
Copy link
Member

ljharb commented Jul 20, 2020

You can unwrap a boxed primitive; you can't unwrap a Proxy around one (because Record.prototype[Symbol.toPrimitive].call(proxy) would always throw.

I don't have a concrete example to offer - i'm coming from a position of maximizing composability and consistency with the way primitives and objects and internal slots already work. I'm also thinking vaguely in terms of what tricks might be employed by a R&T polyfill, and I don't want to discover too-late that something that I'd expect to work, doesn't :-)

@littledan
Copy link
Member

Yes, I can see how the difference is observable. I'd like to understand more what you'd like to compose this with. I agree that it's important to follow typical patterns in the language, and I think "behaving like a frozen object" achieves that. I don't think Records and Tuples need to slot into all existing cases which expect mutable, extensible objects, since the whole point of Records and Tuples is to be deeply immutable, but I'm open to being convinced with examples until Stage 3.

@littledan
Copy link
Member

littledan commented Aug 9, 2021

I remain confused by the arguments above for allowing additional properties on Box wrappers. I think we should conclude that Box wrappers are also prevent-extensions, like Record and Tuple wrappers.

Edit: Oh, wow, I completely misunderstood this thread when re-skimming it. Anyway, I still agree with my previous comments above 😇 and would prefer to settle this issue on the frozen-wrappers side. Note that if we move to identityless objects, the question is moot--they must be frozen.

@mhofman
Copy link
Member

mhofman commented Aug 12, 2021

In the context of R&T as identity-less objects, as @littledan mentions the question would be moot. There would be no object wrapper, the R/T would be the object, which would be frozen and prevent any extension.

In the context of identity-less objects I was suggesting that proxies of R&T would behave like regular objects. However I don't think that would solve @ljharb's use case with adding properties to a wrapper of R/T, since with a frozen target, all properties would need to exist on the shadow target. However I'm still not sure I understand the motivation to extend a R/T that way.

@nicolo-ribaudo
Copy link
Member

After implementing R&T in SpiderMonkey, I'm in favor of letting the object wrappers to be mutable (as they are for other primitives).

There are many places where the spec calls ToObject on a generic ECMAScript value, and this means that object wrappers are frequent. They can be optimized away in hot paths, but optimizing them away everywhere introduces a complexity that might not be worth the performance improvement.

When implementing R&T wrappers, I used the same approach already used for string wrappers (since strings are the only existing primitive with own properties): Object(tuple) returns an empty object, and its properties are lazily defined only when you try to observe them.

For example, this is how the wrapper object internally transitions between the different shapes:

let o = Object(#{ x: 1, y: 2, z: 3 }); // o is {}
o.x; // o is { x: 1 }
o.w; // o is { x: 1 }
delete o.y; // o is { x: 1, y: 2 }

This is not observable from JS since properties appear as soon as you try to observe them, but gives a big advantage by making Object(record) an O(1) operation rather than an O(n) operation, since it doesn't require to eagerly copy all the properties.

If object wrappers were not extensible, this would be harder to accomplish. It's possible to internally define new properties and skip the "is it extensible?" check, but there are optimizations for non-extensible objects that would stop being safe (since you cannot rely anymore on the non-extensibility to be sure that it won't get new properties).

@mhofman
Copy link
Member

mhofman commented Nov 12, 2021

What would the following do:

let o = Object(#{ x: 1, y: 2, z: 3 });
Object.defineProperty(o, 'w', {value: 0, enumerable: true});

If modeled like other object wrappers it'd be allowed, right?

@nicolo-ribaudo
Copy link
Member

Yes, I'm proposing that it should work.

@ljharb
Copy link
Member

ljharb commented Nov 12, 2021

It’s an object; it’d be strange if it’s not allowed.

@mhofman
Copy link
Member

mhofman commented Nov 12, 2021

How do we ensure the following:

const r = #{ x: 1 };
Object(r).y = 2;
assert.throws(() => r.y = 2);

I know that strings behaves that way, but I'm unclear what in the spec confers that behavior.

@nicolo-ribaudo
Copy link
Member

I don't understand exactly why it works for strings, but I plan to copy the same behavior.

@gibson042
Copy link

In what sense does it "work" for strings?

$ eshost -sx 'const s = "foo", p = 1; Object(s)[p] = "x"; s[p] = "x"; print("no error")'
#### ChakraCore, engine262, GraalJS, Hermes, JavaScriptCore, Moddable XS, SpiderMonkey, V8
no error

@mhofman
Copy link
Member

mhofman commented Nov 13, 2021

Sorry I should have said strict mode is important:

$ eshost -sx '(function() {"use strict"; const s = "foo", p = 3; Object(s)[p] = "x"; s[p] = "x"; print("no error")})()'
#### ChakraCore

TypeError: Assignment to read-only properties is not allowed in strict mode

#### engine262

TypeError: Cannot set property '3' on 'foo'

#### Hermes, V8

TypeError: Cannot create property '3' on string 'foo'

#### JavaScriptCore

TypeError: Attempted to assign to readonly property.

#### Moddable XS

TypeError: ?: set 0: not extensible

#### SpiderMonkey

TypeError: can't assign to property 3 on "foo": not an object

I would have assumed String Exotic object [[DefineOwnProperty]] had a check for the this value being the string primitive or an object wrapper, but that doesn't seem to be it, so I'm not sure.

Edit: I assumed wrong and the check is done earlier in OrdinarySetWithOwnDescriptor step 2.b. If Type(Receiver) is not Object, return false.

@ljharb
Copy link
Member

ljharb commented Nov 13, 2021

I think it might be in the actual assignment semantics, but I’m not certain. Either way, I’d assume assigning to any non-nullish primitive behaved identically as any other, in sloppy mode and strict mode.

@mhofman
Copy link
Member

mhofman commented Nov 13, 2021

@nicolo-ribaudo could you clarify your implementation feedback?

The current spec text doesn't seem to allow #{ x: 1 }.y = 2 whether in strict mode or not, neither does it allow Object.defineProperty on a record wrapper object (unless same description of course).

Why would [[Set]] and [[DefineOwnProperty]] operations that allow those be better for implementations? Like proxy objects that lazily fetch their properties on access, you should be able to lazily surface the properties of the record wrappers while disallowing the actual operations that modifies the properties?

@ljharb while I agree it'd be more consistent with other primitives, I'm concerned the [[Set]] semantics in non-strict mode are a foot gun for this new primitive type (we shouldn't be bound to sloppy compatibility on this). And it'd probably be weird to disallow [[Set]] but to allow [[DefineOwnProperty]], but that's more debatable. Also, I have even less of an opinion whether the restriction should only apply to string properties, or to symbol properties as well.

@ljharb
Copy link
Member

ljharb commented Nov 13, 2021

To be fair, i wouldn't object to a new primitive behaving strictly even in sloppy mode, since that seems in keeping with the spirit of strict mode - altho that might break code that assumes such assignment will never throw.

@mhofman
Copy link
Member

mhofman commented Nov 13, 2021

Arguably the following is still a sloppy mode footgun:

const r = #{ x: 1 };
const s = Symbol();
r[s] = 'foo';

So while having [[Set]] disallow any assignment but allow [[DefineOwnProperty]] to define symbols might be weird, it probably is the only sensible approach if you want to allow symbols on wrapper objects. I still find it questionable one would want to defineOwnProperty of strings properties on record wrapper though.

@mhofman
Copy link
Member

mhofman commented Nov 13, 2021

that might break code that assumes such assignment will never throw.

Actually I forgot, and sloppy would never throw in that case. Even if [[Set]] returns false, sloppy code assignment would not cause a runtime error. For example the following code doesn't throw in sloppy mode:

var o = Object.freeze({});
o.foo = 'bar';
console.log(JSON.stringify(o)); // "{}"

So I suppose there is no way to cause a runtime error for a property assignment to a non-null-ish primitive, unless we drastically change PutValue to special case records and tuples, which seems wrong.

That also means that unless we want to allow explicitly defining extra string properties while disallowing assignment, there is probably no reason to have a special [[Set]] operation on the record/tuple exotic objects, and the ordinary set delegating to [[DefineOwnProperty]] would be more consistent.

@acutmore
Copy link
Collaborator

Object.defineProperty(String.prototype, 'no', { set() { throw 'nope' } });
'yes?'.no = 1; // ⚠️ 'nope'

A little part of me wishes that web JS consoles were in strict-mode by default, as I feel that is where many people play around with new code/features and may not realise that they are getting slightly different semantics to when they try the same thing in a module/class. So I personally lean ever so slightly towards records&tuples primitives (not the object wrappers) still throwing on assignment in sloppy mode - even if that is not how the existing primitives behave.

@ljharb
Copy link
Member

ljharb commented Nov 13, 2021

@acutmore if there were a non-eval way to get back sloppy mode, that'd probably already be the case.

I think the possibility it might break code likely guarantees that assignment to a non-object can never throw in sloppy mode. I've had packages break this way when people put them in strict mode, because they were designed for sloppy mode.

@acutmore
Copy link
Collaborator

Maybe one day consoles will log a little warning when specific sloppy mode behaviours are invoked, to help people using consoles as a language schoolroom/playground.

@mhofman
Copy link
Member

mhofman commented Nov 13, 2021

I'd love if consoles had a toggle to switch between sloppy and strict mode. But the problem here IMO is that the semantics of assignment in sloppy mode are pretty well defined, even for new types (symbols) and new object behaviors (non-extensible): it should never throw if it's non null-ish. So I'd be in favor of not doing deep surgery to PutValue and keep those semantics for record/tuple primitives, especially since the goal is to make them behave as much as possible as frozen objects.

So to sum up, my personal opinion on this is:

  • Do not allow define of new strings properties on record/tuple exotic objects. It's inconsistent with other primitive wrappers, but unlike those other primitives, property access and enumeration is the common way to use records and tuples *
  • Undecided whether to allow defining symbol properties on exotic objects. The only spec precedent I can think of is the module namespace exotic object, which is written like it wants to allow defining symbol props, but in practice doesn't. @ljharb seem to have a "participating in protocols" use case I do not understand.
  • The "set" operation would be the ordinary set. Aka if you can define a symbol property, you can set it through assignment to the object wrapper. The ordinary set semantics has a check that the receiver is an object instead of a primitive, so assignment to the primitive form would still throw in strict mode.

*: I believe we should prevent the following:

const r = #{ x: 1 }
const o = Object(r);
Object.defineProperty(o, y, {value: 2, enumerable: true});
o.y !== r.y;
Record(o) !== r;

@ljharb
Copy link
Member

ljharb commented Nov 14, 2021

I think it would be very weird to do that.

I think that snippet is perfectly reasonable and should work as-is (meaning, mutations to the boxed Record do not affect the internal slot Record)

@acutmore
Copy link
Collaborator

acutmore commented Nov 14, 2021

Yes, I don't think anyone is suggesting that mutations to the object-wrappers would mutate the actual real record/tuple.

const r = #{x: 1};
const o1 = Object(r);
const o2 = Object(r);
const o3 = {...r};
new Set([o1, o2, o3]).size === 3; // 3 separate objects have been created

o3.y = 1; // this is OK and will succeed
o2.y = 1; // this could also be OK?, as it only mutates o2

o3.x = 2; // this is OK and will succeed
o2.x = 2; // this will fail (sloppy-silently/strict-exception)

Considering the prevalence of using Object(v) as a way to test v [ref] it seems valuable to ensure that Object(possibleRecordOrTuplePrimitive) is specced in a way that engines can more easily return a fast result.
And from what @nicolo-ribaudo has said, in SpiderMonkey it is more likely that it will be slower if the returned value is non-extensible, because it will eagerly copy all properties.

@acutmore
Copy link
Collaborator

In #142 it was decided to use Record as the way to expose a way to directly read the original [[RecordData]] value.

If the wrapper is extensible this would mean that any extended/introduced properties on the record-object-wrapper would be ignored when going back to a record-primitive. This could be confusing.

@ljharb
Copy link
Member

ljharb commented Nov 14, 2021

If engines are concerned with speed, it seems like a very trivial optimization that Object(x) === x need not actually ever create an object at all; it could be silently replaced with typeof x === 'function' || (x && typeof x === 'object') or similar. I don't think we should be bending over backwards creating discontinuities in the language to allow optimization of a case that's already optimizable.

@nicolo-ribaudo
Copy link
Member

I am exploring how many places would need to be special-cased in SpiderMonkey to handle R&T wrappers that look as if they are frozen but internally can still be mutated, and they are way less than I initially expected. I think we can ignore and just keep the discussion to be based on user experience.

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Dec 20, 2021
The proposal currently specifies R&T wrappers as frozen.

This patch makes them look as if they were frozen, while still internally
keeping them marked as extensible. This allows lazily resolving the
wrapper properties (so that we can avoid a full copy every time we
create the wrapper object), but they are exposed as non-extensible to JS
code.

It's still possible that the proposal will switch to make them mutable;
you can follow the discussion (and participate!) at
tc39/proposal-record-tuple#137.

Differential Revision: https://phabricator.services.mozilla.com/D131124
@rricard
Copy link
Member Author

rricard commented Jul 8, 2022

We had a R&T community meeting recently in which we stated that we would prefer frozen wrappers. @ljharb stated that he would provide examples of non-frozen wrapper use cases.


Here is the content of the slide we intend to bring up to committee:

Record & Tuple exotic object wrappers are defined as frozen:

  • Record.isRecord/Tuple.isTuple return true for wrappers as well
  • Record(recordWrapper) effectively unwraps, returning underlying primitive
  • Frozen matches the mental model of R&T and helps catch bugs
    • Critical to maintain integrity for access to Record properties
  • Polyfilling implications
    • Tuple.prototype is mutable and methods/protocols can be added there
    • Record.prototype is null: no anticipated need to polyfill

@rricard rricard changed the title Should we allow the boxing object to have additional properties Should we allow the wrapper object to have additional properties Jul 25, 2022
@rricard rricard changed the title Should we allow the wrapper object to have additional properties Should we allow the wrapper object to have additional symbol properties Jul 25, 2022
@rricard
Copy link
Member Author

rricard commented Jul 25, 2022

After discussing both in the last meeting and after with the champion group, we would like to limit this discussion to only adding the capability to add symbol properties to a R&T wrapper object.

The advantage is that we wouldn't leave the possibility to create interfering string properties to record wrappers while keeping them open to add symbol properties on them: we are still awaiting use cases on it but it seems like being able to add symbol-based protocols could be useful.

Our champion group still prefers a completely readonly wrapper object but a symbol are writeable-only version of this would work if well motivated.

@rricard rricard mentioned this issue Jul 25, 2022
25 tasks
@ljharb
Copy link
Member

ljharb commented Jul 26, 2022

Only being able to add symbol properties to a boxed Record is perfectly acceptable to me, fwiw (and covers my use cases).

@acutmore
Copy link
Collaborator

(and covers my use cases).

Going further than the abstract/technical use case of being able to create an object that will both pass a record brand check and also participate in a symbol based protocol. If possible, it would be great to have a more concrete use case of where this might be useful to help motivate the additional complexity compared to "being frozen", which is something engines already implement.

@ljharb
Copy link
Member

ljharb commented Jul 27, 2022

When I’m back from vacation I’ll try to write something up.

@acutmore
Copy link
Collaborator

When I’m back from vacation I’ll try to write something up.

Thanks! Have a great vacation 🏝

@rricard
Copy link
Member Author

rricard commented Sep 27, 2022

We changed direction since then and made the wrapper an ordinary frozen object with an internal slot that is checkable using Record[Symbol.hasInstance] like any other primitives. Allowing additional symbol properties would mean this object would become exotic.

@rricard rricard closed this as completed Sep 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

8 participants