Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Transpile bindings for u32 map to number in *.d.ts #402

Open
nuke-web3 opened this issue Mar 12, 2024 · 4 comments
Open

Transpile bindings for u32 map to number in *.d.ts #402

nuke-web3 opened this issue Mar 12, 2024 · 4 comments

Comments

@nuke-web3
Copy link

nuke-web3 commented Mar 12, 2024

I noticed in making this little demo that I could not use signed or out-of-bounds numbers for wasmtime but that in the browser and via node it does.

https://stackoverflow.com/help/minimal-reproducible-example adding to the cli:

../wit/calculator.wit

interface calculate {
    enum op {
        add,
    }
    eval-expression: func(op: op, x: u32, y: u32) -> u32;
}

jco version 1.0.3 generated bindings/interfaces/docs-calculator-calculate.d.ts

export namespace DocsCalculatorCalculate {
  export function evalExpression(op: Op, x: number, y: number): number;
}

cli-calc.js:

// u32 is specified in wit, but typescript bindings specify only `number`
console.log("Unsigned overflow = " + calculate.evalExpression("add", 4294967300, 0));
console.log("Unsigned underflow = " + calculate.evalExpression("add", -4294967200, 0));
//                                   negative should fail types... 😬 ^^^^^
// $ node cli-calc.js
Answer (to life) = 42
Unsigned overflow = 4
Unsigned underflow = 96

Is there a way now to ensure that bindings generated use the WIT specified stronger types? I would think softening the interface to number should not be the default 😅

@guybedford
Copy link
Collaborator

guybedford commented Mar 12, 2024

U32 lowering does coercion currently - https://github.com/bytecodealliance/jco/blob/main/crates/js-component-bindgen/src/function_bindgen.rs#L260.

We could possibly move to validation with traps for all user-provided values.

@nuke-web3
Copy link
Author

// Use `>>>0` to ensure the bits of the number are treated as unsigned.
Instruction::U32FromI32 => results.push(format!("{} >>> 0", operands[0])),

Curious, IIUC the intent here is to ensure no negative numbers should be possible, yet in my example above,at runtime we happily underflow with a negative number passed in... Should that trap?

I would be a strong advocate for the behavior to try and do its best to fail at runtime if wit import / export type properties are violated (by default at least).

As it is now, devs need to make those assertions wrapping the bindings in JS, correct?

@guybedford
Copy link
Collaborator

Yes, currently the trap behaviour is inconsistent, some type errors trap and others coerce.

@jocrau
Copy link

jocrau commented Oct 6, 2024

This inconsistency lead to a somewhat unexpected behavior in my code. I totally agree with @nuke-web3 when they state "I would be a strong advocate for the behavior to try and do its best to fail at runtime if wit import / export type properties are violated (by default at least)." Shouldn't the behavior be at least as strict as the generated TypeScript types require it? Is there a plan to change this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants