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

Remove type_check from Property trait #74

Merged
merged 1 commit into from
Mar 2, 2024

Conversation

RCasatta
Copy link
Collaborator

I was looking to port rust-bitcoin/rust-miniscript#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.

@RCasatta RCasatta changed the title Remove type_check from Property trait Remove type_check from Property trait Feb 29, 2024
@apoelstra
Copy link
Member

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

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.
@RCasatta
Copy link
Collaborator Author

RCasatta commented Feb 29, 2024

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
Copy link
Collaborator Author

RCasatta commented Mar 1, 2024

Ci is fixed, does this need something?

Copy link
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 500b4a5

@apoelstra apoelstra merged commit 32fee57 into ElementsProject:master Mar 2, 2024
17 checks passed
apoelstra added a commit to rust-bitcoin/rust-miniscript that referenced this pull request Mar 4, 2024
5f41cb6 change FnMut -> Fn (Riccardo Casatta)
cdcd53e Refactor out type_check (Riccardo Casatta)

Pull request description:

  This apply the same logic of ElementsProject/elements-miniscript#74
  removing a couple of unreachable, simplify things and very likely reduce binary bloat.

  ~~However, in doing so, I don't think this MR is equivalent to master for the logic used for `get_child`.~~

  ~~I suspect current master is broken and this fix it (or viceversa?), but I have to suspend this work for now.~~

ACKs for top commit:
  apoelstra:
    ACK 5f41cb6 this is great, thanks!

Tree-SHA512: 4b904fde55ba75895d377babde5e7a3a215cdf3b0ccff0c90572bb286de9d8a80234fc6a4921ad83bcb8c947affa1e3c23b2135534cb4363f38f1bab89368ba4
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

Successfully merging this pull request may close these issues.

2 participants