third batch of suggested changes for PR #2547 #2657
Conversation
Set() returns "a Completion Record normally containing a Boolean", so PutValue's 4.c Return ? Set(...) would cause PutValue to possibly return a Boolean, which doesn't appear to be what's wanted. (Every op that has "Return ? PutValue(...)" claims that its only normal return is ~unused~.)
As it stands, PDE returns "a Completion Record normally containing a Boolean", but this is incorrect... - the 2nd definition has `Return ? CopyDataProperties`, which will return an Object, - the 4th definition has `Return ~empty~`, and - the 5th definition has `Return ? MethodDefinitionEvaluation ...`, which might return a PrivateElement. However, rather than add these possibilities into the return type, we can actually simplify it. Note that the spec has 3 calls to PDE: - 2 are `Perform ? PDE of ...`, and - 1 is `Return ? PDE of ...`, but it's within the definition of PDE itself, which means that ultimately, nobody cares about PDE's normal return. So it might as well return ~unused~.
because there's already 3 return-types that are "a Generator". (It would be good to get clear direction on this from the rest of the spec, but it's inconsistent.)
jmdyck
changed the title
| @@ -6497,7 +6498,7 @@ <h1> | |||
| _F_: a constructor, | |||
| optional _argumentsList_: unknown, | |||
| optional _newTarget_: a constructor, | |||
| ): a Completion Record normally containing an ECMAScript language value | |||
| ): a Completion Record normally containing an Object | |||
There was a problem hiding this comment.
Only if we also change [[Construct]] to return "a Completion Record normally containing an Object", right?
There was a problem hiding this comment.
Yes, sorry, those should change as well.
| @@ -12159,7 +12160,7 @@ <h1>Agents</h1> | |||
| </emu-note> | |||
|
|
|||
| <emu-clause id="sec-agentsignifier" type="abstract operation"> | |||
| <h1>AgentSignifier ( ): an opaque value used to identify an Agent</h1> | |||
| <h1>AgentSignifier ( ): an agent signifier</h1> | |||
There was a problem hiding this comment.
If we want to use this term, we should <dfn> it and change the description of the [[Signifier]] field. Not sure that's in scope for this PR though, and the type we have currently works well enough for now.
There was a problem hiding this comment.
We've been using "an agent signifier" as a parameter-type since the memory model was added. Is there a higher bar for using it as a return-type? In the absence of a <dfn> (for now), surely we should at least use one of the existing terms for it rather than invent a new one. E.g.:
- Agent Record's
[[Signifier]]says it's "a globally-unique value". - Agent Events Record's
[[AgentSignifier]]says it's "a value that admits equality testing". - AddWaiter, RemoveWaiter, SuspendAgent, and NotifyWaiter say it's "an agent signifier".
There was a problem hiding this comment.
I am fine with using a term like "agent signifier" as long as it's obvious (it's not), self-describing (also not), or there's a <dfn> for it. Already having something that's none of those as a parameter type is unfortunate, but I don't think we should further propagate it without adding a <dfn>. It should be pretty simple to just use the description here in a <dfn>.
An <dfn variants="agent signifiers">agent signifier</dfn> is a globally-unique opaque value used to identify an Agent.| </h1> | ||
| <dl class="header"> | ||
| </dl> | ||
| <emu-grammar>PropertyDefinitionList : PropertyDefinitionList `,` PropertyDefinition</emu-grammar> | ||
| <emu-alg> | ||
| 1. Perform ? PropertyDefinitionEvaluation of |PropertyDefinitionList| with argument _object_. | ||
| 1. Return ? PropertyDefinitionEvaluation of |PropertyDefinition| with argument _object_. | ||
| 1. Perform ? PropertyDefinitionEvaluation of |PropertyDefinition| with argument _object_. |
There was a problem hiding this comment.
These still have to return ~unused~. Algorithms cannot reach the end without hitting a return. That would be an editorial error.
There was a problem hiding this comment.
Algorithms cannot reach the end without hitting a return. That would be an editorial error.
Not currently, and nothing as of this PR technically says that, I think? I agree it's best avoided at least until we give a definition, but I don't think it's clearly an editorial error for an AO not to return as long as the AO is only invoked with Perform.
There was a problem hiding this comment.
If it's not defined, it's not permitted IMO. Anyway, we shouldn't do it here or anywhere else for now.
There was a problem hiding this comment.
Return isn't defined either. We don't define lots of our editorial conventions.
But agreed about not doing it for now.
There was a problem hiding this comment.
I've been meaning to explicitly document at least the early exit behaviour of "return" in the algorithm conventions section.
| @@ -41525,7 +41526,7 @@ <h1> | |||
| RemoveWaiters ( | |||
| _WL_: a WaiterList, | |||
| _c_: a non-negative integer or +∞, | |||
| ): a List of agents | |||
| ): a List of agent signifiers | |||
There was a problem hiding this comment.
A WaiterList is a semantic object that contains an ordered list of those agents that are waiting [...]
We should update the WaiterList definition to match whatever we do here.
|
We'll make changes manually in #2547. |
No description provided.