Use `open` instead of `final` in the text format #413

bvisness · 2023-07-28T21:53:23Z

Addresses discussion from #333. With this patch, the keyword final in subtypes has been replaced with the keyword open:

(type $s1 (sub (struct)))                       ;; final with no super types
(type $s2 (sub open (struct)))                  ;; non-final with no super types
(type $s3 (sub $s2 (struct (field i32))))       ;; final with one super type
(type $s4 (sub open $s2 (struct (field i32))))  ;; non-final with one super type

(type $s5 (struct))                             ;; equivalent to $s1 (final)

This avoids a confusing flip-flopping behavior where a seemingly-unnecessary empty sub was commonly used to make types non-final. An empty sub is now equivalent to no sub, making the abbreviation more obvious.

This patch is currently quite minimal, and only modifies enough of the spec and interpreter to support open in the text format. The word final is otherwise used throughout the spec and interpreter. If we would rather use the word open everywhere, I'd be happy to update the rest of the spec and interpreter to match.

bvisness · 2023-07-28T21:54:16Z

document/core/text/types.rst

-     \Tsubtype &\equiv&
-     \text{(}~~\text{rec}~~\Tsubtype~~\text{)} \\
+     \Tdeftype &\equiv&
+     \text{(}~~\text{rec}~~\Tdeftype~~\text{)} \\


This is not related to my work, but I believe it to be a bug in the spec that is easy to fix.

No, I think this was correct before.

I don't see how it could be correct before. According to the grammar above, recursion groups contain deftypes ((rec vec(deftype))), and deftypes contain subtypes ((type subtype)). The production subtype = (rec subtype) therefore doesn't make sense, as it implies that it expands to something like (rec (sub (struct))), which violates the main grammar.

@bvisness's fix here looks right to me. We don't want to allow (sub (struct)), a subtype, to appear on its own, but we do want to allow (type (sub (struct))), a deftype, to appear on its own.

Ah, you are right, I was confused by the impedance mismatch with the abstract syntax, where deftype is something else. We should probably not be naming this one in the text format deftype, but rather something else (typedef?).

bvisness · 2023-07-28T21:54:32Z

document/core/text/types.rst

-   \begin{array}{llclll}
-   \production{reference type} &
+   \begin{array}{lcl}
+   \production{reference type}


This is also unrelated to my work, but this table was jank, and now it is not.

Ah, the production name needs its own column, in case its used. The correct fix here is to add &'s below, i.e., change each \\ to \\& (perhaps except the last). You can simplify the column scheme to {llcl}.

bvisness · 2023-07-28T21:55:12Z

document/core/text/types.rst

   \production{sub type} &
     \Tcomptype &\equiv&
-     \text{(}~~\text{sub}~~\text{final}~~\epsilon~~\Tcomptype~~\text{)} \\
+     \text{(}~~\text{sub}~~\Tcomptype~~\text{)} \\


It's not clear to me why the epsilon was in here, but I can add it back if it was actually necessary.

It probably represented the empty vector of supertypes. I think it would make sense to put it and the "and arguments" language back.

What @tlively said, this was intentional.

rossberg

I'm afraid this PR is rather incomplete.

The text format is intended to be a direct reflection of the abstract syntax. So, if we toggle the text format, then it's necessary that we apply the same change to the abstract syntax. Similarly the interpreter AST.

A further consequence then is that we'll also need to adjust the spec terminology. It's confusing if it talks about final types everywhere when the syntax inversely only speaks of open types. We'd probably want to replace all occurrence of "final type" with "non-open" type.

Unfortunately, I think that would have a rather adverse affect on overall spec clarity. Less clear terminology for the sole purpose of a shorthand in the text format is perhaps not a good trade-off.

The one way out I see is to fall back to the earlier suggestion of having both final and open keyword and neither is optional. That might also lead to less surprise in the text format than this PR – I suspect very few readers would expect subtype definitions to implicitly mean final.

rossberg · 2023-09-06T12:56:44Z

document/core/text/types.rst

