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

Normalize projections appearing in `impl Trait` #62221

Merged
merged 2 commits into from Jul 9, 2019

Conversation

@jonas-schievink
Copy link
Member

commented Jun 28, 2019

Fixes #60414

This does not try to do the same for existential types (which have the same bug), since that always seems to lead to cycle errors.

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

commented Jun 28, 2019

r? @cramertj

(rust_highfive has picked a reviewer for you, use r? to override)

@cramertj

This comment has been minimized.

Copy link
Member

commented Jun 28, 2019

@jonas-schievink jonas-schievink force-pushed the jonas-schievink:normalize-impl-trait branch from bb4df01 to 02fe125 Jun 28, 2019

Show resolved Hide resolved src/librustc_typeck/collect.rs Outdated
@bors

This comment was marked as resolved.

Copy link
Contributor

commented Jul 3, 2019

☔️ The latest upstream changes (presumably #61775) made this pull request unmergeable. Please resolve the merge conflicts.

@jonas-schievink jonas-schievink force-pushed the jonas-schievink:normalize-impl-trait branch from 02fe125 to 2969406 Jul 3, 2019

@jonas-schievink

This comment has been minimized.

Copy link
Member Author

commented Jul 3, 2019

So this now fails the test with lifetimes:

error[E0308]: mismatched types
  --> /home/jonas/dev/rust/src/test/ui/impl-trait/bound-normalization-pass.rs:46:56
   |
LL |     fn foo2_pass<'a, T: Trait<'a, Assoc=()> + 'a>() -> impl FooLike<Output=T::Assoc> + 'a {
   |                                                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ lifetime mismatch
   |
   = note: expected type `lifetimes::Trait<'a>`
              found type `lifetimes::Trait<'static>`
note: the lifetime 'a as defined on the function body at 46:18...
  --> /home/jonas/dev/rust/src/test/ui/impl-trait/bound-normalization-pass.rs:46:18
   |
LL |     fn foo2_pass<'a, T: Trait<'a, Assoc=()> + 'a>() -> impl FooLike<Output=T::Assoc> + 'a {
   |                  ^^
   = note: ...does not necessarily outlive the static lifetime

Not sure whether this would be fixed by @nikomatsakis' first suggestion in #60414 (comment)...

@jonas-schievink jonas-schievink force-pushed the jonas-schievink:normalize-impl-trait branch from 2969406 to 2477c65 Jul 4, 2019

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

commented Jul 9, 2019

Hmm, I'm not able to build the compiler at all with these changes. (I have a local clone.) I'm trying a build now from HEAD^ to see which commit causes the problems.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

commented Jul 9, 2019

OK, building from HEAD^ works. I'm going to take a look at that failing test, but my hunch is that we should cut if from the PR and leave it for "future work" :P

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

commented Jul 9, 2019

Applying this diff does indeed make the test pass:

modified   src/test/ui/impl-trait/bound-normalization-pass.rs
@@ -43,12 +43,14 @@ mod lifetimes {
     }

     /// Like above.
-    fn foo2_pass<'a, T: Trait<'a, Assoc=()> + 'a>() -> impl FooLike<Output=T::Assoc> + 'a {
+    fn foo2_pass<'a, T: Trait<'a, Assoc=()> + 'a>(
+    ) -> impl FooLike<Output=<T as Trait<'a>>::Assoc> + 'a {
         Foo(())
     }

     /// Normalization to type containing bound region.
-    fn foo2_pass2<'a, T: Trait<'a, Assoc=&'a ()> + 'a>() -> impl FooLike<Output=T::Assoc> + 'a {
+    fn foo2_pass2<'a, T: Trait<'a, Assoc=&'a ()> + 'a>(
+    ) -> impl FooLike<Output=<T as Trait<'a>>::Assoc> + 'a {
         Foo(&())
     }
 }

Still, I'm not entirely sure why that is needed, given the + 'a that appears.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

commented Jul 9, 2019

OK, I think I know the cause. It is indeed pretty unrelated to this PR and is basically just a bug in the way we are handling the impl trait desugaring. It arises from the hack where we use 'static as the value for our parent's lifetime parameters, so that the T: Trait<'a> constraint in scope becomes T: Trait<'static>.

@nikomatsakis nikomatsakis force-pushed the jonas-schievink:normalize-impl-trait branch from 2477c65 to 66e0266 Jul 9, 2019

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

commented Jul 9, 2019

Ah dang it this is all explained in #51525 =)

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

commented Jul 9, 2019

@jonas-schievink forgive me, but I force pushed to your branch -- I think this PR though is ready to land now.

@jonas-schievink

This comment has been minimized.

Copy link
Member Author

commented Jul 9, 2019

Okay, go ahead! I didn't have Internet since friday so couldn't do any work on this unfortunately.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

commented Jul 9, 2019

@bors r+

@bors

This comment has been minimized.

Copy link
Contributor

commented Jul 9, 2019

📌 Commit 66e0266 has been approved by nikomatsakis

@bors

This comment has been minimized.

Copy link
Contributor

commented Jul 9, 2019

⌛️ Testing commit 66e0266 with merge 0b680cf...

bors added a commit that referenced this pull request Jul 9, 2019

Auto merge of #62221 - jonas-schievink:normalize-impl-trait, r=nikoma…
…tsakis

Normalize projections appearing in `impl Trait`

Fixes #60414

This does not try to do the same for `existential type`s (which have the same bug), since that always seems to lead to cycle errors.
@bors

This comment has been minimized.

Copy link
Contributor

commented Jul 9, 2019

☀️ Test successful - checks-azure, checks-travis, status-appveyor
Approved by: nikomatsakis
Pushing 0b680cf to master...

@bors bors added the merged-by-bors label Jul 9, 2019

@bors bors merged commit 66e0266 into rust-lang:master Jul 9, 2019

4 checks passed

homu Test successful
Details
pr Build #20190709.22 succeeded
Details
pr (Linux mingw-check) Linux mingw-check succeeded
Details
pr (Linux x86_64-gnu-llvm-6.0) Linux x86_64-gnu-llvm-6.0 succeeded
Details
@kpp

This comment has been minimized.

Copy link
Contributor

commented Jul 17, 2019

Now this signature is allowed:

pub fn flatten_stream<Fut, St>(future: Fut) -> impl Stream<Item = Fut::Output>
    where Fut: Future<Output = St>,
          St: Stream<Item = Fut::Output>,
{

(kpp/futures-async-combinators@06702ec#diff-7afe74b649f99a176a8b54176b0859a8R105)

Whereas is was impossible in the previous releases.

And this looks as a bug to me.

@jonas-schievink

This comment has been minimized.

Copy link
Member Author

commented Jul 17, 2019

@kpp On first glance, that looks like an expected outcome of this. Where do you see the bug?

This equivalent example fails to compile due to ICEs on stable and beta, which this PR fixes:

pub trait Stream {
    type Item;
}

pub trait Future {
    type Output;
}

struct MyStream<T> {
    v: Vec<T>,
}

impl<T> Stream for MyStream<T> {
    type Item = T;
}

pub fn flatten_stream<Fut, St>(future: Fut) -> impl Stream<Item = Fut::Output>
    where Fut: Future<Output = St>,
          St: Stream<Item = Fut::Output>,
{
    MyStream {
        v: Vec::new(),
    }
}
internal compiler error: broken MIR in DefId(0:22 ~ playground[abf7]::flatten_stream[0]) (Terminator { source_info: SourceInfo { span: src/lib.rs:22:12: 22:22, scope: scope[0] }, kind: _2 = const std::vec::Vec::<T>::new() -> [return: bb2, unwind: bb3] }): call dest mismatch (std::vec::Vec<<Fut as Future>::Output> <- std::vec::Vec<St>): NoSolution

In particular: call dest mismatch (std::vec::Vec<<Fut as Future>::Output> <- std::vec::Vec<St>): NoSolution. The added normalization fixes this mismatch.

@kpp

This comment has been minimized.

Copy link
Contributor

commented Jul 17, 2019

Where do you see the bug?

I got the following error on nightly-2019-07-06 with the signature above:

error[E0271]: type mismatch resolving `<impl core::future::future::Future as core::future::future::Future>::Output == std::option::Option<(<Fut as core::future::future::Future>::Output, (std::option::Option<Fut>, std::option::Option<std::pin::Pin<std::boxed::Box<St>>>))>`
  --> src/future.rs:99:48
   |
99 | pub fn flatten_stream<Fut, St>(future: Fut) -> impl Stream<Item = Fut::Output>
   |                                                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected type parameter, found associated type
   |
   = note: expected type `std::option::Option<(St, (std::option::Option<Fut>, std::option::Option<std::pin::Pin<std::boxed::Box<St>>>))>`
              found type `std::option::Option<(<Fut as core::future::future::Future>::Output, (std::option::Option<Fut>, std::option::Option<std::pin::Pin<std::boxed::Box<St>>>))>`
   = note: required because of the requirements on the impl of `futures_core::stream::Stream` for `futures_util::stream::unfold::Unfold<(std::option::Option<Fut>, std::option::Option<std::pin::Pin<std::boxed::Box<St>>>), [closure@src/future.rs:104:51: 118:6], impl core::future::future::Future>`
   = note: the return type of a function must have a statically known size

error: aborting due to previous error
@jonas-schievink

This comment has been minimized.

Copy link
Member Author

commented Jul 17, 2019

That does look expected to me.

In the type mismatch error here you can see that the <Fut as core::future::future::Future>::Output did not get normalized to St properly, but now does ("expected" vs. "found" is swapped around here, that's another old bug with impl Trait).

   = note: expected type `std::option::Option<(St, (std::option::Option<Fut>, std::option::Option<std::pin::Pin<std::boxed::Box<St>>>))>`
              found type `std::option::Option<(<Fut as core::future::future::Future>::Output, (std::option::Option<Fut>, std::option::Option<std::pin::Pin<std::boxed::Box<St>>>))>`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.