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

Allow quoted identifiers#23

Merged
rossberg merged 5 commits intomainfrom
names
May 21, 2024
Merged

Allow quoted identifiers#23
rossberg merged 5 commits intomainfrom
names

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.

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

~~~~~~~~~~~

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. ;)

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