rust-lang / libs-team

The home of the library team
Apache License 2.0
123 stars 19 forks source link

Add `ptr::fn_addr_eq` to compare functions pointers. #323

Closed Urgau closed 1 month ago

Urgau commented 9 months ago

Proposal

Problem statement

As discussed in the 2024-01-03 T-lang triage meeting, equality for function pointers is awkward (due to "equal" functions being merged together or duplicated across different CGUs).

Motivating examples or use cases

fn a() {}
fn b() {}

fn main() {
    assert_ne!(a as fn(), b as fn());
    // ^- Depending a optimizations this may (and does) not always hold.
}

Solution sketch

In core::ptr and std::ptr:

/// Compares the *addresses* of the two function pointers for equality.
///
/// Function pointers comparisons can have surprising results since
/// they are never guaranteed to be unique and could vary between different
/// code generation units. Furthermore, different functions could have the
/// same address after being merged together.
///
/// This is the same as `f == g` but using this function makes clear
/// that you are aware of these potentially surprising semantics.
///
/// # Examples
///
/// ```
/// fn a() { println!("a"); }
/// fn b() { println!("b"); }
/// assert!(!std::ptr::fn_addr_eq(a as fn(), b as fn()));
/// ```
#[allow(unpredictable_function_pointer_comparisons)]
pub fn fn_addr_eq<T: FnPtr, U: FnPtr>(f: T, g: U) -> bool {
    f.addr() == g.addr()
}

Alternatives

Links and related work

https://github.com/rust-lang/rust/pull/118833 a lint I'm trying to add to warn against functions pointers and T-lang decided that they want to recommend something that won't warn, similar to ptr::addr_eq for thin pointer comparisons.

What happens now?

This issue contains an API change proposal (or ACP) and is part of the libs-api team feature lifecycle. Once this issue is filed, the libs-api team will review open proposals as capability becomes available. Current response times do not have a clear estimate, but may be up to several months.

Possible responses

The libs team may respond in various different ways. First, the team will consider the problem (this doesn't require any concrete solution or alternatives to have been proposed):

Second, if there's a concrete solution:

joshtriplett commented 9 months ago

Suggestion that came up in the @rust-lang/libs-api meeting: should this accept two pointers of the same FnPtr type, or two different FnPtr types?

pub fn fn_addr_eq<T: FnPtr, U: FnPtr>(f: T, g: U) -> bool

programmerjake commented 9 months ago

oh, maybe also accept function item and 0-capture lambda types and use the corresponding fn ptr

cuviper commented 9 months ago

maybe also accept function item and 0-capture lambda types

Do we have any way to express that kind of type constraint? I think it would have to be a compiler builtin, either extending FnPtr or adding something similar. Right now those only become fn pointers by coercion.

cuviper commented 9 months ago

should this accept two pointers of the same FnPtr type, or two different FnPtr types?

The current PartialEq for F: FnPtr is not that generic -- but it could be! (ditto PartialOrd)

programmerjake commented 9 months ago

maybe also accept function item and 0-capture lambda types

Do we have any way to express that kind of type constraint? I think it would have to be a compiler builtin, either extending FnPtr or adding something similar. Right now those only become fn pointers by coercion.

add a AsFnPtr trait?

joshtriplett commented 9 months ago

@cuviper wrote:

The current PartialEq for F: FnPtr is not that generic -- but it could be! (ditto PartialOrd)

It wouldn't come up when translating f == g to fn_addr_eq(f, g), but I don't see an obvious reason fn_addr_eq couldn't allow it. The question is whether it should.

tmandry commented 9 months ago

Accepting any two function pointer types without guard rails sounds error prone to me.

Would transmuting function pointers work??

traviscross commented 9 months ago

For ptr::addr_eq we use separate generic parameters. The stated purpose of the function is comparing addresses, so it may be a defense of doing similarly here.

/// Compares the *addresses* of the two pointers for equality,
/// ignoring any metadata in fat pointers.
///
/// If the arguments are thin pointers of the same type,
/// then this is the same as [`eq`].
pub fn addr_eq<T: ?Sized, U: ?Sized>(p: *const T, q: *const U) -> bool { .. }
joshtriplett commented 8 months ago

This produced a fair bit of discussion in today's @rust-lang/lang meeting, and we didn't come to any conclusions about whether we'd recommend allowing different types or the same type in fn_addr_eq. The only case that would ever show up when translating from f == g would be functions of the same type.

CAD97 commented 8 months ago

From the precedent of ptr::eq and ptr::addr_eq, I'd expect a "ptr::fn_eq" to have one generic type and a "ptr::fn_addr_eq" to have two. Even if ptr::fn_eq's implementation is just an address comparison, it still serves as a documentation point to call out the potential pitfalls with comparing function pointers.

joshtriplett commented 1 month ago

We discussed this again in today's @rust-lang/libs-api meeting.

We definitely agreed that there are enough use cases for comparing two different types of function pointers.

Thus, we'd like to accept this with the following signature:

pub fn fn_addr_eq<T: FnPtr, U: FnPtr>(f: T, g: U) -> bool