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

Editorial: clarify ordinary and exotic object definitions and creation #1460

Open
wants to merge 7 commits into
base: master
from

Conversation

Projects
None yet
6 participants
@domenic
Copy link
Member

commented Feb 28, 2019

Closes #1453. Closes #1437.

Main changes

  • Defines ordinary objects as having the ordinary internal methods, and exotic objects as not having the ordinary internal methods.

  • Introduces AllocateBasicObject, which now is the only source of object creation, centralizing the undefined phrase "a newly created object" or "newly created X exotic object" into one location.

  • Introduces explicit definitions for every type of exotic object in terms of how they override the internal methods. This makes phrases like "x is an Array exotic object" well-defined.

  • Renames ObjectCreate to OrdinaryObjectCreate, and clarifies how it should be used.

Related changes to object creation

  • Fixes immutable prototype exotic objects to not inaccurately state that they always have default internal methods besides [[SetPrototypeOf]]; this is not the case for web platform objects, for example. This involved then expanding the definition of %ObjectPrototype% a bit to be more explicit about its internal slots and methods.

  • Improves missing or contradictory internal slot installation, e.g. the introduction for function objects said they had "the same internal slots" as other ordinary objects, but FunctionAllocate installed a list of slots that was missing [[Prototype]] and [[Extensible]].

  • Deduplicates setting [[Extensible]] to its default true value.

  • Clarifies with a note that CreateUnmappedArgumentsObject does not create an exotic object, despite being in the "Arguments Exotic Objects" clause.

  • Slightly reduces the coupling between IntegerIndexedObjectCreate and CreateTypedArray by changing how arguments are passed.

Drive-by fixes

  • Uses the phrase "bound function exotic object" uniformly instead of sometimes "bound function" or "bound function object".

This will involve changes to both HTML and Web IDL, mostly the renaming of ObjectCreate, but possibly others.

One thing I am waffling on is whether AllocateBasicObject should take a proto parameter at all. I am thinking probably it is best to remove. Since you have to pass [[Prototype]] anyway, AllocateBasicObject(« [[x]], [[Prototype]] », proto) is not much worse than a two steps where the second explicitly sets [[Prototype]], and it makes AllocateBasicObject more basic and less coupled.

Edit: I flipped it to remove the proto parameter.

Note that technically "a newly created TypeError object" and its ilk are still ill-defined after this change, but that's pretty minor.

I'll also note that I didn't touch the built-in function objects section, which is kind of a mess of underdefined and confusing things as witnessed by many previous GitHub and es-discuss threads.

Editorial thoughts especially welcome; I think there were some choices e.g. about referencing tables vs. listing slots explicitly, or how to write the intro paragraphs for each type of exotic object, which could be revisited or better.

@ljharb ljharb requested review from zenparsing, ljharb, allenwb, bterlson and tc39/ecma262-editors Feb 28, 2019

spec.html Outdated

<emu-alg>
1. Let _obj_ be a newly created object with an internal slot for each name in _internalSlotsList_.
1. Set _obj_'s essential internal methods to the default ordinary object definitions specified in <emu-xref href="#sec-ordinary-object-internal-methods-and-internal-slots"></emu-xref>.

This comment has been minimized.

Copy link
@allenwb

allenwb Feb 28, 2019

Member

I think this should explicitly say that after returning from this operation only the direct caller of
AllocateBaseObject may modify the definition of the returned object's essential internal methods.

Basely, we don't what to imply that it is possible for internal method definitions to change at arbitrary times during the entire lifetime of an object.

This comment has been minimized.

Copy link
@domenic

domenic Feb 28, 2019

Author Member

Done, thank you! Also added more asserts on callers relating to the interaction with non-overridden methods.

spec.html Outdated
</emu-alg>

<emu-note>
<p>Apart from being slightly easier to call than AllocateBasicObject, using OrdinaryObjectCreate communicates the intention to create an ordinary object, and not an exotic one. Thus, it must not be called as part as of an algorithm that subsequently modifies the internal methods of the object in ways that would make the result non-ordinary. In such cases, use AllocateBasicObject. (Even if it means having to include [[Prototype]] and [[Extensible]] in the internal slots list manually.)</p>

This comment has been minimized.

Copy link
@allenwb

allenwb Feb 28, 2019

Member

I see that this somewhat covers the point I mentioned about mutating the set of essential internal methods. I think it is good to say it both places.

@ljharb ljharb removed the request for review from bterlson Feb 28, 2019

spec.html Outdated

<p>Immutable prototype exotic objects have the same internal slots as ordinary objects. They are exotic only in the following internal methods. All other internal methods of immutable prototype exotic objects that are not explicitly defined below are instead defined as in <a href="#sec-ordinary-object-internal-methods-and-internal-slots">ordinary objects.</a></p>
<emu-note>
<p>Unlike other exotic objects, there is not a dedicated creation abstract operation provided for immutable prototype exotic objects. This is because they are only used by %ObjectPrototype% and by host environments, and in host environments, the relevant objects are potentially exotic in other ways and thus need their own dedicated creation operation.</p>

