include note on variance and example by hkBst · Pull Request #136246 · rust-lang/rust

hkBst · 2025-01-29T14:18:34Z

rustbot · 2025-01-29T14:18:43Z

rustbot has assigned @cuviper.
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

uazu · 2025-01-29T15:38:15Z

The description makes good sense to me. Maybe mention that variance in Rust only applies to lifetimes (as I understand it).

I wonder whether the longer example is too specific and might not make sense without the motivation of the original code. We can see how it looks to the reviewer, coming at this fresh.

Certainly it illustrates the problem with a practical example, very similar to my particular problem case. However, really the essence of the problem is that the type-id of the generic type argument can be changed by the caller from one method call to another on the same instance of a struct. So the struct implementation can't count on that type-id being constant over the lifetime of the instance without further precautions (e.g. using the Invariant type). I guess the type-id is just one symptom of this behaviour by Rust, but you could say that whilst the Rust language treats this casting as not that significant, type-id treats it as very significant and gives a completely different ID. So type-id exposes the behaviour, which is why it would be helpful to document it here.

hkBst · 2025-01-29T16:04:59Z

The description makes good sense to me. Maybe mention that variance in Rust only applies to lifetimes (as I understand it).

AIUI it is parameter positions that can have variance. So for parameter position 1 in G<_> to be

covariant means sub < super implies, G<sub> < G<super>,
contravariant means sub < super implies, G<sub> > G<super> (less than sign gets flipped),
invariant means you that you cannot say anything about the subtyping relation between G<sub> and G<super> from the given information.

hkBst · 2025-01-29T16:10:28Z

I guess the type-id is just one symptom of this behaviour by Rust, but you could say that whilst the Rust language treats this casting as not that significant, type-id treats it as very significant and gives a completely different ID. So type-id exposes the behaviour, which is why it would be helpful to document it here.

This is an interesting perspective, and I will need to think about it some more.

danielhenrymantilla

Nice to see this pitfall documented! 🙏

I have taken the initiative to make a drive-by review; mainly:
- added an introductory section to get to the point before illustrating with examples;
- used some markdown headers and new lines to make it less densely packed;
- avoided the expression "type Sub can be used wherever Super can", since this does not hold true when both are wrapped in an invariant type, so I have amended it as "values of type Sub can be used wherever Super is expected". I've also deëmphasized the as casts part, since it doesn't seem specially important here, when "Just Passing™" the value suffices 🙂

hkBst · 2025-01-30T10:03:29Z

Nice to see this pitfall documented! 🙏

Very happy to do it.

I have taken the initiative to make a drive-by review; mainly:

Hurray for initiative, and thanks for an excellent review.

added an introductory section to get to the point before illustrating with examples;

I like it, but would like to hear what other people think, otherwise I'll just add it soon.

avoided the expression "type Sub can be used wherever Super can", since this does not hold true when both are wrapped in an invariant type, so I have amended it as "values of type Sub can be used wherever Super is expected". I've also deëmphasized the as casts part, since it doesn't seem specially important here, when "Just Passing™" the value suffices 🙂

My version was dead wrong, so thanks for catching this, and I also really like the silent casting without even using "as". It is very sneaky and really shows how easily you could almost accidentally mix up types.

hkBst · 2025-01-31T14:21:21Z

@danielhenrymantilla I've now incorporated all your suggestions with some slight changes/rewordings.

cuviper · 2025-01-31T18:23:19Z

r? libs

ibraheemdev

I like this. The example is a little long but I think that's fine. I left a couple of comments to minimize the example a little bit, but other than that this looks good.

ibraheemdev · 2025-02-11T02:54:32Z

+/// // `FnRef` is a subtype of `FnStaticRef`.
+/// // Both are 'static, and thus have a TypeId.
+/// type FnRef = fn(&());
+/// type FnStaticRef = fn(&'static ());
+///
+/// fn main() {
+///     type TheOneRing = FnStaticRef;
+///
+///     let the_one_ring: Unique<TheOneRing> = Unique::new().unwrap();
+///     assert_eq!(Unique::<TheOneRing>::new(), None);
+///
+///     type OtherRing = FnRef;
+///
+///     let other_ring: Unique<OtherRing> = Unique::new().unwrap();
+///     // Use that `Unique<OtherRing>` is a subtype of `Unique<TheOneRing>` 🚨
+///     let fake_one_ring: Unique<TheOneRing> = other_ring;
+///     assert_eq!(fake_one_ring, the_one_ring);
+///
+///     std::mem::forget(fake_one_ring);
+/// }


Suggested change

/// // `FnRef` is a subtype of `FnStaticRef`.

/// // Both are 'static, and thus have a TypeId.

/// type FnRef = fn(&());

/// type FnStaticRef = fn(&'static ());

///

/// fn main() {

/// type TheOneRing = FnStaticRef;

///

/// let the_one_ring: Unique<TheOneRing> = Unique::new().unwrap();

/// assert_eq!(Unique::<TheOneRing>::new(), None);

///

/// type OtherRing = FnRef;

///

/// let other_ring: Unique<OtherRing> = Unique::new().unwrap();

/// // Use that `Unique<OtherRing>` is a subtype of `Unique<TheOneRing>` 🚨

/// let fake_one_ring: Unique<TheOneRing> = other_ring;

/// assert_eq!(fake_one_ring, the_one_ring);

///

/// std::mem::forget(fake_one_ring);

/// }

/// // `FnRef` is a subtype of `FnStaticRef`. Both are 'static, and thus have a `TypeId`.

