Remove class wrap for constructors in Rust exports

[snOm3ad]

After #1594 constructors of Rust exported structs started using class wrapping when generating JS shims. Wrapping erases prototype information from the object instance in JS and as a result it is not possible to override methods (via inheritance) of the generated class.

Additionally, some checks to ensure constructors always return an instance of Self were lost.

This PR fixes the above two issues by embedding the kind of export into the CallExport instruction and monitoring for constructor exports calls during the export adapter creation.

Once a constructor export call is detected and validated a new instruction SelfFromI32 is pushed into the stack that avoids class wrapping.

Fixes #3213

[snOm3ad]

How useful is this check really? I mean all you have to do to bypass it is to wrap your foreign return type inside an Option or any other parameterized type and you've gone back to the old behaviour.

[Liamolucko]

It should reject Option<T> too - only the Rust struct that the constructor's for should be allowed.

[Liamolucko]

I had a bit of a different idea for how to implement this, which was to replace the generated return {} with this.__wbg_ptr = {} for constructors. return statements get inserted here, and you can already check whether the function's a constructor using self.constructor, so it doesn't require piping extra state around. I should probably have mentioned this when you asked for pointers; sorry about that.

Anyway, this implementation is also fine so you can stick with it if you want, but getting rid of the return entirely is a bit cleaner IMO.

[Liamolucko]

I don't think anyone cares about which internal crate caused the problem; you should probably just say wasm-bindgen or 'the wasm-bindgen CLI'.

[snOm3ad]

I wasn't sure if it was worth even including this. But yeah good point, I'll make these changes.

[Liamolucko]

I had a bit of a different idea for how to implement this, which was to replace the generated return {} with this.__wbg_ptr = {} for constructors. return statements get inserted here, and you can already check whether the function's a constructor using self.constructor, so it doesn't require piping extra state around.

Oops, that wouldn't actually work, that'd try to assign an object to this.__wbg_ptr. You could work around that by manually getting rid of the final RustFromI32 instruction if a constructor's being generated. (You could also check that it has the right return value at the same time: if the last instruction isn't a RustFromI32, it doesn't return a Rust struct, and then you can check that the contained class is correct.)

[snOm3ad]

I had a bit of a different idea for how to implement this, which was to replace the generated return {} with this.__wbg_ptr = {} for constructors. return statements get inserted here, and you can already check whether the function's a constructor using self.constructor, so it doesn't require piping extra state around. I should probably have mentioned this when you asked for pointers; sorry about that.

Anyway, this implementation is also fine so you can stick with it if you want, but getting rid of the return entirely is a bit cleaner IMO.

Hmm, I had not thought about this!

You could work around that by manually getting rid of the final RustFromI32 instruction if a constructor's being generated.

I was thinking of instead passing the constructor field as a parameter to the instruction function. Since it's not enough to get rid of RustFromI32 I'd also need to do the same for OptionRustFromI32. Something like this:

fn instruction(js: &mut JsBuilder, instr: &Instruction, log_error: &mut bool, constructor: &Option<String>) -> Result<(), Error> {
    match instr {
        Instruction::RustFromI32 { class } => {
            if let Some(name) = constructor {
                if name != class {
                    bail!("constructor for `{}` cannot return `{}`", class, name);
                }
                let val = js.pop();
                js.push(format!("{} >>> 0;", val));
            } else {
                js.cx.require_class_wrap(class);
                let val = js.pop();
                js.push(format!("{}.__wrap({})", class, val));
            }
        }
    }
}

What do you think? I'd also have to modify the return like you mentioned above. And, thank you for looking into this! It is indeed much cleaner. I'll probably make another PR based off from this one.