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 Record wrappers have a null prototype? #71

Closed
3 tasks done
bmeck opened this issue Sep 23, 2019 · 38 comments
Closed
3 tasks done

Should Record wrappers have a null prototype? #71

bmeck opened this issue Sep 23, 2019 · 38 comments

Comments

@bmeck
Copy link
Member

bmeck commented Sep 23, 2019

  • My issue is not bikeshedding. (we can bikeshed at
    a later proposal stage
    )
  • My issue is not a request to add lots of stuff to
    the semantics. (we can add things in follow-on proposals)
  • My issue is appropriate for a stage 0 proposal and not too
    in-the-weeds.

I have concerns about value types using prototypes. Object.prototype is soo widely used that mutating and adding properties to it is virtually impossible without Symbols I feel like the same would become true for Record quickly. Likewise, adding methods to Array is very fraught with compatibility problems and have similar concerns for Tuple. Do we have reasons that we want property delegation to occur on the value itself vs something like using static methods instead? Is the usage of mutable prototypes intended to be for consistency or some other reason?

Could prototypes be added later if desired? It seems mutable prototypes goes against some of the intentions of immutable structures as properties on the prototype could be added or removed at runtime thereby affecting things like record.isSafe from const record = {| foo: 1 |} by the prototype delegation.

@ljharb
Copy link
Member

ljharb commented Sep 23, 2019

Not speaking to the reasons, but: every non-nullish primitive must have a boxed object form and thus a prototype; this is an axiom of the language so far. iow, Object(record).__proto__ must be an object - and a mutable one, or else additions can't be polyfilled later - or else null, which would prevent toString, Symbol.toStringTag, and other generic protocols from working (without special cases for these new primitive types)

@bmeck
Copy link
Member Author

bmeck commented Sep 24, 2019

I'm not sure I understand the multiple must statements as axioms as these value types are not necessarily primitives, which already is a loose term as the definition of primitive isn't set beyond a list of types.

which would prevent toString, Symbol.toStringTag, and other generic protocols from working (without special cases for these new primitive[sic] types)

This is a good point, but I think the weight of having mutable immutable values is enough of a counter to warrant more discussion than an abrupt statement about requiring a single design. These types do need protocols to be cased out in the spec even if they delegate so there effectively is special casing in some sense. It seems the value of these types are high enough that we should weigh if they are special, and if they are not special why they shouldn't be inheriting from Array or Object as they would act much like frozen forms with validated construction if they are forced to behave the same way as those types.

It seems discussion can come towards documenting or exploring:

  • Can these not have access to prototypes directly? Do they need boxing?
  • What does it mean to extend these types, like other builtins?
  • If we were forced to use prototypes, why are we not using the already existing types as the super types?
  • If we were to not use prototypes, what would the differences that could be pro/con?

@ljharb
Copy link
Member

ljharb commented Sep 24, 2019

@bmeck they're either primitives or objects; each category brings with it a number of criteria for consistency, such as:

  • Object(x) === x for objects and Object(x) !== x for primitives
  • all primitives can't be used as WeakMap keys, all objects can
  • all dot/bracket access on primitives actually applies to a boxed object, since primitives have no properties
  • Object.keys(primitive) and friends is always an empty array, because primitives can have no own properties.
  • Object(primitive) produces an object with a [[Prototype]] in the current realm (which could be null, i suppose)

etc.

@bmeck
Copy link
Member Author

bmeck commented Sep 24, 2019

I don't see how they must be one or the other, but do see value in keeping consistency. I would note there are some exceptions with strings and own-ish properties. Additionally, various things like x={__proto__:null};ObjectIntrinsicFromOtherRealm(x) === x does seem to avoid the cross Realm conversions so having a null [[Prototype]] seems to kind of evade the idea of primitives coercing to same Realm proto if they do not have prototypes. Primitives as WeakMap keys is interesting, because in the past there has been discussion about allowing symbols in WeakMaps. dot/computed access applying to a boxed object is a bit strange as well for string since it doesn't go through prototype for integer indexed properties as they somewhat have their own properties.

Overall these all seem up for discussion and not actual requirements to my knowledge and seem to already have a variety of edge cases in the spec.

