jco icon indicating copy to clipboard operation
jco copied to clipboard

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

Open nuke-web3 opened this issue 1 year ago • 4 comments

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 :sweat_smile:

nuke-web3 avatar Mar 12 '24 21:03 nuke-web3

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.

guybedford avatar Mar 12 '24 23:03 guybedford

// 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?

nuke-web3 avatar Mar 13 '24 18:03 nuke-web3

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

guybedford avatar Mar 14 '24 15:03 guybedford

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.

jocrau avatar Oct 06 '24 20:10 jocrau