-   \begin{array}{llclll}
-   \production{reference type} &
+   \begin{array}{lcl}
+   \production{reference type}


Ah, the production name needs its own column, in case its used. The correct fix here is to add &'s below, i.e., change each \\ to \\& (perhaps except the last). You can simplify the column scheme to {llcl}.

rossberg · 2023-09-06T13:00:05Z

document/core/text/types.rst

       &\Rightarrow& \TSUB~\TFINAL^?~x^\ast~\X{ct} \\
   \end{array}

+.. note::
+   A subtype is :ref:`final <syntax-subtype>` if the :math:`\text{open}` keyword is not present, and vice versa.


See above, this comment shouldn't be needed if the text format is consistent with the AST.

rossberg · 2023-09-06T13:01:20Z

document/core/text/types.rst


 Abbreviations
 .............

-Singular recursive types can omit the |Trec| keyword:
+Singular recursive types can omit the :math:`\text{rec}` keyword:


Why this change?

Because it refers to a keyword of the text format, not a structural element of a wasm module. The primary difference is in how they render, but suppose the structural element were named "recgrp" instead of "rec" - if that were the case, the sentence would make no sense, as recgrp is not a text format keyword.

It may be useful to note that macros.def does not define Trec, so I think it is parsing as |TREC|, which does link to the abstract syntax rather than the textual keyword.

Ah, good points.

Done as a standalone fix in #426.

rossberg · 2023-09-06T13:01:26Z

document/core/text/types.rst

-     \Tsubtype &\equiv&
-     \text{(}~~\text{rec}~~\Tsubtype~~\text{)} \\
+     \Tdeftype &\equiv&
+     \text{(}~~\text{rec}~~\Tdeftype~~\text{)} \\


No, I think this was correct before.

rossberg · 2023-09-06T13:02:30Z

document/core/text/types.rst


 .. math::
-   \begin{array}{llclll}
+   \begin{array}{lcl}


I think this needs to be at least llcl, otherwise Latex will barf.

rossberg · 2023-09-06T13:03:20Z

document/core/text/types.rst

   \production{sub type} &
     \Tcomptype &\equiv&
-     \text{(}~~\text{sub}~~\text{final}~~\epsilon~~\Tcomptype~~\text{)} \\
+     \text{(}~~\text{sub}~~\Tcomptype~~\text{)} \\


What @tlively said, this was intentional.

tlively · 2023-09-08T16:37:11Z

I would support having both sub open and sub final. That seems strictly clearer, although slightly more verbose. It's also useful to still have the "final" language for non-open types.

@bvisness, will you have time to work on this more today and Monday? If not, I can help out.

tlively · 2023-09-09T01:04:14Z

@rossberg, would it suffice to add sub open just to the text format and keep final? in the abstract syntax, or would you want to change the abstract syntax to have final | open as well?

rossberg · 2023-09-09T16:08:01Z

I think we should keep them consistent. It actually simplifies some formulations as well when we have a named phrase like ext ::= FINAL | OPEN, because then ext can be used as a metavariable instead of clumsy FINAL?.

tlively · 2023-09-11T04:40:17Z

@rossberg, would you be able to work on completing this change? If not, I will take a stab at it in the morning pacific time.

rossberg · 2023-09-11T12:02:21Z

@tlively, I can try, but I can't promise anything — I'm on an a flight Odyssee right now after I was dumped from my flight back to Europe yesterday. Hence my bigger concern is that I even make it back in time for the meeting tomorrow. ;)

bvisness · 2023-09-11T19:29:14Z

Apologies for my absence. I can resume work on this tomorrow if it's not already done.

Instead of using final? in the syntax, use ext ::= open | final. Implements the design discussed in #413. Update the interpreter, the abstract syntax, the binary and text formats, and MVP.md.

bvisness

@tlively's work in #423 looks good to me, so this PR can probably be closed, but I did respond to a couple of the questions up above. In particular, I still feel that the one abbreviation does contain a bug.

bvisness · 2023-09-11T21:04:14Z

document/core/text/types.rst

