Editorial: Eliminate "unknown" as a parameter type #2745

jmdyck · 2022-04-18T18:39:42Z

In the current spec, there are ~~250~~ 240 AO/SDO parameters with a type of "unknown". This PR replaces ~~almost~~ all of those unknowns with a reasonable type.

~~I say "almost all" because it skips:~~

~~the 3 _state_ parameters handled by PR Editorial: Declare "JSON Serialization Record" #2741, and~~
~~the 5 _V_ and _W_ parameters of GetValue, PutValue, and InitializeReferencedBinding, because they're weird, and need some attention before they can get sensible types.~~

(PR #2741 and PR #2842 have both been merged, so the above exceptions have already been dealt with.)

The PR is broken into commits according to what replaces "unknown", in case that helps review. Squash before merging.

spec.html

ljharb · 2022-04-18T18:43:21Z

spec.html

@@ -9274,7 +9274,7 @@ <h1>Contains</h1>
    <emu-clause id="sec-static-semantics-contains" oldids="sec-object-initializer-static-semantics-contains,sec-static-semantics-static-semantics-contains,sec-function-definitions-static-semantics-contains,sec-arrow-function-definitions-static-semantics-contains,sec-generator-function-definitions-static-semantics-contains,sec-async-generator-function-definitions-static-semantics-contains,sec-class-definitions-static-semantics-contains,sec-async-function-definitions-static-semantics-Contains,sec-async-arrow-function-definitions-static-semantics-Contains" type="sdo">
      <h1>
        Static Semantics: Contains (
-          _symbol_: unknown,
+          _symbol_: a grammar symbol,


is "a grammar symbol" something defined already?

It's a term that the spec uses already, but (as with most grammar-related terms) it's not defined with a <dfn>.

jmdyck · 2022-08-06T03:35:51Z

(force-pushed to resolve merge conflicts)

michaelficarra · 2022-08-08T14:29:38Z

FYI #2842 handles the 5 _V_ and _W_ parameters of GetValue, PutValue, and InitializeReferencedBinding. So after these two PRs, all unknown parameters should be eliminated, correct?

jmdyck · 2022-08-08T17:37:28Z

FYI #2842 handles the 5 _V_ and _W_ parameters of GetValue, PutValue, and InitializeReferencedBinding.

Yes, I was planning to deal with those once #2744 was merged, but bakkot beat me to it!

So after these two PRs, all unknown parameters should be eliminated, correct?

Yup. There will probably be some lurking in in-flight PRs though.

jmdyck · 2022-08-16T00:01:35Z

(force-pushed to resolve merge conflicts)

spec.html

michaelficarra · 2022-08-19T21:43:59Z

spec.html

@@ -39963,7 +39962,7 @@ <h1>Map ( [ _iterable_ ] )</h1>
      <emu-clause id="sec-add-entries-from-iterable" type="abstract operation">
        <h1>
          AddEntriesFromIterable (
-            _target_: unknown,
+            _target_: an ordinary object,


is this worth calling out over just saying an Object?

Hard to say if the benefit is worth the cost, because they're both fairly small (I think). I'm okay either way, whatever the editors want.

@jmdyck Decision from editor call was to not use "an ordinary object" as a parameter type unless that refinement was needed for the algorithm steps (which it isn't here).

michaelficarra · 2022-08-19T21:57:39Z

spec.html

@@ -40907,7 +40906,7 @@ <h1>
        <h1>
          DetachArrayBuffer (
            _arrayBuffer_: an ArrayBuffer,
-            optional _key_: unknown,
+            optional _key_: an ECMAScript language value,


I think this can be anything.

Yeah, the spec isn't very clear on this point. I'll change it to anything.

On second thought, DetachArrayBuffer passes _key_ to SameValue(), which only accepts language values, which is presumably why I set _key_'s type to an ECMAScript language value. I'll leave it that way unless something else changes.

I think we should probably change the SameValue usage here to simply is. We can talk about it at the editor call.

@jmdyck Decision from editor call was to make the value as opaque as possible, so any would be better here. That will also necessitate the change from SameValue to is for comparison.

michaelficarra

LGTM otherwise.

jmdyck · 2022-08-30T20:03:31Z

force-pushed to rebase to main and address review comments.

jmdyck · 2022-09-03T03:31:05Z

Force-pushed to rebase to main.
Also added commits to address decisions from editors call.

jmdyck · 2022-09-14T19:04:20Z

(force-pushed to resolve merge conflict)

bakkot

LGTM other than the CreateBuiltInFunction thing.

I didn't review in full detail, but the types all look reasonable.

bakkot · 2022-09-20T06:41:01Z

spec.html

@@ -13865,7 +13865,7 @@ <h1>
        CreateBuiltinFunction (
          _behaviour_: an Abstract Closure, a set of algorithm steps, or some other definition of a function's behaviour provided in this specification,
          _length_: a non-negative integer or +&infin;,
-          _name_: a property key,
+          _name_: a property key or a Private Name,


I don't believe CreateBuiltinFunction actually does take a Private Name.

Not sure about this, but consider:

class Outer { #Inner = class {}; }

I think the (inner) ClassExpression will be passed with a Private Name to NamedEvaluation, which then invokes ClassDefinitionEvaluation, which then invokes CreateBuiltinFunction to create the constructor method.

Oh, yup, that's correct. The default class constructor case hadn't occurred to me.

syg

lgtm

SameValue accepts only ECMAScript language values as args, so DetachArrayBuffer's use of SameValue asserts that detach keys are ECMAScript language values. But detach keys should be as opaque as possible, so replace `SameValue` with `is`.

jmdyck · 2022-09-23T03:32:48Z

(force-pushed to squash it down to 2 commits)

SameValue accepts only ECMAScript language values as args, so DetachArrayBuffer's use of SameValue asserts that detach keys are ECMAScript language values. But detach keys should be as opaque as possible, so replace `SameValue` with `is`.

jmdyck added the editorial change label Apr 18, 2022

ljharb reviewed Apr 18, 2022

View reviewed changes

jmdyck force-pushed the unknown branch 3 times, most recently from a80692d to 76fe65d Compare April 28, 2022 13:53

jmdyck force-pushed the unknown branch from 76fe65d to fbcea07 Compare August 6, 2022 03:35

jmdyck force-pushed the unknown branch from fbcea07 to 52366a3 Compare August 16, 2022 00:00

michaelficarra reviewed Aug 18, 2022

View reviewed changes

spec.html Show resolved Hide resolved

michaelficarra reviewed Aug 19, 2022

View reviewed changes

spec.html Outdated Show resolved Hide resolved

michaelficarra reviewed Aug 19, 2022

View reviewed changes

spec.html Outdated Show resolved Hide resolved

michaelficarra reviewed Aug 19, 2022

View reviewed changes

spec.html Outdated Show resolved Hide resolved

michaelficarra reviewed Aug 19, 2022

View reviewed changes

michaelficarra approved these changes Aug 19, 2022

View reviewed changes

jmdyck force-pushed the unknown branch from 52366a3 to c24817c Compare August 30, 2022 20:02

jmdyck changed the title ~~Editorial: Almost eliminate "unknown" as a parameter type~~ Editorial: Eliminate "unknown" as a parameter type Aug 30, 2022

michaelficarra added editor call to be discussed in the next editor call and removed editor call to be discussed in the next editor call labels Aug 30, 2022

jmdyck force-pushed the unknown branch from c24817c to 10419f7 Compare September 3, 2022 03:29

jmdyck force-pushed the unknown branch from 10419f7 to c3891ba Compare September 14, 2022 19:03

bakkot approved these changes Sep 20, 2022

View reviewed changes

michaelficarra added the editor call to be discussed in the next editor call label Sep 20, 2022

syg approved these changes Sep 21, 2022

View reviewed changes

michaelficarra added ready to merge Editors believe this PR needs no further reviews, and is ready to land. and removed editor call to be discussed in the next editor call labels Sep 21, 2022

jmdyck added a commit to jmdyck/ecma262 that referenced this pull request Sep 23, 2022

Editorial: Eliminate "unknown" as a parameter type (tc39#2745)

18a82b5

jmdyck force-pushed the unknown branch from c3891ba to 18a82b5 Compare September 23, 2022 03:32

jmdyck added 2 commits September 22, 2022 22:32

Editorial: Eliminate "unknown" as a parameter type (tc39#2745)

fd49240

ljharb force-pushed the unknown branch from 18a82b5 to fd49240 Compare September 23, 2022 05:32

ljharb merged commit fd49240 into tc39:main Sep 23, 2022

jmdyck deleted the unknown branch September 24, 2022 00:37

jmdyck mentioned this pull request Sep 30, 2022

Editorial: Complete structured headers in “Specification Types” #2589

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Editorial: Eliminate "unknown" as a parameter type #2745

Editorial: Eliminate "unknown" as a parameter type #2745

jmdyck commented Apr 18, 2022 •

edited

Loading

ljharb Apr 18, 2022

jmdyck Apr 18, 2022

jmdyck commented Aug 6, 2022

michaelficarra commented Aug 8, 2022

jmdyck commented Aug 8, 2022

jmdyck commented Aug 16, 2022

michaelficarra Aug 19, 2022

jmdyck Aug 30, 2022

michaelficarra Aug 31, 2022

michaelficarra Aug 19, 2022

jmdyck Aug 30, 2022

jmdyck Aug 30, 2022

michaelficarra Aug 30, 2022

michaelficarra Aug 31, 2022

michaelficarra left a comment

jmdyck commented Aug 30, 2022

jmdyck commented Sep 3, 2022

jmdyck commented Sep 14, 2022

bakkot left a comment

bakkot Sep 20, 2022

jmdyck Sep 20, 2022

bakkot Sep 20, 2022

syg left a comment

jmdyck commented Sep 23, 2022

Editorial: Eliminate "unknown" as a parameter type #2745

Editorial: Eliminate "unknown" as a parameter type #2745

Conversation

jmdyck commented Apr 18, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmdyck commented Aug 6, 2022

michaelficarra commented Aug 8, 2022

jmdyck commented Aug 8, 2022

jmdyck commented Aug 16, 2022

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

michaelficarra left a comment

Choose a reason for hiding this comment

jmdyck commented Aug 30, 2022

jmdyck commented Sep 3, 2022

jmdyck commented Sep 14, 2022

bakkot left a 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

syg left a comment

Choose a reason for hiding this comment

jmdyck commented Sep 23, 2022

jmdyck commented Apr 18, 2022 •

edited

Loading