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

Allow quoted identifiers #23

Merged
merged 5 commits into from
May 21, 2024
Merged

Allow quoted identifiers #23

merged 5 commits into from
May 21, 2024

Conversation

rossberg
Copy link
Member

@rossberg rossberg commented Apr 4, 2024

Address #21.

Allow arbitrary non-empty names (well-formed unicode string literals) as identifiers.

Extend spec, interpreter, and test suite.

Note: This PR does not per se remove the explicit @name annotations from the proposal, because they are not equivalent: they express the actual presence of a corresponding custom section, whereas the use of symbolic names doesn't, even though they can be utilised by a tool to construct such a custom section (as before). Also, annotations are more expressive, e.g., can provide alternative names to the same binder, i.e., quoted identifiers are not sufficient to convert all forms of name section into text.

@rossberg rossberg requested a review from tlively April 4, 2024 10:22
@tlively
Copy link
Member

tlively commented Apr 4, 2024

The name annotation might technically be more expressive, but is there any demand for it? We shouldn't spec something that nobody implements or uses.

@rossberg
Copy link
Member Author

rossberg commented Apr 4, 2024

Well, the idea underlying this proposal is that every custom section should have a text equivalent.


.. math::
\begin{array}{llclll@{\qquad}l}
\production{identifier} & \Tid &::=&
\text{\$}~\Tidchar^+ \\
\text{\$}~c^\ast{:}\Tidchar^+ &\Rightarrow& c^\ast \\ &&|&
\text{\$}~c^\ast{:}\Tname &\Rightarrow& c^\ast & (\iff |c^\ast| > 0) \\
Copy link
Member

Choose a reason for hiding this comment

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

Why disallow the empty name? $"" seems like it should be a valid identifier.

@@ -133,7 +133,7 @@ The *look-ahead* restrictions on the productions for |Tblockchar| disambiguate t
Annotations
~~~~~~~~~~~

An *annotation* is a bracketed token sequence headed by an *annotation id* of the form :math:`\T{@id}`.
An *annotation* is a bracketed token sequence headed by an *annotation id* of the form :math:`\text{@id}` or :math:`\text{@"..."}`.
Copy link
Member

Choose a reason for hiding this comment

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

The rendering of \text{@"..."} is pretty ugly. Can we use \ldots in there to make it prettier?

@tlively
Copy link
Member

tlively commented Apr 4, 2024

Well, the idea underlying this proposal is that every custom section should have a text equivalent.

The syntax for arbitrary custom sections means that this goal is met without the @name annotation, albeit not in a very useful way. I would still like to see evidence that anyone wants to use the @name annotations. @alexcrichton, you were concerned about reduced expressivity without the @name annotations in this comment. What are you current thoughts on the usefulness of the @name annotations?

@alexcrichton
Copy link
Contributor

In the abstract I do not personally have a use case for @name insofar as all practical wasm modules I've worked with all have a name section where names are unique and fit the existing identifier grammer. I've only used @name in testing/fuzzing/etc, never for a real-world module.

My comment there though was also specifically addressing the point that if the purpose of @name is to have a text format representation for any valid name section then somehow duplicate names need to be handled as well. Removing @name, which this PR is not proposing, would mean that such a name section could not be represented in the text format. I don't know if this is a big issue though. A name section which names function 10 in a module of 8 functions also can't be represented in the text format.

@rossberg
Copy link
Member Author

rossberg commented Apr 5, 2024

A name section which names function 10 in a module of 8 functions also can't be represented in the text format.

I'm afraid that's apples vs potatoes: the existing spec of the name section already requires referenced indices to exist, but at the same time explicitly allows multiple names for a given index. The custom section validator implemented for the reference interpreter as part of this proposal even checks it accordingly. ;)

@rossberg rossberg merged commit ff86dc2 into main May 21, 2024
5 checks passed
@rossberg rossberg deleted the names branch May 21, 2024 17:29
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.

3 participants