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

allow opaque_hidden_inferred_bound warning on nightly #2275

Merged
merged 2 commits into from Feb 27, 2023
Merged

Conversation

hawkw
Copy link
Member

@hawkw hawkw commented 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) 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.

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`.
```

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.
@hawkw hawkw marked this pull request as ready for review February 27, 2023 17:30
@hawkw hawkw requested a review from a team as a code owner February 27, 2023 17:30
@mateiidavid
Copy link
Member

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:

Am I understanding this right? So basically, we have a Send bound on the associated type, and if we use an associated type that uses impl Future (so whatever opaque type that implements Future), and we add that this type has a Send bound, it won't work?

Also, just curious but how come we use an opaque type (impl Send) instead of just specifying a bound Send on it for more flexibility? Sorry bit besides the point, but while I'm here... 🤷🏻

@olix0r
Copy link
Member

olix0r commented Feb 27, 2023

FYI, i believe this change is also included in Rust 1.66; so we may need a more robust change than changing the nightly workflow.

@hawkw
Copy link
Member Author

hawkw commented Feb 27, 2023

@mateiidavid

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:

Am I understanding this right? So basically, we have a Send bound on the associated type, and if we use an associated type that uses impl Future (so whatever opaque type that implements Future), and we add that this type has a Send bound, it won't work?

I think what you're describing here is kind of backwards: we have an associated type where the trait bounds the associated type with Future:

pub trait Service {
     type Future: Future<Output = Result<Self::Response, Self::Error>>;
    // ...
}

and here, we are returning a Service and saying that the Future type is impl Send, to indicate that the future is also Send:

fn stuff() -> impl Service<
    Future = impl Send,
    // ...
> {
    // ...
}

Since Service bounds its Future associated type with the Future trait, the impl Service's Future associated type is already required to implement Future. We don't need to return impl Future there, although the opaque_hidden_inferred_bound lint asks us to add it (presumably for explicitness)

Also, just curious but how come we use an opaque type (impl Send) instead of just specifying a bound Send on it for more flexibility? Sorry bit besides the point, but while I'm here... 🤷🏻

We use an opaque type in return type position --- trait bounds constrain generic type parameters, which are (generally) inputs to a function1; we cannot add trait bounds to a return type that a function determines on its own. Instead we return opaque types (impl Traits) to say "this function returns 'something' which implements these traits".

Footnotes

  1. a function can return one of its type parameters, but it isn't responsible for determining what type that value is --- type parameters are types determined by the caller, while opaque types (impl Trait) are determined by the returner. If that makes sense...

@hawkw
Copy link
Member Author

hawkw commented Feb 27, 2023

FYI, i believe this change is also included in Rust 1.66; so we may need a more robust change than changing the nightly workflow.

Yeah...we'll have to address that separately when we actually update our compiler version to 1.66, though, because adding -A opaque_hidden_inferred_bound will cause a compiler error on compiler versions that don't know about that lint.

@hawkw hawkw merged commit 9619c64 into main Feb 27, 2023
@hawkw hawkw deleted the eliza/fix-nightly branch February 27, 2023 18:43
olix0r added a commit to linkerd/linkerd2 that referenced this pull request Mar 1, 2023
Revert inbound policy discovery behavior changes (from v2.191.0) and
update dependencies, especially h2.

---

