Skip to content
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

Regression from stable to nightly: nested impl trait is not allowed #57979

Closed
KodrAus opened this issue Jan 29, 2019 · 22 comments
Closed

Regression from stable to nightly: nested impl trait is not allowed #57979

KodrAus opened this issue Jan 29, 2019 · 22 comments

Comments

@KodrAus
Copy link
Contributor

@KodrAus KodrAus commented Jan 29, 2019

I have some code that looks a bit like this:

use std::borrow::Borrow;

pub struct Data<TBody>(TBody);

pub fn collect(_: impl IntoIterator<Item = impl Borrow<Data<impl AsRef<[u8]>>>>) {
    unimplemented!()
}

On 1.32.0 (and the current beta 1.33.0-beta.4 2019-01-24 635817b9db20ecdcd036) this compiles successfully.

On the current nightly 1.34.0-nightly 2019-01-28 d8a0dd7ae88023bd09fa this fails with:

error[E0666]: nested `impl Trait` is not allowed
 --> src/main.rs:5:61
  |
5 | pub fn collect(_: impl IntoIterator<Item = impl Borrow<Data<impl AsRef<[u8]>>>>) {
  |                                            -----------------^^^^^^^^^^^^^^^^--
  |                                            |                |
  |                                            |                nested `impl Trait` here
  |                                            outer `impl Trait`

A workaround is just to use generics. Did we mean to change the rules around nested impl trait here?

@jonas-schievink
Copy link
Member

@jonas-schievink jonas-schievink commented Jan 29, 2019

Good: rustc 1.33.0-nightly (19f8958 2019-01-23)
Bad: rustc 1.33.0-nightly (01f8e25 2019-01-24)

Commits in that range

@jonas-schievink
Copy link
Member

@jonas-schievink jonas-schievink commented Jan 29, 2019

Likely cuplrit: #57730 (this comment seems relevant)

cc @Zoxc

Zoxc added a commit to Zoxc/rust that referenced this issue Jan 29, 2019
Zoxc added a commit to Zoxc/rust that referenced this issue Jan 30, 2019
@Zoxc Zoxc mentioned this issue Jan 30, 2019
@Centril Centril added the T-lang label Jan 30, 2019
@Centril
Copy link
Contributor

@Centril Centril commented Jan 30, 2019

Also nominated for T-Lang's Thursday meeting; from what I can tell, this shouldn't have been allowed in the first place and appears to have been allowed due to a bug in the nested-impl-trait checking logic.

IOW, the snippet above could be interpreted as:

pub fn collect(_: impl IntoIterator<Item = impl for<T: AsRef<[u8]>> Borrow<Data<T>>>) {
    unimplemented!()
}
@nikomatsakis
Copy link
Contributor

@nikomatsakis nikomatsakis commented Jan 31, 2019

I do not know that we have to discuss in the @rust-lang/lang meeting, but I'm not opposed. I agree with @Centril that this should not have been accepted.

In particular, we decided to forbid nested impl trait of this kind:

fn foo() -> impl Foo<impl Bar> { .. }

Basically there are two hidden variables here, let's call them X and Y, and we must prove that:

X: Foo<Y>
Y: Bar

However, the user is only giving us the return type X. It's true that in some cases, we can infer Y from X, because X only implements Foo for one type (and indeed the compiler does inference of this kind), but I do recall that we intended to forbid this -- in part because such inference is fragile, and there is not necessarily a way for the user to be more explicit should the inference fail (so you could get stuck with no way to port your code forward if, for example, more impls are added to an existing type).

The same seems to apply in this situation. Here there are three impl traits, so we have

X: IntoIterator<Item = Y>
Y: Borrow<Data<Z>>
Z: AsRef<[u8]>

Now, based on X, we can determine Y, but knowing Y does not help us to know Z.

So I would argue that this is a bug fix and we should tag the issue as E-needstest (I'd prefer not to issue a warning period unless we know that this affects a broader swath of code).

@KodrAus
Copy link
Contributor Author

@KodrAus KodrAus commented Jan 31, 2019

This issue is more subtle than the compiler error message leads us to believe:

nested `impl Trait` is not allowed
$ rustc --explain E0666
error: no extended information for E0666

and is a bit perplexing without the explanation @nikomatsakis gave, given it can be worked around by mechanically replacing impl Trait with generics. It wasn't obvious to me that this was footgun prevention (but to be fair if it had worked as-intended right away it would probably be a bit different).

I'd prefer not to issue a warning period unless we know that this affects a broader swath of code

Hmm, I'm not sure how we could reasonably determine that given the dark matter closed-source floating around? It certainly affected our product code. Is there a reason not to issue a warning period?

@joshtriplett
Copy link
Member

@joshtriplett joshtriplett commented Feb 1, 2019

Why can we not support "I return something implementing Iterator where the items implement Foo"? That seems useful.

@Centril
Copy link
Contributor

@Centril Centril commented Feb 1, 2019

@joshtriplett We can and we do (playground):

fn foo() -> impl Iterator<Item = impl core::fmt::Debug> { 0..10 }

This issue is about something else. The distinction is between impl Alpha<impl Gamma> and impl Alpha<Beta = impl Gamma>. The former was never intended to be allowed (for reasons mentioned by @nikomatsakis and myself) and the latter was, is, and will be allowed.

@cramertj
Copy link
Member

@cramertj cramertj commented Feb 1, 2019

+1 to "this was a bug"

nikomatsakis added a commit to Zoxc/rust that referenced this issue Feb 1, 2019
nikomatsakis added a commit to Zoxc/rust that referenced this issue Feb 1, 2019
@KodrAus
Copy link
Contributor Author

@KodrAus KodrAus commented Feb 3, 2019

This isn't just a bug, it's also stabilized behaviour. I'm all for patching this up, but am not so keen on an immediate hard error on the next stable release.

@pnkfelix
Copy link
Member

@pnkfelix pnkfelix commented Feb 14, 2019

is this going to be addressed (in some manner) by PR #57981? Is the manner in which that PR addresses this also "acceptable"?

Its not 100% clear whether a warning-cycle makes sense in this case, versus jumping to an immediate hard error. I suppose it depends on what semantics is being assigned when this code is accepted in the first place.

@pnkfelix
Copy link
Member

@pnkfelix pnkfelix commented Feb 14, 2019

triage: P-high, assigning to self.

@pnkfelix pnkfelix self-assigned this Feb 14, 2019
@Centril Centril removed the T-lang label Feb 14, 2019
TheBiggerGuy added a commit to TheBiggerGuy/rust that referenced this issue Feb 17, 2019
@pnkfelix
Copy link
Member

@pnkfelix pnkfelix commented Feb 19, 2019

@nikomatsakis wrote above:

I do recall that we intended to forbid this -- in part because such inference is fragile, and there is not necessarily a way for the user to be more explicit should the inference fail (so you could get stuck with no way to port your code forward if, for example, more impls are added to an existing type)

In case people want further illustration of this point, here is an example of the above scenario (play):

#![allow(dead_code)]

pub trait Quux { type Assoc; }
pub trait Foo<T> { }
pub trait Bar { }

struct S1;
struct S2;
struct S3;
struct S4;

impl Quux for S1 { type Assoc = S2; }
impl Foo<S3> for S2 { }
impl Bar for S3 { }
impl Bar for S4 { }

// The presence/absence of this line dictates whether
// we get inference failures or not
impl Foo<S4> for S2 { }

pub fn foobar(_: impl Quux<Assoc=impl Foo<impl Bar>>) {

}

fn main() {
    foobar(S1);
}

(and, as far as I know, there's no way to add type annotations to the call site within main to disambiguate whether we want the impl Foo<S3> or impl Foo<S4> for the associated S2 implied by impl Quux for S1.)

@pnkfelix
Copy link
Member

@pnkfelix pnkfelix commented Feb 19, 2019

Having said that, it seems like we at least attempt to do inference when encountering the code in question (at least back when we managed to sneak it pass the compiler's initial static checks).

This leads me to wonder if a warning cycle is at least possible via a relatively small delta to the rustc source code.

@Zoxc
Copy link
Contributor

@Zoxc Zoxc commented Feb 21, 2019

is this going to be addressed (in some manner) by PR #57981?

No. That turned out to be unrelated to this. There was a similar bug in the old NestedImplTraitVisitor where it used walk_ty instead of visit_ty, fixed in #57730.

Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this issue Feb 22, 2019
@bors bors closed this in 4f89846 Feb 25, 2019
@Zoxc
Copy link
Contributor

@Zoxc Zoxc commented Feb 25, 2019

@pnkfelix Did you mean to close #57464 instead there?

@Zoxc Zoxc reopened this Feb 25, 2019
@pnkfelix
Copy link
Member

@pnkfelix pnkfelix commented Feb 26, 2019

@Zoxc oops sorry about that.

@pnkfelix
Copy link
Member

@pnkfelix pnkfelix commented Feb 28, 2019

visiting for triage: if we approve beta-backport of PR #58608, then we'll plan to land that PR and subsequently close this as fixed. If we don't approve beta-backport of PR #58608, then I currently assume we won't land that PR at all, and will instead just close this as an unfortunate "wont fix" (in the sense that we'll just live with the absence of a warning-period)

  • Obviously even if we do not approve a beta backport now, if it subsequently comes to light that a large amount of code does run into this issue, we can then resurrect PR #58608 and backport it as necessary.
@Centril
Copy link
Contributor

@Centril Centril commented Feb 28, 2019

Given that this slipped, I agree with the sentiment that there's little point in a hard error then warning then hard error so I'd say let's not 🔙.

@pnkfelix
Copy link
Member

@pnkfelix pnkfelix commented Mar 5, 2019

@Centril current pre-disposition of compiler team is to go ahead and backport to beta; see #58608 (comment)

But we haven't "finalized" that decision; we decided to leave the PR open a week to give people a chance to object asynchronously to the backport.

So, @Centril, are you actually objecting to a backport of PR #58608 ?

@Centril
Copy link
Contributor

@Centril Centril commented Mar 5, 2019

@pnkfelix Yep; I think it is of little value to make things a hard error and then a warning again and I also think the chance of this occurring in the wild is low due to the inference problems per your notes.

@pnkfelix
Copy link
Member

@pnkfelix pnkfelix commented Mar 7, 2019

(Discussed in @Centril during T-compiler meeting, and clarified that when I said that this "slipped", I meant that the PR didn't manage to make it into nightly in time for the nightly-to-beta lift. @Centril had thought I meant that the bug had already leaked into the stable channel. Once we clarified that, @Centril retracted their objection.)

bors added a commit that referenced this issue Mar 12, 2019
…mpl-trait, r=zoxc

Warning period for detecting nested impl trait

Here is some proposed code for making a warning period for the new checking of nested impl trait.

It undoes some of the corrective effects of PR #57730, by using boolean flags to track parts of the analysis that were previously skipped prior to PRs #57730 and #57981 landing.

Cc #57979
@pnkfelix
Copy link
Member

@pnkfelix pnkfelix commented Mar 18, 2019

As of PRs #58608 and #59235, we now report a future-incompatibility warning (#59014) on nightly and beta for the code reported here.

Closing as "fixed", in the sense that we are going to have a migration period before this becomes a hard error.

@pnkfelix pnkfelix closed this Mar 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
8 participants