Skip to content

2024 impl_trait_overcaptures false negative, plus bad suggestion in 2024 #132809

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Closed
ehuss opened this issue Nov 9, 2024 · 4 comments
Closed

2024 impl_trait_overcaptures false negative, plus bad suggestion in 2024 #132809

ehuss opened this issue Nov 9, 2024 · 4 comments
Assignees
Labels
A-edition-2024 Area: The 2024 edition A-lints Area: Lints (warnings about flaws in source code) such as unused_mut. C-bug Category: This is a bug. D-edition Diagnostics: An error or lint that should account for edition differences. D-invalid-suggestion Diagnostics: A structured suggestion resulting in incorrect code. L-false-negative Lint: False negative (should have fired but didn't). L-impl_trait_overcaptures Lint: impl_trait_overcaptures T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@ehuss
Copy link
Contributor

ehuss commented Nov 9, 2024

The 2024 migration for actix-web-lab@0.23.0 failed to detect a situation where an impl trait needs some kind of capture. The following error occurs after updating to 2024:

error[E0515]: cannot return value referencing local data `this`
   --> examples/from_fn.rs:100:26
    |
100 |             async move { Self::mw_cb(&this, req, next).await }
    |                          ^^^^^^^^^^^^-----^^^^^^^^^^^^^^^^^^
    |                          |           |
    |                          |           `this` is borrowed here
    |                          returns a value referencing data owned by the current function
    |
note: this call may capture more lifetimes than intended, because Rust 2024 has adjusted the `impl Trait` lifetime capture rules
   --> examples/from_fn.rs:100:26
    |
100 |             async move { Self::mw_cb(&this, req, next).await }
    |                          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
help: add a precise capturing bound to avoid overcapturing
    |
72  |     ) -> Result<ServiceResponse<impl MessageBody + use<impl MessageBody + 'static>>, Error> {
    |

I would expect impl_trait_overcaptures to migrate this, or if migration isn't possible to at least generate a warning.

Additionally, the suggestion provided in this error message is not valid syntax. use<> cannot have impl MessageBody.

I believe this is an example of APIT migration. That is, the following change I think is a suggestion it could provide:

