Editorial: add a SetFunctionLength abstract operation #1096

tobie · 2018-02-09T23:50:39Z

No description provided.

tobie · 2018-02-09T23:54:10Z

spec.html

+ <emu-alg>
+ 1. Assert: _F_ is an extensible object that does not have a `length` own property.
+ 1. Assert: Type(_length_) is Number.
+ 1. Assert: ToLength(_length_) is equal to _length_.


Unsure if that's the best way to assert that length is an integer within the right bounds.

ljharb

Thanks, this seems like a great cleanup.

tobie · 2018-02-09T23:55:24Z

spec.html

 1. Let _len_ be the ExpectedArgumentCount of _ParameterList_.
- 1. Perform ! DefinePropertyOrThrow(_F_, `"length"`, PropertyDescriptor{[[Value]]: _len_, [[Writable]]: *false*, [[Enumerable]]: *false*, [[Configurable]]: *true*}).
+ 1. Perform ! SetFunctionLength(_F_, _len_).


Not sure whether this should be prefixed with "!". I assume it should, but then SetFunctionName often isn't elsewhere in the spec and I'm not sure why.

If an operation cannot ever return an abrupt completion, then I'd say invocations of it shouldn't be preceded by '!'. It isn't wrong, but it's unnecessary clutter. (This would be more persuasive if you could easily tell from an operation's 'declaration' whether it ever returns an abrupt completion. See issue #253.)

So just to clarify my understanding here:

While DefinePropertyOrThrow can throw in some circumstances, it can't in this case, because we know from the first assertion in SetFunctionLength that DefinePropertyOrThrow's first argument is extensible and doesn't have a length own prop which could interfere with setting.

Thus we know that SetFunctionLength won't ever throw.

Preceding an abstract operation call by "!" means that we are in a situation where an abstract operation won't throw, although it might do so in different circumstances.

Operations which never throw should never be prefixed with "!" even though we can't know from looking at the abstract operation's signature or its name that this is the case.

So for now, SetFunctionLength should not be prefixed with anything?

Is my understanding correct?

Your understanding matches mine. Whether it's correct is up to the editors.

I don't agree with this; I think we should use ! everywhere. (I am also not an editor.)

tobie · 2018-02-10T00:36:26Z

Thanks. As mentioned over irc, I'll make especially good use of it in WebIDL.

bmeck · 2018-02-10T13:02:37Z

spec.html

+ </emu-alg>
+ </emu-clause>
+
+ <!-- es6num="9.2.13" -->


To my understanding we should not be moving/adding es6num since it is referring to where a clause was in the ES2015 specification: http://www.ecma-international.org/ecma-262/6.0/

bmeck

LGTM except es6num

ljharb · 2018-02-10T14:03:41Z

spec.html

 1. Let _len_ be the ExpectedArgumentCount of _ParameterList_.
- 1. Perform ! DefinePropertyOrThrow(_F_, `"length"`, PropertyDescriptor{[[Value]]: _len_, [[Writable]]: *false*, [[Enumerable]]: *false*, [[Configurable]]: *true*}).
+ 1. Perform SetFunctionLength(_F_, _len_).


I disagree with the above; this should be prefixed with !.

Noise is less of a concern to me than not knowing whether an abstract operation might throw or not, at the callsite, without having to read the resulting operation definition - otherwise i’m not sure what the point of the ! is.

Would you prefix all currently-unprefixed operation-invocations with "!"? There's about 2300 of them.

Alternatively, we could come up with a naming convention that distinguished those operations that can return abrupt completions from those that can't. That would convey the information you want to see at the invocation-point, in a maybe less obtrusive way.

Personally, I think I'd prefer if information about what the operation can return were only a click or mouse-hover away.

(I wonder if we should move this discussion to a separate issue.)

Yes, i would, and yes, I’d say that’d be a separate issue.

That there’s 2300 mean it’d be a tedious conversion; when I’m reading a specific part of the spec there wouldn’t be 2300 - there’d be just enough so I’d have to jump elsewhere as rarely as possible.

Separate issue now created: #1097

ljharb · 2018-02-10T14:03:54Z

spec.html

@@ -24700,7 +24710,7 @@ <h1>Function.prototype.bind ( _thisArg_, ..._args_ )</h1>
 1. Let _targetLen_ be ToInteger(_targetLen_).
 1. Let _L_ be the larger of 0 and the result of _targetLen_ minus the number of elements of _args_.
 1. Else, let _L_ be 0.
- 1. Perform ! DefinePropertyOrThrow(_F_, `"length"`, PropertyDescriptor {[[Value]]: _L_, [[Writable]]: *false*, [[Enumerable]]: *false*, [[Configurable]]: *true*}).
+ 1. Perform SetFunctionLength(_F_, _L_).


This also needs a !

ljharb · 2018-02-10T14:31:29Z

spec.html

+ <h1>SetFunctionLength ( _F_, _length_ )</h1>
+ <p>The abstract operation SetFunctionLength requires a Function argument _F_, and a Number argument _length_. This operation adds a `length` property to _F_ by performing the following steps:</p>
+ <emu-alg>
+ 1. Assert: _F_ is an extensible object that does not have a `length` own property.


Should this also assert on IsCallable? Should it call the internal IsExtensible methodminstead of using “is extensible” prose?

I copied the prose used elsewhere in the spec word for word. I suggest changing all of it in a separate pull request.

Sounds good :-)

tobie · 2018-02-12T14:15:33Z

What's the outcome of the discussion as to whether calls to SetFunctionLength should be prefixed by "!" in this particular pull request? I don't have an opinion either way, but would like to be able to move forward with this PR.

