Join GitHub today
GitHub is home to over 31 million developers working together to host and review code, manage projects, and build software together.
Sign upRemove markers and replace with a default rule for unused type/lifetime parameters #12
Conversation
This comment has been minimized.
This comment has been minimized.
bill-myers
commented
Mar 17, 2014
|
Could also make unused types and lifetimes a compiler error, and suggest to the user to apply a variance marker in the message. |
This comment has been minimized.
This comment has been minimized.
edwardw
commented
Mar 17, 2014
|
Then the marker types themselves become illegal. |
This comment has been minimized.
This comment has been minimized.
bill-myers
commented
Mar 17, 2014
|
The marker types have lang items, and those lang items would count as usage of the parameter. |
nrc
reviewed
Mar 17, 2014
| code that may encode uses for types that are not immediately evident. | ||
| For example, one idiom is to attach a lifetime to a struct that | ||
| contains unsafe pointers, to prevent that struct from being used | ||
| outside of a certain scope. For example, at some point the iterator |
This comment has been minimized.
This comment has been minimized.
nrc
Mar 17, 2014
Member
Maybe it is just me, but from an engineering point of view, this seems like a really horrible idiom and we should not encourage it. If there is a need for this we should have some sort of explicit visibility thing in the language. I don't think we should change our type system to help its use.
This comment has been minimized.
This comment has been minimized.
cmr
Mar 18, 2014
Member
It's not obvious to me why this is horrible or what the alternatives would be.
This comment has been minimized.
This comment has been minimized.
nrc
Apr 7, 2014
Member
Sorry, that was strong language to use without explanation.
I meant it is horrible because it feels like a hack - you add an unused parameter to enforce using the struct in a certain scope - there is no indication of intent there. Someone reading the code would not know that that was what the lifetime parameter was for if they didn't know about the idiom. I would prefer something explicit to limit the usage, rather than implicitly relying on lifetime checking where there is not really a lifetime (from the perspective of inside the strucure). I also have no idea what such a thing would look like though.
This comment has been minimized.
This comment has been minimized.
nikomatsakis
Apr 9, 2014
Author
Contributor
Well, we have an existing design that gives a hint at what it looks like -- marker::ContravariantLifetime<'a>
nrc
reviewed
Mar 17, 2014
| follows: **If a type or lifetime parameter is not used within the body | ||
| of a type, we default to covariance for types and contravariance for | ||
| lifetimes.** | ||
|
|
This comment has been minimized.
This comment has been minimized.
nrc
Mar 17, 2014
Member
I would encourage invariant as the default for types. I think programmers should assume invariant subtyping, even though covariant is the intuitive thing. We should not encourage this wrong-thinking.
nrc
reviewed
Mar 17, 2014
| That is equivalent (from the variance algorithm's point of view) to | ||
| a struct that contains an instance of `T`: | ||
|
|
||
| struct Foo<T> { field: T } // Foo<T> above equivalent to this |
This comment has been minimized.
This comment has been minimized.
nrc
Mar 17, 2014
Member
I would expect this to make Foo invariant wrt T since field can be both read and written. If we are talking about coercion rather than subtyping this makes sense and so does the covariant default above.
This comment has been minimized.
This comment has been minimized.
nikomatsakis
Mar 18, 2014
Author
Contributor
This is not how Rust works, though. Mutability is imposed from the outside: field is not mutable, in this declaration.
This comment has been minimized.
This comment has been minimized.
nikomatsakis
Mar 18, 2014
Author
Contributor
(Note that if you had field: RefCell<T>, that'd be different (and the algorithm would do the right thing))
This comment has been minimized.
This comment has been minimized.
nrc
Apr 7, 2014
Member
So you will infer different variances for Foo<T> and mut Foo<T> (covariant and invariant, respectively)? (My mistake was assuming you propose inferring variance per poly-type, vs per type).
This comment has been minimized.
This comment has been minimized.
cmr
Apr 8, 2014
Member
No, because mut Foo<T> does not exist. There is no such thing as a mutable field. It would do the right thing with RefCell<T> because RefCell would need to be invariant with respect to T since it contains Unsafe<T>
cmr
reviewed
Mar 18, 2014
|
|
||
| That is equivalent to a struct containing a reference of lifetime `'a`: | ||
|
|
||
| struct Bar<'a> { f: &'a () } // Note: 'a is not used |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
@bill-myers yes, you're right, I should at least list "error" as an alternative. It's also appealing to force explicitness. I think my hope was that we could remove -- or almost remove -- markers entirely. In fact, given the rules here, I think we could remove them entirely, since they could be defined "in language". For example,
Similar tricks should work for lifetimes. |
pnkfelix
reviewed
Mar 18, 2014
| continue to be safe. In other words, if I have `&Wrap<int>`, that is | ||
| interchangable with `&Wrap<uint>`. The algorithm is not wrong. Since | ||
| `Wrap` has no fields, there is nothing you can do with this `T` that | ||
| is incorrect per se. But the result is certainly surprising. |
This comment has been minimized.
This comment has been minimized.
pnkfelix
Mar 18, 2014
Member
Wouldn't a lint (warning you about bivariant type parameters or lifetime parameters) deal with this? The lint could then point the programmer at the std::kinds::marker module, and then the programmer who had some particular intuition about what that phantom type was for could actually explicitly write out their intent.
(You have of course mentioned std::kinds::marker below. I just want to understand if the purpose of this RFC is to reduce risk, which I think a lint will be better at, or for programmer convenience, which is where choosing the defaults to match expectations (or perhaps to match actual frequency of occurrence) would be relevant.)
((ugh, this is exactly what bill-myers said above. Sorry for the noise.. :( ))
This comment has been minimized.
This comment has been minimized.
|
(Ah, niko's response to bill-myers implies to me that there are indeed goals here beyond just reducing risk. That is fine, though I think it may make the RFC stronger to list those other goals explicitly, if possible.) |
This comment has been minimized.
This comment has been minimized.
|
On Tue, Mar 18, 2014 at 09:07:33AM -0700, Felix S Klock II wrote:
I released the RFC prematurely, I think. I updated it. I'm also |
This comment has been minimized.
This comment has been minimized.
|
I just pushed a revised version of the RFC. |
edwardw
referenced this pull request
Mar 19, 2014
Closed
It is possible to call object member function after object had been dropped #12470
alexcrichton
referenced this pull request
Mar 19, 2014
Closed
WIP: Incorporate variance into typeck #12828
This comment has been minimized.
This comment has been minimized.
|
I agree with @bill-myers here. Unused type/lifetime parameters should be an error, or at the very least a deny-by-default lint. See https://botbot.me/mozilla/rust/msg/12337250/ for confusion in the wild. Especially since this is such a sublte issue (at least IMO) it's better to have compiler help than just documentation. |
This comment has been minimized.
This comment has been minimized.
|
Actually I take that back. I re-read the RFC and am in favor. +1 |
This comment has been minimized.
This comment has been minimized.
|
There is one complication -- I hadn't considered cycles among types, (I'm a bit torn about this myself; I definitely prefer error to the |
This comment has been minimized.
This comment has been minimized.
|
@cmr Unused lifetime parameters are sometimes useful, as in https://github.com/mozilla/rust/blob/ce62032/src/libstd/fmt/mod.rs#L1167-L1179 which allows for macros with optionally used lifetime parameters. |
This comment has been minimized.
This comment has been minimized.
|
cc rust-lang/rust#5922 (@lifthrasiir, that macro is actually incorrect if we ever get hygienic lifetimes; and if we don't then one should be able to disable the warning lint (assuming it's a lint...).) |
nikomatsakis
referenced
this pull request
in edwardw/rust
Mar 21, 2014
This comment has been minimized.
This comment has been minimized.
gasche
commented
Mar 25, 2014
|
I have bad feelings about the proposed behavior in this RFC. My impression is that ML languages have under-estimated the importance of bivariance for too long (e.g. there is no syntax for that in the OCaml surface syntax), and that the discussed change makes Rust take the same road. I think it is important, for the language to be "complete" and robust to unplanned scenarios, that bivariance of parameters (type or lifetime) be supported. Given that std::kind::marker does not have a BivariantType marker (which is also a sign of Rust's design neglecting bivariance), how would one specify that a type is bivariant if the variance inference is tweaked not to detect it? It is very likely that I misunderstood the proposed change and that there is a way to opt-in to bivariance -- in which case I think the RFC could be debated for taste reasons, but is acceptable. An alternative proposal that I think would solve the problem of user surprise at bivariant parameters is the following: warn on "unused" type and lifetime parameters, with a warning message that mentions the problem and prompt people to explicitly add a variance annotation (or marker type or, better in most cases, In ML, bivariant type parameters are very useful to build "phantom types" (which rely on module abstraction). The idea is to have a type definition One could also consider scenarios where users may want to remove fields of a The last argument would be that Niko's proposed change to the current inference algorithms makes the logic sensibly more complex; simple is always better, especially in a type system. (Another experience I had in ML-land is that variance have very different meanings for the author and the users of a library, that people tend to conflate. There are two interpretations for |
This comment has been minimized.
This comment has been minimized.
This is helpful feedback. I have indeed intentionally neglected
I don't think Rust has an equivalent to this ML separation between I think the Rust-y way to do what you are describing would be to have Right now, at least as much as I understand how bivariance plays out, |
This comment has been minimized.
This comment has been minimized.
|
On Tue, Mar 25, 2014 at 09:19:10AM -0700, gasche wrote:
Maybe my view is blurred, but I don't really understand this point. |
This comment has been minimized.
This comment has been minimized.
gasche
commented
Mar 26, 2014
|
Sorry for being unclear. I guess my general point is that not being able to express bivariance could become, not a major blocker, but at least an occasional pain, if combined with other type-system features that are not present in Rust today. Formulated as is, you see that it is a rather weak objection from a pragmatic point of view. It would not be worth a design change if the present design naturally omitted bivariance; but it does suggest that willingly making your system slightly less regular and more complex to remove bivariance (as proposed in this RFC) may be a cause of regret in a (far) future. The two features I considered were:
|
This comment has been minimized.
This comment has been minimized.
|
On Wed, Mar 26, 2014 at 05:57:45AM -0700, gasche wrote:
It is also plausible to imagine a future way to explicitly "opt in" to
I don't quite follow this part. While I don't know why we would want |
This comment has been minimized.
This comment has been minimized.
flaper87
commented
Mar 29, 2014
|
I'm not an expert on this field but based on my understanding, I like this proposal. I like the fact that it clears out the inference algorithm by removing possible weird cases that users may actually ignore (which I think makes the type-system safer) and it removes the variance markers. That said, I think having a way to opt-in to bivariance is also important. The whole point of this RFC should be to make the type inference safer without forbidding other things - which follows pretty much what is being done for kinds. As a final note, I think the lint that has been mentioned could be implemented now and warn the user about unused types and lifetimes. |
nikomatsakis
changed the title
Add RFC for tweaked variance inference rules
Remove markers and replace with a default rule for unused type/lifetime parameters
Apr 15, 2014
This comment has been minimized.
This comment has been minimized.
|
Just a brief update: I am not sure if I still like this plan. In weekly meeting we decided to do some measurements (which I still haven't done) to try and understand how often this comes up. |
edwardw
referenced this pull request
Jun 26, 2014
Closed
test: Add a test for variance in trait matching. #15191
eddyb
reviewed
Aug 8, 2014
| struct CovariantType<T>; | ||
| struct ContravariantType<T> { m: CovariantType<fn(T)> } | ||
| struct InvariantType<T> { m: CovariantType<fn(T) -> T> } | ||
| struct CovariantLifetime<'a> { m: CovariantType<fn(&'a int)> } |
This comment has been minimized.
This comment has been minimized.
eddyb
Aug 8, 2014
Member
I was really worried about this, but now I like the idea, seeing how ContravariantType and CovariantLifetime can be emulated.
nikomatsakis commentedMar 17, 2014
Inspired by @edwardw's PR rust-lang/rust#12828, this RFC proposes a small change to the behavior of variance inference if a type or lifetime parameter is unused. The goal is to more accurately capture user's intended behavior.