reserve impl<T> From<!> for T
#62661
Conversation
rust-highfive
added
the
S-waiting-on-review
|
If we do a crater run, it should probably also include the change to make |
src/librustc/ty/mod.rs
Outdated
| } | ||
| (ImplPolarity::Positive, ImplPolarity::Negative) | | ||
| (ImplPolarity::Negative, ImplPolarity::Positive) => { | ||
| // FIXME: when can this happen? |
There was a problem hiding this comment.
Isn't that this case? (playground)
#![feature(optin_builtin_traits)]
auto trait Foo { }
impl Foo for u32 { }
impl !Foo for u32 { }
fn main() { }| &hir::ImplPolarity::Positive => ImplPolarity::Positive, | ||
| &hir::ImplPolarity::Negative => ImplPolarity::Negative, | ||
| &ty::ImplPolarity::Positive | | ||
| // FIXME: do we want to do something else here? |
There was a problem hiding this comment.
Almost certainly yes -- I think this means that reservation impls will show up in rustdoc as if they were real impls, which seems bound to lead to confusion, no?
There was a problem hiding this comment.
Is it enough to show the reservation attribute?
There was a problem hiding this comment.
cc @QuietMisdreavus (I think you know Rustdoc?)
src/librustc/traits/select.rs
Outdated
| @@ -1325,17 +1325,21 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> { | |||
| (result, dep_node) | |||
| } | |||
|
|
|||
| // Treat negative impls as unimplemented | |||
| fn filter_negative_impls( | |||
| // Treat negative impls as unimplemented, and reservation impls as Ok(None) | |||
There was a problem hiding this comment.
instead of "as Ok(None), perhaps:
Treat negative impls as unimplemented, and reservation impls as ambiguity.
|
So, this isn't the approach I had intended to take, though it may well be better. I thought I'd write out what I was thinking and see what you thought of it. Basically, I had intended to modify the |
|
In general, I think we need to document clearly what the semantics of a "reservation" impl are. They aren't quite what the name alone would suggest to me. In particular, just having a "reservation" impl doesn't guarantee that you can add the impl later, because of the fact that they never conflict for coherence reasons (though, given specialization, and a sufficiently simple reservation impl (without auxiliary where clauses), it may well mean that you could add the impl later). I guess that the semantics are something like:
The upshot is:
|
|
So yes, some things I'd like to address before landing this PR:
|
|
Regarding a crater run: are we concerned that people are already relying on negative reasoning? |
We might want to add a knob later on to specify "how hard" to reserve, e.g banning overlap, but this seems like future work for other scenarios when this would be useful.
Seems like a good idea. I would also suggest that UI tests should be provided in the cases of reservation as a start to actually confirm what the reservation does.
Would be nice to have better diagnostics yes, but I'm also cognizant of the delay to stabilizing |
An interim solution would be to minimally expand the current error with a note pointing at this or the tracking issue. As long as diagnostics for other more stablished features don't regress, it should be ok to land this on nightly, but let's cut tickets for common errors we can anticipate and see if we can get to them quickly enough that they can be shipped together. I am personally reactive on these cases, but it'd be lovely if we could avoid every time we stabilize a new feature having a subpar experience, because it is likely people will go out to check the new toy out, only to cut themselves with the sharp edges. |
Yes, they might already assume this impl does not exist for |
|
Notably, if you try to make a function like It worries me that some may have already done so, and therefore a change to a type alias of |
Where do you think would be the right place to document this? |
The rustc guide probably? |
|
One problem I noticed with trait Uninhabitable { fn never(self) -> !; }
impl Uninhabitable for ! { fn never(self) -> !; }
struct MyType {}
// ok
impl<T> From<T> for MyType where T: Uninhabitable {
fn from(t: T) -> Self { t.never() }
}This would make it more difficult to implement the overlap using lattice specialization (as opposed to "overlapping marker traits"), and therefore we might want to forbid that sort of downstream impl, while allowing the |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
src/libcore/convert.rs
Outdated
| #[rustc_reservation_impl] | ||
| #[rustc_reservation_impl="a future version of Rust might implement `From<!>` for \ | ||
| all types. \ | ||
| However, it is OK to implement `From<!>` for types you own - \ |
There was a problem hiding this comment.
The complexity of this error message and describing the situation to users pushes me further in the direction of thinking that we should go ahead and provide the impl<T> From<!> for T.
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
☔ The latest upstream changes (presumably #63214) made this pull request unmergeable. Please resolve the merge conflicts. |
ok, right, duh. I was only thinking of the core mechanism. |
|
@bors try -- well, shall we attempt the crater run then? |
reserve `impl<T> From<!> for T` this is necessary for never-type stabilization. cc #57012 #35121 I think we wanted a crater run for this @nikomatsakis? r? @nikomatsakis
|
☀️ Try build successful - checks-azure |
|
@craterbot run mode=check-only |
|
👌 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
arielb1
force-pushed
the
never-reserve
branch
from
189eadd
to
e70724c
Compare
|
rebased @bors r=nikomatsakis |
|
📌 Commit e70724c has been approved by |
bors
added
S-waiting-on-bors
reserve `impl<T> From<!> for T` this is necessary for never-type stabilization. cc rust-lang#57012 rust-lang#35121 I think we wanted a crater run for this @nikomatsakis? r? @nikomatsakis
reserve `impl<T> From<!> for T` this is necessary for never-type stabilization. cc rust-lang#57012 rust-lang#35121 I think we wanted a crater run for this @nikomatsakis? r? @nikomatsakis
reserve `impl<T> From<!> for T` this is necessary for never-type stabilization. cc rust-lang#57012 rust-lang#35121 I think we wanted a crater run for this @nikomatsakis? r? @nikomatsakis
reserve `impl<T> From<!> for T` this is necessary for never-type stabilization. cc #57012 #35121 I think we wanted a crater run for this @nikomatsakis? r? @nikomatsakis
|
☀️ Test successful - checks-azure |
…li-obk Stabilize `!` in Rust 1.41.0 This PR stabilizes the `never_type` (written `!`). The type represents computations that we know diverge in the type system and therefore has no values / inhabitants / elements / members. The current nightly version is 1.40.0 which will become stable on 2019-12-19. Tracking issue: rust-lang#35121. Closes rust-lang#57012. Closes rust-lang#58184. Original stabilization report: rust-lang#57012 (comment) Additional notes: - In rust-lang#62661 we reserved `impl<T> From<!> for T` so this concern should be resolved. - The type inference fallback change is moved to `#![feature(never_type_fallback)]` (rust-lang#65992). - You can find all of the tests referencing `never_type` in this PR which also reorganizes these tests whereas they were more scattered before. r? @nikomatsakis
this is necessary for never-type stabilization.
cc #57012 #35121
I think we wanted a crater run for this @nikomatsakis?
r? @nikomatsakis