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.

Sign up for GitHub

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

third batch of suggested changes for PR #2547 #2657

Merged
merged 6 commits into from
Feb 9, 2022
Merged

third batch of suggested changes for PR #2547 #2657

Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
c4ba786
Tweak the body of PutValue
jmdyck Feb 8, 2022
af9e018
Simplify PropertyDefinitionEvaluation's return type
jmdyck Feb 8, 2022
d59d56a
Cases where we can use a more specific type
jmdyck Feb 9, 2022
79f4d96
Cases where we need to add another alternative to the return type
jmdyck Feb 9, 2022
e477f5d
Change 2x "a generator" to "a Generator"
jmdyck Feb 9, 2022
5048fd4
misc other fixes
jmdyck Feb 9, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
71 changes: 36 additions & 35 deletions spec.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1789,7 +1789,7 @@ <h1>
<h1>
Number::bitwiseNOT (
_x_: a Number,
): a Number
): an integral Number
</h1>
<dl class="header">
</dl>
Expand Down Expand Up @@ -1985,7 +1985,7 @@ <h1>
Number::leftShift (
_x_: a Number,
_y_: a Number,
): a Number
): an integral Number
</h1>
<dl class="header">
</dl>
Expand All @@ -2002,7 +2002,7 @@ <h1>
Number::signedRightShift (
_x_: a Number,
_y_: a Number,
): a Number
): an integral Number
</h1>
<dl class="header">
</dl>
Expand All @@ -2019,7 +2019,7 @@ <h1>
Number::unsignedRightShift (
_x_: a Number,
_y_: a Number,
): a Number
): an integral Number
</h1>
<dl class="header">
</dl>
Expand Down Expand Up @@ -2116,7 +2116,7 @@ <h1>
_op_: `&amp;`, `^`, or `|`,
_x_: a Number,
_y_: a Number,
): a Number
): an integral Number
</h1>
<dl class="header">
</dl>
Expand All @@ -2137,7 +2137,7 @@ <h1>
Number::bitwiseAND (
_x_: a Number,
_y_: a Number,
): a Number
): an integral Number
</h1>
<dl class="header">
</dl>
Expand All @@ -2151,7 +2151,7 @@ <h1>
Number::bitwiseXOR (
_x_: a Number,
_y_: a Number,
): a Number
): an integral Number
</h1>
<dl class="header">
</dl>
Expand All @@ -2165,7 +2165,7 @@ <h1>
Number::bitwiseOR (
_x_: a Number,
_y_: a Number,
): a Number
): an integral Number
</h1>
<dl class="header">
</dl>
Expand Down Expand Up @@ -4234,7 +4234,8 @@ <h1>
1. If IsUnresolvableReference(_V_) is *true*, then
1. If _V_.[[Strict]] is *true*, throw a *ReferenceError* exception.
1. Let _globalObj_ be GetGlobalObject().
1. Return ? Set(_globalObj_, _V_.[[ReferencedName]], _W_, *false*).
1. Perform ? Set(_globalObj_, _V_.[[ReferencedName]], _W_, *false*).
1. Return ~unused~.
1. If IsPropertyReference(_V_) is *true*, then
1. [id="step-putvalue-toobject"] Let _baseObj_ be ? ToObject(_V_.[[Base]]).
1. If IsPrivateReference(_V_) is *true*, then
Expand Down Expand Up @@ -4359,7 +4360,7 @@ <h1>
<h1>
FromPropertyDescriptor (
_Desc_: a Property Descriptor or *undefined*,
): an Object
): an Object or *undefined*
</h1>
<dl class="header">
</dl>
Expand Down Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only if we also change [[Construct]] to return "a Completion Record normally containing an Object", right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, sorry, those should change as well.

</h1>
<dl class="header">
<dt>description</dt>
Expand Down Expand Up @@ -10504,7 +10505,7 @@ <h1>HasSuperBinding ( ): *false*</h1>
</emu-clause>

<emu-clause id="sec-object-environment-records-withbaseobject" type="concrete method">
<h1>WithBaseObject ( ): an ECMAScript language value</h1>
<h1>WithBaseObject ( ): an Object or *undefined*</h1>
<dl class="header">
<dt>for</dt>
<dd>an object Environment Record _envRec_</dd>
Expand Down Expand Up @@ -10674,7 +10675,7 @@ <h1>GetThisBinding ( ): a Completion Record normally containing an ECMAScript la
</emu-clause>