-     \Tsubtype &\equiv&
-     \text{(}~~\text{rec}~~\Tsubtype~~\text{)} \\
+     \Tdeftype &\equiv&
+     \text{(}~~\text{rec}~~\Tdeftype~~\text{)} \\


I don't see how it could be correct before. According to the grammar above, recursion groups contain deftypes ((rec vec(deftype))), and deftypes contain subtypes ((type subtype)). The production subtype = (rec subtype) therefore doesn't make sense, as it implies that it expands to something like (rec (sub (struct))), which violates the main grammar.

bvisness · 2023-09-11T21:09:03Z

document/core/text/types.rst


 Abbreviations
 .............

-Singular recursive types can omit the |Trec| keyword:
+Singular recursive types can omit the :math:`\text{rec}` keyword:


Because it refers to a keyword of the text format, not a structural element of a wasm module. The primary difference is in how they render, but suppose the structural element were named "recgrp" instead of "rec" - if that were the case, the sentence would make no sense, as recgrp is not a text format keyword.

@bvisness

When referring to the `rec` keyword, do not link to the abstract syntax that happens to have the same name. Fix suggested by @bvisness and split out from #413. Do not link to the abstract syntax

@bvisness

Since a deftype is already something else outside the text format. Also fix an abbreviation to correctly use "typedef" instead of "subtype". Adapted from a fix by @bvisness in #413.

bvisness added 2 commits July 28, 2023 16:43

Update text format spec to use open instead of final

5d13b53

Update interpreter and tests to use open

21f85de

bvisness commented Jul 28, 2023

View reviewed changes

tlively requested a review from rossberg July 29, 2023 02:13

rossberg reviewed Sep 6, 2023

View reviewed changes

tlively mentioned this pull request Sep 8, 2023

Make final types the default WebAssembly/binaryen#5918

Merged

rossberg mentioned this pull request Sep 9, 2023

Spec authoring tracking issue #376

Open

53 tasks

tlively mentioned this pull request Sep 11, 2023

Add (sub open ...) and make it mandatory #423

Open

bvisness commented Sep 11, 2023

View reviewed changes

tlively mentioned this pull request Sep 12, 2023

Remove incorrect link to abstract syntax #426

Merged

tlively mentioned this pull request Sep 12, 2023

Rename text "deftype" to "typedef" #427

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use `open` instead of `final` in the text format #413

Use `open` instead of `final` in the text format #413

bvisness commented Jul 28, 2023

bvisness Jul 28, 2023

rossberg Sep 6, 2023

bvisness Sep 11, 2023

tlively Sep 11, 2023

rossberg Sep 12, 2023

bvisness Jul 28, 2023

rossberg Sep 6, 2023

bvisness Jul 28, 2023

tlively Jul 29, 2023

rossberg Sep 6, 2023

rossberg left a comment

rossberg Sep 6, 2023

rossberg Sep 6, 2023

rossberg Sep 6, 2023

bvisness Sep 11, 2023

tlively Sep 11, 2023

rossberg Sep 12, 2023

tlively Sep 12, 2023

rossberg Sep 6, 2023

rossberg Sep 6, 2023

rossberg Sep 6, 2023

tlively commented Sep 8, 2023

tlively commented Sep 9, 2023

rossberg commented Sep 9, 2023

tlively commented Sep 11, 2023

rossberg commented Sep 11, 2023

bvisness commented Sep 11, 2023

bvisness left a comment •

edited

bvisness Sep 11, 2023

bvisness Sep 11, 2023

Use open instead of final in the text format #413

Are you sure you want to change the base?

Use open instead of final in the text format #413

Conversation

bvisness commented Jul 28, 2023

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rossberg 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

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

tlively commented Sep 8, 2023

tlively commented Sep 9, 2023

rossberg commented Sep 9, 2023

tlively commented Sep 11, 2023

rossberg commented Sep 11, 2023

bvisness commented Sep 11, 2023

bvisness left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Use `open` instead of `final` in the text format #413

Use `open` instead of `final` in the text format #413

bvisness left a comment •

edited