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

revert stabilization of `!` and `TryFrom` #49691

Closed
nikomatsakis opened this Issue Apr 5, 2018 · 16 comments

Comments

Projects
None yet
6 participants
@nikomatsakis
Copy link
Contributor

nikomatsakis commented Apr 5, 2018

Per #49593, we need to rollback the stabilization of ! (and a few dependent things) from 1.26. That's sad, but hopefully only temporary -- we'll bring it back once we can fix the coercion, but we don't want to be tinkering with the core type system in a beta backport. I'm opening this P-high issue specifically to track the backport, not the response.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Apr 5, 2018

triage: P-high

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Apr 5, 2018

cc @canndrew -- this is sorta sad :(

@mitsuhiko

This comment has been minimized.

Copy link
Contributor

mitsuhiko commented Apr 10, 2018

Since ! is hard to search for I just left a comment on probably not the best issue. However I came across a case which now fails to compile I want to ensure does not fall under the radar: #48950 (comment)

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Apr 11, 2018

@mitsuhiko

Interesting.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Apr 12, 2018

So @alexcrichton and I were saying that it would make sense to revert the TryFrom stabilization PRs first, and then the ! changes. I think he said @SimonSapin was going to do that?

@SimonSapin

This comment has been minimized.

Copy link
Contributor

SimonSapin commented Apr 12, 2018

FWIW I think it sucks if we can’t find a satisfying solution to #49593 in time. If we must revert it should’t be in the opposite order but there’s no need to separate them either. I could prepare a commit that changes some #[stable] attributes back to #[unstable] and it can land together with the rest.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Apr 13, 2018

Argh. I meant to do this today, but I failed to get it done. It's going to be a bit tricky to revert !. If you look at the actual PR (#47630), you'll see there was a lot of complicated stuff.

I do not want to just revert back to what we had -- in particular I do not want to keep that flag we had on () type (we used to have two flavors of tuples, bad). However, that was the future compat warning mechanism we were using. We could use a coarser mechanism, where we check for any trait impls that concern () -- or just omit warnings for now. I'm inclined to do the latter at least to start (which in fact sidesteps the majority of the diff from #47630).

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Apr 13, 2018

Hmm, ping @pnkfelix -- do you think you could take on reverting this next week?

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Apr 13, 2018

FWIW I think it sucks if we can’t find a satisfying solution to #49593 in time

@SimonSapin I agree, but I feel reluctant to make changes to inference in a beta backport. But maybe it is worth expending some effort to see if we can find a minimal delta that we'd feel comfortable with.

@SimonSapin

This comment has been minimized.

Copy link
Contributor

SimonSapin commented Apr 13, 2018

I feel reluctant to make changes to inference in a beta backport

That’s definitely fair enough. I’m just sad we didn’t catch all this sooner.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Apr 13, 2018

Brainstorming:

  • So, what happens with #49593 is:
    • Box::new(x) has an expected type of Box<dyn Error> and returns a value of type Box<?T>
    • From this, we deduce that ?T = dyn Error would be helpful
    • This in turn propagates up to the argument, so that we have x with a type of ! being coerced to dyn Error
  • This "back propagation" from return type to argument is very useful for things like Some(foo) being coerced into Option<fn()> etc
    • However, it backfires in this case

Some options:

  • We could try and take into account the fact that we know that ?T: Sized
    • there is no clear mechanism for doing this, and I've historically been reluctant;
    • among other things, we might expect Box::new to take unsized values in the future
      • (though it might not be compatible for other reasons?)
  • I guess that might look like one of the following:
    • look to see if this "back propagation" results in a type variable being unified when there is also a pending T: Sized predicate
      • if so, disable it
    • or, look to see if this "back propagation" results in a signature where some of the arguments are unsized
      • but this seems pretty clearly forwards incompat
  • In general, it seems.. potentially reasonable to be avoid back-propagating dyn Trait types, on the premise that a more specific type is better and we can always coerce into the dyn Trait later

I'm not sure what other options there are, but I guess that trying to disable the back-propagation in the event that it generates constraints that violate T: Sized seems potentially reasonable, though I don't like the side-effect that making type parameters become T: ?Sized would break callers. (This would be true not just for Box::new, of course.) The only saving grace is that maybe in the future we will come up with better coercion mechanisms that avoid this downside.

I'm also a bit unclear: would we check for any unifications that violate T: Sized? What if the back-propagation is useful sometimes but not others?

(cc @eddyb for thoughts)

@SimonSapin

This comment has been minimized.

Copy link
Contributor

SimonSapin commented Apr 13, 2018

we might expect Box::new to take unsized values in the future

If so, back-propagation becomes a non-issue in this case, doesn’t it?

@glaebhoerl

This comment has been minimized.

Copy link
Contributor

glaebhoerl commented Apr 14, 2018

Another point in the solution space would be to make ! automatically implement all traits whose possible implementation is uniquely determined by the fact that Self = !, as @canndrew has previously proposed. That is, all traits where every method takes Self (that is, !) as an argument, and where therefore all of them are actually uncallable. Notably, I think this encompasses every object-safe trait. (Unless where Self: Sized methods throw a wrench into the works?)

Now, on some level we might abstractly think, "but ! should on principle coerce to any type, and object-safe traits would only be covering one set of cases"; but types involving dyn Trait can of course only exist for object-safe traits, so (again, unless where Self: Sized messes it up), this would in theory solve the whole problem.

(This sounds like a more radical solution, but I dunno. It's something we've (at least in my impression) only held off on in the name of conservatism and incrementality. The idea itself seems "obviously correct", and like something we'd surely want to have eventually, whereas it doesn't seem obviously true that we'd want to have the special-case type inference hacks if we already had this. So it's at least not self-evident which one is less conservative.)

@dtolnay

This comment has been minimized.

Copy link
Member

dtolnay commented Apr 14, 2018

@glaebhoerl but ! already implements Error so automatically implementing more traits does not solve the current problem -- #49593 (comment).

@glaebhoerl

This comment has been minimized.

Copy link
Contributor

glaebhoerl commented Apr 14, 2018

Oh, I see. Thanks for explaining. In that case, we only need to implement #48055 :P

@pnkfelix pnkfelix self-assigned this Apr 19, 2018

@pnkfelix

This comment has been minimized.

Copy link
Member

pnkfelix commented Apr 19, 2018

assigning to self to try to ensure I actually attack this problem tomorrow. But leaving niko on assignee list in case I do not finish it tomorrow (because he and I will be swapping our PTO-states)

pnkfelix added a commit to pnkfelix/rust that referenced this issue Apr 20, 2018

Bring back old fallback semantics: Without feature(never_type), fallb…
…ack to `()`, not `!`.

Note that this commit, since it is trying to be minimal in order to
ease backporting to the beta and release channels, does *not* include
the old future-proofing warnings that we used to have associated with
such fallback to `()`; see discussion at this comment:

rust-lang#49691 (comment)

bors added a commit that referenced this issue Apr 21, 2018

Auto merge of #50121 - pnkfelix:revert-stabilization-of-never-type-et…
…-al, r=alexcrichton

Revert stabilization of never_type (!) et al

Fix #49691

I *think* this correctly adopts @nikomatsakis 's desired fix of:
 * reverting stabilization of `!` and `TryFrom`, and
 * returning to the previous fallback semantics (i.e. it is once again dependent on whether the crate has opted into `#[feature(never_type)]`,
 * **without** attempting to put back in the previous future-proofing warnings regarding the change in fallback semantics.

(I'll be away from computers for a week starting now, so any updates to this PR should be either pushed into it, or someone else should adopt the task of polishing this fix and put up their own PR.)

@bors bors closed this in #50121 Apr 22, 2018

alexcrichton added a commit to alexcrichton/rust that referenced this issue Apr 23, 2018

Bring back old fallback semantics: Without feature(never_type), fallb…
…ack to `()`, not `!`.

Note that this commit, since it is trying to be minimal in order to
ease backporting to the beta and release channels, does *not* include
the old future-proofing warnings that we used to have associated with
such fallback to `()`; see discussion at this comment:

rust-lang#49691 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment