rust-lang / rust-lang/rfcs

Surprising bivariance error in safe code

Open
#1,197 2 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

T-lang
Dominant language
Markdown
Stars
6.6k
Forks
1.7k
Avg merge
16h 14m
Merged PRs (30d)
1

Description

Unit structs are desirable for various reasons; one of them is to allow dynamically choosing, via trait objects, between implementations of functions that don't inherently require a self argument. In this simplified motivating example, I have a trait which has a static method to create an instance of the implementing type:

trait ThingDoer {
    fn create() -> Self;
    fn do_something(&self);
}

Suppose I want to allow callers to choose at runtime which implementation to create an instance of, receiving a trait object. I can do this by creating a generic wrapper unit struct which represents a single implementation, and can be converted to a trait object for a "factory" trait that wraps the create() static method:

trait ThingDoerFactory {
    fn create(&self) -> Box<ThingDoer>;
}
struct ThingDoerFactoryImpl<T: ThingDoer>;
impl<T: ThingDoer> ThingDoerFactory for ThingDoerFactoryImpl<T> {
    fn create(&self) -> Box<ThingDoer> { Box::new(T::create()) }
}

As written, this doesn't compile because ThingDoer is not object safe (bleh). That's easily fixed by splitting the static method(s) out into a separate trait, but there's another problem:

test.rs:10:29: 10:30 error: parameter `T` is never used [E0392]
test.rs:10 struct ThingDoerFactoryImpl<T: ThingDoer>;
                                       ^
test.rs:10:29: 10:30 help: consider removing `T` or using a marker such as `core::marker::PhantomData`

The suggestion to use PhantomData is initially confusing, because ThingDoerFactoryImpl instances do not "contain" any instances of T, not even logically / indirectly via unsafe code. After reading up on it, I guess the issue is that as written, T is bivariant because the struct does not have any fields putting variance constraints on it, and bivariance isn't usually particularly useful. In this case, since there is no point in converting instances of ThingDoerFactoryImpl<A> to ThingDoerFactoryImpl<B>, the issue can be resolved by adding a PhantomData<*const T> field to indicate invariance.

However, PhantomData is, frankly, rather arcane feeling, especially with the raw pointer type inside it; while it's necessary as a way for unsafe code to inform the compiler about some things it's doing behind its back, and the way it works does make sense once you understand the problem, there is no unsafe code here, so I think requiring this type of thing should be avoided except as a last resort. In this case, while, as I said, converting instances is not useful, it's also not harmful, so nothing bad would happen if the compiler simply accepted the struct as bivariant - but I realize that there's no really good way to differentiate between what I'm doing and the unsafe cases the error was designed for, so I guess silently accepting it would be a bad idea.

(EDIT: Another problem is that PhantomData causes the factory to lose Sync, Copy, etc., which it shouldn't.)

I can think of two possible solutions:

  1. Add a simpler standard library marker type that just marks a parameter as invariant, without having the more complicated interface of PhantomData. (I can't think of a good reason to want phantom covariant and contravariant parameters in safe code, so invariant should be enough.)
  2. Turn the bivariance error into a deny-by-default lint, so an #[allow()] declaration could be used if a type is really meant to be bivariant .

Notes:

  • Incidentally, it would be nice if it were somehow possible to have a trait object corresponding to a type rather than an instance of one: just like a single implementation of a trait containing instance methods supports use via both monomorphization and dynamic dispatch "for free", without additional boilerplate, something similar should apply to traits containing static methods.
  • In some cases a simpler approach would be to remove the static methods altogether, expect every ThingDoer implementation to also implement their own ThingDoerFactory, and move the real create logic to there, but that would result in it only being possible to create ThingDoers as trait objects. I want to keep the static interface for callers that know at compile time which implementation they want, in addition to the trait object interface.

Contributor guide

No contributing guide indexed for this repository

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Begin with the motivating Rust example and the discussion of bivariance, PhantomData, and object safety. Compare the two proposed changes—an invariant marker type or an allowable lint—and define a settled language-design direction; no file or test is named in the issue.

Written by the indexing model from the issue text.

Assessment

Tech stack
rust
Domain
compilers
Issue type
Feature
Difficulty
5/5
Estimated time
Over a week
Activity status
Stale
Clarity
Needs clarification
Newbie friendliness
25/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.