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

Function pointer docs may need updating #46989

Open
vermiculus opened this issue Dec 24, 2017 · 31 comments
Open

Function pointer docs may need updating #46989

vermiculus opened this issue Dec 24, 2017 · 31 comments

Comments

@vermiculus
Copy link

@vermiculus vermiculus commented Dec 24, 2017

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:

  1. Rust Playground (Sharlin)
  2. Rust Playground (spear2)
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
@nagisa

This comment has been minimized.

Copy link
Contributor

@nagisa nagisa commented Dec 24, 2017

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.

@jdahlstrom

This comment has been minimized.

Copy link

@jdahlstrom jdahlstrom commented Dec 24, 2017

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.

@vermiculus

This comment has been minimized.

Copy link
Author

@vermiculus vermiculus commented Dec 25, 2017

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.

@nagisa

This comment has been minimized.

Copy link
Contributor

@nagisa nagisa commented Dec 25, 2017

@jethrogb

This comment has been minimized.

Copy link
Contributor

@jethrogb jethrogb commented Feb 4, 2019

Triage: something changed here, but I don't know what. @nagisa's example compiles on beta.

@jethrogb

This comment has been minimized.

Copy link
Contributor

@jethrogb jethrogb commented Feb 4, 2019

Related #45048

@nagisa

This comment has been minimized.

Copy link
Contributor

@nagisa nagisa commented Feb 4, 2019

So this now needs a test. #55986 looks somewhat related, @eddyb pls confirm?

@eddyb

This comment has been minimized.

Copy link
Member

@eddyb eddyb commented Feb 4, 2019

I doubt #55986 is related. @rust-lang/wg-traits might know which PR it was.

@nagisa

This comment has been minimized.

Copy link
Contributor

@nagisa nagisa commented Feb 4, 2019

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

@jethrogb

This comment has been minimized.

Copy link
Contributor

@jethrogb jethrogb commented Feb 4, 2019

Through bisection I found it started working between ec19464 (nightly-2019-01-03) and c0bbc39 (nightly-2019-01-04)

@jethrogb

This comment has been minimized.

Copy link
Contributor

@jethrogb jethrogb commented Feb 4, 2019

So #57282, #56507, or #55517

@eddyb

This comment has been minimized.

Copy link
Member

@eddyb eddyb commented Feb 4, 2019

Only #55517 seems plausible, IMO.

@jethrogb

This comment has been minimized.

Copy link
Contributor

@jethrogb jethrogb commented Feb 4, 2019

Confirmed with rustup-toolchain-install-master

@jethrogb

This comment has been minimized.

Copy link
Contributor

@jethrogb jethrogb commented Feb 4, 2019

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

This comment has been minimized.

Copy link
Contributor

@jethrogb jethrogb commented Feb 6, 2019

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

This comment has been minimized.

Copy link
Contributor

@jethrogb jethrogb commented Feb 12, 2019

bors added a commit that referenced this issue Feb 20, 2019
[WIP] Re-implement leak check in terms of universes

This PR temporarily restores the leak-check, but implemented in terms of universes. This is not because the leak check behavior was necessarily **correct**, but because (a) we may want to have a transition period and because (b) we want to have more breathing room to work through the full implications of handling higher-ranked types correctly. Note that this PR builds atop #58056.

Fixes #58451
Fixes #46989
Fixes #57639

r? @aturon
cc @arielb1, @lqd

~~Temporary note: I've not finished running `./x.py test` locally -- I'm confident a lot of error messages in tests will need updating. I sort of expect them to revert to the older, (imo) less good error messages, which is mildly unfortunate. There might be a way to preserve the new error messages, not sure.~~
bors added a commit that referenced this issue Feb 21, 2019
Re-implement leak check in terms of universes

This PR temporarily restores the leak-check, but implemented in terms of universes. This is not because the leak check behavior was necessarily **correct**, but because (a) we may want to have a transition period and because (b) we want to have more breathing room to work through the full implications of handling higher-ranked types correctly. Note that this PR builds atop #58056.

Fixes #58451
Fixes #46989
Fixes #57639

r? @aturon
cc @arielb1, @lqd

~~Temporary note: I've not finished running `./x.py test` locally -- I'm confident a lot of error messages in tests will need updating. I sort of expect them to revert to the older, (imo) less good error messages, which is mildly unfortunate. There might be a way to preserve the new error messages, not sure.~~
bors added a commit that referenced this issue Feb 21, 2019
Re-implement leak check in terms of universes

This PR temporarily restores the leak-check, but implemented in terms of universes. This is not because the leak check behavior was necessarily **correct**, but because (a) we may want to have a transition period and because (b) we want to have more breathing room to work through the full implications of handling higher-ranked types correctly. Note that this PR builds atop #58056.

