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: use consistent wording for abstract operation preambles #1914

Open
wants to merge 3 commits into
base: master
from

Conversation

@bakkot
Copy link
Contributor

bakkot commented Mar 24, 2020

This unifies the wording used to introduce abstract operations. The intent is that the new wording should provide all and only the information in the current wording (except in a couple cases where the existing wording is obviously wrong). This is a step towards #545 (structured headers could be used to generate this text) and #1796 (which wants a consistent place to describe the return type of abstract operations).

We're wildly inconsistent about what information we bother to include in these descriptions. This is more obvious after this PR because the wording is now consistent, but since it doesn't actually make the information included any less consistent, I would prefer we not try to address that here.

This PR also removes abstract operation preambles entirely when they contain no information other than the name of the operation and its arguments (though I am open to going the other direction and such adding introductions everywhere they are currently absent).

After we land this I will try to make a PR to ecmarkup which enforces this style, at least to some extent.

Thanks to @jmdyck for ecmaspeak-py, which has hardcoded all the many, many forms of preamble currently in use and allowed me to extract the information I needed from them.

@bakkot bakkot force-pushed the consistent-headers branch from d79a900 to 6bb541f Mar 24, 2020
Copy link
Member

ljharb left a comment

This is a long PR, so before I read the rest, I want to make sure that these kinds of tweaks are something you consider in scope for this PR (ie, adding correct parentheticals on each argument to indicate their type). Whether to add explicit asserts to match these is something I assume would go in a separate PR.

spec.html Outdated
@@ -4124,7 +4122,6 @@ <h1>ThrowCompletion</h1>

<emu-clause id="sec-updateempty" aoid="UpdateEmpty">
<h1>UpdateEmpty ( _completionRecord_, _value_ )</h1>
<p>The abstract operation UpdateEmpty with arguments _completionRecord_ and _value_ performs the following steps:</p>

This comment has been minimized.

Copy link
@ljharb

ljharb Mar 24, 2020

Member

i think i'd prefer to always include a preamble; but if you wouldn't want to make it correct/consistent in this PR, then i'm also fine landing this first, and then adding correct preambles in a followup.

This comment has been minimized.

Copy link
@bakkot

bakkot Mar 24, 2020

Author Contributor

It's easy to add them in this PR. I actually had them all in originally and then decided to take them all out instead, so it should just be a matter of reverting the second commit.

This comment has been minimized.

Copy link
@bakkot

bakkot Mar 24, 2020

Author Contributor

That said, they're a lot of noise, so I will maybe wait to push that commit so the more substantive preambles can be reviewed first.

@@ -4309,7 +4306,7 @@ <h1>IsGenericDescriptor ( _Desc_ )</h1>

<emu-clause id="sec-frompropertydescriptor" aoid="FromPropertyDescriptor">
<h1>FromPropertyDescriptor ( _Desc_ )</h1>
<p>When the abstract operation FromPropertyDescriptor is called with Property Descriptor _Desc_, the following steps are taken:</p>
<p>The abstract operation FromPropertyDescriptor takes argument _Desc_ (a Property Descriptor or *undefined*). It performs the following steps:</p>

This comment has been minimized.

Copy link
@ljharb

ljharb Mar 24, 2020

Member

I notice that in some places, we have explicit asserts for any preamble-declared types, but eg in this one, we don't. Is that an example of the inconsistency you don't want to handle in this PR?

This comment has been minimized.

Copy link
@bakkot

bakkot Mar 24, 2020

Author Contributor

Is that an example of the inconsistency you don't want to handle in this PR?

Yes. I agree those would also be good to address, I just don't want to consider them in scope for this.

spec.html Outdated
@@ -5322,7 +5317,7 @@ <h1>Testing and Comparison Operations</h1>

