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

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

Closed
Urgau opened this issue Jan 3, 2024 · 11 comments
Closed

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

Urgau opened this issue Jan 3, 2024 · 11 comments
Labels
ACP-accepted API Change Proposal is accepted (seconded with no objections) api-change-proposal A proposal to add or alter unstable APIs in the standard libraries T-libs-api

Comments

@Urgau
Copy link
Member

Urgau commented Jan 3, 2024

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

  • Different name? Method instead of function?

Links and related work

rust-lang/rust#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 Urgau added api-change-proposal A proposal to add or alter unstable APIs in the standard libraries T-libs-api labels Jan 3, 2024
@joshtriplett
Copy link
Member

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
Copy link
Member

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

@cuviper
Copy link
Member

cuviper commented Jan 9, 2024

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
Copy link
Member

cuviper commented Jan 9, 2024

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
Copy link
Member

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
Copy link
Member

@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
Copy link
Member

tmandry commented Jan 10, 2024

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

Would transmuting function pointers work??

@traviscross
Copy link

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
Copy link
Member

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

CAD97 commented Feb 8, 2024

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
Copy link
Member

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 joshtriplett added the ACP-accepted API Change Proposal is accepted (seconded with no objections) label Aug 20, 2024
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Aug 24, 2024
…acrum

Implement `ptr::fn_addr_eq`

This PR implements rust-lang/libs-team#323: `ptr::fn_addr_eq`.

r? libs
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Aug 24, 2024
…acrum

Implement `ptr::fn_addr_eq`

This PR implements rust-lang/libs-team#323: `ptr::fn_addr_eq`.

r? libs
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Aug 25, 2024
Rollup merge of rust-lang#129323 - Urgau:ptr_fn_addr_eq, r=Mark-Simulacrum

Implement `ptr::fn_addr_eq`

This PR implements rust-lang/libs-team#323: `ptr::fn_addr_eq`.

r? libs
github-actions bot pushed a commit to rust-lang/miri that referenced this issue Aug 26, 2024
Implement `ptr::fn_addr_eq`

This PR implements rust-lang/libs-team#323: `ptr::fn_addr_eq`.

r? libs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ACP-accepted API Change Proposal is accepted (seconded with no objections) api-change-proposal A proposal to add or alter unstable APIs in the standard libraries T-libs-api
Projects
None yet
Development

No branches or pull requests

7 participants