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

nightly 2022-06-29 rejects previously compiling code with missing trait implementations #99536

Closed
hawkw opened this issue Jul 20, 2022 · 15 comments · Fixed by #99860
Closed

nightly 2022-06-29 rejects previously compiling code with missing trait implementations #99536

hawkw opened this issue Jul 20, 2022 · 15 comments · Fixed by #99860
Assignees
Labels
C-bug Category: This is a bug. P-high High priority regression-from-stable-to-beta Performance or correctness regression from stable to beta. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@hawkw
Copy link
Contributor

hawkw commented Jul 20, 2022

Code

The linkerd2-proxy project fails to compile on nighty 2022-06-29 and later due to a "missing" implementation of a trait in an impl Trait return value. The same code compiles successfully on stable and on prior nightlies. The error occurs when using impl Trait as an associated type of a return value that is itself an impl Trait, when the associated type has trait bounds placed on it by the outer impl Trait.

An error like this is generated:

error[E0277]: the trait bound `impl std::marker::Send + Unpin: linkerd_app_core::linkerd_io::AsyncRead` is not satisfied
   --> linkerd/app/inbound/src/lib.rs:158:9
    |
158 | /         impl svc::MakeConnection<
159 | |                 T,
160 | |                 Connection = impl Send + Unpin,
161 | |                 Metadata = impl Send + Unpin,
162 | |                 Error = Error,
163 | |                 Future = impl Send,
164 | |             > + Clone,
    | |_____________________^ the trait `linkerd_app_core::linkerd_io::AsyncRead` is not implemented for `impl std::marker::Send + Unpin`
    |
    = help: the following other types implement trait `linkerd_app_core::linkerd_io::AsyncRead`:
              &[u8]
              &mut T
              Box<T>
              BoxedIo
              BufStream<RW>
              DuplexStream
              EitherIo<L, R>
              Pin<P>
            and 38 others
    = note: required because of the requirements on the impl of `linkerd_app_core::svc::MakeConnection<T>` for `linkerd_app_core::svc::Filter<linkerd_app_core::svc::MapErr<linkerd_app_core::svc::linkerd_stack::Timeout<ConnectTcp>, impl (FnOnce(Box<(dyn std::error::Error + Sync + std::marker::Send + 'static)>) -> Box<(dyn std::error::Error + Sync + std::marker::Send + 'static)>) + Clone>, [closure@linkerd/app/inbound/src/lib.rs:186:38: 193:18]>`

error[E0277]: the trait bound `impl std::marker::Send + Unpin: linkerd_app_core::linkerd_io::AsyncWrite` is not satisfied
   --> linkerd/app/inbound/src/lib.rs:158:9
    |
158 | /         impl svc::MakeConnection<
159 | |                 T,
160 | |                 Connection = impl Send + Unpin,
161 | |                 Metadata = impl Send + Unpin,
162 | |                 Error = Error,
163 | |                 Future = impl Send,
164 | |             > + Clone,
    | |_____________________^ the trait `linkerd_app_core::linkerd_io::AsyncWrite` is not implemented for `impl std::marker::Send + Unpin`
    |
    = help: the following other types implement trait `linkerd_app_core::linkerd_io::AsyncWrite`:
              &mut T
              Box<T>
              BoxedIo
              BufStream<RW>
              DuplexStream
              EitherIo<L, R>
              Pin<P>
              PrefixedIo<I>
            and 38 others
    = note: required because of the requirements on the impl of `linkerd_app_core::svc::MakeConnection<T>` for `linkerd_app_core::svc::Filter<linkerd_app_core::svc::MapErr<linkerd_app_core::svc::linkerd_stack::Timeout<ConnectTcp>, impl (FnOnce(Box<(dyn std::error::Error + Sync + std::marker::Send + 'static)>) -> Box<(dyn std::error::Error + Sync + std::marker::Send + 'static)>) + Clone>, [closure@linkerd/app/inbound/src/lib.rs:186:38: 193:18]>`

error[E0277]: the trait bound `impl std::marker::Send + Unpin: linkerd_app_core::linkerd_io::AsyncRead` is not satisfied
  --> linkerd/app/outbound/src/tcp/connect.rs:39:9
   |
