-
Notifications
You must be signed in to change notification settings - Fork 492
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
Document variance #186
Document variance #186
Conversation
I don't know nearly enough about this to be reviewing it. |
cc @gankro @nikomatsakis @arielb1 ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm high on nyquil qhat's upp
src/subtyping.md
Outdated
Since `'static` outlives `'a`, `&'static str` is a subtype of `&'a str`. | ||
|
||
Subtyping also exists for [higher-ranked types]. Replacing a higher ranked | ||
lifetime with a concrete lifetime produces a subtype. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: I don't believe the documentation you linked specifies what a "higher ranked type" is. Not entirely sure what you mean here. Literally just function pointers?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Function pointers and trait objects
src/subtyping.md
Outdated
// Error: in type `&'static &'a i32`, reference has a longer lifetime than | ||
// the data it references | ||
} | ||
``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unclear to me why this merits discussion (is this meaningfully different from adding a note that "the expression must be parse"?)
src/subtyping.md
Outdated
* `std::cell::UnsafeCell<T>` is invariant in `T`. | ||
* `std::marker::PhantomData<T>` is covariant in `T`. | ||
* Trait objects are invariant in their type parameters and associated types and | ||
covariant in their [lifetime bound]. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this would be better written as a table, but idk
src/subtyping.md
Outdated
The variance of `struct`, `enum`, `union` and tuple types is decided by looking | ||
at the variance of the types of their fields. If a type parameter is used more | ||
than once then the more restrictive variance is used. For example the following | ||
struct is covariant in `'a` and `T` and invariant in `'b` and `U`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a bit unclear (and either very subtle or just actually wrong for co+contra). There's basically two interesting cases for multi-occurence:
- If everything agrees on the variance, then that variance is used
- If two different variances would apply, it's invariant
1427232
to
522e767
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Much better, mostly minor editing comments.
| `fn(T) -> ()` | | contravariant | | ||
| `std::cell::UnsafeCell<T>` | | invariant | | ||
| `std::marker::PhantomData<T>` | | covariant | | ||
| `Trait<T> + 'a` | covariant | invariant | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thing I don't know but someone should check: is &'a mut Trait<T> + 'a
actually invariant in 'a
? (this is implied my the entries for &mut T
and Trait<T> + 'a
, but I would almost expect the compiler to magic this away.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like it is invariant, but there is a coercion from &mut (Trait + 'a)
to &mut Trait + 'b
where 'a: 'b
. The same as &mut (Trait + Send)
and &mut Trait
.
trait A {}
// This compiles (also with `*mut`, or `Box<RefCell>`)
fn coercion<'a, 'b>(x: &'b mut (A + 'static)) -> &'b mut (A + 'a) {
x
}
// This does not
fn not_subtype<'a, 'b, 'c>(x: &'c &'b mut (A + 'static)) -> &'c &'b mut (A + 'a) {
x
}
src/subtyping.md
Outdated
Since `'static` "lives longer" than `'a`, `&'static str` is a subtype of | ||
`&'a str`. | ||
|
||
Since `'static` outlives `'a`, `&'static str` is a subtype of `&'a str`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it's not declared above, could we clarify that 'a
here is a standin for "any generic lifetime in scope" (or maybe just "some lifetime"?)
src/subtyping.md
Outdated
Since `'static` outlives `'a`, `&'static str` is a subtype of `&'a str`. | ||
|
||
Subtyping also exists for [higher-ranked] function pointer and trait object | ||
types. Replacing a higher ranked lifetime with a concrete lifetime produces a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would rewrite this to "function pointers and trait objects" (current way reads awkward)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the notion of a concrete lifetime really well-defined and/or clear here?
src/subtyping.md
Outdated
|
||
Subtyping also exists for [higher-ranked] function pointer and trait object | ||
types. Replacing a higher ranked lifetime with a concrete lifetime produces a | ||
subtype. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an interesting phrasing, I'm not sure if it makes sense to most.
src/subtyping.md
Outdated
// Explicitly f: &(for<'a> Fn(&'a i32) -> &'a i32). | ||
let f: &(Fn(&i32) -> &i32) = &(|x| x); | ||
let g: &(Fn(&'static i32) -> &'static i32) = f; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unless I'm mistaken, doctest implicitly wraps the example in main
, so you can get rid of these distracting functions.
Also consider renaming f and g to "base" and "subtype" or something more self-descriptive.
src/subtyping.md
Outdated
y: *const T, | ||
z: UnsafeCell<&'b f64>, | ||
w: *mut U, | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This example would benefit from inline comments or outline discussion (I originally missed that U was used twice).
522e767
to
af6bb40
Compare
@gankro Wanna give this a re-review? (You're allowed to say no) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the delay, this totally fell off my radar. This looks great!
Since `'static` outlives the lifetime parameter `'a`, `&'static str` is a | ||
subtype of `&'a str`. | ||
|
||
[Higher-ranked] [function pointers] and [trait objects] have another |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is this escaped space necessary? otherwise markdown parses it as a reference?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Markdown would read it as a link with text of "[Higher ranked]" and a link reference to "function pointers" instead of as two separate links.
Rebase this on master and leave a note saying you've done so and I'll get it merged. |
rebased, but mdbook isn't building on travis. |
First of a few PRs to improve the documentation of lifetimes in the reference. Based on the documentation in the Nomicon.
cc #9 (rust-lang/rfcs#738)