GenericDetour doesn't work with unsafe fns

[notpeelz]

On Rust stable (1.69.0), GenericDetour fails to compile for unsafe fn detours.

It compiles if you either: a) remove unsafe from the signatures; or b) re-enable the nigthly feature and switch to a nightly toolchain

fn main() {
  use retour::GenericDetour;
  unsafe extern "system" fn add5(val: i32) -> i32 {
    val + 5
  }

  unsafe extern "system" fn add10(val: i32) -> i32 {
    val + 10
  }

  let mut hook = unsafe {
    GenericDetour::<unsafe extern "system" fn(i32) -> i32>::new(add5, add10)
  }
  .unwrap();
}

error[E0277]: the trait bound `unsafe fn(i32) -> i32: retour::HookableWith<unsafe fn(i32) -> i32 {main::add10}>` is not satisfied
  --> detourtest\src\lib.rs:33:64
   |
33 |     unsafe { GenericDetour::<unsafe fn(i32) -> i32>::new(add5, add10) }
   |              -------------------------------------------       ^^^^^ the trait `retour::HookableWith<unsafe fn(i32) -> i32 {main::add10}>` is not implemented for `unsafe fn(i32) -> i32`
   |              |
   |              required by a bound introduced by this call
   |
   = help: the following other types implement trait `retour::HookableWith<D>`:
             <unsafe extern "C" fn() -> Ret as retour::HookableWith<extern "C" fn() -> Ret>>
             <unsafe extern "C" fn(A) -> Ret as retour::HookableWith<extern "C" fn(A) -> Ret>>
             <unsafe extern "C" fn(A, B) -> Ret as retour::HookableWith<extern "C" fn(A, B) -> Ret>>
             <unsafe extern "C" fn(A, B, C) -> Ret as retour::HookableWith<extern "C" fn(A, B, C) -> Ret>>
             <unsafe extern "C" fn(A, B, C, D) -> Ret as retour::HookableWith<extern "C" fn(A, B, C, D) -> Ret>>
             <unsafe extern "C" fn(A, B, C, D, E) -> Ret as retour::HookableWith<extern "C" fn(A, B, C, D, E) -> Ret>>
             <unsafe extern "C" fn(A, B, C, D, E, F) -> Ret as retour::HookableWith<extern "C" fn(A, B, C, D, E, F) -> Ret>>
             <unsafe extern "C" fn(A, B, C, D, E, F, G) -> Ret as retour::HookableWith<extern "C" fn(A, B, C, D, E, F, G) -> Ret>>
           and 97 others
note: required by a bound in `retour::GenericDetour::<T>::new`
  --> C:\Users\peelz\.cargo\registry\src\github.com-1ecc6299db9ec823\retour-0.1.0\src\detours\generic.rs:59:8
   |
59 |     T: HookableWith<D>,
   |        ^^^^^^^^^^^^^^^ required by this bound in `GenericDetour::<T>::new`

[notpeelz]

I can get it to compile (on 1.69.0 and nightly) by commenting out this line: https://github.com/Hpmason/retour-rs/blob/9624176683c36840016e22c5e870363c03903a2d/src/macros.rs#L204

That part of the macro expands to something like this:

unsafe impl<Ret: 'static, A: 'static> HookableWith<extern "system" fn(A) -> Ret>
  for unsafe extern "system" fn(A) -> Ret
{
}

Somehow it's preventing the compiler from seeing the blanket implementation: https://github.com/Hpmason/retour-rs/blob/9624176683c36840016e22c5e870363c03903a2d/src/traits.rs#L25

[notpeelz]

After doing some more research, I realized why this trait is needed in the first place. I thought it would be possible to simplify the signature of GenericDetour::new like this:

pub unsafe fn new<F: Function>(target: F, detour: F) -> Result<Self>

...but that fails to compile because rustc sees different function pointers (with identical signatures) as distinct types:

unsafe fn a() {}
unsafe fn b() {}
GenericDetour::<unsafe fn()>::new(a, b)

|       let hook = GenericDetour::new(a, b).expect(
|                  ------------------    ^ expected fn item, found a different fn item
|                  |
|                  arguments to this function are incorrect

This change was introduced in this PR: https://github.com/rust-lang/rust/pull/19891

It's possible to work around it like this (not particularly pretty, but could be easily improved with a macro):

unsafe fn a() {}
unsafe fn b() {}
GenericDetour::<unsafe fn()>::new(a as unsafe fn(), b)

[Hpmason]

Thanks for doing so much research into this. I would like to improve some of the ergonomics of GenericDetour and the stable parts of this crate. I saw your PR (#15), but there are some parts that need to be ironed out before. This expanded impl:

unsafe impl<Ret: 'static, A: 'static> HookableWith<extern "system" fn(A) -> Ret>
  for unsafe extern "system" fn(A) -> Ret
{ /* ... */}

actually serves a specific purpose. It allows you to detour an unsafe function with a safe function, such as a closure. Rust doesn't support unsafe closures. Could be useful for use of Fn trait generics, since unsafe functions don't implement Fn traits. Not sure if anyone makes use of that though, since a majority of users seem to use StaticDetour anyways.

I'm not 100% sure why the extra impl messes with the blanket impl. Maybe Rust can't choose the how to coerce add10 since there's 2 impls for extern "system" fn(i32) -> i32, HookableWith<extern "system" fn(i32) -> i32> from and HookableWith<unsafe extern "system" fn(i32) -> i32>. Though, the error message doesn't seem to suggest that.

[notpeelz]

actually serves a specific purpose. It allows you to detour an unsafe function with a safe function, such as a closure.

I thought the same thing, but turns out that still works even without the trait impl.

Here's a minimal reproduction of the issue: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=8c469a109720b1fb7a270374986fa358

You'll notice all 3 blocks compile just fine until you uncomment the trait impl.

[Hpmason]

Interesting that it works, but sounds good. I'll merge your PR when I get home tonight and I'll bump the crate to 0.2.0 because it does seem to break some syntax.

[notpeelz]

because it does seem to break some syntax

I didn't notice anything like that. Do you have an example?

[Hpmason]

Sorry, I meant to put it into my earlier comment. This syntax only seems to work with the macro impl:

unsafe fn add5(val: i32) -> i32 {
  val + 5
}

GenericDetour::<unsafe fn(i32) -> i32>::new::<fn(i32) -> i32>(add5, |val: i32| val + 10)

playground link

It's a more verbose way to solve the problem. Your PR reduces some of the verbosity, coercing the second parameter.

[Hpmason]

I'll wait until this discussion is done before I push 0.2.0, just so there's not any additional changes.

[notpeelz]

This syntax only seems to work with the macro impl

Oh I see. That's interesting.

Personally I think a patch bump would suffice (0.1.1) because 0.x crates aren't required to be fully backward-compatible. I'll leave it to your discretion.

[Hpmason]

From my understanding, 0.1.z and 0.2.z don't have to be backwards compatible, but 0.1.0 and 0.1.1 do. The distinction between 0.y and 1.y is that once a crate's version hits 1.y, all 1.y versions need to be backwards compatible. So no breaking changes, unless you bump to 2.y.

I'll go ahead and publish your changes to crates.io under 0.2.0 and close this issue. If there's anything more you want to discuss about the issue, don't be afraid to reopen it or create a new issue