/// type FnRef = fn(&());

/// type FnStaticRef = fn(&'static ());

///

/// fn main() {

/// let unique_static_ref: Unique<FnStaticRef> = Unique::new().unwrap();

/// assert_eq!(Unique::<FnStaticRef>::new(), None);

///

/// let other_ref: Unique<FnRef> = Unique::new().unwrap();

/// // Use that `Unique<FnRef>` is a subtype of `Unique<FnStaticRef>` 🚨

/// let fake_unique_static_ref: Unique<FnStaticRef> = other_ref;

/// assert_eq!(fake_unique_static_ref, unique_static_ref);

///

/// std::mem::forget(fake_unique_static_ref);

/// }

I like the one ring analogy, but I feel it is cleaner to stick with the original type names (also trims the line count a bit).

ibraheemdev · 2025-02-11T02:58:21Z

+/// use std::any::TypeId;
+/// use std::collections::BTreeSet;
+/// use std::marker::PhantomData;
+/// use std::sync::Mutex;
+///
+/// use unique::Unique;
+///
+/// mod unique {
+///     use super::*;
+///
+///     static ID_SET: Mutex<BTreeSet<TypeId>> = Mutex::new(BTreeSet::new());
+///
+///     // Due to its private data member, outside this module,
+///     // this struct can only be created using `new`.
+///     #[derive(Debug, PartialEq)]
+///     pub struct Unique<TypeAsId: 'static>(
+///         // this usage of `TypeAsId` makes `Unique` covariant 🚨
+///         PhantomData<TypeAsId>,
+///     );
+///
+///     impl<TypeAsId: 'static> Unique<TypeAsId> {
+///         pub fn new() -> Option<Self> {
+///             let mut set = ID_SET.lock().unwrap();
+///             set.insert(TypeId::of::<TypeAsId>())
+///                 .then(|| Self(PhantomData))
+///         }
+///     }
+///
+///     impl<TypeAsId: 'static> Drop for Unique<TypeAsId> {
+///         fn drop(&mut self) {
+///             let mut set = ID_SET.lock().unwrap();
+///             (!set.remove(&TypeId::of::<TypeAsId>())).then(|| panic!("duplicity detected"));
+///         }
+///     }
+/// }


Suggested change

/// use std::any::TypeId;

/// use std::collections::BTreeSet;

/// use std::marker::PhantomData;

/// use std::sync::Mutex;

///

/// use unique::Unique;

///

/// mod unique {

/// use super::*;

///

/// static ID_SET: Mutex<BTreeSet<TypeId>> = Mutex::new(BTreeSet::new());

///

/// // Due to its private data member, outside this module,

/// // this struct can only be created using `new`.

/// #[derive(Debug, PartialEq)]

/// pub struct Unique<TypeAsId: 'static>(

/// // this usage of `TypeAsId` makes `Unique` covariant 🚨

/// PhantomData<TypeAsId>,

/// );

///

/// impl<TypeAsId: 'static> Unique<TypeAsId> {

/// pub fn new() -> Option<Self> {

/// let mut set = ID_SET.lock().unwrap();

/// set.insert(TypeId::of::<TypeAsId>())

/// .then(|| Self(PhantomData))

/// }

/// }

///

/// impl<TypeAsId: 'static> Drop for Unique<TypeAsId> {

/// fn drop(&mut self) {

/// let mut set = ID_SET.lock().unwrap();

/// (!set.remove(&TypeId::of::<TypeAsId>())).then(|| panic!("duplicity detected"));

/// }

/// }

/// }

/// mod unique {

/// use std::any::TypeId;

/// use std::collections::BTreeSet;

/// use std::marker::PhantomData;

/// use std::sync::Mutex;

///

/// static ID_SET: Mutex<BTreeSet<TypeId>> = Mutex::new(BTreeSet::new());

///

/// // Due to its private field, this struct can only be created using `new` outside this module.

/// #[derive(Debug, PartialEq)]

/// pub struct Unique<TypeAsId: 'static>(

/// // this usage of `TypeAsId` makes `Unique` covariant 🚨

/// PhantomData<TypeAsId>,

/// );

///

/// impl<TypeAsId: 'static> Unique<TypeAsId> {

/// pub fn new() -> Option<Self> {

/// let mut set = ID_SET.lock().unwrap();

/// set.insert(TypeId::of::<TypeAsId>()).then(|| Self(PhantomData))

/// }

/// }

///

/// impl<TypeAsId: 'static> Drop for Unique<TypeAsId> {

/// fn drop(&mut self) {

/// let mut set = ID_SET.lock().unwrap();

/// (!set.remove(&TypeId::of::<TypeAsId>())).then(|| panic!("duplicity detected"));

/// }

/// }

/// }

///

/// use unique::Unique;

Just trimming down some lines.

Fixes rust-lang#89150 Co-authored-by: Daniel Henry-Mantilla <daniel.henry.mantilla@gmail.com>

hkBst · 2025-02-11T08:24:31Z

I like this. The example is a little long but I think that's fine. I left a couple of comments to minimize the example a little bit, but other than that this looks good.

Minimized the example without losing the intuition of the ring analogy.

ibraheemdev · 2025-02-11T08:32:55Z

This looks good, thanks. @bors r+

bors · 2025-02-11T08:32:58Z

📌 Commit e279c4e has been approved by ibraheemdev

It is now in the queue for this repository.

fmease · 2025-02-11T14:49:51Z

@bors rollup

rustbot assigned cuviper Jan 29, 2025

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Jan 29, 2025