libs-team icon indicating copy to clipboard operation
libs-team copied to clipboard

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

Open Urgau opened this issue 1 year ago • 10 comments

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>(f: T, g: T) -> bool {
    f.addr() == g.addr()
}

Alternatives

  • Different name? Method instead of function?

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):

  • We think this problem seems worth solving, and the standard library might be the right place to solve it.
  • We think that this probably doesn't belong in the standard library.

Second, if there's a concrete solution:

  • We think this specific solution looks roughly right, approved, you or someone else should implement this. (Further review will still happen on the subsequent implementation PR.)
  • We're not sure this is the right solution, and the alternatives or other materials don't give us enough information to be sure about that. Here are some questions we have that aren't answered, or rough ideas about alternatives we'd want to see discussed.

Urgau avatar Jan 03 '24 17:01 Urgau

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

joshtriplett avatar Jan 09 '24 17:01 joshtriplett

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

programmerjake avatar Jan 09 '24 17:01 programmerjake

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 avatar Jan 09 '24 18:01 cuviper

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)

cuviper avatar Jan 09 '24 18:01 cuviper

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?

programmerjake avatar Jan 09 '24 18:01 programmerjake

@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.

joshtriplett avatar Jan 09 '24 19:01 joshtriplett

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

Would transmuting function pointers work??

tmandry avatar Jan 10 '24 02:01 tmandry

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 { .. }

traviscross avatar Jan 10 '24 06:01 traviscross

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.

joshtriplett avatar Jan 10 '24 17:01 joshtriplett

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.

CAD97 avatar Feb 08 '24 22:02 CAD97

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

joshtriplett avatar Aug 20 '24 17:08 joshtriplett