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

Rust 1.66 broke warp impl Reply on return types #107729

Open
jxs opened this issue Feb 6, 2023 · 10 comments
Open

Rust 1.66 broke warp impl Reply on return types #107729

jxs opened this issue Feb 6, 2023 · 10 comments
Labels
C-bug Category: This is a bug. P-medium Medium priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@jxs
Copy link
Contributor

jxs commented Feb 6, 2023

Hi there,
we just noticed on warp CI that one of the examples was broken (we have #![deny(warnings)] for the examples). The output is as follows:

warning: opaque type `impl warp::Filter + warp::filter::FilterBase<Extract = impl Reply, Error = Rejection> + Clone` does not satisfy its associated type bounds
  --> examples/todos.rs:39:22
   |
39 |     ) -> impl Filter<Extract = impl warp::Reply, Error = warp::Rejection> + Clone {
   |                      ^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
  ::: /home/jxs/dev/oss/warp/src/filter/mod.rs:40:19
   |
40 |     type Extract: Tuple; // + Send;
   |                   ----- this associated type bound is unsatisfied for `impl Reply`
   |
   = note: `#[warn(opaque_hidden_inferred_bound)]` on by default
help: add this bound
   |
39 |     ) -> impl Filter<Extract = impl warp::Reply + warp::generic::Tuple, Error = warp::Rejection> + Clone {
   |                                                 ++++++++++++++++++++++

warning: opaque type `impl warp::Filter + warp::filter::FilterBase<Extract = impl Reply, Error = Rejection> + Clone` does not satisfy its associated type bounds
  --> examples/todos.rs:49:22
   |
49 |     ) -> impl Filter<Extract = impl warp::Reply, Error = warp::Rejection> + Clone {
   |                      ^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
  ::: /home/jxs/dev/oss/warp/src/filter/mod.rs:40:19
   |
40 |     type Extract: Tuple; // + Send;
   |                   ----- this associated type bound is unsatisfied for `impl Reply`
   |
help: add this bound
   |
49 |     ) -> impl Filter<Extract = impl warp::Reply + warp::generic::Tuple, Error = warp::Rejection> + Clone {
   |                                                 ++++++++++++++++++++++

warning: opaque type `impl warp::Filter + warp::filter::FilterBase<Extract = impl Reply, Error = Rejection> + Clone` does not satisfy its associated type bounds
  --> examples/todos.rs:60:22
   |
60 |     ) -> impl Filter<Extract = impl warp::Reply, Error = warp::Rejection> + Clone {
   |                      ^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
  ::: /home/jxs/dev/oss/warp/src/filter/mod.rs:40:19
   |
40 |     type Extract: Tuple; // + Send;
   |                   ----- this associated type bound is unsatisfied for `impl Reply`
   |
help: add this bound
   |
60 |     ) -> impl Filter<Extract = impl warp::Reply + warp::generic::Tuple, Error = warp::Rejection> + Clone {
   |                                                 ++++++++++++++++++++++

warning: opaque type `impl warp::Filter + warp::filter::FilterBase<Extract = impl Reply, Error = Rejection> + Clone` does not satisfy its associated type bounds
  --> examples/todos.rs:71:22
   |
71 |     ) -> impl Filter<Extract = impl warp::Reply, Error = warp::Rejection> + Clone {
   |                      ^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
  ::: /home/jxs/dev/oss/warp/src/filter/mod.rs:40:19
   |
40 |     type Extract: Tuple; // + Send;
   |                   ----- this associated type bound is unsatisfied for `impl Reply`
   |
help: add this bound
   |
71 |     ) -> impl Filter<Extract = impl warp::Reply + warp::generic::Tuple, Error = warp::Rejection> + Clone {
   |                                                 ++++++++++++++++++++++

warning: opaque type `impl warp::Filter + warp::filter::FilterBase<Extract = impl Reply, Error = Rejection> + Clone` does not satisfy its associated type bounds
  --> examples/todos.rs:82:22
   |
82 |     ) -> impl Filter<Extract = impl warp::Reply, Error = warp::Rejection> + Clone {
   |                      ^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
  ::: /home/jxs/dev/oss/warp/src/filter/mod.rs:40:19
   |
40 |     type Extract: Tuple; // + Send;
   |                   ----- this associated type bound is unsatisfied for `impl Reply`
   |
help: add this bound
   |
82 |     ) -> impl Filter<Extract = impl warp::Reply + warp::generic::Tuple, Error = warp::Rejection> + Clone {
   |                                                 ++++++++++++++++++++++

It seems to have been introduced with Rust version 1.66 as with 1.65 the example compiles fine. I also tried with the latest nightly (cargo 1.69.0-nightly (e84a7928d 2023-01-31)) which still reproduces the problem. This feels like a bug as it being a warning doesn't provide help, btw warp::generic::Tuple is private.
The todos example should work as a MRE, but if required I can try to provide a simpler use case.

Thanks!

@jxs jxs added the C-bug Category: This is a bug. label Feb 6, 2023
@clubby789
Copy link
Contributor

searched nightlies: from nightly-2022-10-01 to nightly-2023-01-01
regressed nightly: nightly-2022-10-05
searched commit range: f83e026...01af504
regressed commit: 02cd79a

bisected with cargo-bisect-rustc v0.6.5

Host triple: x86_64-unknown-linux-gnu
Reproduce with:

cargo bisect-rustc 2022-10-01 --end 2023-01-01 

The lint was introduced in #102568

@compiler-errors
Copy link
Member

compiler-errors commented Feb 6, 2023

Hey, yeah so I authored that warning. Sorry that it's so vague.

So the gist is that Extract requires the associate type to implement Tuple, but impl Reply doesn't implement Tuple though, at least not the public-facing side of the type. This is code that only compiles due to a hack in the compiler. We could downgrade the lint, I guess, but that just masks the issue that the impl Filter<...> still is not "valid".

The suggestion kinda sucks, because yeah, Tuple is unreachable, but the underlying issue still remains I think.

cc @oli-obk, maybe you have additional or different thoughts about this lint.

@apiraino apiraino added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. regression-from-stable-to-stable Performance or correctness regression from one stable version to another. labels Feb 6, 2023
@rustbot rustbot added the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Feb 6, 2023
@oli-obk oli-obk removed the regression-from-stable-to-stable Performance or correctness regression from one stable version to another. label Feb 7, 2023
@jonas-schievink
Copy link
Contributor

Hmm, why should code like this be rejected? The caller knows that the opaque type meets the bounds because the bound is on the associated type in the first place (and the function compiles, so whatever type it used there must meet the bound).

If anything it's weird that the equivalent -> RequiresTuple<impl Reply> doesn't compile, no? (where struct RequiresTuple<T: Tuple>(T);)

@oli-obk
Copy link
Contributor

oli-obk commented Feb 7, 2023

It makes sense from inside the body of the function, but from the outside it is a bit weird: impl Reply does not implement Tuple (since Tuple is not a super trait of Reply), so the bound on the RequiresTuple struct doesn't hold.

@apiraino
Copy link
Contributor

WG-prioritization assigning priority (Zulip discussion).

@rustbot label -I-prioritize +P-medium

@rustbot rustbot added P-medium Medium priority and removed I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Feb 14, 2023
@ThomasHabets
Copy link

I encountered this warning. Everything seems to be working, but I want to fix the warning.

Could someone explain how to fix this, as if I've only used Rust for a week and this is happening in the first real program I write in it?

@compiler-errors
Copy link
Member

compiler-errors commented Feb 20, 2023

You can replace Extract = impl warp::Reply with Extract = (impl warp::Reply,), I think.

See: seanmonstar/warp#1020 (comment)

@ThomasHabets
Copy link

Indeed that gets rid of the warning. Thanks! I would not have guessed this solution.

@olix0r
Copy link

olix0r commented Feb 27, 2023

May I ask why this warning is desirable? Let's say I have a function like

fn svc() -> impl Service<Request<Body>, Response = Response<Body>, Error = Error, Future = impl Send> {
  // ...
}

This warning prefers this as:

fn svc() -> impl Service<Request<Body>, Response = Response<Body>, Error = Error, Future = impl Future<Output = Result<Response<Body>, Error>> + Send> {
  // ...
}

This feels like needless boilerplate in our application -- it's obvious that the Future type attribute has to implement Future. Why is it desirable that this be made explicit everywhere the type is referenced?

@olix0r
Copy link

olix0r commented Feb 27, 2023

Actually, what I wrote above is not correct. We are apparently unable to satisfy the proper Future bounds?

See @hawkw's comment on linkerd/linkerd2-proxy#2275:

Unfortunately, we cannot simply change our code to make the trait bound's type explicit, as changing impl Send to
impl Future<...> + Send in this position results in a surprising error which I don't think is correct:

error[E0277]: `impl std::marker::Send` is not a future
  --> linkerd/app/outbound/src/http/logical.rs:97:30
   |
97 |                     Future = impl Future<Output = Result<http::Response<http::BoxBody>, Error>> + Send,
   |                              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ `impl std::marker::Send` is not a future
   |
   = help: the trait `futures::Future` is not implemented for `impl std::marker::Send`
   = note: impl std::marker::Send must be a future or must implement `IntoFuture` to be awaited
   = note: required for `stack::map_err::ResponseFuture<(), impl std::marker::Send>` to implement `futures::Future`

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

See linkerd/linkerd2-proxy#2268 (comment) for details.

hawkw added a commit to linkerd/linkerd2-proxy that referenced this issue Feb 27, 2023
Currently, our nightly builds are failing due to the new warning
`opaque_hidden_inferred_bound`, which triggers when an opaque type
(`impl Trait`) in an associated type position does not explicitly
include the associated type's trait bounds (e.g. returning a
`Service<Future = impl Send, ...>`) or similar.