* Revert inbound policy discovery changes (linkerd/linkerd2-proxy#2267)
* Bump v38 to v39 (linkerd/linkerd2-proxy#2269)
* dev: override CXX for rust-analyzer (linkerd/linkerd2-proxy#2270)
* build(deps): bump syn from 1.0.107 to 1.0.109 (linkerd/linkerd2-proxy#2274)
* build(deps): bump tokio-util from 0.7.4 to 0.7.7 (linkerd/linkerd2-proxy#2272)
* build(deps): bump tj-actions/changed-files from 35.5.6 to 35.6.0 (linkerd/linkerd2-proxy#2271)
* build(deps): bump prost-build from 0.11.6 to 0.11.8 (linkerd/linkerd2-proxy#2273)
* ci: Add a linkerd install workflow (linkerd/linkerd2-proxy#2268)
* allow `opaque_hidden_inferred_bound` warning on nightly (linkerd/linkerd2-proxy#2275)
* build(deps): bump h2 from 0.3.15 to 0.3.16 (linkerd/linkerd2-proxy#2278)
* build(deps): bump tempfile from 3.3.0 to 3.4.0 (linkerd/linkerd2-proxy#2277)
* build(deps): bump tokio-stream from 0.1.11 to 0.1.12 (linkerd/linkerd2-proxy#2276)
* stack: Make `failfast::Gate` general purpose (linkerd/linkerd2-proxy#2279)
* build(deps): bump slab from 0.4.7 to 0.4.8 (linkerd/linkerd2-proxy#2283)
* build(deps): bump async-stream from 0.3.3 to 0.3.4 (linkerd/linkerd2-proxy#2282)
* build(deps): bump jobserver from 0.1.25 to 0.1.26 (linkerd/linkerd2-proxy#2281)
* build(deps): bump tj-actions/changed-files from 35.6.0 to 35.6.1 (linkerd/linkerd2-proxy#2280)

Signed-off-by: Oliver Gould <ver@buoyant.io>
olix0r added a commit to linkerd/linkerd2 that referenced this pull request Mar 1, 2023
* proxy: v2.191.0

This release updates the proxy to require control-plane-discovered
policies for all inbound connections. Previously, the proxy would
use a locally-configured default policy while it synced policies from
the control plane.

---

* trace: never log access log spans to stdout (linkerd/linkerd2-proxy#2244)
* build(deps): bump thread_local from 1.1.4 to 1.1.7 (linkerd/linkerd2-proxy#2243)
* build(deps): bump tj-actions/changed-files from 35.5.4 to 35.5.5 (linkerd/linkerd2-proxy#2242)
* build(deps): bump serde_json from 1.0.91 to 1.0.93 (linkerd/linkerd2-proxy#2217)
* dev: Remove docker config mount (linkerd/linkerd2-proxy#2246)
* build(deps): bump anyhow from 1.0.68 to 1.0.69 (linkerd/linkerd2-proxy#2216)
* build(deps): bump actions/checkout from 3.2.0 to 3.3.0 (linkerd/linkerd2-proxy#2130)
* http: Use `ExtractParam` in `NewNormalizeUri` (linkerd/linkerd2-proxy#2245)
* http: Replace `classify::CanClassify` with `Param` (linkerd/linkerd2-proxy#2247)
* http: Improve timeout module (linkerd/linkerd2-proxy#2248)
* outbound: Decouple outbound HTTP server from logical target (linkerd/linkerd2-proxy#2249)
* build(deps): bump http from 0.2.8 to 0.2.9 (linkerd/linkerd2-proxy#2253)
* build(deps): bump fastrand from 1.8.0 to 1.9.0 (linkerd/linkerd2-proxy#2252)
* build(deps): bump signal-hook-registry from 1.4.0 to 1.4.1 (linkerd/linkerd2-proxy#2254)
* Use `ExtractParam` in `http::NewHeaderFromTarget` (linkerd/linkerd2-proxy#2255)
* outbound: Introduce a new outbound-route watch type (linkerd/linkerd2-proxy#2250)
* build(deps): bump tj-actions/changed-files from 35.5.5 to 35.5.6 (linkerd/linkerd2-proxy#2256)
* build(deps): bump hyper from 0.14.23 to 0.14.24 (linkerd/linkerd2-proxy#2257)
* build(deps): bump once_cell from 1.17.0 to 1.17.1 (linkerd/linkerd2-proxy#2258)
* build(deps): bump clang-sys from 1.4.0 to 1.6.0 (linkerd/linkerd2-proxy#2259)
* outbound: Add a policy router (linkerd/linkerd2-proxy#2251)
* inbound: connections wait for ServerPolicy discovery (linkerd/linkerd2-proxy#2186)
* inbound: Remove default policies (linkerd/linkerd2-proxy#2204)
* inbound: Introduce a `policy::LookupAddr` type (linkerd/linkerd2-proxy#2264)
* build(deps): bump futures from 0.3.25 to 0.3.26 (linkerd/linkerd2-proxy#2263)
* build(deps): bump proc-macro2 from 1.0.50 to 1.0.51 (linkerd/linkerd2-proxy#2261)
* build(deps): bump tinyvec_macros from 0.1.0 to 0.1.1 (linkerd/linkerd2-proxy#2262)

Signed-off-by: Oliver Gould <ver@buoyant.io>

* proxy: v2.191.2

Revert inbound policy discovery behavior changes (from v2.191.0) and
update dependencies, especially h2.

---

* Revert inbound policy discovery changes (linkerd/linkerd2-proxy#2267)
* Bump v38 to v39 (linkerd/linkerd2-proxy#2269)
* dev: override CXX for rust-analyzer (linkerd/linkerd2-proxy#2270)
* build(deps): bump syn from 1.0.107 to 1.0.109 (linkerd/linkerd2-proxy#2274)
* build(deps): bump tokio-util from 0.7.4 to 0.7.7 (linkerd/linkerd2-proxy#2272)
* build(deps): bump tj-actions/changed-files from 35.5.6 to 35.6.0 (linkerd/linkerd2-proxy#2271)
* build(deps): bump prost-build from 0.11.6 to 0.11.8 (linkerd/linkerd2-proxy#2273)
* ci: Add a linkerd install workflow (linkerd/linkerd2-proxy#2268)
* allow `opaque_hidden_inferred_bound` warning on nightly (linkerd/linkerd2-proxy#2275)
* build(deps): bump h2 from 0.3.15 to 0.3.16 (linkerd/linkerd2-proxy#2278)
* build(deps): bump tempfile from 3.3.0 to 3.4.0 (linkerd/linkerd2-proxy#2277)
* build(deps): bump tokio-stream from 0.1.11 to 0.1.12 (linkerd/linkerd2-proxy#2276)
* stack: Make `failfast::Gate` general purpose (linkerd/linkerd2-proxy#2279)
* build(deps): bump slab from 0.4.7 to 0.4.8 (linkerd/linkerd2-proxy#2283)
* build(deps): bump async-stream from 0.3.3 to 0.3.4 (linkerd/linkerd2-proxy#2282)
* build(deps): bump jobserver from 0.1.25 to 0.1.26 (linkerd/linkerd2-proxy#2281)
* build(deps): bump tj-actions/changed-files from 35.6.0 to 35.6.1 (linkerd/linkerd2-proxy#2280)

Signed-off-by: Oliver Gould <ver@buoyant.io>

---------

Signed-off-by: Oliver Gould <ver@buoyant.io>
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Mar 21, 2023
…r=oli-obk

Suppress `opaque_hidden_inferred_bound` for nested RPITs

They trigger too much, making repos like linkerd/linkerd2-proxy#2275 sad.

Ideally, at least for RPITs (and probably TAITs?), specifically when we have `impl Trait<Assoc = impl ..>`, that nested opaque should have the necessary `Assoc` item bounds elaborated into its own item bounds. But that's another story.

r? `@oli-obk`
Nilstrieb added a commit to Nilstrieb/rust that referenced this pull request Mar 21, 2023
…r=oli-obk

Suppress `opaque_hidden_inferred_bound` for nested RPITs

They trigger too much, making repos like linkerd/linkerd2-proxy#2275 sad.

Ideally, at least for RPITs (and probably TAITs?), specifically when we have `impl Trait<Assoc = impl ..>`, that nested opaque should have the necessary `Assoc` item bounds elaborated into its own item bounds. But that's another story.

r? ``@oli-obk``
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 23, 2023
…r=oli-obk

Suppress `opaque_hidden_inferred_bound` for nested RPITs

They trigger too much, making repos like linkerd/linkerd2-proxy#2275 sad.

Ideally, at least for RPITs (and probably TAITs?), specifically when we have `impl Trait<Assoc = impl ..>`, that nested opaque should have the necessary `Assoc` item bounds elaborated into its own item bounds. But that's another story.

r? `@oli-obk`
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 23, 2023
…r=oli-obk

Suppress `opaque_hidden_inferred_bound` for nested RPITs

They trigger too much, making repos like linkerd/linkerd2-proxy#2275 sad.

Ideally, at least for RPITs (and probably TAITs?), specifically when we have `impl Trait<Assoc = impl ..>`, that nested opaque should have the necessary `Assoc` item bounds elaborated into its own item bounds. But that's another story.

r? ``@oli-obk``
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 23, 2023
…r=oli-obk

Suppress `opaque_hidden_inferred_bound` for nested RPITs

They trigger too much, making repos like linkerd/linkerd2-proxy#2275 sad.

Ideally, at least for RPITs (and probably TAITs?), specifically when we have `impl Trait<Assoc = impl ..>`, that nested opaque should have the necessary `Assoc` item bounds elaborated into its own item bounds. But that's another story.

r? ```@oli-obk```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants