Initialize imported module

[sampaioletti]

While playing with my emlibc branch I was having to explicitly set my module.GlobalIndexSpace for my imports rather than just specifying module.Global after comparing the code in wasm/imports.go to wasm/module.go it appears we may be missing the initiliization code

	for _, fn := range []func() error{
		m.populateGlobals,
		m.populateFunctions,
		m.populateTables,
		m.populateLinearMemory,
	} {
		if err := fn(); err != nil {
			return nil, err
		}
	}

So I dropped it in after a module is imported and it seemed to work. I'm nor sure if this has other implications or if the original is the intended behavior...so i'm sharing

Thanks.

[twitchyliquid64]

How are you constructing the Module ?

We should think about how the API for manually building a module should look like.

Some ideas:

1. We have a ModuleBuilder{} with methods to add globals/functions/etc, and then you call Build().
2. We add an Build() or Setup() method to type Module, so the user can get these things populated.
3. We have a new ResolveModule package method for bringing in user-specified modules.

What do you think?

[sampaioletti]

I'm constructing the module in the vain of that found in the Example from the docs

https://github.com/sampaioletti/wagon/blob/1e71fcd3777b154c901701743ae3930e472e86fc/internal/emlibc/resolver.go#L30-L113

1. I would say it should either be done by running both imported modules and user modules through the same initialization pipeline (what i was trying to accomplish)
2. Through a builder, but only if there is some advantage for user modules..i.e. further customization or...

Either way I think we could at least come up with some helper functions that make it easier to develop user modules (happy to take a stab once this part is discussed)

If opt 1 was chosen..then maybe either some documentation around the Module struct type or evaluating if some fields can be made private.

If 2 then I have a branch where I was pondering the same thing...something along the lines of...

type BuilderOpts = func(*Module)

func ModuleBuilder(opts ...BuilderOpts) *Module {
	m := NewModule()
	for _, v := range opts {
		//optionally have a return type with error
		v(m)
	}
	//add finilization
	return m
}
//or
type Builder struct {
	m *Module
}

func (b *Builder) WithFunc() {
	//add func to module
}
func (b *Builder) Build() *Module {
	//add finilization
	return b.m
}

I dont know that I have an opinion..nice thing about the first is with no ops it just makes a module..so could technically just modify the NewModule function...i've seen a lot of go libs doing that...so I think the first one might be more idiomatic...but I'm used to the second option personally. Your call.

[twitchyliquid64]

I think the 'builder' pattern makes the most sense for the use case where developers are dynamically building modules.

That said, I don't have a strong opinion here so feel free to lead this in a different direction if you do.

[codecov-io]

Codecov Report

Merging #164 into master will increase coverage by <.01%.
The diff coverage is 75%.

@@            Coverage Diff             @@
##           master     #164      +/-   ##
==========================================
+ Coverage   69.71%   69.71%   +<.01%     
==========================================
  Files          48       48              
  Lines        5484     5492       +8     
==========================================
+ Hits         3823     3829       +6     
- Misses       1317     1318       +1     
- Partials      344      345       +1

Impacted Files	Coverage Δ
wasm/imports.go	43.68% <75%> (+2.63%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 01d3d33...83180e4. Read the comment docs.

[twitchyliquid64]

My only concern is that the populate* methods append to the internal slices, so if they are called twice (as they would be because they are called in ReadModule and resolveImports in this PR), there will be duplicate entries.

Does it work if you zero out GlobalIndexSpace / FunctionIndexSpace etc etc, before you run the populate methods? That way we can avoid the duplicate problem.