WebAssembly / component-model

Repository for design and specification of the Component Model
Other
914 stars 78 forks source link

Disallow aliasing core module types into a core module's index space #265

Closed fitzgen closed 9 months ago

fitzgen commented 9 months ago

AFAICT, this is currently permitted by the spec, but until/unless core module linking exists, this is problematic because it means that one can't reuse off-the-shelf core module validation within component module validation. It requires extending core module validation structures to also be able to store core module types.

In fact, the wasmparser crate doesn't even support this today, and it was only noticed as I did a large ish refactoring of how it internally represents types.

cc @lukewagner @alexcrichton

alexcrichton commented 9 months ago

To confirm, you mean an outer type alias from within a core module type declared within a component, right? I ask because core modules don't have outer aliases but core module types to, so I think core modules themselves won't have any issues.

fitzgen commented 9 months ago

I think so? What we ran into here: https://github.com/bytecodealliance/wasm-tools/pull/1257/files#diff-c612a3d76ae2ac5f0386d8b3ecd10b8dafd3900f99648771aeb88c0138a82e3fR1505-R1510

alexcrichton commented 9 months ago

Ah ok yes, so the question here is about this component:

(component $C
  (core type $t (module))
  (core type (module
    (alias outer $C $t (type $my_t))
  ))
)

As of yesterday wasm-tools accepts this component. Once https://github.com/bytecodealliance/wasm-tools/pull/1257 lands, however, that module will fail with:

error: not implemented: aliasing core module types into a core module's types index space (at offset 0xd)

The spec issue here is the question of whether wasm-tools is "in the wrong" and should implement this, or whether it should become a hard validation error. The rationale for not supporting this is that wasmparser's state for validating a core module type is the same as the state for validating a core module itself. In core wasm core module types themselves don't need to be handled, so there's no support for modeling this.

Another loose argument in favor is that there's nothing you can do with a core module type in a core module type itself, so it's somewhat more conservative to disallow it for now.

lukewagner commented 9 months ago

That's a great point: it's a bit silly to allow the definition of a core module type that is impossible to implement. The current allowance of this mostly just falls out of the grammar, but we could easily and conservatively make it a validation error. If core wasm ever gets module-linking, then we'd naturally relax this restriction. I'll write up a PR in a sec.

lukewagner commented 9 months ago

Ah, the explainer (Type Definitions section, first paragraph after core:moduletype) and Binary.md (second bullet) already reject nested core module types with the explicit intention of disallowing core-modules-importing-or-exporting-core-modules; I just forgot to reject the outer-alias case.

lukewagner commented 9 months ago

Fixed in #266