Clarify and regularize text format index parsing rules#655
Conversation
| embeds an expanded version of [`core:externidx`] prefixed with `core`. | ||
| ```ebnf | ||
| idx ::= <u32> | <core:id> | ||
| core:{Xᵢ}sortidx ::= (Xᵢ <idx>) (for Xᵢ ∈ <core:sort>) |
There was a problem hiding this comment.
I think this should be (core Xᵢ <idx>), as the Xᵢ will be un-prefixed (e.g. memory alone).
There was a problem hiding this comment.
When a core:{X}idx is used inside a (core ...) block (e.g., (core instance (instantiate $M (with "" (instance (export "mem" (memory $i "mem"))))))), there is no core needed, so the approach used in this PR is that core:{X}idx rules always have no core prefix, and when you need one, the grammar has an explicit core-prefix(<core:{X}idx>).
| idx ::= <u32> | <core:id> | ||
| core:{Xᵢ}sortidx ::= (Xᵢ <idx>) (for Xᵢ ∈ <core:sort>) | ||
| core:{Xᵢ}idx ::= <idx> | <core:{Xᵢ}sortidx> (for Xᵢ ∈ <core:sort>) | ||
| core:externidx ::= <core:{X₁}sortidx> | ... | <core:{Xₙ}sortidx> (for X₁,…,Xₙ ∈ <core:sort>) |
There was a problem hiding this comment.
Is this too permissive? It seems to include e.g. (core module $foo), but the new rule for core:inlineexport is:
core:inlineexport ::= (export <core:name> <core:externidx>)
That would seemingly imply that we could have a core module exporting a core module, which is not the case. I guess the problem is that core:sort includes not just things in core WebAssembly, but also core modules and core instances themselves.
There was a problem hiding this comment.
So this is where the validation rules come in (in this and other cases) to winnow down what the grammar allows and throw out cases that aren't currently supported by core wasm (but hypothetically could in the future); I think this is mentioned below in the explainer.
| aliastarget ::= export <idx> <name> | ||
| | core export <idx> <core:name> | ||
| | outer <idx> <idx> |
There was a problem hiding this comment.
This seems unfortunate to me because it's no longer clear which kind of index is required here. I'm not sure if this is how it works today, but could it possibly take e.g. <core:instanceidx> instead of just <idx>? This would allow either a bare index or (core instance $foo).
Alternatively you could maybe just define a little notation for what kind of bare index is required, just for reader comprehension.
There was a problem hiding this comment.
With the grammar rules as defined, a <core:instanceidx> would mean that you could use an inline alias inside an alias which seemed like maybe too inceptive. Ultimately, the grammar bottoms out at a <u32> or <id>, and validation rules say "it has to be an instance in the instance index space", so that seemed kinda good enough, given that the second <idx> also needs special validation rules?
This PR intends to resolve #648 by regularizing and more-clearly defining the text format rules for core- and component-level indices, identifiers and inline aliases. One nice thing with the changes in this PR is that there is now an explicit
core-prefix(...)in the grammar at all the places wherecoremight (when a plain$idis not used) get injected.The
test/syntax/indices.wasttries to test all the syntactic cases touched by this PR. The commented-out lines currently fail inwasm-toolsand the rest pass as-is. This helps identify what the concrete delta from the status quo is. I didn't add any negative tests since I expect we'll need a phase-out process before rejecting anything that is accepted now.PTAL @bvisness @alexcrichton