anba · 2018-02-12T16:05:02Z

spec.html

@@ -7535,6 +7534,17 @@ <h1>SetFunctionName ( _F_, _name_ [ , _prefix_ ] )</h1>
 </emu-alg>
 </emu-clause>

+ <emu-clause id="sec-setfunctionlength" aoid="SetFunctionLength">
+ <h1>SetFunctionLength ( _F_, _length_ )</h1>
+ <p>The abstract operation SetFunctionLength requires a Function argument _F_, and a Number argument _length_. This operation adds a `length` property to _F_ by performing the following steps:</p>


Nit: No comma here because there only two arguments.

anba · 2018-02-12T16:09:29Z

spec.html

+ <emu-alg>
+ 1. Assert: _F_ is an extensible object that does not have a `length` own property.
+ 1. Assert: Type(_length_) is Number.
+ 1. Assert: ToLength(_length_) is equal to _length_.


ToLength is not correct here, because if called from Function.prototype.bind length can exceed 2^53-1. If a bounds check is wanted here, something like

Assert: length ≥ 0 and ToInteger(length) is equal to length.

is necessary.

ljharb · 2018-02-12T16:52:14Z

@tobie yes, please include the ! in this PR.

littledan

I think the PR is good as it is, though I'd still LGTM it with an extra !

tobie · 2018-04-07T15:37:46Z

Hey, any idea what the timeline looks like to get this merged in?

bterlson · 2018-04-19T20:13:55Z

Sorry for the delay. It looks great.

FWIW I think we should be consistently using !/? operators until it is made more clear which abstract ops can never throw.

tobie · 2018-04-19T21:34:18Z

Thanks a lot. Getting put to good use here: whatwg/webidl#549. :)

Editorial: add a SetFunctionLength abstract operation

5136723

tobie commented Feb 9, 2018

View reviewed changes

ljharb approved these changes Feb 9, 2018

View reviewed changes

ljharb added the editorial change label Feb 9, 2018

tobie commented Feb 9, 2018

View reviewed changes

ljharb requested review from bterlson and bmeck February 10, 2018 01:46

bmeck reviewed Feb 10, 2018

View reviewed changes

bmeck suggested changes Feb 10, 2018

View reviewed changes

tobie added 3 commits February 10, 2018 14:25

fixup!

0e5731a

fixup! address feedback

3e54fab

fixup! ws

293548a

bmeck approved these changes Feb 10, 2018

View reviewed changes

ljharb requested changes Feb 10, 2018

View reviewed changes

ljharb reviewed Feb 10, 2018

View reviewed changes

jmdyck mentioned this pull request Feb 10, 2018

Editorial: usage of "!" #1097

Closed

anba suggested changes Feb 12, 2018

View reviewed changes

fixup! address grammar nit and length arg bounding

11c9c07

littledan approved these changes Feb 12, 2018

View reviewed changes

fixup! Add "!" back in.

0ca337c

ljharb approved these changes Feb 12, 2018

View reviewed changes

ljharb assigned bterlson Feb 12, 2018

bterlson merged commit 1b2cb5f into tc39:master Apr 19, 2018

tobie deleted the SetFunctionLength branch April 19, 2018 21:04

ljharb mentioned this pull request May 10, 2018

Update ES2018 from master #1191

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Editorial: add a SetFunctionLength abstract operation #1096

Editorial: add a SetFunctionLength abstract operation #1096

tobie commented Feb 9, 2018

tobie Feb 9, 2018

ljharb left a comment

tobie Feb 9, 2018 •

edited

Loading

jmdyck Feb 10, 2018 •

edited

Loading

tobie Feb 10, 2018

jmdyck Feb 10, 2018

domenic Feb 10, 2018

tobie commented Feb 10, 2018

bmeck Feb 10, 2018

bmeck left a comment

ljharb Feb 10, 2018 •

edited

Loading

jmdyck Feb 10, 2018

jmdyck Feb 10, 2018

ljharb Feb 10, 2018

ljharb Feb 10, 2018

jmdyck Feb 10, 2018

ljharb Feb 10, 2018

ljharb Feb 10, 2018

tobie Feb 10, 2018 •

edited

Loading

ljharb Feb 10, 2018

tobie commented Feb 12, 2018

anba Feb 12, 2018

anba Feb 12, 2018

ljharb commented Feb 12, 2018

littledan left a comment

tobie commented Apr 7, 2018

bterlson commented Apr 19, 2018

tobie commented Apr 19, 2018

Editorial: add a SetFunctionLength abstract operation #1096

Editorial: add a SetFunctionLength abstract operation #1096

Conversation

tobie commented Feb 9, 2018

Choose a reason for hiding this comment

ljharb left a comment

Choose a reason for hiding this comment

tobie Feb 9, 2018 • edited Loading

Choose a reason for hiding this comment

jmdyck Feb 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tobie commented Feb 10, 2018

Choose a reason for hiding this comment

bmeck left a comment

Choose a reason for hiding this comment

ljharb Feb 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tobie Feb 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tobie commented Feb 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ljharb commented Feb 12, 2018

littledan left a comment

Choose a reason for hiding this comment

tobie commented Apr 7, 2018

bterlson commented Apr 19, 2018

tobie commented Apr 19, 2018

tobie Feb 9, 2018 •

edited

Loading

jmdyck Feb 10, 2018 •

edited

Loading

ljharb Feb 10, 2018 •

edited

Loading

tobie Feb 10, 2018 •

edited

Loading