Fixes #58451
Fixes #46989
Fixes #57639

r? @aturon
cc @arielb1, @lqd

~~Temporary note: I've not finished running `./x.py test` locally -- I'm confident a lot of error messages in tests will need updating. I sort of expect them to revert to the older, (imo) less good error messages, which is mildly unfortunate. There might be a way to preserve the new error messages, not sure.~~
bors added a commit that referenced this issue Feb 21, 2019
Re-implement leak check in terms of universes

This PR temporarily restores the leak-check, but implemented in terms of universes. This is not because the leak check behavior was necessarily **correct**, but because (a) we may want to have a transition period and because (b) we want to have more breathing room to work through the full implications of handling higher-ranked types correctly. Note that this PR builds atop #58056.

Fixes #58451
Fixes #46989
Fixes #57639

r? @aturon
cc @arielb1, @lqd

~~Temporary note: I've not finished running `./x.py test` locally -- I'm confident a lot of error messages in tests will need updating. I sort of expect them to revert to the older, (imo) less good error messages, which is mildly unfortunate. There might be a way to preserve the new error messages, not sure.~~
bors added a commit that referenced this issue Feb 21, 2019
Re-implement leak check in terms of universes

This PR temporarily restores the leak-check, but implemented in terms of universes. This is not because the leak check behavior was necessarily **correct**, but because (a) we may want to have a transition period and because (b) we want to have more breathing room to work through the full implications of handling higher-ranked types correctly. Note that this PR builds atop #58056.

Fixes #58451
Fixes #46989
Fixes #57639

r? @aturon
cc @arielb1, @lqd

~~Temporary note: I've not finished running `./x.py test` locally -- I'm confident a lot of error messages in tests will need updating. I sort of expect them to revert to the older, (imo) less good error messages, which is mildly unfortunate. There might be a way to preserve the new error messages, not sure.~~
@bors bors closed this in #58592 Feb 22, 2019
@jethrogb

This comment has been minimized.

Copy link
Contributor

@jethrogb jethrogb commented Feb 25, 2019

I don't think this issue is actually fixed? #58592 just brings this back to the state the issue was in before #55517.

@varkor varkor reopened this Feb 25, 2019
@pnkfelix

This comment has been minimized.

Copy link
Member

@pnkfelix pnkfelix commented Feb 27, 2019

removing E-needstest since #58592 implies it is no longer implicitly working, at least for now...

@pnkfelix pnkfelix removed the E-needstest label Feb 27, 2019
@pnkfelix

This comment has been minimized.

Copy link
Member

@pnkfelix pnkfelix commented Feb 28, 2019

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)

@pnkfelix pnkfelix added P-high and removed I-nominated labels Feb 28, 2019
@pnkfelix

This comment has been minimized.

Copy link
Member

@pnkfelix pnkfelix commented Apr 4, 2019

note: blocks #59490

@pnkfelix

This comment has been minimized.

Copy link
Member

@pnkfelix pnkfelix commented Apr 4, 2019

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.

@jesskfullwood

This comment has been minimized.

Copy link

@jesskfullwood jesskfullwood commented Jul 23, 2019

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

@eddyb

This comment has been minimized.

Copy link
Member

@eddyb eddyb commented Jul 23, 2019

@jesskfullwood You can just cast them safely, e.g.: f as usize.

pnkfelix added a commit to pnkfelix/rust that referenced this issue Oct 17, 2019
(Or more precisely, a pair of such traits: one for `derive(PartialEq)` and one
for `derive(Eq)`.)

