Skip to content
This repository was archived by the owner on Apr 25, 2025. It is now read-only.

Add prose for typing rule of try-catch-catch_all. #222

Merged
merged 2 commits into from
Sep 9, 2022

Conversation

ioannad
Copy link
Collaborator

@ioannad ioannad commented Sep 7, 2022

Fixes related .. todo:: section.

Also changed notation to match the formal overview document.

@ioannad ioannad force-pushed the spec-prose-valid-try-catch branch from e172a0c to 06cb90a Compare September 7, 2022 12:33
@ioannad ioannad merged commit b7cf92c into WebAssembly:main Sep 9, 2022
Copy link
Member

@rossberg rossberg left a comment

Choose a reason for hiding this comment

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

Sorry, being a bit late.

* Under context :math:`C'`,
the instruction sequence :math:`\instr_1^\ast` must be :ref:`valid <valid-instr-seq>` with type :math:`[t_1^\ast] \to [t_2^\ast]`.

* Let :math:`C''` be the same :ref:`context <context>` as :math:`C`, but with the :ref:`label <exec-label>` :math:`\LCATCH~[t_2^\ast]` prepended to the |CLABELS| vector.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* Let :math:`C''` be the same :ref:`context <context>` as :math:`C`, but with the :ref:`label <exec-label>` :math:`\LCATCH~[t_2^\ast]` prepended to the |CLABELS| vector.
* Let :math:`C''` be the same :ref:`context <context>` as :math:`C`, but with the :ref:`label type <syntax-labeltype>` :math:`\LCATCH~[t_2^\ast]` prepended to the |CLABELS| vector.


* Let :math:`C''` be the same :ref:`context <context>` as :math:`C`, but with the :ref:`label <exec-label>` :math:`\LCATCH~[t_2^\ast]` prepended to the |CLABELS| vector.

* For every :math:`(\CATCH~x~\instr_2^\ast)`:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* For every :math:`(\CATCH~x~\instr_2^\ast)`:
* For every :math:`x_i` and :math:`\instr_{2i}^\ast` in :math:`(\CATCH~x~\instr_2^\ast)^\ast`:


* The tag :math:`C.\CTAGS[x]` must be defined in the context :math:`C`.

* Let :math:`[t^\ast] \to []` be its :ref:`tag type <syntax-tagtype>`.
Copy link
Member

Choose a reason for hiding this comment

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

This should actually check that the return type is [].

Suggested change
* Let :math:`[t^\ast] \to []` be its :ref:`tag type <syntax-tagtype>`.
* Let :math:`[t_{3i}^\ast] \to [t_{4i}^\ast]` be the :ref:`tag type <syntax-tagtype>` :math:`C.\tags[x]`.
* The :ref:`result type <syntax-resulttype>` :math:`[t_{4i}^\ast]` must be empty.

* Let :math:`[t^\ast] \to []` be its :ref:`tag type <syntax-tagtype>`.

* Under context :math:`C''`,
the instruction sequence :math:`\instr_2^\ast` must be :ref:`valid <valid-instr-seq>` with type :math:`[t^\ast] \to [t_2^\ast]`.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
the instruction sequence :math:`\instr_2^\ast` must be :ref:`valid <valid-instr-seq>` with type :math:`[t^\ast] \to [t_2^\ast]`.
the instruction sequence :math:`\instr_{2i}^\ast` must be :ref:`valid <valid-instr-seq>` with type :math:`[t_{3i}^\ast] \to [t_2^\ast]`.

* Under context :math:`C''`,
the instruction sequence :math:`\instr_2^\ast` must be :ref:`valid <valid-instr-seq>` with type :math:`[t^\ast] \to [t_2^\ast]`.

* If there is a :math:`(\CATCHALL~\instr_3^\ast)`, then:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* If there is a :math:`(\CATCHALL~\instr_3^\ast)`, then:
* If :math:`(\CATCHALL~\instr_3^\ast)^?` is not empty, then:

@aheejin
Copy link
Member

aheejin commented Sep 16, 2022

@rossberg Thanks for the review! Just in case you'd like to do final check, there also have been several recently landed PRs.

@rossberg
Copy link
Member

@aheejin, yes, I saw. I'll comment on the ones I find issues with.

@rossberg
Copy link
Member

Btw, with the introduction of label type, I think all places that say something like "add result type [t*] to C.labels" need to change to say "label type" instead.

ioannad added a commit to ioannad/exception-handling that referenced this pull request Oct 21, 2022
… everywhere.

This addresses additional review comments to PR WebAssembly#222, that were made after it was merged.
The last review comment in the discussion suggests to adjust all validation labels to use label types instead of just result types.
Should address all occurrences of validation labels.

Additionally adds a boolean catch_label to control frames in the validation algorithm,
and some related functionality, fixing the cases for opcodes `catch` and `catch_all`.
@ioannad
Copy link
Collaborator Author

ioannad commented Oct 21, 2022

Btw, with the introduction of label type, I think all places that say something like "add result type [t*] to C.labels" need to change to say "label type" instead.

@rossberg created PR #241 with the additional changes requested here, also changing to "label type" I think in all appropriate places, including ctrl_frames.

@ioannad ioannad deleted the spec-prose-valid-try-catch branch October 24, 2022 17:09
ioannad added a commit that referenced this pull request Dec 23, 2022
#241)

This addresses additional review comments to PR #222, that were made after it was merged.
The last review comment in the discussion suggests to adjust all validation labels to use label types instead of just result types.
Should address all occurrences of validation labels.

Additionally adds a boolean catch_label to control frames in the validation algorithm,
and some related functionality, fixing the cases for opcodes `catch` and `catch_all`.

* Apply suggestions from code review

Co-authored-by: Andreas Rossberg <[email protected]>
Co-authored-by: Heejin Ahn <[email protected]>

* Reverting changes to typing of CAUGHTadm.

Changes to this rule are now done in PR #244
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants