stacks-network / clarity-wasm

`clar2wasm` is a compiler for generating WebAssembly from Clarity.
GNU General Public License v3.0
12 stars 10 forks source link

Check if `typechecker_workaround` works for nested `filter`/`fold` expressions #492

Open matteojug opened 2 weeks ago

matteojug commented 2 weeks ago

We may need to fix typechecker_workaround as currently it only operates on top level expressions, thus ignoring nested ones: (example from map issue, solved by using local workaround instead of typechecker_workaround)

#[test]
fn crosscheck_map_ok_buff(buf in buffer(50)) {
    let snippet = format!(r#"
    (define-private (foo (a (response (buff 50) int))) (len (unwrap! a u0)))
    (begin (map foo (list (ok {buf})))) ;; added a wrapping `begin` here to an otherwise passing test
    "#);

    crosscheck(
        &snippet,
        Ok(Some(Value::cons_list_unsanitized(vec![Value::UInt(50)]).unwrap()))
    )
}
Err(Wasm(UnableToLoadModule(WebAssembly translation error

Caused by:
    Invalid input WebAssembly code at offset 7800: type mismatch: expected i64, found i32

This may be impacting also filter/fold.

_Originally posted by @matteojug in https://github.com/stacks-network/clarity-wasm/pull/474#discussion_r1727632230_

Acaccia commented 2 weeks ago

It does impact filter and fold. I created in no time this failing example https://github.com/stacks-network/clarity-wasm/pull/466#pullrequestreview-2254214629

I had this idea to have a Set containing the expressions fixed in typechecker_workaround, and to check if an expression is not in this set when we use WasmGenerator::set_expr_type (poorly explained here). However, I'm not sure this idea would work, or it might need some reworking in all functions that accept a list (and new unit tests for each of them) to make sure that the set_expr_type happen in the right place to have the correct type.

matteojug commented 2 weeks ago

One thing that we may need to double check: is it possible for the same expression (eg, loading a data-var and manipulating it) to be coerced, in different places, to two different types based on the function used (eg, in map)? If so, is it bad?

Since those workarounds basically interpret the list type based on the called function, I was wondering if it could be possible for this to introduce subtle bugs where you define a mistyped function, but this type coercion makes so it's still accepted (eventually reading garbage), but then it's possible to manipulate the original values in the list so that the garbage is no longer garbage but something bad.

It could help to have a clear characterization of the cases that required those workaround in the first place.

Acaccia commented 2 weeks ago

As much as I know from the testing I did, data-vars seems okay, because you have to set the type at declaration. But for constants, that is something I was wondering too. I never wrote a test to verify this assumption though.