<emu-clause id="sec-requireobjectcoercible" aoid="RequireObjectCoercible">
<h1>RequireObjectCoercible ( _argument_ )</h1>
<p>The abstract operation RequireObjectCoercible throws an error if _argument_ is a value that cannot be converted to an Object using ToObject. It is defined by <emu-xref href="#table-14"></emu-xref>:</p>
<p>The abstract operation RequireObjectCoercible takes argument _argument_. It throws an error if _argument_ is a value that cannot be converted to an Object using ToObject. It is defined by <emu-xref href="#table-14"></emu-xref>:</p>

This comment has been minimized.

Copy link
@ljharb

ljharb Mar 24, 2020

Member
Suggested change
<p>The abstract operation RequireObjectCoercible takes argument _argument_. It throws an error if _argument_ is a value that cannot be converted to an Object using ToObject. It is defined by <emu-xref href="#table-14"></emu-xref>:</p>
<p>The abstract operation RequireObjectCoercible takes argument _argument_ (an ECMAScript language value). It throws an error if _argument_ is a value that cannot be converted to an Object using ToObject. It is defined by <emu-xref href="#table-14"></emu-xref>:</p>

per some of the below ones where this parenthetical exists

@@ -5303,7 +5298,7 @@ <h1>CanonicalNumericIndexString ( _argument_ )</h1>

<emu-clause id="sec-toindex" aoid="ToIndex">
<h1>ToIndex ( _value_ )</h1>
<p>The abstract operation ToIndex returns _value_ argument converted to a non-negative integer if it is a valid integer index value. This abstract operation functions as follows:</p>
<p>The abstract operation ToIndex takes argument _value_. It returns _value_ argument converted to a non-negative integer if it is a valid integer index value. It performs the following steps:</p>

This comment has been minimized.

Copy link
@ljharb

ljharb Mar 24, 2020

Member
Suggested change
<p>The abstract operation ToIndex takes argument _value_. It returns _value_ argument converted to a non-negative integer if it is a valid integer index value. It performs the following steps:</p>
<p>The abstract operation ToIndex takes argument _value_ (an ECMAScript language value). It returns _value_ argument converted to a non-negative integer if it is a valid integer index value. It performs the following steps:</p>
@@ -5290,7 +5285,7 @@ <h1>ToLength ( _argument_ )</h1>

<emu-clause id="sec-canonicalnumericindexstring" aoid="CanonicalNumericIndexString">
<h1>CanonicalNumericIndexString ( _argument_ )</h1>
<p>The abstract operation CanonicalNumericIndexString returns _argument_ converted to a Number value if it is a String representation of a Number that would be produced by ToString, or the string *"-0"*. Otherwise, it returns *undefined*. This abstract operation functions as follows:</p>
<p>The abstract operation CanonicalNumericIndexString takes argument _argument_. It returns _argument_ converted to a Number value if it is a String representation of a Number that would be produced by ToString, or the string *"-0"*. Otherwise, it returns *undefined*. It performs the following steps:</p>

This comment has been minimized.

Copy link
@ljharb

ljharb Mar 24, 2020

Member
Suggested change
<p>The abstract operation CanonicalNumericIndexString takes argument _argument_. It returns _argument_ converted to a Number value if it is a String representation of a Number that would be produced by ToString, or the string *"-0"*. Otherwise, it returns *undefined*. It performs the following steps:</p>
<p>The abstract operation CanonicalNumericIndexString takes argument _argument_ (a String). It returns _argument_ converted to a Number value if it is a String representation of a Number that would be produced by ToString, or the string *"-0"*. Otherwise, it returns *undefined*. It performs the following steps:</p>
spec.html Outdated
@@ -5448,7 +5442,7 @@ <h1>IsExtensible ( _O_ )</h1>

<emu-clause id="sec-isinteger" aoid="IsInteger">
<h1>IsInteger ( _argument_ )</h1>
<p>The abstract operation IsInteger determines if _argument_ is a finite integer Number value.</p>
<p>The abstract operation IsInteger takes argument _argument_. It determines if _argument_ is a finite integer Number value. It performs the following steps:</p>

This comment has been minimized.

Copy link
@ljharb

ljharb Mar 24, 2020

