needless-lifetime - nested elision sites

[tnielens]

Closes #2944

changelog: fix needless-lifetime nested elision site FPs

[rust-highfive]

r? @flip1995

(rust_highfive has picked a reviewer for you, use r? to override)

[ebroto]

r? @ebroto

(I started the review, will try to finish it later in the day)

[ebroto]

Sorry for the delay in getting the PR reviewed, this is a complex topic with many edge cases :)

I have two main concerns about the current approach:

• We bail out if we find function pointers or closure trait bounds, but the only case that creates false positives is when those have named lifetimes. This leads to unnecessary false negatives.
• We are duplicating efforts (see below).

Looking at the (long) history of the lint, it seems that it was trying to prevent the same cases you identified in your analysis here (modulo function pointers, which is a very good catch of yours 👍)

(I can't put comments on code that has not been edited, so this will be a bit tough to explain correctly)

• For the outlives bound on impl Trait in return position, a check was added in support impl trait for needless lifetimes #1400. There is even a test which is lost in the crashes directory (needless_lifetimes_impl_trait, I think that it's there by mistake). The problem in the issue we are resolving here is that the outlives bound is nested as a type parameter of another impl Trait, so the RefVisitor needs to recurse into nested items, which we can accomplish by using NestedVisitorMap::All in RefVisitor::nested_visit_map.

• For trait bounds (in general), the lint is already bailing out if they are found in the generic parameters. This works also for impl Trait in parameter position, but not in return position. The fix was introduced in Fix false positive unused_lifetime when referenced in generic bounds. #2855. We can use the same approach for impl Trait in return position and for function pointers by "manually" recursing into those looking for named lifetimes, and bail out if we find them.

In RefVisitor::visit_ty, we can add this branch to the match for the function pointers:

            TyKind::BareFn(bare_fn_ty) => {
                let mut visitor = RefVisitor::new(self.cx);
                walk_ty(&mut visitor, ty);
                if visitor.abort || visitor.lts.iter().any(|lt| matches!(lt, RefLt::Named(_))) {
                    self.abort = true;
                }
                return; // don't recurse into the fn declaration
            },

For the trait bounds, we can add this snippet inside the for that iterates on exist_ty.bounds:

                        let mut visitor = RefVisitor::new(self.cx);
                        walk_param_bound(&mut visitor, bound);
                        if visitor.abort || visitor.lts.iter().any(|lt| matches!(lt, RefLt::Named(_))) {
                            self.abort = true;
                            return;
                        }

(We can maybe factor something out by adding a method to the visitor)

I think this way we can avoid duplicating efforts and the false negatives. Please tell me if something is not clear :)

[tnielens]

thanks for the review @ebroto, I'll dive into asap.

[tnielens]

I reworked the impl and tried to integrate it better in the existing code base. I still have to go over the list of test cases suggested by @ebroto.

[ebroto]

I did not have time to dig into the rewrite (sorry about that, I had smaller changes in mind), and sadly won't have until around the end of the week. Hopefully I will recover my normal schedule after that :)

I see some review comments were not applied (here, both cases), and some ui tests are crashing. Please ping me back with those addressed.

[ebroto]

I can't reproduce the test crashes locally now 👍

Besides my two last comments, I think we still miss some cases with fn pointers inside fn pointers/trait bounds:

    fn fun1<'a>(_: &'a i32) -> fn(fn(&'a i32) -> &'a i32) -> i32 {
        |f| 42
    }
    fn fun2<'a>(_: &'a i32) -> fn(fn(&i32) -> &i32) -> i32 {
        |f| 42
    }

    fn fun3<'a>(_: &'a i32) -> impl Fn(fn(&'a i32)) {
        |f| ()
    }
    fn fun4<'a>(_: &'a i32) -> impl Fn(fn(&i32)) {
        |f| ()
    }

(I've tested them and they work as expected)

[tnielens]

Thanks @ebroto .
Didn't have much contribution time last week. Will try to finish this asap.

[tnielens]

@ebroto I should have addressed all your remarks.

[ebroto]

LGTM!

Sorry for the long review process. I'd like to thank you for this fix and in general for your continued efforts in fixing long standing bugs. Your work is very much appreciated.

[ebroto]

@bors r+

[bors]

📌 Commit cb2be6f has been approved by ebroto

[bors]

⌛ Testing commit cb2be6f with merge d431373...

[tnielens]

Happy to see this merged! Thanks @ebroto for the multiple reviews ;)

[bors]

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: ebroto
Pushing d431373 to master...