This caused a ton of conflicts in #3130.. given that this was just a nice to have refactor, that could have waited.

Oh, sorry, I actually thought this would help to tackle the Host related issues. If you want, I can take a look at merging it into your PR?

I've already made all the changes.
It does not change the issue I mentioned in that PR, we still need to find a way to pass the current memory index type to those finalize calls.

Would it be possible to pass it upon construction of the respective expression and keep it as a field, like in the builder?

That would be fine with me.. @kripken does that sound acceptable? I guess the total number of memory.size / memory.grow ops will never affect total memory usage :)

I would add it as an argument to the finalize calls and force everyone that explicitly calls finalize on those expressions to supply it. That way it just has to be a field passed in the Builder or ReFinalize or ReFinalizeNode constructors.

Given these are rare nodes, memory isn't an issue, yeah. So it could be either way. I think I agree with @tlively that passing it to finalize makes sense (avoids duplicating info that could get stale).

@tlively that's what we had so far, but it was problematic, since finalize is called from sites that don't have the current memory/module already available.

@aardappel can you share a link to such a site in your PR?

@tlively it's in one of the comments on the PR #3130:

Ok, changed the way Host::finalize works and is called to satisfy the tests, but it is very broken. What really needs to happen is for it to use the current pointer size, which depends on the current memory/module, which is not accessible from most of these functions calling finalize. We could stick an is64 bool in the visitors calling this function, e.g. BinaryenExpressionFinalize would need to take a module argument, changing the JS API etc.

But I've already changed it to work the way @dcodeIO suggested.