allow `opaque_hidden_inferred_bound` warning on nightly by hawkw · Pull Request #2275 · linkerd/linkerd2-proxy

hawkw · 2023-02-27T17:30:07Z

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.

mateiidavid · 2023-02-27T17:59:53Z

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 · 2023-02-27T18:34:07Z

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 · 2023-02-27T18:38:21Z

@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 function¹; 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".

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 · 2023-02-27T18:39:20Z

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.

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>

* 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>

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

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

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

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

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

hawkw marked this pull request as ready for review February 27, 2023 17:30

hawkw requested a review from a team as a code owner February 27, 2023 17:30

hawkw mentioned this pull request Feb 27, 2023

ci: Add a linkerd install workflow #2268

Merged

whoops fuzz is pinned to an older nightly

5f5b3ce

olix0r mentioned this pull request Feb 27, 2023

Rust 1.66 broke warp impl Reply on return types rust-lang/rust#107729

Open

olix0r approved these changes Feb 27, 2023

View reviewed changes

hawkw merged commit 9619c64 into main Feb 27, 2023

hawkw deleted the eliza/fix-nightly branch February 27, 2023 18:43

compiler-errors mentioned this pull request Feb 27, 2023

Suppress opaque_hidden_inferred_bound for nested RPITs rust-lang/rust#108541

Merged

olix0r mentioned this pull request Mar 1, 2023

proxy: v2.191.2 linkerd/linkerd2#10427

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

allow `opaque_hidden_inferred_bound` warning on nightly#2275

allow `opaque_hidden_inferred_bound` warning on nightly#2275
hawkw merged 2 commits intomainfrom
eliza/fix-nightly

hawkw commented Feb 27, 2023 •

edited

Loading

Uh oh!

mateiidavid commented Feb 27, 2023

Uh oh!

olix0r commented Feb 27, 2023

Uh oh!

hawkw commented Feb 27, 2023 •

edited

Loading

Uh oh!

hawkw commented Feb 27, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

hawkw commented Feb 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mateiidavid commented Feb 27, 2023

Uh oh!

olix0r commented Feb 27, 2023

Uh oh!

hawkw commented Feb 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Footnotes

Uh oh!

hawkw commented Feb 27, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

hawkw commented Feb 27, 2023 •

edited

Loading

hawkw commented Feb 27, 2023 •

edited

Loading