@ljharb
Copy link
Member

ljharb commented Sep 25, 2019

Indeed, that is true - strings are an exotic legacy exception, one I would not wish to see repeated.

In your case, that's because x is an object, so ObjectIntrinsicFromAnyRealm(x) makes no changes to x - that only applies to primitives. There are no current primitives that box to an object with a null [[Prototype]], so there's no way to test what that would do.

@bmeck
Copy link
Member Author

bmeck commented Sep 25, 2019

I don't see how the string issues like having keys would be prevented, Tuples and Records have their "own" properties.

@littledan
Copy link
Member

littledan commented Jan 20, 2020

#{ }.__proto__ === null makes sense to me; what gets trickier is #[].__proto__. I think it's pretty important that we support Array-like methods somehow or other on Tuples. The most closely analogous way would be if they are just methods that can be called, which would be easiest to put on Tuple.prototype.

Something I don't understand is how this case differs from, say, "".__proto__, which is String.prototype, even though Strings, like Tuples, are built to be based on integer indexed property access, and String.prototype has various methods. So, right now, I'm not convinced that Tuples should have a null prototype.

@bmeck
Copy link
Member Author

bmeck commented Jan 20, 2020

@littledan we saw presentations about issues with integer indexed types at TC39 and issues of how they are a great source of issues. I'm not necessarily convinced in light of that presentation that duplicating some pattern that has historically caused issues is a good path to take.

@littledan
Copy link
Member

@bmeck Huh, that doesn't match my takeaway from the presentation. I guess my takeaways were things like, 1) Don't forward integer property access up the prototype chain (we could do this whether or not we have a null prototype) 2) Don't go back and make pre-existing things subclassable/add another @@species/change their superclass hierarchy. I didn't get the sense that String semantics were a mistake and not to be repeated.

Anyway, it'd be great to get a review from @natashenka at some point on this proposal!

@rickbutton
Copy link
Member

rickbutton commented Mar 5, 2020

To bring the discussion back around to the original question in the issue, here are some thoughts I have regarding prototypes for records and tuples:

All non-nullish primitives so far have an exotic "boxing object" wrapper equivalent that has a prototype (it must, because it is an object). record and tuple primitives themselves do not have prototypes, it is only by an operation that creates a Record or Tuple exotic object do prototypes actually matter, (so, property access, or anything that boxes primitives). I don't see any reason to deviate from that pattern.

In terms of the exotic object wrapper's prototype for Record and Tuple, it makes sense to me that Record.prototype is either empty or null (it is currently empty in the explainer, and it's prototype is Object.prototype). Tuple.prototype is an analogue to Array.prototype, sharing methods common to array/tuple manipulation. I think that this is extremely useful, and has analogues in other primitives (String.prototype.substring for example, or any other String prototype method).

On mutable prototypes, or Record.prototype.isSafe = true as in your example @bmeck, I don't think that preventing this is a goal of the proposal, (in fact, it is useful for Tuple.prototype to be mutable so that future additions to the prototype can be polyfilled), and sounds like the domain of the SES proposal.

Further, on "custom prototypes", the current proposal does not specify any ability to define a "custom" prototype for a record or tuple. While I think this might be possible, it would require more investigation, and should in theory be possible in a follow-on if so desired (see #46 where this concept is also mentioned).

@ljharb
Copy link
Member

ljharb commented Mar 5, 2020

(It would still make sense to me to have Record.prototype.__proto__ === null)

@rickbutton
Copy link
Member

@ljharb I think that would make sense, preventing Object.prototype from polluting records seems desirable.

@bmeck
Copy link
Member Author

bmeck commented Mar 6, 2020

On mutable prototypes, or Record.prototype.isSafe = true as in your example @bmeck, I don't think that preventing this is a goal of the proposal, (in fact, it is useful for Tuple.prototype to be mutable so that future additions to the prototype can be polyfilled), and sounds like the domain of the SES proposal.

This doesn't seem to have grounding to me. The usefulness of Tuple.prototype is being stated as being useful to allow extending Tuple.prototype from what I am reading.

I don't believe this is in purely the realm of SES as prototypes and boxings do have affects when shared across Realms (whatever your VM/host is calling them / frames / whatever). If you mutate a prototype in 1, you have to mutate it in other Realms to keep the code consistent.

A lot of my concerns are around if the previous path is a good path. Using a prototype seems to me as though we are just using a new namespace very similar to Object.prototype and stating that it won't be a problem this time around but I'm not seeing evidence about why it won't be a problem. I do see some claims that we already do things this way, so we should continue them, but not reasoning about why it differs from the problems of the heavily inherited builtins like Object since this proposal is very much in the same level of utility.

@littledan
Copy link
Member

@bmeck Do you have another suggestion for the semantics? Personally, I'm open to Record.prototype being null (or Record.prototype.__proto__ === null), but I have a really hard time seeing how a null prototype for Tuples could meet our goal of being analogous to Arrays (including both existing Array methods and future methods added through polyfills). Honestly, I'm still having trouble understanding what problem you're referring to, with the existing model; I think it's pretty sound.

@bmeck
Copy link
Member Author

bmeck commented Mar 9, 2020

@littledan I think we should think about the reasons we never add things to Object.prototype but do add things to Array.prototype and make a reasonable decision about that. My thoughts are as follows:

  • We never add things to Object.prototype largely because objects can use any possible name (and in particular literal syntax allows for any names) for a field and collisions make for odd behaviors (particularly if people conditionally add fields to objects). A large amount of lint errors exist for using behaviors from Object.prototype.
  • We do add things to Array.prototype but we never overload the properties that can be defined with an Array literal (numeric indices or .length). Expando properties on Array-like returns from jQuery/MooTools are not real arrays, but Prototype is still using a real Array and it causes us problems.
  • Anything we add to either faces increasingly uphill compatibility problems from existing usages (not necessarily naming problems) AND naming collisions.

Given the potential of banning holes on Tuples, and Tuples thus having a constrained set of property names being usable I think it is probably fine to use a prototype for Tuples as the collisions for own properties are purely on numeric indices and length which are likely not problematic for developers as the usage of Tuples is entirely coupled to items in numeric indices and the length. I do think there are problems with allowing overriding string named properties like then() in particular, but I don't think they are any greater than other specialized types having prototypes. Once again, even if the prototype is expanded, a reasonable developer can still rely on the indices and length for the intended usage of Tuples.

However, for Records I do not think it wise to give them a prototype. I cannot think of a reasonable way in which we could safely ever put anything on it and would think it to be the same for developers of libraries and applications. A developer must check for any sort of behavior alteration in order to understand how a Record may be used since they are effectively arbitrary key/values unlike Tuples. Given things like __proto__ CVEs, then, etc. gotchas if they are overriden in the Record literals I do not see a good path for extending the prototype in a way that is either reliable or not leading towards a dangerous path. Record is not a specialized type that has a well defined set of properties, if it were I believe it would be possible to argue that it should have a prototype.

A potential compromise is to give Tuples a prototype and a null prototype for Records or give Records a empty prototype that is not extensible for now and see if we can loosen it later if we want to continue arguing (strict mode is stuck throwing anyway); I believe that this is potentially fine even if they are not completely mimicking a duality Object/Array since Object.prototype has proven problematic and it would be extremely difficult to prove to me that Record is not in a similar situation without also proving that extending Object.prototype is reasonable/something people should rely on given the plethora of things like lint rules and dangers in the wild.

@ljharb
Copy link
Member

ljharb commented Mar 9, 2020

Record.prototype imo has to be an object, and a mutable one, to be able to polyfill future additions, including Symbol-based protocols that wouldn’t have any conflict with Record keys.

@bmeck
Copy link
Member Author

bmeck commented Mar 9, 2020

I do see issues in the wild with using a prototype, but those are based upon conflicts with own keys and things like spreading naively. If the prototype cannot have conflicts with keys, I have no concrete objections to a prototype. I do have strong objections to the conflicts as we have seen real world issues with Object.prototype

@littledan
Copy link
Member

@bmeck I like this compromise position you explain. I'd add that Tuple wrappers would be integer indexed exotic objects, and so not inherit indexed properties.

@ljharb Is there anything more you could say about future additions to Object.prototype that would be important to inherit?

@ljharb
Copy link
Member

ljharb commented Mar 9, 2020

@littledan No, to be clear, I think Object.getPrototypeOf(#{}) must not be null (because that prevents us from ever being able to add methods, or have it participate in protocols like "iterable", or like the discussed-but-not-yet-proposed "object spread" symbol, etc). I think it's fine if it in turn inherits from Object.prototype, but i don't see any downsides from Object.getPrototypeOf(Record.prototype) being null, because any relevant new protocols could be added to Record.prototype, whether or not they were added to Object.prototype.

@littledan
Copy link
Member

Right, my question was, if you could elaborate on the protocols that we might want to add later for Objects and Object-like things like Records. Thinking through the use cases could help us understand whether we need the Tuple-like treatment of allowing further methods. (Even if this need is clear to you, it might not be clear to everyone on the thread.)

@ljharb
Copy link
Member

ljharb commented Mar 9, 2020

There was discussion during the stage 4 advancement of object spread about a possible symbol protocol that could let an object override what Object.assign/spread saw; that would apply conceptually to records as well, I’d think.

@littledan
Copy link
Member

Maybe someone could develop that idea as a more detailed proposal/investigation, so we can understand the implications better. That could help us work through this issue. Anyone interested?

Are there any other ideas protocols that would potentially make sense? (Pattern matching???) Beyond this one, I'm pretty skeptical that it'd make sense to add more protocols to ordinary objects.

Even for spread, I could imagine that a symbol-based protocol could be used to override behavior, but then the default behavior (if no method is found) could be the current one. This would give a good combination of reliability for simple cases like Records with extensibility for other types.

@bmeck
Copy link
Member Author

bmeck commented Mar 10, 2020

I'd be interested in forming a document on any results we form about these design discussions and conclusions so that they can apply towards things in the future.

@pabloalmunia
Copy link

Sorry in advance if this is a silly question, but are the Tuple and Registry prototypes sealed? Is it possible to apply something like Object.setPrototypeOf (# {}, {...})?

@ljharb
Copy link
Member

ljharb commented Apr 7, 2020

@pabloalmunia you can't change the [[Prototype]] of a primitive; the prototype object is as mutable as any other prototype in the language.

@littledan
Copy link
Member

Right, the operation to set the prototype would throw when applied to Record or Tuple wrapper objects.

@ljharb If we do add a protocol for rest/spread, it will need to treat the absence of a Symbol-based method (i.e., Get returns undefined) to lead to the current behavior--this would be necessary for objects with a null prototype to work as they do currently. In this case, I think it would be fine for Records to also take that null-prototype path. (And maybe we shouldn't even bother adding the symbol to Object.prototype, I'm not sure.) That wouldn't exclude us from creating a protocol that gets used when the symbol is added, though. Do you have any ideas for other protocols that we'd need for Object.prototype, or reasons why this story wouldn't work?

@ljharb
Copy link
Member

ljharb commented Apr 15, 2020

In this case, I think it would be fine for Records to also take that null-prototype path.

I don't; if we added such a protocol, the presence of it would have to override the default behavior (which, you're correct, would be the same in the absence of the symbol). In other words, if i added Record.prototype[Symbol.whatever], I would expect it to be invoked on Records.

@pabloalmunia
Copy link

pabloalmunia commented Apr 15, 2020

In my humble opinion, you can write some as:

Record.prototype[Symbol.whatever] = () => ...;

#{}[Symbol.wathever]()

When you get the property, the abstract operation GetV (https://tc39.es/ecma262/#sec-getv) is called and this operation ToObject(V) is called too. As a result, you obtain the object wrapper before to get the property.

@ljharb
Copy link
Member

ljharb commented Apr 16, 2020

Yes, as long as Record primitives inherit from Record.prototype (see #71 (comment))

@littledan
Copy link
Member

@ljharb @bmeck @rricard @rickbutton @devsnek and I had a call where we discussed this issue. To summarize some key points:

  • @bmeck and I share the goal that, when doing string-keyed property access on Records, this operation should have integrity--it should only give the entries contained in the Record. Such integrity is analogous to how, for Records and Strings, integer-indexed property access similarly "has integrity" and doesn't forward up the prototype chain.
  • I argued that, as far as protocols which may be on Object.prototype, we will always need to provide a path for Objects with a null prototype, and Records could take this path. Further, as Records generally "have integrity", this is the path that we should follow. (I'm not sure whether @ljharb found this persuasive.)
  • @ljharb pointed out two more issues with a null prototype which I didn't understand previously from this thread:
    • If we don't have any sort of valueOf or Symbol.toPrimitive method, then there's no operation which brand-checks Records. We agreed that Record.isRecord would fit the bill--I'd be up for adding that (and generally, Type.isType checks, but we can go incrementally as we did with Array.isArray).
    • If we don't end up finding valueOf or Symbol.toPrimitive methods on Records, then we'd need extra logic in ToPrimitive if we wanted it to convert Record wrappers to Records.

After the call, I looked at the callers of ToPrimitive, and I think they all should throw when applied to Records and Tuples. Maybe we could think of ToPrimitive as "convert to atomic primitives" which doesn't include Records and Tuples--that seems to be what the current callers are after.

We discussed two possible paths
A: Record wrappers have a null prototype with the Record.isRecord check (and possibly extra ToPrimitive logic, but now that seems unnecessary to me)
B: Be exotic in that Records forward up symbol accesses to their prototype chain, while not forwarding up string property accesses.
- Then, we could support a Symbol.toPrimitive method which converts Records wrappers to Records, lessening the urgency of Record.isRecord and removing the need from considering any extra ToPrimitive logic. We could also support monkey-patchable protocols in the future.
- @rickbutton notes that this would permanently prevent us from permitting Symbol keys in Records

@ljharb concluded that, if we take the integrity goals at high priority, that he'd prefer A to B, as B seemed a bit too exotic.

I have trouble understanding the concern about wrappers being exotic objects, since lots of things (e.g., String and Tuple wrappers, and TypedArrays) behave exotically with integer indices, and Record wrappers will likely be specified as exotic objects anyway, but I'm fine with settling on A.

I'd like to tentatively conclude that, for now, we'd have a null prototype and follow option A, but be open to considering B until Stage 3, when we should draw a final conclusion. I think this exploration of the design space, where we've found multiple plausible options including one good enough for experimentation in the playground, is sufficient for demonstrating viability for Stage 2.

@littledan littledan changed the title Concerns of value types using prototypes. Should Record wrappers have a null prototype? May 26, 2020
@Jack-Works
Copy link
Member

B: Be exotic in that Records forward up symbol accesses to their prototype chain, while not forwarding up string property accesses.

Now we don't allow symbols in records. but what if we allow them in the future?

@littledan
Copy link
Member

@Jack-Works To select option B, we'd have to be OK with ruling out Symbols as property keys for Records in the future (or being OK with them not having these same integrity properties). I'm currently leaning towards option A.

@ljharb

This comment has been minimized.

@littledan
Copy link
Member

@ljharb The two were never in contrast...

@ghost
Copy link

ghost commented Oct 17, 2020

If a developer wants a prototype, while still maintaining immutability, would using __proto__ in the record literal be allowed?

Ex:

const myRecord = #{
    __proto__: Object.prototype
};

Allowing opt-in prototypes?

@ljharb
Copy link
Member

ljharb commented Oct 17, 2020

I'd hope not; my understanding is that it'd just be a normal string "__proto__" key.

@ghost
Copy link

ghost commented Oct 17, 2020

...it'd just be a normal string "__proto__" key.

Hmm... it would definitely break immediate script author's expectations, but considering that it's from annex B I would personally be fine with it.

So, that code above would just throw a type error due to the value of the simple string property "__proto__" being an object?

I really think I'm just too used to storing functions in arrays/objects, if/when this proposal passes and is implemented, I'll get used to it.

@rricard
Copy link
Member

rricard commented Jul 8, 2022

The current spec defines a null prototype for Records. This is a decision that has been working well so far. The rationale has been explained earlier by @littledan here: #71 (comment)

Finally, to clarify: ["__proto__"] is a normal string key in this situation, __proto__: something should be rejected so I opened #313 so we can clarify in the spec.

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

No branches or pull requests

7 participants