--- a/examples/from_fn.rs
+++ b/examples/from_fn.rs
@@ -65,11 +65,11 @@ async fn timeout_10secs(
 struct MyMw(bool);

 impl MyMw {
-    async fn mw_cb(
+    async fn mw_cb<T: MessageBody + 'static>(
         &self,
         req: ServiceRequest,
-        next: Next<impl MessageBody + 'static>,
-    ) -> Result<ServiceResponse<impl MessageBody>, Error> {
+        next: Next<T>,
+    ) -> Result<ServiceResponse<impl MessageBody + use<T>>, Error> {
         let mut res = match self.0 {
             true => req.into_response("short-circuited").map_into_right_body(),
             false => next.call(req).await?.map_into_left_body(),

Here the impl trait gets lifted to have a named type parameter which can then be used in the use<> bound.

Or...There might be other suggestions, I don't know. The offending code involves some tricky async-block stuff.

Meta

rustc --version --verbose:

rustc 1.84.0-nightly (59cec72a5 2024-11-08)
binary: rustc
commit-hash: 59cec72a57af178767a7b8e7f624b06cc50f1087
commit-date: 2024-11-08
host: aarch64-unknown-linux-gnu
release: 1.84.0-nightly
LLVM version: 19.1.3
@ehuss ehuss added A-edition-2024 Area: The 2024 edition C-bug Category: This is a bug. D-edition Diagnostics: An error or lint that should account for edition differences. labels Nov 9, 2024
@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Nov 9, 2024
@jieyouxu jieyouxu added A-lints Area: Lints (warnings about flaws in source code) such as unused_mut. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. L-impl_trait_overcaptures Lint: impl_trait_overcaptures D-invalid-suggestion Diagnostics: A structured suggestion resulting in incorrect code. L-false-negative Lint: False negative (should have fired but didn't). and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Nov 9, 2024
@compiler-errors
Copy link
Member

compiler-errors commented Nov 9, 2024

That is, the following change I think is a suggestion it could provide [...]

It would be very cool if someone worked on this. I probably do not have the patience to do this for the time being since it's incredibly tediuous and annoying and error-prone to provide a general suggestion that handles APITs correctly, but I'd love to mentor or review a change to robustify this diagnostic, and also the edition lint (impl_trait_overcaptures). They probably could use a shared set of helper functions to construct a subdiagnostic.

@compiler-errors
Copy link
Member

@ehuss: The 2024 migration for actix-web-lab@0.23.0 failed to detect a situation where an impl trait needs some kind of capture.

Seems like we're not recursing into async fn. I'll fix that.

@compiler-errors
Copy link
Member

Actually wait, I think I did implement something to turn APITs into params. I'll try to adapt that.......

@compiler-errors compiler-errors self-assigned this Nov 9, 2024
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Nov 10, 2024
Dont suggest `use<impl Trait>` when we have an edition-2024-related borrowck issue

rust-lang#131186 implements some machinery to detect in borrowck when we may have RPIT overcaptures due to edition 2024, and suggests adding `+ use<'a, T>` to go back to the edition 2021 capture rules. However, we weren't filtering out cases when there are APITs in scope.

This PR implements a more sophisticated diagnostic where we will suggest turning any APITs in scope into type parameters, and applies this to both the borrowck error note, and to the `impl_trait_overcaptures` migration lint.

cc rust-lang#132809
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Nov 10, 2024
Rollup merge of rust-lang#132816 - compiler-errors:2024-apit, r=jieyouxu

Dont suggest `use<impl Trait>` when we have an edition-2024-related borrowck issue

rust-lang#131186 implements some machinery to detect in borrowck when we may have RPIT overcaptures due to edition 2024, and suggests adding `+ use<'a, T>` to go back to the edition 2021 capture rules. However, we weren't filtering out cases when there are APITs in scope.

This PR implements a more sophisticated diagnostic where we will suggest turning any APITs in scope into type parameters, and applies this to both the borrowck error note, and to the `impl_trait_overcaptures` migration lint.

cc rust-lang#132809
mati865 pushed a commit to mati865/rust that referenced this issue Nov 12, 2024
Dont suggest `use<impl Trait>` when we have an edition-2024-related borrowck issue

rust-lang#131186 implements some machinery to detect in borrowck when we may have RPIT overcaptures due to edition 2024, and suggests adding `+ use<'a, T>` to go back to the edition 2021 capture rules. However, we weren't filtering out cases when there are APITs in scope.

This PR implements a more sophisticated diagnostic where we will suggest turning any APITs in scope into type parameters, and applies this to both the borrowck error note, and to the `impl_trait_overcaptures` migration lint.

cc rust-lang#132809
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Nov 15, 2024
…tures-apit, r=BoxyUwU

Recurse into APITs in `impl_trait_overcaptures`

We were previously not detecting cases where an RPIT was located in the return type of an async function, leading to underfiring of the `impl_trait_overcaptures`. This PR does this recursion properly now.

cc rust-lang#132809
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Nov 15, 2024
Rollup merge of rust-lang#132817 - compiler-errors:impl-trait-overcaptures-apit, r=BoxyUwU

Recurse into APITs in `impl_trait_overcaptures`

We were previously not detecting cases where an RPIT was located in the return type of an async function, leading to underfiring of the `impl_trait_overcaptures`. This PR does this recursion properly now.

cc rust-lang#132809
@ehuss
Copy link
Contributor Author

ehuss commented Nov 19, 2024

I'm going to close as fixed:

  • The migration lint now fires (though it is maybe-incorrect, so doesn't auto-apply). The issues with APIT are known and documented.
  • The suggestion is now correct.

@ehuss ehuss closed this as completed Nov 19, 2024
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
A-edition-2024 Area: The 2024 edition A-lints Area: Lints (warnings about flaws in source code) such as unused_mut. C-bug Category: This is a bug. D-edition Diagnostics: An error or lint that should account for edition differences. D-invalid-suggestion Diagnostics: A structured suggestion resulting in incorrect code. L-false-negative Lint: False negative (should have fired but didn't). L-impl_trait_overcaptures Lint: impl_trait_overcaptures 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

4 participants