Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Spec text format #385

Merged
merged 5 commits into from
Jun 14, 2023
Merged

Spec text format #385

merged 5 commits into from
Jun 14, 2023

Conversation

rossberg
Copy link
Member

Also, to sync up, implement dependent name spaces for fields in interpreter.

@rossberg rossberg requested a review from tlively June 12, 2023 19:49
@rossberg rossberg changed the base branch from main to spec.binary June 12, 2023 19:49
@rossberg rossberg mentioned this pull request Jun 12, 2023
53 tasks
Base automatically changed from spec.binary to main June 13, 2023 09:09
Copy link
Member

@tlively tlively left a comment

Choose a reason for hiding this comment

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

Does this contain the abbreviation allowing fields to be combined, e.g. (field i32 i64)? I didn't see that when reading through this PR.

Comment on lines 99 to 101
If inline declarations are given, their types must be *syntactically* equal to the types from the indexed definition;
possible type :ref:`substitutions <notation-subst>` from other definitions that might make them equal are not taken into account.
This is to simplify syntactic pre-processing in the presence of recursive types.
Copy link
Member

Choose a reason for hiding this comment

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

Can you give an example of what this allows and disallows? I don't grok the implications yet.

Copy link
Member Author

@rossberg rossberg Jun 14, 2023

Choose a reason for hiding this comment

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

For example, if you have

(type $s1 (struct))
(type $s2 (struct))
(type $t (func (param (ref $s1))))
(func $f (param (ref $s2)))

then it would not pick up $t but introduce a new $t2 that is using $t2. This is so that desugaring does not depend on type canonicalisation. This choice doesn't affect typability, and the difference is not observable for well-formed modules, unless you were to inspect the desugared syntax itself to discover the additional type entries.

Edit: Actually, type definitions added by desugaring are "observable" if you are sneaky and predict their raw type index. For example, this is technically valid, and always has been:

(func $f (param i32))
(func $g (type 0))

You could abuse this to observe that the above example indeed introduces an additional type as well.

Copy link
Member

Choose a reason for hiding this comment

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

Makes sense, thanks!

Comment on lines 133 to 134
where :math:`x` is the smallest existing :ref:`type index <syntax-typeidx>` whose definition in the current module is the :ref:`final <syntax-final>` :ref:`function type <syntax-functype>` :math:`[t_1^\ast] \toF [t_2^\ast]`.
If no such index exists, then a new :ref:`recursive type <text-rectype>` of the form
Copy link
Member

Choose a reason for hiding this comment

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

Does this capture the constraint that only types in singleton rec groups are eligible to be found? IIRC, that's how we decided this should work.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, you are right, that's missing. Fixed and made more precise.


(func (param (ref 0)) (result i32) (struct.get 0 $x (local.get 0)))
(func (param (ref $t1)) (result f32) (struct.get 1 $x (local.get 0)))
(func (param (ref $t2)) (result i64) (struct.get $t2 $x (local.get 0)))
Copy link
Member

Choose a reason for hiding this comment

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

It would be good to have tests for all combinations of #names and raw numbers for the type index and the field index if we don't have that already. Did you decide to do anything to anticipate having instructions with field indices but no type indices?

Copy link
Member Author

Choose a reason for hiding this comment

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

I added a simple test for each combination. (To be honest, we could probably use more tests for the various struct/array instructions in general.)

In terms of anticipation, no, what do you have in mind?

Copy link
Member

Choose a reason for hiding this comment

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

I had one idea for future-proofing the syntax described in the second paragraph here: #361 (comment)

Copy link
Member Author

Choose a reason for hiding this comment

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

Can you elaborate on that idea? I'm not really understanding why the syntax struct.get t (field t i) is more future-proof than struct.get t i. What does repeating the type twice buy us?

Copy link
Member

Choose a reason for hiding this comment

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

In the future if we have some struct.get_unannotated <fieldidx> instruction that has no type annotation, and if the text syntax for fieldidx contains a type index along with the symbolic field name, then we can still unambiguously figure out how to resolve the symbolic field name to an index. For the current instructions, which already contain type indices, the extra type index in the fieldidx syntax is redundant, so we can define abbreviations to make the repetition unnecessary.

Copy link
Member Author

Choose a reason for hiding this comment

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

Why couldn't we just use the text syntax struct.get_unannotated tidx? $x in that case? (Allowing omitting the type index only when the field index is numeric.)

Copy link
Member

Choose a reason for hiding this comment

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

That would work, too! If you'd be happy with that, then it sounds good to me.

Copy link
Member Author

@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.

Comments addressed, PTAL.

Comment on lines 133 to 134
where :math:`x` is the smallest existing :ref:`type index <syntax-typeidx>` whose definition in the current module is the :ref:`final <syntax-final>` :ref:`function type <syntax-functype>` :math:`[t_1^\ast] \toF [t_2^\ast]`.
If no such index exists, then a new :ref:`recursive type <text-rectype>` of the form
Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, you are right, that's missing. Fixed and made more precise.


(func (param (ref 0)) (result i32) (struct.get 0 $x (local.get 0)))
(func (param (ref $t1)) (result f32) (struct.get 1 $x (local.get 0)))
(func (param (ref $t2)) (result i64) (struct.get $t2 $x (local.get 0)))
Copy link
Member Author

Choose a reason for hiding this comment

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

I added a simple test for each combination. (To be honest, we could probably use more tests for the various struct/array instructions in general.)

In terms of anticipation, no, what do you have in mind?

Copy link
Member

@tlively tlively left a comment

Choose a reason for hiding this comment

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

LGTM % my previous comment about the (field i32 i64 ...) abbreviation.

@rossberg
Copy link
Member Author

LGTM % my previous comment about the (field i32 i64 ...) abbreviation.

Oops, sorry, had missed that comment. Indeed, added now.

@rossberg rossberg merged commit 5c128c9 into main Jun 14, 2023
@rossberg rossberg deleted the spec.text branch June 14, 2023 17:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants