Re: [whatwg/webidl] Give SharedArrayBuffer a dedicated type (PR #1311) from Domenic Denicola on 2023-05-31 (public-webapps-github@w3.org from May 2023)

From: Domenic Denicola <notifications@github.com>
Date: Wed, 31 May 2023 00:09:50 -0700
To: whatwg/webidl <webidl@noreply.github.com>
Cc: Subscribed <subscribed@noreply.github.com>
Message-ID: <whatwg/webidl/pull/1311/review/1452297350@github.com>
@domenic commented on this pull request.

Biggest issue is the mass conversion from using the typedefs to using the "X type" definitions, which aren't really suitable since we're talking about instances of types, not types themselves.

> @@ -8480,33 +8491,41 @@ that correspond to the union's [=member types=].
 
 <h4 id="es-buffer-source-types">Buffer source types</h4>
 
-Values of the IDL [=buffer source types=]
-are represented by objects of the corresponding ECMAScript class, with the following additional restrictions on those objects.
+A value of an IDL {{ArrayBuffer}} is represented by an object of the corresponding ECMAScript class.
+If it is not [=extended attributes associated with|associated with=] the [{{AllowResizable}}]
+[=extended attribute=], it can only be backed by ECMAScript {{ArrayBuffer}} objects |V| for which
+<a abstract-op>IsResizableArrayBuffer</a>(|V|) is false.
+
+A value of an IDL {{SharedArrayBuffer}} is represented by an object of the corresponding ECMAScript
+class. If it is not [=extended attributes associated with|associated with=] the [{{AllowResizable}}]
+[=extended attribute=], it can only be backed by ECMAScript {{ArrayBuffer}} objects |V| for which

```suggestion
[=extended attribute=], it can only be backed by ECMAScript {{SharedArrayBuffer}} objects |V| for which
```

> @@ -8480,33 +8491,41 @@ that correspond to the union's [=member types=].
 
 <h4 id="es-buffer-source-types">Buffer source types</h4>
 
-Values of the IDL [=buffer source types=]
-are represented by objects of the corresponding ECMAScript class, with the following additional restrictions on those objects.
+A value of an IDL {{ArrayBuffer}} is represented by an object of the corresponding ECMAScript class.

In both these paragraphs, it would be best if "ECMAScript `X`" linked to the ES spec, instead of the IDL spec. This is a preexisting issue though.

> @@ -8646,30 +8696,33 @@ a reference to the same object that the IDL value represents.
 </div>
 
 <div algorithm>
-    The <dfn export for="BufferSource">byte length</dfn> of a {{BufferSource}} |bufferSource| is
-    the value returned by the following steps:
+    The <dfn id=buffersource-byte-length export for="buffer source type">byte length</dfn> of a
+    [=buffer source type=] |bufferSource| is the value returned by the following steps:
 
     1.  Let |esBufferSource| be the result of [=converted to an ECMAScript value|converting=]
         |bufferSource| to an ECMAScript value.
     1.  If |esBufferSource| has a \[[ViewedArrayBuffer]] internal slot, then return
         |esBufferSource|.\[[ByteLength]].

Preexisting issue:

```suggestion
        |esBufferSource|.\[[ViewedArrayBuffer]].\[[ByteLength]].
```

>      1.  Let |esBufferSource| be the result of [=converted to an ECMAScript value|converting=]
         |bufferSource| to an ECMAScript value.
     1.  Return the result of [=converted to an IDL value|converting=]
-        |esBufferSource|.\[[ViewedArrayBuffer]] to an IDL value of type {{ArrayBuffer}}.
+        |esBufferSource|.\[[ViewedArrayBuffer]] to an IDL value of type [=buffer type=].

This doesn't seem right. You need to switch on whether [[ViewedArrayBuffer]] IsSharedArrayBuffer() or not, and then convert appropriately.

> @@ -8718,6 +8772,8 @@ a reference to the same object that the IDL value represents.
     1.  Let |esArrayBuffer| be the result of [=converted to an ECMAScript value|converting=]
         |arrayBuffer| to an ECMAScript value.
     1.  Assert: [$IsSharedArrayBuffer$](|esArrayBuffer|) is false.
+        <!-- We'll keep this in for now as we're generally bad at distinguishing IDL and JS
+             objects. -->

What's the problem? We converted in step 1.

> @@ -8728,8 +8784,9 @@ a reference to the same object that the IDL value represents.
 </div>
 
 <div algorithm>
-    A {{BufferSource}} |bufferSource| is <dfn for="BufferSource" export>detached</dfn> if the
-    following steps return true:
+    A [=buffer source types=] |bufferSource| is

```suggestion
    A [=buffer source type=] |bufferSource| is
```

> @@ -5465,6 +5465,8 @@ The <dfn id="dfn-string-type" export>string types</dfn> are
 {{DOMString}}, all [=enumeration types=],
 {{ByteString}} and {{USVString}}.
 
+The <dfn export>buffer types</dfn> are {{ArrayBuffer}} and {{SharedArrayBuffer}}.

So, Web IDL is quite picky about distinguishing types. In particular, a "type" like "string type" or "buffer type" refers to something in the category of types. It does not refer to things in the category of values.

Thus, you really shouldn't say things like "If _bufferSource_ is a buffer type". Because a value can never be a type. A value can be an instance of a type, I guess. (But I don't see much use of that language for the other types defined in this section?)