((The addition of the second marker trait, `StructuralEq`, is largely a hack to
work-around `fn (&T)` not implementing `PartialEq` and `Eq`; see also issue
rust-lang#46989; otherwise I would just check if `Eq` is implemented.))

Note: this does not use trait fulfillment error-reporting machinery; it just
uses the trait system to determine if the ADT was tagged or not. (Nonetheless, I
have kept an `on_unimplemented` message on the new trait for structural_match
check, even though it is currently not used.)

Note also: this does *not* resolve the ICE from rust-lang#65466, as noted
in a comment added in this commit. Further work is necessary to resolve that and
other problems with the structural match checking, especially to do so without
breaking stable code (adapted from test fn-ptr-is-structurally-matchable.rs):

```rust
fn r_sm_to(_: &SM) {}

fn main() {
    const CFN6: Wrap<fn(&SM)> = Wrap(r_sm_to);
    let input: Wrap<fn(&SM)> = Wrap(r_sm_to);
    match Wrap(input) {
        Wrap(CFN6) => {}
        Wrap(_) => {}
    };
}
```

where we would hit a problem with the strategy of unconditionally checking for
`PartialEq` because the type `for <'a> fn(&'a SM)` does not currently even
*implement* `PartialEq`.
pnkfelix added a commit to pnkfelix/rust that referenced this issue Oct 18, 2019
(Or more precisely, a pair of such traits: one for `derive(PartialEq)` and one
for `derive(Eq)`.)

((The addition of the second marker trait, `StructuralEq`, is largely a hack to
work-around `fn (&T)` not implementing `PartialEq` and `Eq`; see also issue
rust-lang#46989; otherwise I would just check if `Eq` is implemented.))

Note: this does not use trait fulfillment error-reporting machinery; it just
uses the trait system to determine if the ADT was tagged or not. (Nonetheless, I
have kept an `on_unimplemented` message on the new trait for structural_match
check, even though it is currently not used.)

Note also: this does *not* resolve the ICE from rust-lang#65466, as noted
in a comment added in this commit. Further work is necessary to resolve that and
other problems with the structural match checking, especially to do so without
breaking stable code (adapted from test fn-ptr-is-structurally-matchable.rs):

```rust
fn r_sm_to(_: &SM) {}

fn main() {
    const CFN6: Wrap<fn(&SM)> = Wrap(r_sm_to);
    let input: Wrap<fn(&SM)> = Wrap(r_sm_to);
    match Wrap(input) {
        Wrap(CFN6) => {}
        Wrap(_) => {}
    };
}
```

where we would hit a problem with the strategy of unconditionally checking for
`PartialEq` because the type `for <'a> fn(&'a SM)` does not currently even
*implement* `PartialEq`.
pnkfelix added a commit to pnkfelix/rust that referenced this issue Oct 21, 2019
(Or more precisely, a pair of such traits: one for `derive(PartialEq)` and one
for `derive(Eq)`.)

((The addition of the second marker trait, `StructuralEq`, is largely a hack to
work-around `fn (&T)` not implementing `PartialEq` and `Eq`; see also issue
rust-lang#46989; otherwise I would just check if `Eq` is implemented.))

Note: this does not use trait fulfillment error-reporting machinery; it just
uses the trait system to determine if the ADT was tagged or not. (Nonetheless, I
have kept an `on_unimplemented` message on the new trait for structural_match
check, even though it is currently not used.)

Note also: this does *not* resolve the ICE from rust-lang#65466, as noted
in a comment added in this commit. Further work is necessary to resolve that and
other problems with the structural match checking, especially to do so without
breaking stable code (adapted from test fn-ptr-is-structurally-matchable.rs):

```rust
fn r_sm_to(_: &SM) {}

fn main() {
    const CFN6: Wrap<fn(&SM)> = Wrap(r_sm_to);
    let input: Wrap<fn(&SM)> = Wrap(r_sm_to);
    match Wrap(input) {
        Wrap(CFN6) => {}
        Wrap(_) => {}
    };
}
```

where we would hit a problem with the strategy of unconditionally checking for
`PartialEq` because the type `for <'a> fn(&'a SM)` does not currently even
*implement* `PartialEq`.
pnkfelix added a commit to pnkfelix/rust that referenced this issue Oct 25, 2019
(Or more precisely, a pair of such traits: one for `derive(PartialEq)` and one
for `derive(Eq)`.)

((The addition of the second marker trait, `StructuralEq`, is largely a hack to
work-around `fn (&T)` not implementing `PartialEq` and `Eq`; see also issue
rust-lang#46989; otherwise I would just check if `Eq` is implemented.))

Note: this does not use trait fulfillment error-reporting machinery; it just
uses the trait system to determine if the ADT was tagged or not. (Nonetheless, I
have kept an `on_unimplemented` message on the new trait for structural_match
check, even though it is currently not used.)

Note also: this does *not* resolve the ICE from rust-lang#65466, as noted
in a comment added in this commit. Further work is necessary to resolve that and
other problems with the structural match checking, especially to do so without
breaking stable code (adapted from test fn-ptr-is-structurally-matchable.rs):

```rust
fn r_sm_to(_: &SM) {}

fn main() {
    const CFN6: Wrap<fn(&SM)> = Wrap(r_sm_to);
    let input: Wrap<fn(&SM)> = Wrap(r_sm_to);
    match Wrap(input) {
        Wrap(CFN6) => {}
        Wrap(_) => {}
    };
}
```

where we would hit a problem with the strategy of unconditionally checking for
`PartialEq` because the type `for <'a> fn(&'a SM)` does not currently even
*implement* `PartialEq`.

----

added review feedback:
* use an or-pattern
* eschew `return` when tail position will do.
* don't need fresh_expansion; just add `structural_match` to appropriate `allow_internal_unstable` attributes.

also fixed example in doc comment so that it actually compiles.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.