Unfortunately, we cannot simply change our code to make the trait
bound's type explicit, as changing `impl Send` to `impl Future<...> +
Send` in this position results in a surprising error which I don't think
is correct:

```
error[E0277]: `impl std::marker::Send` is not a future
  --> linkerd/app/outbound/src/http/logical.rs:97:30
   |
97 |                     Future = impl Future<Output = Result<http::Response<http::BoxBody>, Error>> + Send,
   |                              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ `impl std::marker::Send` is not a future
   |
   = help: the trait `futures::Future` is not implemented for `impl std::marker::Send`
   = note: impl std::marker::Send must be a future or must implement `IntoFuture` to be awaited
   = note: required for `stack::map_err::ResponseFuture<(), impl std::marker::Send>` to implement `futures::Future`

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

See
#2268 (comment)
as well as the upstream Rust issue rust-lang/rust#107729, for details.

This should probably be reported on the Rust issue tracker, since a
warning that's (apparently) impossible to fix seems not great. However,
for now, we can simply allow this warning for our nightly builds.

This should fix CI.
roger-oliver pushed a commit to roger-oliver/qa-api that referenced this issue Apr 10, 2023
added controllers and routers to access login and registration mocks;
fixed the warn on "impl Reply" [rust-lang/rust#107729];
roger-oliver pushed a commit to roger-oliver/rust-question-api that referenced this issue Apr 10, 2023
roger-oliver pushed a commit to roger-oliver/rust-question-api that referenced this issue Apr 10, 2023
commit 50a2682
Author: Rogerio Oliveira <roliveira@cyclomedia.com>
Date:   Tue Apr 11 00:24:02 2023 +0200

    removed some pub modifiers from internal functions;
    fixed the warn on "impl Reply" [rust-lang/rust#107729];

Changes to be committed:
	modified:   mock-server/src/lib.rs
	modified:   src/lib.rs
	modified:   src/routes/authentication.rs
roger-oliver pushed a commit to roger-oliver/qa-api that referenced this issue Apr 11, 2023
commit d4ffc0e
Author: Rogerio Oliveira <roliveira@cyclomedia.com>
Date:   Tue Apr 11 12:36:06 2023 +0200

    removed associated function to create an account;

commit 929ece9
Author: Rogerio Oliveira <roliveira@cyclomedia.com>
Date:   Tue Apr 11 00:16:31 2023 +0200

    added mvc structure (only controllers and routers);
    added controllers and routers to access login and registration mocks;
    fixed the warn on "impl Reply" [rust-lang/rust#107729];

Changes to be committed:
	modified:   Cargo.lock
	modified:   Cargo.toml
	new file:   src/controllers/authentication/login.rs
	new file:   src/controllers/authentication/mod.rs
	new file:   src/controllers/authentication/registration.rs
	new file:   src/controllers/mod.rs
	modified:   src/lib.rs
	new file:   src/models/account.rs
	new file:   src/models/mod.rs
	new file:   src/routes/authentication.rs
	new file:   src/routes/mod.rs
roger-oliver added a commit to roger-oliver/qa-api that referenced this issue Apr 28, 2023
commit d4ffc0e
Author: Rogerio Oliveira <roliveira@cyclomedia.com>
Date:   Tue Apr 11 12:36:06 2023 +0200

    removed associated function to create an account;

commit 929ece9
Author: Rogerio Oliveira <roliveira@cyclomedia.com>
Date:   Tue Apr 11 00:16:31 2023 +0200

    added mvc structure (only controllers and routers);
    added controllers and routers to access login and registration mocks;
    fixed the warn on "impl Reply" [rust-lang/rust#107729];

Changes to be committed:
	modified:   Cargo.lock
	modified:   Cargo.toml
	new file:   src/controllers/authentication/login.rs
	new file:   src/controllers/authentication/mod.rs
	new file:   src/controllers/authentication/registration.rs
	new file:   src/controllers/mod.rs
	modified:   src/lib.rs
	new file:   src/models/account.rs
	new file:   src/models/mod.rs
	new file:   src/routes/authentication.rs
	new file:   src/routes/mod.rs
roger-oliver added a commit to roger-oliver/qa-api that referenced this issue Apr 28, 2023
added controllers and routers to access login and registration mocks;
fixed the warn on "impl Reply" [rust-lang/rust#107729];
roger-oliver added a commit to roger-oliver/rust-question-api that referenced this issue Apr 29, 2023
commit 50a2682
Author: Rogerio Oliveira <roliveira@cyclomedia.com>
Date:   Tue Apr 11 00:24:02 2023 +0200

    removed some pub modifiers from internal functions;
    fixed the warn on "impl Reply" [rust-lang/rust#107729];

Changes to be committed:
	modified:   mock-server/src/lib.rs
	modified:   src/lib.rs
	modified:   src/routes/authentication.rs
roger-oliver added a commit to roger-oliver/rust-question-api that referenced this issue Apr 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: This is a bug. P-medium Medium priority 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

9 participants