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: Treat not present parameters as undefined #1411

Merged
merged 1 commit into from
Nov 14, 2019

Conversation

shvaikalesh
Copy link
Member

@shvaikalesh shvaikalesh commented Jan 15, 2019

Using missing parameters directly eliminates extra steps, resulting in more descriptive variable names and increased visibility for functions where "X is (not) present" is absolutely required. ES6+ era algorithms already do treat missing parameters as undefined and use them directly.

spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
@ljharb ljharb requested review from bterlson, zenparsing and a team January 15, 2019 21:42
@shvaikalesh shvaikalesh force-pushed the optional-params branch 2 times, most recently from b8afb68 to 8d13db3 Compare January 15, 2019 21:58
@ljharb ljharb requested a review from a team January 15, 2019 22:01
spec.html Outdated
@@ -6348,7 +6348,7 @@ <h1>GetActiveScriptOrModule ( )</h1>
<h1>ResolveBinding ( _name_ [ , _env_ ] )</h1>
<p>The ResolveBinding abstract operation is used to determine the binding of _name_ passed as a String value. The optional argument _env_ can be used to explicitly provide the Lexical Environment that is to be searched for the binding. During execution of ECMAScript code, ResolveBinding is performed using the following algorithm:</p>
<emu-alg>
1. If _env_ is not present or if _env_ is *undefined*, then
1. If _env_ is *undefined*, then
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we can currently assume that "not present" is equivalent to "undefined" for abstract operations.

Copy link
Member

Choose a reason for hiding this comment

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

This is correct, however I would be open to some clause 5 changes to make this work for abstract ops. I was also confused by this pattern until Allen explained it to me some years ago.

spec.html Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
@shvaikalesh shvaikalesh force-pushed the optional-params branch 2 times, most recently from 971c9f1 to 05f04c8 Compare January 16, 2019 02:07
spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
@ljharb ljharb removed the request for review from bterlson February 28, 2019 19:53
@shvaikalesh shvaikalesh force-pushed the optional-params branch 2 times, most recently from 4a8b6a1 to 38716ea Compare March 3, 2019 17:57
@ljharb ljharb requested review from zenparsing and jmdyck March 4, 2019 06:58
@jmdyck
Copy link
Collaborator

jmdyck commented Mar 4, 2019

@shvaikalesh

resulting in more descriptive variable names

I don't understand this claim. Can you give an example?

@shvaikalesh
Copy link
Member Author

shvaikalesh commented Mar 5, 2019

@jmdyck

That claim was regarding e3f2a97: thisArg is more descriptive than T.

@jmdyck
Copy link
Collaborator

jmdyck commented Mar 6, 2019

@shvaikalesh: Ah, thanks. I agree that thisArg is more descriptive than T.

Alternatively, one can avoid introducing T by replacing steps of the form:
If _thisArg_ is present, let _T_ be _thisArg_; else let _T_ be *undefined*.
with:
If _thisArg_ is not present, set _thisArg_ to *undefined*.

Copy link
Collaborator

@jmdyck jmdyck left a comment

Choose a reason for hiding this comment

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

I'm guessing these changes are predicated on the paragraph in Clause 17 that says:

Unless otherwise specified in the description of a particular function, if a built-in function or constructor is given fewer arguments than the function is specified to require, the function or constructor shall behave exactly as if it had been given sufficient additional arguments, each such argument being the undefined value.

However, note the phrase "fewer arguments than the function is specified to require". Now, the number of arguments that a function "requires" isn't really defined, but a plausible meaning is the "default argument count" (i.e., the default value for the length property) defined a few paragraphs later. Note:

Optional parameters (which are indicated with brackets: [ ]) or rest parameters (which are shown using the form «...name») are not included in the default argument count.

On this basis, the "missing -> *undefined*" equivalence is not specified to hold for optional parameters. Algorithms for built-in functions are obliged to explicitly handle missing arguments for optional parameters (typically via "is [not] present").

So I think the changes of this PR are justified for non-optional parameters, but not for optional parameters.

@bakkot bakkot added the editor call to be discussed in the next editor call label Nov 6, 2019
@michaelficarra
Copy link
Member

michaelficarra commented Nov 9, 2019

@shvaikalesh Can you add wording to the Notational Conventions section to address the concerns of @zenparsing and @bterlson (and me) above?

edit: I see that this change now only includes parameters for EcmaScript functions, not abstract operations.

spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
@shvaikalesh
Copy link
Member Author

shvaikalesh commented Nov 9, 2019

On this basis, the "missing -> undefined" equivalence is not specified to hold for optional parameters. Algorithms for built-in functions are obliged to explicitly handle missing arguments for optional parameters (typically via "is [not] present").

Without this change, there are quite a few methods and constructors that assume "missing -> undefined" equivalence for optional parameters:

Complete list (22 functions, 25 parameters)
  1. String.prototype.endsWith, endPosition at step 7
  2. String.prototype.includes, position at step 6
  3. String.prototype.indexOf, position at step 4
  4. String.prototype.lastIndexOf, position at step 4
  5. String.prototype.padEnd, fillString at step 2
  6. String.prototype.padStart, fillString at step 2
  7. String.prototype.startsWith, position at step 6
  8. Array.from, mapfn at step 2
  9. Array.prototype.copyWithin, end at step 7
  10. Array.prototype.fill, start at step 3 and end at step 5
  11. Array.prototype.flat, depth at step 4
  12. Array.prototype.includes, fromIndex at step 4
  13. Array.prototype.indexOf, fromIndex at step 4
  14. Symbol, description at step 2
  15. SharedArrayBuffer, length at step 2
  16. DataView, byteOffset at step 3
  17. %TypedArray%, byteOffset at step 6
  18. %TypedArray%.prototype.copyWithin, end at step 8
  19. %TypedArray%.prototype.fill, start at step 6 and end at step 8
  20. %TypedArray%.prototype.set, offset at step 5
  21. JSON.parse, reviver at step 7
  22. JSON.stringify, replacer at step 4 and space at step 5

Are Clause 17 changes welcome?

@ljharb ljharb requested review from jmdyck, syg and bakkot November 13, 2019 23:26
@ljharb ljharb removed the editor call to be discussed in the next editor call label Nov 13, 2019
Copy link
Collaborator

@jmdyck jmdyck left a comment

Choose a reason for hiding this comment

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

This PR bumps up against some places in the current spec that are odd/underspecified (including the point I raised earlier), but I'll grant that it's not this PR's responsibility to resolve those problems.

The PR title and/or commit message should probably clarify that this is only dealing with parameters of built-in functions.

spec.html Outdated
@@ -26609,8 +26609,8 @@ <h1>The Number Constructor</h1>
<h1>Number ( _value_ )</h1>
<p>When `Number` is called with argument _value_, the following steps are taken:</p>
<emu-alg>
1. If no arguments were passed to this function invocation, let _n_ be *+0*.
1. Else, let _n_ be ? ToNumber(_value_).
1. If _value_ is present, let _n_ be ? ToNumber(_value_).
Copy link
Collaborator

Choose a reason for hiding this comment

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

(It's odd to check if _value_ is present when _value_ isn't marked as optional, but the current spec has an analogous oddity.)

Copy link
Member

@michaelficarra michaelficarra Nov 14, 2019

Choose a reason for hiding this comment

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

I don't think it's odd. It's a mandatory parameter (Number.length is 1), and Number() is different than Number(void 0).

edit: Also, it's necessary to make an exception to the requirement you mentioned above:

Unless otherwise specified in the description of a particular function, if a built-in function or constructor is given fewer arguments than the function is specified to require, the function or constructor shall behave exactly as if it had been given sufficient additional arguments, each such argument being the undefined value.

Checking the presence of a mandatory parameter is the "otherwise specified".

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's a mandatory parameter (Number.length is 1),

It could be made an 'optional' parameter and Number.length could still be defined to be 1.

and Number() is different than Number(void 0).

Agreed. But that just means that the algorithm has to distinguish between "not present" and *undefined*, which I'm not questioning.

Checking the presence of a mandatory parameter is the "otherwise specified".

Agreed. So it's allowed, but it's still odd. Or maybe I'll downgrade to "unusual". (I.e., I think Number() and String() are the only built-ins where the spec checks for the presence of a 'mandatory' parameter.)

When you say "it's necessary", are you saying that the presence of the "unless otherwise specified" phrase necessitates that there be some place in the spec where it is otherwise specified? Because I'm pretty sure there are several "unless otherwise specified" sentences where it never is otherwise specified.


Here's a thought experiment: if the spec explicitly gave the value of the length property for every built-in function, would there be any useful distinction between 'optional' and 'mandatory' arguments? (I had thought that one distinction was that optionals didn't automatically get undefined-for-missing, and so would need an explicit 'present' test, but apparently that's not the case in practice.)

Copy link
Member

Choose a reason for hiding this comment

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

@jmdyck currently, the only explicit lengths are for where it differs from "the number of non-optional params in the signature", all of which are legacy.

imo every argument should be implicitly undefined when omitted, and may separately be present or absent.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@jmdyck currently, the only explicit lengths are for where it differs from "the number of non-optional params in the signature",

(That's not quite true. E.g., the length of the Array constructor is given explicitly as 1, which is the same as its "default argument count". Similarly, the length of the %TypedArray% constructor is explicitly given as 0.)

all of which are legacy.

Well, Number() and String() have both been around since ES1, so surely they qualify as legacy.

imo every argument should be implicitly undefined when omitted, and may separately be present or absent.

Right, so to that extent, you're not drawing a distinction between 'optional' and 'required' parameters.

spec.html Outdated
@@ -29328,7 +29327,7 @@ <h1>The String Constructor</h1>
<h1>String ( _value_ )</h1>
<p>When `String` is called with argument _value_, the following steps are taken:</p>
<emu-alg>
1. If no arguments were passed to this function invocation, let _s_ be *""*.
1. If _value_ is not present, let _s_ be the empty String.
Copy link
Collaborator

Choose a reason for hiding this comment

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

(Similar to previous comment, _value_ is not marked as optional.)

@ljharb ljharb merged commit edeeafa into tc39:master Nov 14, 2019
@devsnek
Copy link
Member

devsnek commented Nov 14, 2019

This left out Array.prototype.flatMap. I can open a PR soonish if no one else does.

@shvaikalesh
Copy link
Member Author

Thank you @devsnek. BigInt.prototype.toString was also left out, and possibly something else. I am making a full sweep now and will open a follow-up PR today.

@devsnek
Copy link
Member

devsnek commented Nov 14, 2019

@shvaikalesh #1781

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

Successfully merging this pull request may close these issues.

9 participants