Member
Suggested change
<p>The abstract operation IsInteger takes argument _argument_. It determines if _argument_ is a finite integer Number value. It performs the following steps:</p>
<p>The abstract operation IsInteger takes argument _argument_ (an ECMAScript language value). It determines if _argument_ is a finite integer Number value. It performs the following steps:</p>
@bakkot

This comment has been minimized.

Copy link
Contributor Author

bakkot commented Mar 24, 2020

@ljharb

I want to make sure that these kinds of tweaks are something you consider in scope for this PR (ie, adding correct parentheticals on each argument to indicate their type)

I would prefer to consider those out of scope for this PR, even though I also would like to have those added at some point. My intent was to have this PR be a very mechanical rewording of the existing prose, rather than introducing any new information.

That said, if you notice any obvious ones as you're reading through and you want to note them as you go rather than someone having to re-discover them all later, feel free to leave comments like the ones above and just mark them as resolved, and I'll collect them into a followup PR once this lands.

@michaelficarra

This comment has been minimized.

Copy link
Member

michaelficarra commented Mar 24, 2020

I would prefer "It performs the following steps when called:" to "It performs the following steps:".

@jmdyck

This comment has been minimized.

Copy link
Collaborator

jmdyck commented Mar 25, 2020

It seems to me that the "performs the following steps" wording in operation/function preambles is superfluous. (Note that several preambles don't have it, and people don't appear to be confused about them.) If we just dropped it, would any readers not understand when the following steps were performed?

(Or, put it this way: in a real programming language, would you ever write the comment: "When called, this function executes the following statements"?)

spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
spec.html Outdated
@@ -5459,7 +5453,7 @@ <h1>IsInteger ( _argument_ )</h1>

<emu-clause id="sec-isnonnegativeinteger" aoid="IsNonNegativeInteger">
<h1>IsNonNegativeInteger ( _argument_ )</h1>
<p>The abstract operation IsNonNegativeInteger determines if _argument_ is non-negative integer Number value.</p>
<p>The abstract operation IsNonNegativeInteger takes argument _argument_. It determines if _argument_ is non-negative integer Number value. It performs the following steps:</p>

This comment has been minimized.

Copy link
@michaelficarra

michaelficarra Mar 25, 2020

Member
Suggested change
<p>The abstract operation IsNonNegativeInteger takes argument _argument_. It determines if _argument_ is non-negative integer Number value. It performs the following steps:</p>
<p>The abstract operation IsNonNegativeInteger takes argument _argument_. It determines if _argument_ is a non-negative integral Number value. It performs the following steps:</p>

This comment has been minimized.

Copy link
@bakkot

bakkot Mar 26, 2020

Author Contributor

Added the missing "a", but leaving this as "integer" rather than "integral" to match other operations. Feel free to submit a followup PR fixing all of them.

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 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 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
@@ -37689,7 +37689,7 @@ <h1>NotifyWaiter ( _WL_, _W_ )</h1>

<emu-clause id="sec-atomicreadmodifywrite" aoid="AtomicReadModifyWrite">
<h1>AtomicReadModifyWrite ( _typedArray_, _index_, _value_, _op_ )</h1>
<p>The abstract operation AtomicReadModifyWrite takes arguments _typedArray_, _index_, _value_, and _op_ (a pure combining operation). It atomically loads a value, combines it with another value, and stores the result of the combination. It returns the loaded value. It performs the following steps:</p>
<p>The abstract operation AtomicReadModifyWrite takes arguments _typedArray_, _index_, _value_, and _op_ (a pure combining operation). _op_ takes two List of byte values arguments and returns a List of byte values. This operation atomically loads a value, combines it with another value, and stores the result of the combination. It returns the loaded value. It performs the following steps:</p>

This comment has been minimized.

Copy link
@michaelficarra

michaelficarra Mar 27, 2020

Member
Suggested change
<p>The abstract operation AtomicReadModifyWrite takes arguments _typedArray_, _index_, _value_, and _op_ (a pure combining operation). _op_ takes two List of byte values arguments and returns a List of byte values. This operation atomically loads a value, combines it with another value, and stores the result of the combination. It returns the loaded value. It performs the following steps:</p>
<p>The abstract operation AtomicReadModifyWrite takes arguments _typedArray_, _index_, _value_, and _op_ (a pure combining operation). _op_ takes two arguments, each a List of byte values, and returns a List of byte values. This operation atomically loads a value, combines it with another value, and stores the result of the combination. It returns the loaded value. It performs the following steps:</p>
@michaelficarra

This comment has been minimized.

Copy link
Member

michaelficarra commented Mar 27, 2020

Needs to resolve conflicts after #1918.

@bakkot bakkot force-pushed the consistent-headers branch from 834f158 to 19c6381 Mar 27, 2020
@bakkot

This comment has been minimized.

Copy link
Contributor Author

bakkot commented Mar 27, 2020

i think i'd prefer to always include a preamble

Done.

I would prefer "It performs the following steps when called:" to "It performs the following steps:".

Done.

Needs to resolve conflicts after #1918.

Done.

@bakkot bakkot force-pushed the consistent-headers branch from 19c6381 to 8f7305c Mar 27, 2020
@ljharb
ljharb approved these changes Mar 27, 2020
Copy link
Member

ljharb left a comment

All three commits look good, pending my one question about redundant blank lines.

For posterity (things we've already discussed on our editors call), followups hopefully will:

  1. ensure all AO preamble args have a parenthesized type after them
  2. ensure all of those types are matched with either an explicit list of assertions, or, with a notational convention that implicitly asserts this for all AOs
  3. ensure all AO preambles specify the type(s) of what they return
<p>When an Await rejected function is called with argument _reason_, the following steps are taken:</p>


This comment has been minimized.

Copy link
@ljharb

ljharb Mar 27, 2020

Member

why do we always have/want two blank lines here? can we make it always one?

This comment has been minimized.

Copy link
@bakkot

bakkot Mar 27, 2020

Author Contributor

The ones which got changed here were an artifact of the process I was using. Whitespace is pretty inconsistent in general, so I figured it would be fine, but I can back those changes out if you'd prefer. I'd be happy to make it consistent in a separate PR, though I think would want to wait for the ability to enforce it (cf tc39/ecmarkup#173).

This comment has been minimized.

Copy link
@ljharb

ljharb Mar 27, 2020

Member

I'm fine waiting on it; i'd just prefer to end up never having two blank lines in a row :-)

This comment has been minimized.

Copy link
@bakkot

bakkot Mar 27, 2020

Author Contributor

Added to my list of desiderata for linting the spec.

@ljharb ljharb self-assigned this Mar 27, 2020
@bakkot

This comment has been minimized.

Copy link
Contributor Author

bakkot commented Mar 27, 2020

ensure all of those types are matched with either an explicit list of assertions, or, with a notational convention that implicitly asserts this for all AOs

There is not yet agreement on exactly this one of these two possible alternatives, just that we would like it to be consistent somehow. There are other possible ways that could happen - for example, it would also be consistent to just leave out the assertions entirely.

@jmdyck

This comment has been minimized.

Copy link
Collaborator

jmdyck commented Mar 28, 2020

ensure all of those types are matched with either an explicit list of assertions, or, with a notational convention that implicitly asserts this for all AOs

[...] There are other possible ways that could happen - for example, it would also be consistent to just leave out the assertions entirely.

Isn't that the same as @ljharb's second alternative? That is: don't Assert parameter types in the <emu-alg>, just have parenthesized parameter types in the preamble, and then say in 5.2.1 Abstract Operations that the latter have the same effect as Asserts. (If that isn't the suggestion, please clarify.)

@bakkot

This comment has been minimized.

Copy link
Contributor Author

bakkot commented Mar 28, 2020

If that isn't the suggestion, please clarify.

The suggestion (well, not a suggestion per se, just a statement of possibility) is to omit the modification to 5.2.1, or to modify it in such a way that it does not make mention of Assert in explaining the interpretation of these type parameters.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.