This comment has been minimized.

Copy link
@allenwb

allenwb Feb 28, 2019

Member

I wondered whether we should add an ImmutablePrototypeObjCreate, but I'm ok with not having it for now since we would host object to define there on xxxCreate operations that incorporate this. Perhaps the sort of thing that should be explained in a FAQ on defining "host objects"

Suggested change
<p>Unlike other exotic objects, there is not a dedicated creation abstract operation provided for immutable prototype exotic objects. This is because they are only used by %ObjectPrototype% and by host environments, and in host environments, the relevant objects are potentially exotic in other ways and thus need their own dedicated creation operation.</p>
<p>Unlike other exotic objects, there is not a dedicated creation abstract operation provided for immutable prototype exotic objects. This is because they are only used by %ObjectPrototype% and by host environments, and in host environments, the relevant objects are potentially exotic in other ways and thus need their own dedicated creation operation.</p>
@allenwb
Copy link
Member

left a comment

General looks good, but see a couple comments.

@jmdyck

This comment has been minimized.

Copy link
Collaborator

commented Feb 28, 2019

@domenic, in a comment in issue #1453:

One thing I have run into is that per https://tc39.github.io/ecma262/#sec-ecmascript-function-objects, we say that function objects are ordinary objects.

It actually says that ECMAScript function objects are ordinary objects. ECMAScript function objects are a proper subset of the function objects specified by the ECMAScript spec. (See Issue #1273 for some discussion.)

However, they have an overriden [[Call]] internal method.

I wouldn't say "overridden". They certainly do have a [[Call]] internal method, but it doesn't override some prior behaviour.

So I guess the definition of "ordinary object" (besides the glib one) is "has the default internal methods, or maybe has a [[Call]] as specified for function objects". Kind of unsatisfying, but I will not change anything for now.

How it seems to work in 6.1.7.2 Object Internal Methods and Internal Slots is that if an object has a [[Call]] internal method, then [[Call]] is an essential internal method for that object. Similarly for [[Construct]] (although some of the wording around Table 6 implies that [[Construct]] is essential for all function objects, which of course it isn't).

So you could say that an ordinary object "has the default behaviour for all of its essential internal methods" and that will include [[Call]] and [[Construct]] as appropriate. (Mind you, the spec doesn't currently say it that way.)

@zenparsing
Copy link
Member

left a comment

I would definitely like to see the OrdinaryObjectCreate rename. I think that's a clear, easy win. I'm a bit less sure of the other changes and will need more time to think about them.

Would you be opposed to submitting that change as it's own PR?

@domenic

This comment has been minimized.

Copy link
Member Author

commented Feb 28, 2019

I'm sorry to say I would be. Personally, I think the rename is sad, because of all the churn it will cause. But I would be willing to take that hit if we also actually clarified the attendant issues. For example, adding normative requirements that OrdinaryObjectCreate should only use be used to create ordinary objects, and providing clear instructions for how to create exotic objects.

Just renaming by itself doesn't improve clarity, and causes lots of specs to need updates. Indeed, I think it would reduce clarity, since now we'd have something named OrdinaryObjectCreate which does the exact same three steps as many exotic object creation routines also include.

@zenparsing
Copy link
Member

left a comment

@domenic Totally understand, and thanks for this PR. After reading through, I like everything here, except perhaps for a nit over the name "AllocateBasicObject". As a reader, I'm not really sure what "basic" implies, since it's not used elsewhere. Do you have any thoughts on the naming? Would "AllocateObject" be sufficient?

@allenwb

This comment has been minimized.

Copy link
Member

commented Mar 1, 2019

How about AllocateObjectStructure or AllocateObjectFoundation or AllocateObjectCore or AllocateCommonObjectCore or ...

Or perhaps substitute "Make" for "Allocate" in any of the above.

the point is that this operation isn't trying to introduce a new kind of entity into the spec. It is just there to factor common behavior out of the xxxObjectCreate operations

@domenic

This comment has been minimized.

Copy link
Member Author

commented Mar 1, 2019

I am happy with any of those names, and if the editor(s) can come to a final decision, I will change it. Just don't want to do it twice :).

@jmdyck

This comment has been minimized.

Copy link
Collaborator

commented Mar 2, 2019

@zenparsing

I like everything here, except perhaps for a nit over the name "AllocateBasicObject". As a reader, I'm not really sure what "basic" implies, since it's not used elsewhere.

That's what I was thinking too. Maybe replace "basic" with "default" (e.g. AllocateDefaultObject or MakeDefaultObject), alluding to the default ordinary definitions for the essential internal methods.

@allenwb

This comment has been minimized.

Copy link
Member

commented Mar 2, 2019

I think MakeDefaultObject is the best alternative.

I prefer "Make" over "Allocate" because "Allocate" sounds too concrete and suggestive of an implementation detail.

@domenic

This comment was marked as resolved.

Copy link
Member Author

commented Mar 2, 2019

@jmdyck is it possible for you to use GitHub's review feature, so that all your comments come through as a single review, instead of many individual reviews? That would make it easier to tell when the review is done, would reduce the number of emails sent to one, and would group the review together so that future interactions are distinct from your current one. Thanks!

@jmdyck

This comment was marked as resolved.

Copy link
Collaborator

commented Mar 3, 2019

@jmdyck is it possible for you to use GitHub's review feature, so that all your comments come through as a single review, instead of many individual reviews?

Ah, okay, will do.

That would make it easier to tell when the review is done,

(That's assuming I know when the review is done.)

@jmdyck

This comment was marked as resolved.

Copy link
Collaborator

commented Mar 3, 2019

@domenic: For cases where I'm proposing a specific edit, it could go in:

  • a comment,
  • a "suggested change" (assuming it's only 1 line), or
  • a PR against the original branch.

Do you have a preference?

@ljharb

This comment was marked as resolved.

Copy link
Member

commented Mar 3, 2019

@jmdyck use github’s PR review feature, and the “suggestion” button on each comment where you have a specific edit - and then you can post them all in one batch.

Show resolved Hide resolved spec.html Outdated
Show resolved Hide resolved spec.html Outdated
Show resolved Hide resolved spec.html Outdated
Show resolved Hide resolved spec.html Outdated
Show resolved Hide resolved spec.html Outdated
Show resolved Hide resolved spec.html
Show resolved Hide resolved spec.html Outdated
Show resolved Hide resolved spec.html Outdated
Show resolved Hide resolved spec.html Outdated
Show resolved Hide resolved spec.html Outdated
Show resolved Hide resolved spec.html Outdated
Show resolved Hide resolved spec.html Outdated
Show resolved Hide resolved spec.html Outdated

domenic added some commits Feb 28, 2019

Editorial: clarify ordinary and exotic object definitions and creation
Closes #1453.

### Main changes

* Defines ordinary objects as having the ordinary internal methods, and exotic objects as not having the ordinary internal methods.

* Introduces AllocateBasicObject, which now is the only source of object creation, centralizing the undefined phrase "a newly created object" or "newly created X exotic object" into one location.

* Introduces explicit definitions for every type of exotic object in terms of how they override the internal methods. This makes phrases like "x is an Array exotic object" well-defined.

* Renames ObjectCreate to OrdinaryObjectCreate, and clarifies how it should be used.

### Related changes to object creation

* Fixes immutable prototype exotic objects to not inaccurately state that they always have default internal methods besides [[SetPrototypeOf]]; this is not the case for web platform objects, for example. This involved then expanding the definition of %ObjectPrototype% a bit to be more explicit about its internal slots and methods.

* Improves missing or contradictory internal slot installation, e.g. the introduction for function objects said they had "the same internal slots" as other ordinary objects, but FunctionAllocate installed a list of slots that was missing [[Prototype]] and [[Extensible]].

* Deduplicates setting [[Extensible]] to its default true value.

* Clarifies with a note that CreateUnmappedArgumentsObject does not create an exotic object, despite being in the "Arguments Exotic Objects" clause.

* Slightly reduces the coupling between IntegerIndexedObjectCreate and CreateTypedArray by changing how arguments are passed.* Uses the phrase "bound function exotic object" uniformly instead of sometimes "bound function" or "bound function object".

### Drive-by fixes

* Gives "array index" a definition instead of just making it italicized.

@domenic domenic force-pushed the domenic:explicit-exotics branch from 09046f8 to a64c37b Apr 26, 2019

@domenic

This comment has been minimized.

Copy link
Member Author

commented Apr 26, 2019

I've updated this PR with most of @jmdyck's feedback. (I disagreed with some of it.) I'd love if the editors would take another look. I'm also happy to rename AllocateBasicObject to whatever they prefer.

Show resolved Hide resolved spec.html
Show resolved Hide resolved spec.html
@@ -4504,6 +4505,22 @@ <h1>Strict Equality Comparison</h1>
<emu-clause id="sec-operations-on-objects">
<h1>Operations on Objects</h1>

<emu-clause id="sec-allocatebasicobject" aoid="AllocateBasicObject">
<h1>AllocateBasicObject ( _internalSlotsList_ )</h1>

This comment has been minimized.

Copy link
@ljharb

ljharb May 2, 2019

Member

i have a complete lack of opinion here about the name; ECMAScriptObjectCreate maybe, but this one is fine too

Show resolved Hide resolved spec.html
Show resolved Hide resolved spec.html
Show resolved Hide resolved spec.html
Show resolved Hide resolved spec.html
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.