<emu-clause id="sec-getsuperbase" type="concrete method">
<h1>GetSuperBase ( ): a Completion Record normally containing an ECMAScript language value</h1>
<h1>GetSuperBase ( ): a Completion Record normally containing an Object, *null*, or *undefined*</h1>
<dl class="header">
<dt>for</dt>
<dd>a function Environment Record _envRec_</dd>
Expand Down Expand Up @@ -11035,7 +11036,7 @@ <h1>WithBaseObject ( ): *undefined*</h1>
</emu-clause>

<emu-clause id="sec-global-environment-records-getthisbinding" type="concrete method">
<h1>GetThisBinding ( ): a Completion Record normally containing an ECMAScript language value</h1>
<h1>GetThisBinding ( ): a Completion Record normally containing an Object</h1>
<dl class="header">
<dt>for</dt>
<dd>a global Environment Record _envRec_</dd>
Expand Down Expand Up @@ -11680,7 +11681,7 @@ <h1>
<h1>
SetDefaultGlobalBindings (
_realmRec_: unknown,
): a Completion Record normally containing an ECMAScript language value
): a Completion Record normally containing an Object
</h1>
<dl class="header">
</dl>
Expand Down Expand Up @@ -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>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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".

Copy link
Member

@michaelficarra michaelficarra Feb 9, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

<dl class="header">
</dl>
<emu-alg>
Expand Down Expand Up @@ -13525,7 +13526,7 @@ <h1>
_key_: a property key or Private Name,
_closure_: a function object,
_enumerable_: a Boolean,
): a Private Name or ~unused~
): a PrivateElement or ~unused~
</h1>
<dl class="header">
</dl>
Expand Down Expand Up @@ -14020,7 +14021,7 @@ <h1>
ArraySpeciesCreate (
_originalArray_: unknown,
_length_: a non-negative integer,
): a Completion Record normally containing an ECMAScript language value
): a Completion Record normally containing an Object
</h1>
<dl class="header">
<dt>description</dt>
Expand Down Expand Up @@ -17344,7 +17345,7 @@ <h2>Syntax</h2>
</emu-note>

<emu-clause id="sec-static-semantics-tv" type="sdo" oldids="sec-static-semantics-tv-and-trv">
<h1>Static Semantics: TV ( ): a String</h1>
<h1>Static Semantics: TV ( ): a String or *undefined*</h1>
<dl class="header">
<dt>description</dt>
<dd>A template literal component is interpreted by TV as a value of the String type. TV is used to construct the indexed components of a template object (colloquially, the template values). In TV, escape sequences are replaced by the UTF-16 code unit(s) of the Unicode code point represented by the escape sequence.</dd>
Expand Down Expand Up @@ -18299,29 +18300,29 @@ <h1>Runtime Semantics: Evaluation</h1>
<h1>
Runtime Semantics: PropertyDefinitionEvaluation (
_object_: unknown,
): a Completion Record normally containing a Boolean
): a Completion Record normally containing ~unused~
</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_.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These still have to return ~unused~. Algorithms cannot reach the end without hitting a return. That would be an editorial error.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it's not defined, it's not permitted IMO. Anyway, we shouldn't do it here or anywhere else for now.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Return isn't defined either. We don't define lots of our editorial conventions.

But agreed about not doing it for now.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've been meaning to explicitly document at least the early exit behaviour of "return" in the algorithm conventions section.

