Function pointer docs may need updating

Opened by Sean Allred at 2023-03-16 16:30:19

We may need documentation updated for function pointers when used in enums. I won't pretend I understand half of what's written here, but here's the log from IRC (#rust_beginners) where we debugged this issue:

Sharlin    :: vermiculus: okay... wtf [1]

vermiculus :: Sharlin: Right!?

Sharlin    :: what makes fns with ref arguments not implement Ord or
              anything

vermiculus :: I 100% need the argument to be mutable, but I can't have it be
              mutable without being a ref, and apparently it's choking on the ref

spear2     :: maybe it has to do with the reference having an implicit lifetime?

vermiculus :: ...Ok the '100%' is not completely true, but it's the cleanest option

Sharlin    :: spear2: indeed it seems to be

spear2     :: [2]

vermiculus :: spear2: now there's a topic I still don't understand half or less

Sharlin    :: this is definitely something that should be documented in the fn docs :D

vermiculus feels useful :D

spear2     :: vermiculus: the way i understand it, every reference has an associated
              lifetime, but most of the time it can be 'inferred' and you don't see it

Sharlin    :: so this is about that higher ranked trait bound thingie

vermiculus :: spear2: that's the extent of my understanding as well; I get tripped up
              trying to 'infer' the lifetime myself (effectively seeing what the
              compiler would see)

Sharlin    :: `Bar(fn(&i32))` is actually `Bar(for<'a> fn(&'a i32))`

Sharlin    :: which means the fn is not an ordinary function pointer but a higher
              something

vermiculus :: spooky

vermiculus :: that makes me wonder if this behavior is intentional or if there's a
              more explicit means to express this idea

vermiculus :: throwing that explicit lifetime parameter is making the rest of the
              compiler go a little nutty -- wanting lifetime parameters for *everything* :(

Sharlin    :: yeah

Sharlin    :: so it goes

This probably refers to:

Function pointers implement the following traits:

Clone PartialEq Eq PartialOrd Ord Hash Pointer Debug

Due to a temporary restriction in Rust's type system, these traits are only implemented on functions that take 12 arguments or less, with the "Rust" and "C" ABIs. In the future, this may change.

Which is not true as proved by the more simple example below:
```
fn assert_partialeq<T: PartialEq<T>>() {}

fn main() {
    assert_partialeq::<fn(&i32)>();
}
```
Rewording the last paragraph so it directs user to see the list of implementations on the relevant traits’ pages should suffice as a fix.
Simonas Kazlauskas at 2017-12-24 22:22:13
Wait... So it's just that eg. impl<A> Eq for fn(A) doesn't actually implement anything for any fn(A) where A is a reference type? You have to have a separate impl<A> Eq for fn(&A)? And another for &mut A? Which obviously would cause a combinatorial explosion in the current language...

Rewording the last paragraph so it directs user to see the list of implementations on the relevant traits’ pages should suffice as a fix.

I find the lists... less than useful. It's impossible to find anything there, really, and if you do, it doesn't help you understand why Eq is not implemented for my reference-taking function.
Johannes Dahlström at 2017-12-24 23:09:06
It's impossible to find anything there, really, and if you do, it doesn't help you understand why Eq is not implemented for my reference-taking function.

Agreed; as a newcomer, it's not obvious why this is the case. (read: I still don't know why this is case…)

If it's possible to write such a impl<A> Eq for fn(&A), that would also be useful information to include.
Sean Allred at 2017-12-25 00:05:16
cc @nikomatsakis
Simonas Kazlauskas at 2017-12-25 20:58:19
Triage: something changed here, but I don't know what. @nagisa's example compiles on beta.
jethrogb at 2019-02-04 10:13:48
Related https://github.com/rust-lang/rust/issues/45048
jethrogb at 2019-02-04 10:15:45
So this now needs a test. https://github.com/rust-lang/rust/pull/55986 looks somewhat related, @eddyb pls confirm?
Simonas Kazlauskas at 2019-02-04 12:45:21
I doubt #55986 is related. @rust-lang/wg-traits might know which PR it was.
Eduard-Mihai Burtescu at 2019-02-04 13:11:26
@nikomatsakis said that somebody has been flxing a problem with Clone impls for higher ranked functions. That fix would most likely also apply to the other traits. I didn’t quite catch who it was and I’m not finding the PR in question either.
Simonas Kazlauskas at 2019-02-04 13:15:41
Through bisection I found it started working between ec194646f (nightly-2019-01-03) and c0bbc3927 (nightly-2019-01-04)
jethrogb at 2019-02-04 13:36:05
So #57282, #56507, or #55517
jethrogb at 2019-02-04 13:37:17
Only #55517 seems plausible, IMO.
Eduard-Mihai Burtescu at 2019-02-04 14:00:02
Confirmed with rustup-toolchain-install-master
jethrogb at 2019-02-04 14:01:13
I tried to test something like fn x<'a, 'b: 'a>(_a: &'a mut i32, _b: &'b mut i32) {} but I'm not sure how to write the type for a pointer to that function. I get “lifetime bounds cannot be used in this context” or “one type is more general than the other”.
jethrogb at 2019-02-04 14:21:42
I tried reading PR #55517 but I have no clue what it does. Can someone (@eddyb @nikomatsakis @scalexm ?) elaborate and also say whether the resulting change to the behavior observed here was intentional? We have about 3 weeks to revert the behavior from beta before it becomes stable.
jethrogb at 2019-02-06 05:59:36
ping @eddyb @nikomatsakis @scalexm
jethrogb at 2019-02-12 06:13:04
So --

