Skip to content

Infinite type-level recursion in ForEachKey impl in Policy #541

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

Closed
apoelstra opened this issue Apr 21, 2023 · 4 comments · Fixed by #546
Closed

Infinite type-level recursion in ForEachKey impl in Policy #541

apoelstra opened this issue Apr 21, 2023 · 4 comments · Fixed by #546

Comments

@apoelstra
Copy link
Member

In this code we have a method for_each which takes a F: FnMut and calls itself with an &mut F. This causes the compiler to try to instantiate the method with F, &mut F, &mut &mut F, etc., forever. Throughout our codebase, we've avoided this infinite recursion by having an internal real_for_each method which directly takes a &mut F, so that it can call itself with the same type that it was called with. Then our public-facing for_each simply sticks a &mut on its argument and calls real_for_each.

In the code in question, though, we didn't do this, and for some reason the Rust compiler has accepted it, until tonight. The latest nightly, 2023-04-21, does not compile the code.

Unfortunately this code has been present for a long time (I think since 1.0 of this crate) so to fix it, we will need to backport fixes to ever released version. Fortunately, there are a couple potential fixes, both of which are pretty simple:

  • Add a real_for_each internal funciton like we do elsewhere
  • Change &mut pred to &mut |key| pred(key) in two places (thanks GPT4 for this one)

See rust-lang/rust#110656

@sanket1729
Copy link
Member

sanket1729 commented May 3, 2023

Interesting. I don't think we should back-port it to every release. How about just the releases in the last two years.

@apoelstra
Copy link
Member Author

That sounds good to me.

@sanket1729
Copy link
Member

Actually, @apoelstra the fact the downstream crates did not report any errors means that we should don't need any backports.

Using this function still results would have still compile error for all versions? I guess that now it results in compile error early on in this library?

@apoelstra
Copy link
Member Author

@sanket1729 downstream crates haven't reported any errors because only recent Rust nightlies expose errors. I worry that the next stable version will expose the error and then we'll have trouble.

gruve-p pushed a commit to Groestlcoin/rust-miniscript that referenced this issue Aug 28, 2023
f2f95fa cargo fmt (Andrew Poelstra)
a6340fe remove a bunch of unnecessary where clauses (Andrew Poelstra)
663bc00 concrete: fix infinite recursion in Policy (Andrew Poelstra)
520b9db semantic: fix todo and infinite recursion in Policy (Andrew Poelstra)

Pull request description:

  Our `ForEachKey` impls for the two policy types did not work. Both had an infinite type recursion which meant that if anybody had tried to use them they would not have compiled. (Though the fact that this has been happening for at least 2 years with zero bug reports suggests that nobody *has* tried to use them..)

  Recent Rust compiler nightlies have started failing compilation even when the offending impls are *not* used, which means we need to prioritize fixing this. Possibly we should even backport it.

  Fixes rust-bitcoin#541

ACKs for top commit:
  sanket1729:
    ACK f2f95fa

Tree-SHA512: a5d673929cd43187b157cd09941e9f922c605668085870d849fff84a404cb19dbc64694d1c173d4dcb5ff0d85dff450de1c6c6d4ceae038971dafca88ba40447
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 a pull request may close this issue.

3 participants
@apoelstra @sanket1729 and others