ElementsProject / elements-miniscript

Creative Commons Zero v1.0 Universal
11 stars 14 forks source link

Remove `type_check` from `Property` trait #74

Closed RCasatta closed 6 months ago

RCasatta commented 6 months ago

I was looking to port https://github.com/rust-bitcoin/rust-miniscript/pull/584 here, but I realized we can probably do better (and maybe port this in rust-miniscript to simplify things)

The blanket implementation of type_check is only used on CompilerExtData while ExtData and Type override the impl and don't need the child parameter. Moreover, the fn isn't called as Property generic.

So the blanket implementation of type_check is moved as a impl in CompilerExtData and the overrides in ExtData and Type are moved as simple impl on the type, making it possible to remove the unused child parameter.

apoelstra commented 6 months ago

concept ACK based on your description. But the CI failures are real; the fuzztests need to be updated.

RCasatta commented 6 months ago

But the CI failures are real; the fuzztests need to be updated.

The issue was that I removed the return_none function because there was only one occurence and I thought a closure would be equivalent, but I didn't read the comment on the function

/// None-returning function to help type inference when we need a
/// closure that simply returns `None`
fn return_none<T>(_: usize) -> Option<T> {
    None
}

the error with the closure was:

error: reached the recursion limit while instantiating `compiler::CompilerExtData::type_check::<String, Segwitv0, {closure@compiler::CompilerExtData::type_check<..., ..., ..., ...>::{closure#0}::{closure#0}::{closure#0}}, ...>`

The compiler error is fixed but I still don't grasp what's going on :S

RCasatta commented 6 months ago

Ci is fixed, does this need something?