This was not fallout I anticipted, but I am not sure if it is wrong. I'm going to try and either investigate myself or (more likely) get somebody else to do so.

I am contemplating rolling back the universes PR, although that might be quite difficult. Not because I don't approve of the approach but because I want to be able to re-examine some of the fallout with time pressure.

(I guess we wouldn't want to roll back so much as to bring back the old system.)
Niko Matsakis at 2019-02-13 14:19:24
"Thing that was broken but we expected to work is now working" isn't something I'd necessarily call fallout :smile:
Sage Griffin at 2019-02-13 14:24:34
I'm nominating this to prioritize/assign
Niko Matsakis at 2019-02-13 16:55:22
FYI: I'm planning to start digging into this issue this week. Going to self-assign for the time being.
Aaron Turon at 2019-02-13 19:47:27
@sgrif It's possible this could be unsound (or rather, that's what I'm worried about).

Consider an impl like this:
```
impl<T, U> Foo for fn(T) -> U {
    type Assoc = (T, U);
    fn foo(self) {
        let x: T;
        let y: U;
        // ...
    }
}
```
If that impl matches for<'a> fn(&'a X) -> &'a Y, then the types for T and U refer to 'a, but T and U are used outside of the fn type, but in the associated type, and even worse, in the method body, which impl search can't take into account.

I hope Chalk can make this work to some extent, but I'd expect an RFC or at least a small discussion on how this would actually work, before it happened.
Eduard-Mihai Burtescu at 2019-02-13 22:44:18
Seems related, the following example now doesn't compile in beta (playground). Beta now considers the impls overlapping when they weren't before:
```
trait Foo {}
impl<Ret, A> Foo for fn(A) -> Ret {}
impl<Ret, A> Foo for for<'a> fn(&'a A) -> Ret {}
```
This was discussed back in #44224 and brought up again in https://github.com/rust-lang/rust/issues/56105#issuecomment-451457706. So it seems someone else noticed breakage from this.

(EDIT: From the timing it seems clear that @SimonSapin noticed the breakage after the PR was merged, so I updated my wording to reflect this. Also, simplified the test case.)
Tyler Mandry at 2019-02-13 23:13:57
The new behavior doesn't seem consistent. @nagisa's example above works now, but the ones in the OP don't. Additionally, see this playground for an example where the compiler accepts the impl as applicable in one line, but doesn't in the next.
Tyler Mandry at 2019-02-13 23:42:28
The breakage in coherence was expected, though I realize that I meant to reach out to @SimonSapin ahead of time and discuss it, but I failed to do that. Indeed though part of the point of the PR was to shift the checking of "higher-ranked constraints" like this out from the "valid subtyping test" that coherence uses and into the "region constraints". However, there is a middle ground, we could still restore the older behavior by adding some form of "satisfiability" check into coherence, at least temporarily. But I'd like to understand more fully what's going on in this test first.
Niko Matsakis at 2019-02-14 17:39:22
I don't think this issue is actually fixed? #58592 just brings this back to the state the issue was in before #55517.
jethrogb at 2019-02-25 23:58:16
removing E-needstest since #58592 implies it is no longer implicitly working, at least for now...
Felix S Klock II at 2019-02-27 21:11:55
triage: P-high.

Reasoning: the fallout that came out of universes (where said fallout has been undone by the re-introduction of a leak-check) implies to me that this is P-high.

Removing nomination label since this is now prioritized and assigned (@aturon volunteered)
Felix S Klock II at 2019-02-28 12:26:45
note: blocks #59490
Felix S Klock II at 2019-04-04 13:19:07
triage: re-prioritizing as P-medium. The investigation of fallout described here still needs to happen, but its not sufficiently pressing to warrant P-high label.
Felix S Klock II at 2019-04-04 13:20:33
I just want to compare two fn pointers to see if they are equal. As a workaround, is it safe simply to transmute them both to usize? It seems to work...
jesskfullwood at 2019-07-23 08:16:21
@jesskfullwood You can just cast them safely, e.g.: f as usize.
Eduard-Mihai Burtescu at 2019-07-23 10:13:54
I believe this will be fixed by https://github.com/rust-lang/rust/pull/108080. See https://github.com/rust-lang/rust/pull/108080#issuecomment-1457830730
Alan Wu at 2023-03-16 16:30:19