For example, look at all the reference sites for https://whatpr.org/webidl/1311.html#dfn-numeric-type , and notice how they're specifically talking about types (e.g. return types, overload resolution algorithm internals) and not about values.

> @@ -8646,30 +8696,33 @@ a reference to the same object that the IDL value represents.
 </div>
 
 <div algorithm>
-    The <dfn export for="BufferSource">byte length</dfn> of a {{BufferSource}} |bufferSource| is
-    the value returned by the following steps:
+    The <dfn id=buffersource-byte-length export for="buffer source type">byte length</dfn> of a
+    [=buffer source type=] |bufferSource| is the value returned by the following steps:

So for example, I think using the typedef here is more correct, because if |bufferSource| were a "buffer source type", then it would either be the type "DataView" or the type "ArrayBuffer" or the type .... It would not be any specific IDL object.

> @@ -8745,6 +8802,8 @@ a reference to the same object that the IDL value represents.
     1.  Let |esArrayBuffer| be the result of [=converted to an ECMAScript value|converting=]
         |arrayBuffer| to an ECMAScript value.
     1.  Assert: [$IsSharedArrayBuffer$](|esArrayBuffer|) is false.
+        <!-- We'll keep this in for now as we're generally bad at distinguishing IDL and JS
+             objects. -->

Again this seems fine?

> @@ -8882,34 +8946,34 @@ See the rules for converting ECMAScript values to IDL [=buffer source types=] in
     <pre highlight="webidl">

Maybe update the intro sentence to explain that you're actually showing all possible combinations.

(Preexisting issue.)

> @@ -14367,6 +14433,19 @@ The {{BufferSource}} typedef is used to represent objects
 that are either themselves an {{ArrayBuffer}} or which
 provide a view on to an {{ArrayBuffer}}.
 
+Note: [{{AllowShared}}] cannot be used in conjunction as {{ArrayBuffer}} does not support it.

```suggestion
Note: [{{AllowShared}}] cannot be used with {{BufferSource}} as {{ArrayBuffer}} does not support it.
```

-- 
Reply to this email directly or view it on GitHub:
https://github.com/whatwg/webidl/pull/1311#pullrequestreview-1452297350
You are receiving this because you are subscribed to this thread.

Message ID: <whatwg/webidl/pull/1311/review/1452297350@github.com>
Received on Wednesday, 31 May 2023 07:09:55 UTC