</emu-alg>
<emu-grammar>PropertyDefinition : `...` AssignmentExpression</emu-grammar>
<emu-alg>
1. Let _exprValue_ be the result of evaluating |AssignmentExpression|.
1. Let _fromValue_ be ? GetValue(_exprValue_).
1. Let _excludedNames_ be a new empty List.
1. Return ? CopyDataProperties(_object_, _fromValue_, _excludedNames_).
1. Perform ? CopyDataProperties(_object_, _fromValue_, _excludedNames_).
</emu-alg>
<emu-grammar>PropertyDefinition : IdentifierReference</emu-grammar>
<emu-alg>
1. Let _propName_ be StringValue of |IdentifierReference|.
1. Let _exprValue_ be the result of evaluating |IdentifierReference|.
1. Let _propValue_ be ? GetValue(_exprValue_).
1. Assert: _object_ is an ordinary, extensible object with no non-configurable properties.
1. Return ! CreateDataPropertyOrThrow(_object_, _propName_, _propValue_).
1. Perform ! CreateDataPropertyOrThrow(_object_, _propName_, _propValue_).
</emu-alg>
<emu-grammar>PropertyDefinition : PropertyName `:` AssignmentExpression</emu-grammar>
<emu-alg>
Expand All @@ -18340,14 +18341,14 @@ <h1>
1. Let _propValue_ be ? GetValue(_exprValueRef_).
1. If _isProtoSetter_ is *true*, then
1. If Type(_propValue_) is either Object or Null, then
1. Return ! <emu-meta effects="user-code">_object_.[[SetPrototypeOf]]</emu-meta>(_propValue_).
1. Return ~empty~.
1. Perform ! <emu-meta effects="user-code">_object_.[[SetPrototypeOf]]</emu-meta>(_propValue_).
1. Return ~unused~.
1. Assert: _object_ is an ordinary, extensible object with no non-configurable properties.
1. Return ! CreateDataPropertyOrThrow(_object_, _propKey_, _propValue_).
1. Perform ! CreateDataPropertyOrThrow(_object_, _propKey_, _propValue_).
</emu-alg>
<emu-grammar>PropertyDefinition : MethodDefinition</emu-grammar>
<emu-alg>
1. Return ? MethodDefinitionEvaluation of |MethodDefinition| with arguments _object_ and *true*.
1. Perform ? MethodDefinitionEvaluation of |MethodDefinition| with arguments _object_ and *true*.
</emu-alg>
</emu-clause>
</emu-clause>
Expand Down Expand Up @@ -20430,7 +20431,7 @@ <h1>
_lval_: an ECMAScript language value,
_opText_: `**`, `*`, `/`, `%`, `+`, `-`, `&lt;&lt;`, `&gt;&gt;`, `&gt;&gt;&gt;`, `&amp;`, `^`, or `|`,
_rval_: an ECMAScript language value,
): a Completion Record normally containing a BigInt or a Number
): a Completion Record normally containing a String, a BigInt, or a Number
</h1>
<dl class="header">
</dl>
Expand Down Expand Up @@ -20496,7 +20497,7 @@ <h1>
_leftOperand_: a Parse Node,
_opText_: a sequence of Unicode code points,
_rightOperand_: a Parse Node,
): a Completion Record normally containing a BigInt or a Number
): a Completion Record normally containing a String, a BigInt, or a Number
</h1>
<dl class="header">
</dl>
Expand Down Expand Up @@ -25705,7 +25706,7 @@ <h1>
_sourceText_: ECMAScript source text,
_realm_: unknown,
_hostDefined_: unknown,
): a Script Record
): a Script Record or a non-empty List of *SyntaxError* objects
</h1>
<dl class="header">
<dt>description</dt>
Expand Down Expand Up @@ -27462,7 +27463,7 @@ <h1>
_sourceText_: ECMAScript source text,
_realm_: unknown,
_hostDefined_: unknown,
): a Source Text Module Record or a List of *SyntaxError* objects
): a Source Text Module Record or a non-empty List of *SyntaxError* objects
</h1>
<dl class="header">
<dt>description</dt>
Expand Down Expand Up @@ -36016,7 +36017,7 @@ <h1>
RegExpBuiltinExec (
_R_: an initialized RegExp instance,
_S_: a String,
): a Completion Record normally containing an Object or *null*
): a Completion Record normally containing an Array exotic object or *null*
</h1>
<dl class="header">
</dl>
Expand Down Expand Up @@ -39711,7 +39712,7 @@ <h1>
CreateMapIterator (
_map_: an ECMAScript language value,
_kind_: ~key+value~, ~key~, or ~value~,
): a Completion Record normally containing a generator
): a Completion Record normally containing a Generator
</h1>
<dl class="header">
<dt>description</dt>
Expand Down Expand Up @@ -40000,7 +40001,7 @@ <h1>
CreateSetIterator (
_set_: an ECMAScript language value,
_kind_: ~key+value~ or ~value~,
): a Completion Record normally containing a generator
): a Completion Record normally containing a Generator
</h1>
<dl class="header">
<dt>description</dt>
Expand Down Expand Up @@ -41525,7 +41526,7 @@ <h1>
RemoveWaiters (
_WL_: a WaiterList,
_c_: a non-negative integer or +&infin;,
): a List of agents
): a List of agent signifiers
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

</h1>
<dl class="header">
</dl>
Expand Down