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

remove weird contravariant #134237

Closed
wants to merge 1 commit into from

Conversation

oriongonza
Copy link
Contributor

@oriongonza oriongonza commented Dec 12, 2024

I'm not sure what purpose this PhantomData<fn(&I)> has, it looks like it wants a contravariant for some reason but... why? It's confusing, does it have any purpose? If it does not let's just remove it.

@rustbot
Copy link
Collaborator

rustbot commented Dec 12, 2024

r? @jieyouxu

rustbot has assigned @jieyouxu.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Dec 12, 2024
@oriongonza oriongonza changed the title remove weird countervariant remove weird contravariant Dec 12, 2024
Copy link
Member

@compiler-errors compiler-errors left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you actually try looking at the PR that introduced it? You tried to change this a year ago with no justification, and there's still no justification other than you not understanding why it's there: 1bf3aee#r1447618595

I'm fairly certain that changing PhantomData<fn(&I)> to PhantomData<I> changes the implications of the type w.r.t. drop, and obviously changes the variance of the I parameter.

I don't believe that this should be changed for literally no reason.

@compiler-errors compiler-errors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 12, 2024
@oriongonza
Copy link
Contributor Author

oriongonza commented Dec 12, 2024

Did you actually try looking at the PR that introduced it?

do you mean this one? #34149 I don't see anywhere explained why they went with that.

You tried to change this a year ago with no justification, and there's still no justification other than you not understanding why it's there
I'm fairly certain that changing PhantomData<fn(&I)> to PhantomData<I> changes the implications of the type w.r.t. drop

I did not understand it then, I don't understand it now that I know more and it's been somewhere in the back of my mind since.
Yes, that subtlety implies that it wants a difference in those aspects, what or why is unknown to me, or what invariant about the type it's asserting. Its existance implies that it must exist for some reason.
At least it should be documented since it's kinda weird.

@fmease
Copy link
Member

fmease commented Dec 12, 2024

It's a pretty common pattern to make map-like type constructors contravariant in the key type parameter. Apart from being conventional (across all languages that feature contravariance), it can be convenient at times (in Rust, it probably tends towards the rarer side in practice).

The "academic" answer: Maps of type HashMap<K, V>, BTreeMap<K, V>, IndexVec<K, V>, etc. are conceptually functions of type fn(K) -> V — functions mapping keys to values (imagine |k| match k { k0 => v0, k1 => v1, …, kn => vn }). Since function type constructors are contravariant in their argument types (arguments are consumed, not produced), it only makes sense that other map-likes follow suit.

In more "practical" terms, it permits you to for example treat IndexVec<&'a str, V> for all lifetimes 'a as IndexVec<&'static str, V>. You can "lifetime-extend" the key type (which makes sense intuitively when you think about the fact that the key is used for lookup / as an input). I couldn't come up with a more concrete use-case for this, sorry. It's more of a "when you need/want it, you need/have it" situation. So, it leads to a nicer more ergonomic API.

@fmease
Copy link
Member

fmease commented Dec 12, 2024

Regarding PhantomData<fn(I)> vs PhantomData<fn(&I)> the answer is indeed dropck. However, I'm not actually sure if it still has an effect. CC https://doc.rust-lang.org/nomicon/phantom-data.html#generic-parameters-and-drop-checking

Also, I requires Idx requires Copy, so it's probably moot in any case.

@jieyouxu
Copy link
Member

idk so i guess r? compiler-errors

@rustbot rustbot assigned compiler-errors and unassigned jieyouxu Dec 13, 2024
@oriongonza
Copy link
Contributor Author

I suggest the following:

  • If this is actually needed for something let's add a comment explaining it
  • If not let's simplify it.

@alex-semenyuk alex-semenyuk added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 22, 2025
@BoxyUwU
Copy link
Member

BoxyUwU commented Jan 23, 2025

I'm going to close this, it's been explained why preferring PhantomData<fn(&I)> over PhantomData<I> makes sense. If you would like to document this rationale then feel free to update this PR otherwise I don't think there's any value keeping this open as I do not believe anyone in this thread as expressed a desire to merge this as-is

@BoxyUwU BoxyUwU closed this Jan 23, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants