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

Backwards propagation of types leads to unhelpful errors [futures] #46606

Open
mqudsi opened this issue Dec 9, 2017 · 7 comments
Open

Backwards propagation of types leads to unhelpful errors [futures] #46606

mqudsi opened this issue Dec 9, 2017 · 7 comments
Labels
A-diagnostics Area: Messages for errors, warnings, and lints C-enhancement Category: An issue proposing an enhancement or a PR with one. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@mqudsi
Copy link
Contributor

mqudsi commented Dec 9, 2017

In general, and I genuinely wish I had a citation for this, developers likely tend to write their software going from input to output, even if the output is the eventual, fixed "goal" they would like to reach.

For example, the code here (git checkout available): https://git.neosmart.net/mqudsi/futuretest/src/rust-46606

    let f = future::result(Ok(()))
        .map_err(|()| "&'static str error")
        .map(|_| future::result(Err("another &'static str error")))
        .and_then(|_|
             future::result(Ok(())
                .map_err(|()| "String error".to_owned())
            )
        )
    ;

Two conflicting error types are mapped, the first is to &'static str and the second is to String, the compiler output indicates an error with the &'static str type instead of the String type usage:

   Compiling futuretest v0.1.0 (file:///mnt/c/Users/Mahmoud/git/futuretest)
error[E0271]: type mismatch resolving `<futures::FutureResult<(), std::string::String> as futures::IntoFuture>::Error == &str`
  --> src/main.rs:13:10
   |
13 |         .and_then(|_|
   |          ^^^^^^^^ expected struct `std::string::String`, found &str
   |
   = note: expected type `std::string::String`
              found type `&str`

error[E0271]: type mismatch resolving `<futures::FutureResult<(), std::string::String> as futures::IntoFuture>::Error == &str`
  --> src/main.rs:20:10
   |
20 |     core.run(f).unwrap();
   |          ^^^ expected struct `std::string::String`, found &str
   |
   = note: expected type `std::string::String`
              found type `&str`
   = note: required because of the requirements on the impl of `futures::Future` for `futures::AndThen<futures::Map<futures::MapErr<futures::FutureResult<(), ()>, [closure@src/main.rs:11:18: 11:43]>, [closure@src/main.rs:12:14: 12:67]>, futures::FutureResult<(), std::string::String>, [closure@src/main.rs:13:19: 16:14]>`

error: aborting due to 2 previous errors

error: Could not compile `futuretest`.

To learn more, run the command again with --verbose.

In an ideal situation, all types would match up and it wouldn't matter which line the type inference began with. But in the event of a type mismatch, such as here, the reported errors are based off the backwards type propagation, which doesn't seem right (opinion) and leads to an unhelpful error message (fact).

(To make the case a bit clearer, I included several uses of the correct type followed by a final usage of the incorrect type).

@mqudsi
Copy link
Contributor Author

mqudsi commented Dec 9, 2017

cc @alexcrichton for futures-rs

@arielb1
Copy link
Contributor

arielb1 commented Dec 9, 2017

So we are pretty much not going to be doing trait-based expected type propagation, which would be required to give an error on the call to to_owned (the IntoFuture part already breaks this - no closures needed I think).

@pietroalbini pietroalbini added C-enhancement Category: An issue proposing an enhancement or a PR with one. A-diagnostics Area: Messages for errors, warnings, and lints labels Jan 23, 2018
@crlf0710 crlf0710 added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jun 11, 2020
@estebank
Copy link
Contributor

Building this repo today gives

error[E0277]: the trait bound `(): futures::Future` is not satisfied
  --> src/main.rs:17:14
   |
17 | fn test() -> MapErr<(), ()> {
   |              ^^^^^^^^^^^^^^ the trait `futures::Future` is not implemented for `()`
   |
  ::: /Users/ekuber/.cargo/registry/src/github.com-1ecc6299db9ec823/futures-0.1.17/src/future/map_err.rs:8:34
   |
8  | pub struct MapErr<A, F> where A: Future {
   |                                  ------ required by this bound in `futures::MapErr`

which is probably because it didn't clamp the crate versions.

Changing the Cargo.toml to use futures 0.1.17 and tokio-core 0.1.11 gives me the same error, but looking at the Cargo.lock newer versions of those crates are being pulled 🤷‍♀️

@estebank estebank added the E-needs-mcve Call for participation: This issue has a repro, but needs a Minimal Complete and Verifiable Example label Feb 13, 2021
@mqudsi
Copy link
Contributor Author

mqudsi commented Feb 13, 2021

Ok, this confused me too. Then I realized back in 2017 that I had one remote repo for all tokio-related bugs and different branches for each. The error you are quoting is from the master branch, if you check out rust-46606 you get the correct errors. The floated versions in Cargo.toml aren't an issue because Cargo.lock is included in the repo.

I don't do this any more, but it seems that at the time I had a Makefile that captured the output of cargo build and saved it the repo as cargo.out, which means I have the exact diff of the output rustc produced in 2017 as compared to the output of 1.51-nightly:

diff --git a/cargo.out b/cargo.out
index d2607d1..2735df2 100644
--- a/cargo.out
+++ b/cargo.out
@@ -1,25 +1,21 @@
-   Compiling futuretest v0.1.0 (file:///mnt/c/Users/Mahmoud/git/futuretest)
-error[E0271]: type mismatch resolving `<futures::FutureResult<(), std::string::String> as futures::IntoFuture>::Error == &str`
+   Compiling futuretest v0.1.0 (/mnt/d/GIT/futuretest)
+error[E0271]: type mismatch resolving `<Failed<(), String> as futures::IntoFuture>::Error == &str`
   --> src/main.rs:13:10
    |
 13 |         .and_then(|_|
-   |          ^^^^^^^^ expected struct `std::string::String`, found &str
-   |
-   = note: expected type `std::string::String`
-              found type `&str`
+   |          ^^^^^^^^ expected struct `String`, found `&str`

-error[E0271]: type mismatch resolving `<futures::FutureResult<(), std::string::String> as futures::IntoFuture>::Error == &str`
+error[E0271]: type mismatch resolving `<Failed<(), String> as futures::IntoFuture>::Error == &str`
   --> src/main.rs:20:10
    |
 20 |     core.run(f).unwrap();
-   |          ^^^ expected struct `std::string::String`, found &str
+   |          ^^^ expected struct `String`, found `&str`
    |
-   = note: expected type `std::string::String`
-              found type `&str`
-   = note: required because of the requirements on the impl of `futures::Future` for `futures::AndThen<futures::Map<futures::MapErr<futures::FutureResult<(), ()>, [closure@src/main.rs:11:18: 11:43]>, [closure@src/main.rs:12:14: 12:67]>, futures::FutureResult<(), std::string::String>, [closure@src/main.rs:13:19: 16:14]>`
+   = note: required because of the requirements on the impl of `futures::Future` for `futures::AndThen<futures::Map<futures::MapErr<Failed<(), ()>, [closure@src/main.rs:11:18: 11:43]>, [closure@src/main.rs:12:14: 12:67]>, Failed<(), String>, [closure@src/main.rs:13:19: 16:14]>`

 error: aborting due to 2 previous errors

-error: Could not compile `futuretest`.
+For more information about this error, try `rustc --explain E0271`.
+error: could not compile `futuretest`

@estebank
Copy link
Contributor

It's a shame to see that the error wasn't improved by any of the changes we've made since then, but at least this is now triaged. Thank you for looking back at what you did 4 years ago to clarify this!

@mqudsi
Copy link
Contributor Author

mqudsi commented Feb 14, 2021

No problem. I guess we can remove E-needs-mcve?

@rustbot label -E-needs-cve

@CraftSpider
Copy link
Contributor

CraftSpider commented Mar 20, 2023

Minimized example with almost identical first diagnostic (removed second, as it's basically a duplicate):

pub trait Foo {
    type Assoc;

    fn foo<F, B>(&self, _: F)
    where
        F: FnOnce() -> B,
        B: Foo<Assoc = Self::Assoc>,
    {}
}

impl<T> Foo for T {
    type Assoc = T;
}

fn item<T>(item: T) -> T {
    item
}

fn main() {
    item("&'static str error")
        .foo(|| "String error".to_owned());
}

This includes a note about type Assoc = T expected to be &str that doesn't show up while building the original reproducer locally, but seems to show up in the playground even when all relevant generics are copied exactly - I suspect something to do with local crate bounds?

Some variants without the FnOnce indirection now add diagnostics pointing at associated types and without the closure, the expression turning a &str into a String. (I think due to #105332)

@jyn514 jyn514 removed the E-needs-mcve Call for participation: This issue has a repro, but needs a Minimal Complete and Verifiable Example label Mar 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-diagnostics Area: Messages for errors, warnings, and lints C-enhancement Category: An issue proposing an enhancement or a PR with one. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

7 participants