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

Delegate Iterator::try_fold for Iterators taken by reference #62483

Open
wants to merge 6 commits into
base: master
from

Conversation

@czipperz
Copy link
Contributor

commented Jul 7, 2019

try_fold is the most important method to delegate because all the others delegate to it when possible. (ie any and the like)

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

commented Jul 7, 2019

r? @KodrAus

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

@czipperz

This comment has been minimized.

Copy link
Contributor Author

commented Jul 8, 2019

If the dang Self: Sized requirement for try_fold wasn't there this would work dang. Why is that a requirement?

@czipperz

This comment has been minimized.

Copy link
Contributor Author

commented Jul 8, 2019

^It's required for trait objects to work

@czipperz czipperz closed this Jul 8, 2019

@czipperz czipperz reopened this Jul 12, 2019

@czipperz

This comment has been minimized.

Copy link
Contributor Author

commented Jul 12, 2019

Trying explicitly specializing ?Sized and Sized differently. Then the sized one can delegate.

@KodrAus

This comment has been minimized.

Copy link
Contributor

commented Jul 15, 2019

Thanks for the PR @czipperz!

It looks like our build failure is because some of the UI tests are producing different errors with this change:

failures:

---- [ui] ui/issues/issue-31173.rs stdout ----
diff of stderr:

14	   |          ^^^^^^^
15	   |
16	   = note: the method `collect` exists but the following trait bounds were not satisfied:
-	           `&mut std::iter::Cloned<std::iter::TakeWhile<&mut std::vec::IntoIter<u8>, [closure@$DIR/issue-31173.rs:6:39: 9:6 found_e:_]>> : std::iter::Iterator`
18	           `std::iter::Cloned<std::iter::TakeWhile<&mut std::vec::IntoIter<u8>, [closure@$DIR/issue-31173.rs:6:39: 9:6 found_e:_]>> : std::iter::Iterator`
19	
20	error: aborting due to 2 previous errors


The actual stderr differed from the expected stderr.
Actual stderr saved to /checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/issues/issue-31173/issue-31173.stderr
To update references, rerun the tests and pass the `--bless` flag
To only update this specific test, also pass `--test-args issues/issue-31173.rs`

error: 1 errors occurred comparing output.
status: exit code: 1
command: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "/checkout/src/test/ui/issues/issue-31173.rs" "-Zthreads=1" "--target=x86_64-unknown-linux-gnu" "--error-format" "json" "-Zui-testing" "-C" "prefer-dynamic" "--out-dir" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/issues/issue-31173" "-Crpath" "-O" "-Cdebuginfo=0" "-Zunstable-options" "-Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/issues/issue-31173/auxiliary" "-A" "unused"
stdout:
------------------------------------------

------------------------------------------
stderr:
------------------------------------------
error[E0271]: type mismatch resolving `<std::iter::TakeWhile<&mut std::vec::IntoIter<u8>, [closure@/checkout/src/test/ui/issues/issue-31173.rs:6:39: 9:6 found_e:_]> as std::iter::Iterator>::Item == &_`
  --> /checkout/src/test/ui/issues/issue-31173.rs:10:10
   |
LL |         .cloned()
   |          ^^^^^^ expected u8, found reference
   |
   = note: expected type `u8`
              found type `&_`

error[E0599]: no method named `collect` found for type `std::iter::Cloned<std::iter::TakeWhile<&mut std::vec::IntoIter<u8>, [closure@/checkout/src/test/ui/issues/issue-31173.rs:6:39: 9:6 found_e:_]>>` in the current scope
  --> /checkout/src/test/ui/issues/issue-31173.rs:14:10
   |
LL |         .collect(); //~ ERROR no method named `collect`
   |          ^^^^^^^
   |
   = note: the method `collect` exists but the following trait bounds were not satisfied:
           `std::iter::Cloned<std::iter::TakeWhile<&mut std::vec::IntoIter<u8>, [closure@/checkout/src/test/ui/issues/issue-31173.rs:6:39: 9:6 found_e:_]>> : std::iter::Iterator`

error: aborting due to 2 previous errors

Some errors have detailed explanations: E0271, E0599.
For more information about an error, try `rustc --explain E0271`.

------------------------------------------


---- [ui] ui/methods/method-call-err-msg.rs stdout ----
diff of stderr:

34	LL |      .take()
35	   |       ^^^^
36	   |
-	   = note: the method `take` exists but the following trait bounds were not satisfied:
-	           `&mut Foo : std::iter::Iterator`
39	   = help: items from traits can only be used if the trait is implemented and in scope
40	   = note: the following traits define an item `take`, perhaps you need to implement one of them:
41	           candidate #1: `std::io::Read`


