fix: module imports should not use type index as their function identifier

[ColinEberhardt]

fixes #406

@xtuc there is a wasm-edit test failing as a result of this change, wondering if you have any pointers?

[ColinEberhardt]

this simple test, which adds a couple of type definitions, was enough to repro the issues from #406

[ColinEberhardt]

this change was not necessarily needed to make tests pass, but this does mean we have the same generated names when parsing wat

[xtuc]

Following https://github.com/xtuc/webassemblyjs/pull/411/files#r202406723 we could add our own func increment, or maybe we already have this information?

[xtuc]

The "AST synchronization" test suite in wasm-edit ensure that we mutate the AST correctly by comparing it to a freshly decoded AST.

[xtuc]

Maybe we should try to eliminate the unique name generator (because statefull), and rather use the typeindex here.

[ColinEberhardt]

Maybe we should try to eliminate the unique name generator (because statefull)

I agree, that maybe the type name generator should be removed. I'm not sure what it's used for!

and rather use the typeindex here

No ... that's the bug that this PR is fixing. Module imports are functions which are referenced by index, i.e. if you have the following:

(type (func (result i32)))
(type (func (param i32) (result i32)))
(import "a" "c" (func $foo (type 1)))
(func $bar)

The function foo has index 0, and bar has index 1. The index does not relate to the type reference.

[ColinEberhardt]

The "AST synchronization" test suite in wasm-edit ensure that we mutate the AST correctly by comparing it to a freshly decoded AST.

OK - I'll explore further and see what I've broken!

[ColinEberhardt]

The "AST synchronization" test suite in wasm-edit ensure that we mutate the AST correctly by comparing it to a freshly decoded AST.

@xtuc ok, I've had a look at the test and understand what it is doing now. I've fixed the test by making the following changes:

• the makeFuncNodes function was adding the AST for the function type as well as the function itself. I've moved the construction of function types into a separate step as the module imports use this too
• I've changed the order, to add the module import first
• following this, the test adds a function, which now has an index of 1, and an identifier of func_1. As discussed in refactor index #409 - this is something that we'll probably remove in future, but for now, this fixes the original issue where type indices were being used as function indices.

[ColinEberhardt]

The tests are currently failing, but looks like a CI issue rather than a unit test failure!

[xtuc]

Thanks for updates!

[ColinEberhardt]

kicked Travis and the build works now :-)