39 | /         impl svc::MakeConnection<
40 | |                 T,
41 | |                 Connection = impl Send + Unpin,
42 | |                 Metadata = ConnectMeta,
43 | |                 Error = Error,
44 | |                 Future = impl Send,
45 | |             > + Clone,
   | |_____________________^ the trait `linkerd_app_core::linkerd_io::AsyncRead` is not implemented for `impl std::marker::Send + Unpin`
   |
   = help: the following other types implement trait `linkerd_app_core::linkerd_io::AsyncRead`:
             &[u8]
             &mut T
             Box<T>
             BoxedIo
             DuplexStream
             EitherIo<L, R>
             Pin<P>
             PrefixedIo<I>
           and 38 others
   = note: required because of the requirements on the impl of `MakeConnection<T>` for `linkerd_app_core::transport::linkerd_transport_metrics::Client<linkerd_app_core::transport::Metrics, BoxFuture<linkerd_app_core::svc::MapErr<linkerd_app_core::svc::linkerd_stack::Timeout<OpaqueTransport<linkerd_app_core::linkerd_tls::Client<linkerd_app_core::identity::NewClient, C>>>, impl (FnOnce(Box<(dyn std::error::Error + Sync + std::marker::Send + 'static)>) -> Box<(dyn std::error::Error + Sync + std::marker::Send + 'static)>) + Clone>>>`

error[E0277]: the trait bound `impl std::marker::Send + Unpin: linkerd_app_core::linkerd_io::AsyncWrite` is not satisfied
  --> linkerd/app/outbound/src/tcp/connect.rs:39:9
   |
39 | /         impl svc::MakeConnection<
40 | |                 T,
41 | |                 Connection = impl Send + Unpin,
42 | |                 Metadata = ConnectMeta,
43 | |                 Error = Error,
44 | |                 Future = impl Send,
45 | |             > + Clone,
   | |_____________________^ the trait `linkerd_app_core::linkerd_io::AsyncWrite` is not implemented for `impl std::marker::Send + Unpin`
   |
   = help: the following other types implement trait `linkerd_app_core::linkerd_io::AsyncWrite`:
             &mut T
             Box<T>
             BoxedIo
             DuplexStream
             EitherIo<L, R>
             Pin<P>
             PrefixedIo<I>
             ScopedIo<I>
           and 38 others
   = note: required because of the requirements on the impl of `MakeConnection<T>` for `linkerd_app_core::transport::linkerd_transport_metrics::Client<linkerd_app_core::transport::Metrics, BoxFuture<linkerd_app_core::svc::MapErr<linkerd_app_core::svc::linkerd_stack::Timeout<OpaqueTransport<linkerd_app_core::linkerd_tls::Client<linkerd_app_core::identity::NewClient, C>>>, impl (FnOnce(Box<(dyn std::error::Error + Sync + std::marker::Send + 'static)>) -> Box<(dyn std::error::Error + Sync + std::marker::Send + 'static)>) + Clone>>>`

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

Unfortunately, I don't have a minimal reproducer yet; I'd like to find one, but for now, checking out linkerd/linkerd2-proxy@0288b9c and running cargo +nigihtly-2022-06-29 build will fail with this error.

Version it worked on

The most recent nightly that can compile the linkerd2-proxy codebase successfully is 2022-06-28. Successful CI build: https://github.com/linkerd/linkerd2-proxy/runs/7088482069?check_suite_focus=true

Version with regression

Nightly 2022-06-29 and later fails to compile the same codebase. Failing CI build: https://github.com/linkerd/linkerd2-proxy/runs/7107925369?check_suite_focus=true

We ran cargo bisect-rustc and it appears that commit 00ebeb8 introduced the regression.

searched nightlies: from nightly-2022-06-28 to nightly-2022-06-30
regressed nightly: nightly-2022-06-29
searched commit range: 2f3ddd9...8308806
regressed commit: 00ebeb8

bisected with cargo-bisect-rustc v0.6.3

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

cargo bisect-rustc --start 2022-06-28 --end 2022-06-30 --test-dir ../linkerd2-proxy -- build 
@hawkw hawkw added C-bug Category: This is a bug. regression-untriaged Untriaged performance or correctness regression. labels Jul 20, 2022
@rustbot rustbot added the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Jul 20, 2022
@hawkw
Copy link
Contributor Author

hawkw commented Jul 20, 2022

Started attempting to find a minimal reproduction; this playground does not reproduce the issue, so clearly there is something more complex involved than just returning an impl Trait as an associated type of another impl Trait...

@compiler-errors
Copy link
Member

Guessing it's #97346

@compiler-errors
Copy link
Member

compiler-errors commented Jul 21, 2022

@hawkw, are there rustdocs for linkerd2-proxy (or, specifically, one of the failing crates like linkerd2-proxy/linkerd/app/inbound)? There's quite a few types here, and those would help, but I couldn't seem to find a rustdoc-style documentation from the website. I also may just be looking in the wrong place.

edit: nvm i just built them myself.

@compiler-errors
Copy link
Member

Soooooo, I think I got it to compile. This is not a correct soltuion, because it causes a bunch of UI tests to fail. Draft PR: #99547

This has to do with the fact that when we Equate::tys on an inference variable and an opaque type, we end up just inferring that inference variable to opaque type, instead of (what we usually do, which is) causing us to constrain the opaque type to the value of the other ty (the infer, in this case).

This means that when we later shallow_resolve a predicate that looks like <_#1t as AsyncRead> (i.e. some inferred type must implement AsyncRead), we get <impl Send + Unpin as AsyncRead> -- without the usual opaque type registering machinery taking place.

side-note: does it usually take 3 minutes to compile linkerd/app/outbound?

@oli-obk
Copy link
Contributor

oli-obk commented Jul 21, 2022

we end up just inferring that inference variable to opaque type, instead of (what we usually do, which is) causing us to constrain the opaque type to the value of the other ty (the infer, in this case).

That's on purpose. The only time we don't do that is for return types so that we can merge the types from all the return sites.

how did we miss this in the crater run :/

@compiler-errors
Copy link
Member

@oli-obk linkerd isn't on crates.io i think? 🤔

@oli-obk
Copy link
Contributor

oli-obk commented Jul 21, 2022

we run github crates, too, and I was certain I have seen linkerd in crater reports before

@oli-obk
Copy link
Contributor

oli-obk commented Jul 21, 2022

{
  "name": "gridgentoo.linkerd2-proxy.f7a8ee98c63b6729e02f54a5182a9b7f56695922",
  "url": "https://github.com/gridgentoo/linkerd2-proxy/tree/f7a8ee98c63b6729e02f54a5182a9b7f56695922",
  "krate": {
    "GitHub": {
      "org": "gridgentoo",
      "name": "linkerd2-proxy",
      "sha": "f7a8ee98c63b6729e02f54a5182a9b7f56695922"
    }
  },
  "res": "build-fail",
  "runs": [
    {
      "res": "build-fail:compiler-error(unused_must_use)",
      "log": "master%2343d9f3859e0204e764161ee085a360274b5f3e9a/gh/gridgentoo.linkerd2-proxy"
    },
    {
      "res": "build-fail:compiler-error(unused_must_use)",
      "log": "try%237306d3110618f84a6a4662b9e2cc4fd0d9b02da8/gh/gridgentoo.linkerd2-proxy"
    }
  ]
}

uhm... did crater not recognize the regression because it hit a deny for a lint?

also it seems like it didn't actually run on the sub-crates like inbound (at least I can't find it in the results).

nominating for t-types discussion, as there was a regression we didn't know about.

I think what all users that hit this desire is not actually impl Send, but "an opaque type that satisfies the bounds of the associated type, PLUS Send".

@oli-obk oli-obk added the I-types-nominated Nominated for discussion during a types team meeting. label Jul 21, 2022
@compiler-errors
Copy link
Member

compiler-errors commented Jul 21, 2022

f7a8ee98c63b6729e02f54a5182a9b7f56695922 seems really old

@apiraino apiraino added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jul 21, 2022
@apiraino
Copy link
Contributor

WG-prioritization assigning priority (Zulip discussion).

@rustbot label -I-prioritize +P-high

@rustbot rustbot added P-high High priority and removed I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Jul 27, 2022
@oli-obk oli-obk added regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. and removed regression-untriaged Untriaged performance or correctness regression. labels Jul 27, 2022
@oli-obk
Copy link
Contributor

oli-obk commented Jul 27, 2022

Let's revert for now, this seems more useful in practice than we thought

@pnkfelix pnkfelix self-assigned this Jul 28, 2022
@olix0r
Copy link

olix0r commented Jul 28, 2022

side-note: does it usually take 3 minutes to compile linkerd/app/outbound?

yes 🥲

@LucioFranco
Copy link
Member

Btw starting to see this in beta now as well. https://github.com/tower-rs/tower/runs/7789919663?check_suite_focus=true

@compiler-errors
Copy link
Member

Thanks, yeah, there is a revert PR in flight but the author is on vacation.

In the mean time, I've beta-backport nominated the revert PR and that'll likely get approved, fixing this in a few more weeks.

Sorry for the delay.

@compiler-errors compiler-errors added regression-from-stable-to-beta Performance or correctness regression from stable to beta. and removed regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. labels Aug 11, 2022
@LucioFranco
Copy link
Member

No problem! Thanks for the quick response 😄

@bors bors closed this as completed in 8064a49 Aug 18, 2022
ehuss pushed a commit to ehuss/rust that referenced this issue Aug 31, 2022
Revert "Rollup merge of rust-lang#97346 - JohnTitor:remove-back-compat-hacks, …

…r=oli-obk"

This reverts commit c703d11, reversing
changes made to 64eb9ab.

it didn't apply cleanly, so now it works the same for RPIT and for TAIT instead of just working for RPIT, but we should keep those in sync anyway. It also exposed a TAIT bug (see the feature gated test that now ICEs).

r? `@pnkfelix`

fixes rust-lang#99536
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-high High priority regression-from-stable-to-beta Performance or correctness regression from stable to beta. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
8 participants