The actual stderr differed from the expected stderr.
Actual stderr saved to /checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/methods/method-call-err-msg/method-call-err-msg.stderr
To update references, rerun the tests and pass the `--bless` flag
To only update this specific test, also pass `--test-args methods/method-call-err-msg.rs`

error: 1 errors occurred comparing output.
status: exit code: 1
command: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "/checkout/src/test/ui/methods/method-call-err-msg.rs" "-Zthreads=1" "--target=x86_64-unknown-linux-gnu" "--error-format" "json" "-Zui-testing" "-C" "prefer-dynamic" "--out-dir" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/methods/method-call-err-msg" "-Crpath" "-O" "-Cdebuginfo=0" "-Zunstable-options" "-Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/methods/method-call-err-msg/auxiliary" "-A" "unused"
stdout:
------------------------------------------

------------------------------------------
stderr:
------------------------------------------
error[E0061]: this function takes 0 parameters but 1 parameter was supplied
  --> /checkout/src/test/ui/methods/method-call-err-msg.rs:12:7
   |
LL |     fn zero(self) -> Foo { self }
   |     -------------------- defined here
...
LL |     x.zero(0)   //~ ERROR this function takes 0 parameters but 1 parameter was supplied
   |       ^^^^ expected 0 parameters

error[E0061]: this function takes 1 parameter but 0 parameters were supplied
  --> /checkout/src/test/ui/methods/method-call-err-msg.rs:13:7
   |
LL |     fn one(self, _: isize) -> Foo { self }
   |     ----------------------------- defined here
...
LL |      .one()     //~ ERROR this function takes 1 parameter but 0 parameters were supplied
   |       ^^^ expected 1 parameter

error[E0061]: this function takes 2 parameters but 1 parameter was supplied
  --> /checkout/src/test/ui/methods/method-call-err-msg.rs:14:7
   |
LL |     fn two(self, _: isize, _: isize) -> Foo { self }
   |     --------------------------------------- defined here
...
LL |      .two(0);   //~ ERROR this function takes 2 parameters but 1 parameter was supplied
   |       ^^^ expected 2 parameters

error[E0599]: no method named `take` found for type `Foo` in the current scope
  --> /checkout/src/test/ui/methods/method-call-err-msg.rs:18:7
   |
LL | pub struct Foo;
   | --------------- method `take` not found for this
...
LL |      .take()    //~ ERROR no method named `take` found for type `Foo` in the current scope
   |       ^^^^
   |
   = help: items from traits can only be used if the trait is implemented and in scope
   = note: the following traits define an item `take`, perhaps you need to implement one of them:
           candidate #1: `std::io::Read`
           candidate #2: `std::iter::Iterator`

error: aborting due to 4 previous errors

Some errors have detailed explanations: E0061, E0599.
For more information about an error, try `rustc --explain E0061`.

------------------------------------------


---- [ui] ui/mismatched_types/issue-36053-2.rs stdout ----
diff of stderr:

5	   |                                                       ^^^^^
6	   |
7	   = note: the method `count` exists but the following trait bounds were not satisfied:
-	           `&mut std::iter::Filter<std::iter::Fuse<std::iter::Once<&str>>, [closure@$DIR/issue-36053-2.rs:7:39: 7:53]> : std::iter::Iterator`
9	           `std::iter::Filter<std::iter::Fuse<std::iter::Once<&str>>, [closure@$DIR/issue-36053-2.rs:7:39: 7:53]> : std::iter::Iterator`
10	
11	error[E0631]: type mismatch in closure arguments


The actual stderr differed from the expected stderr.
Actual stderr saved to /checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/mismatched_types/issue-36053-2/issue-36053-2.stderr
To update references, rerun the tests and pass the `--bless` flag
To only update this specific test, also pass `--test-args mismatched_types/issue-36053-2.rs`

error: 1 errors occurred comparing output.
status: exit code: 1
command: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "/checkout/src/test/ui/mismatched_types/issue-36053-2.rs" "-Zthreads=1" "--target=x86_64-unknown-linux-gnu" "--error-format" "json" "-Zui-testing" "-C" "prefer-dynamic" "--out-dir" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/mismatched_types/issue-36053-2" "-Crpath" "-O" "-Cdebuginfo=0" "-Zunstable-options" "-Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/mismatched_types/issue-36053-2/auxiliary" "-A" "unused"
stdout:
------------------------------------------

------------------------------------------
stderr:
------------------------------------------
error[E0599]: no method named `count` found for type `std::iter::Filter<std::iter::Fuse<std::iter::Once<&str>>, [closure@/checkout/src/test/ui/mismatched_types/issue-36053-2.rs:7:39: 7:53]>` in the current scope
  --> /checkout/src/test/ui/mismatched_types/issue-36053-2.rs:7:55
   |
LL |     once::<&str>("str").fuse().filter(|a: &str| true).count();
   |                                                       ^^^^^
   |
   = note: the method `count` exists but the following trait bounds were not satisfied:
           `std::iter::Filter<std::iter::Fuse<std::iter::Once<&str>>, [closure@/checkout/src/test/ui/mismatched_types/issue-36053-2.rs:7:39: 7:53]> : std::iter::Iterator`

error[E0631]: type mismatch in closure arguments
  --> /checkout/src/test/ui/mismatched_types/issue-36053-2.rs:7:32
   |
LL |     once::<&str>("str").fuse().filter(|a: &str| true).count();
   |                                ^^^^^^ -------------- found signature of `for<'r> fn(&'r str) -> _`
   |                                |
   |                                expected signature of `for<'r> fn(&'r &str) -> _`

error[E0631]: type mismatch in closure arguments
  --> /checkout/src/test/ui/mismatched_types/issue-36053-2.rs:7:32
   |
LL |     once::<&str>("str").fuse().filter(|a: &str| true).count();
   |                                ^^^^^^ -------------- found signature of `for<'r> fn(&'r str) -> _`
   |                                |
   |                                expected signature of `fn(&&str) -> _`

error: aborting due to 3 previous errors

For more information about this error, try `rustc --explain E0599`.

------------------------------------------



failures:
    [ui] ui/issues/issue-31173.rs
    [ui] ui/methods/method-call-err-msg.rs
    [ui] ui/mismatched_types/issue-36053-2.rs

Are you able to run the build locally, or would you like me to take a look at how they've changed?

@Alexendoo

This comment has been minimized.

Copy link
Member

commented Jul 24, 2019

Ping from triage, any updates? @czipperz

@czipperz

This comment has been minimized.

Copy link
Contributor Author

commented Jul 27, 2019

Should be fixed now.

@czipperz czipperz force-pushed the czipperz:delegate-iterator-try_fold-for-by_ref branch from 060c290 to 0ecda3d Jul 27, 2019

@KodrAus

This comment has been minimized.

Copy link
Contributor

commented Jul 28, 2019

Thanks @czipperz!

It looks like we do already have a few cases where we use specialization in iterators, and this one seems possible to back out of. I haven't been very closely following specialization though, so I'm on board with this so long as it falls within the currently expected sound implementation of it.

cc @rust-lang/libs can anyone see any problems with doing this?

@alexcrichton

This comment has been minimized.

Copy link
Member

commented Jul 29, 2019

We've exposed specialization features before for various performance optimizations here and there, but it's intended to currently always be done in such a way that it doesn't expose to users of libstd what's default and what's not. This is changing a critical blanket impl in the standard library and I'm not sure I'd be comfortable leveraging specialization that much.

Is there perhaps compelling motivation for why we should land this even if it's somewhat risky from a specialization point of view?

@KodrAus

This comment has been minimized.

Copy link
Contributor

commented Jul 30, 2019

Thanks for the explanation @alexcrichton! I thought this may be a little risky, but it sounds like it’s quite risky.

I guess the motivation here is to correctly delegate to try_fold in mutable iterators when it’s overridden. But since you can’t override it on stable, and we can’t support this without risky specialization it doesn’t seem like the right time to make this sort of change.

@alexcrichton

This comment has been minimized.

Copy link
Member

commented Jul 30, 2019

Ah ok and to be clear I don't know of where this can go wrong, but it feels risky to be changing such a core implementation to be leveraging specialization for what can be presumably a somewhat critical performance concern.

I do agree though that we probably want to wait on this for now in terms of performance, but this could also use the pattern of other specialization traits which is to use an internal trait to specialize rather than using the external one.

@timvermeulen

This comment has been minimized.

Copy link
Contributor

commented Aug 1, 2019

I don't fully understand why specialization is fine using a private trait but not using a public trait, what makes the two different? Is removing specialization from a public trait a breaking change (for stable users) in a way that I haven't thought of?

@alexcrichton

This comment has been minimized.

Copy link
Member

commented Aug 2, 2019

Functionally it ends up having the same goal, but it's slightly different from a public API perspective because it continue to now allow external users to override the implementation. (even on nightly)

I mostly wanted to point out that's what's been done before. I'm not necessarily sure I'd be willing to commit personally to say it's ok in this case, I still sort of feel like this is such a core impl that it seems weird to have this and only single out try_fold and such.

@JohnCSimon

This comment has been minimized.

Copy link

commented Aug 10, 2019

Ping from triage - @czipperz @KodrAus Is this close to a resolution?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.