Add hidden_static_lifetime lint

[blyxyas]

Fixes #10108

---

changelog: New lint: [hidden_static_lifetime]:
#10123

[rustbot]

r? @Jarcho

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

[Jarcho]

This needs to be limited by quite a bit. The only cases which this is valid for are when the lifetime is used only in immutable references which are neither behind a mutable reference, nor used in function types; and mutable references to static types.

Some valid examples:

• &'a T -> &'static T
• &'a &'b T -> &'static &'static T
• &'a mut T -> &'static mut T where T: 'static

Some invalid examples:

• &'a mut &'b T -> &'static mut &'static T
• fn(&'a T) -> fn(&'static T)

Whether switching struct lifetime parameters to 'static is valid or not depends on the content of the struct, so those can't be easily switched either.

[blyxyas]

How could I test this?
I'm struggling to test it, because functions cannot return references, so &'a &'b T, instead of activating the lint, trigger compiler errors. I cannot use todo!() because functions that panic aren't linted.

[Jarcho]

The current code doesn't do anything to avoid linting on panic. Worst case you could do unsafe { *ptr::null() }.

[blyxyas]

The lint doesn't, but it seems that a code that should trigger the lint, doesn't, if the code has a todo!(), I assume because of how cargo test is made. Maybe there's an option for ignoring function's bodies?

Also, unsafe { *std::ptr::null() } doesn't work, it triggers [E0507]: Cannot move out of a raw pointer:
Using this code:

fn l<'l, T>() -> &'l mut T where T: 'static + Copy {
	unsafe { *std::ptr::null() }
}

I'm clueless, I don't know how to test this lint.

[Jarcho]

Try unsafe { std::ptr::null().read() } instead. That should have no constraints on the type it can return.

[blyxyas]

Thanks for the snippet, it works.

[Jarcho]

This needs a test for with something like struct S<'a>(&'a mut i32) to make sure that doesn't get linted.

[Jarcho]

Struct fields follow the same rules that other types do. Some more examples:

struct S<T>(T);
struct Ref<'a, T: 'a>(&'a T);

struct Both<'a, 'b, T> {
    owned: T,
    borrow: &'a T,
    mut_borrow: &'b mut T,
}

fn f1<'a, 'b>() -> S<&'a mut Ref<'b, u32>> {} // can't change
fn f2<'a, 'b>() -> &'a Both<'a, 'b, &'a str> {} // can't change
fn f3<'a, 'b, 'c>() -> &'a Both<'a, 'b, &'c str> {} // 'a can be 'static
fn f4<'a, 'b>() -> &'a Both<'a, 'b, &'static str> {} // 'a and 'b can be 'static
fn f5<'a> -> Ref<'a, fn(S<Ref<'a, u32>>)> {} // can't change

Handling structs correctly is going to be a fair bit of work and is better handled not at the HIR level since substitutions need to be taken into account.

I would say it's better to just handle the simple case of a top level reference (or multiple references). Unless you really want to go all the way with it.

---

A couple of notes about the the implementation.

• A visitor can be use to simplify looking for a lifetime.
• where clauses shouldn't be changed. T: 'a isn't the same as T: 'static. They still need to be checked for lifetime use though.

[blyxyas]

Better handled not at the HIR level

What other levels can I access? Isn't Clippy specifically for checking the HIR?

[blyxyas]

Also, making this lint only trigger for function returns and ignore structs is probably the best idea. A new lint can possibly be implemented in the future for struct checking. But it doesn't make sense to check structs with a lint in the functions category. In the new commit, if a lifetime is used in any other expressions that isn't the return type reference, it will be discarded.

[Jarcho]

This should use a Visitor for most of the implementation. You can see clippy_utils\visitors for some examples.

[Jarcho]

This is not the same as a 'static bound.

[Jarcho]

This is not the same as a 'static bound.

[Jarcho]

This should be merged with the previous function. While the type is a reference you're unwrapping it looking for lifetimes to change to 'static. Once you hit a type which isn't a reference you're then looking to see if the lifetime is used again.

[Jarcho]

What other levels can I access? Isn't Clippy specifically for checking the HIR?

You would use the middle layer's representation of everything. It can all be retrieved from various TyCtxt queries. If you're just handling top-level references, there's no need to go this route.

[blyxyas]

I've been trying to convert the old code to a new lint using Visitor for a few days and I can't really see it.
Is it necessary? The old code worked fine and I don't see the need to use Visitors for this, is it for performance?

The last commit contains my current code for converting this lint to a Visitor, it doesn't work and I don't even know if I'm doing things right.

Is it really necessary? If so, is there something I'm doing really wrong that I should be fixing right now?

[Jarcho]

You misunderstood how to use a visitor there. You should only need to override check_lifetime, leaving everything else to the default behaviour of walking the AST. You can then call visit_ty and visit_where_predicate to feed data into the visitor.

Essentially something like (not valid code):

struct Visitor {
    lifetimes: Vec<_>,
}
impl Visitor for Visitor {
    fn check_lifetime(&mut self, lifetime) {
        if let Some(i) = self.lifetimes.iter().position(|x| x == lifetime) {
            self.lifetimes.remove(i);
        }
    }
}
fn lint(...) {
    let mut visitor = Visitor { lifetimes: ... };
    visitor.visit_ty(arg1);
    visitor.visit_ty(arg2);
    visitor.visit_where_predicate(pred1);
    // use remaining lifetimes
}

[bors]

☔ The latest upstream changes (presumably #10313) made this pull request unmergeable. Please resolve the merge conflicts.

[blyxyas]

It's been 48 - 49 days since this PR was created. I'm very sorry for wasting your time. Once again I chose an issue with a scope far bigger than I imagined, and thought it would be easy. That's a childish mentality and, in all future PRs I should stick to easier, quicker issues.

For anyone attempting this in the future: this last commit contains my work-in-progress code (It obviously doesn't work). You may also want to check the last commit before the revamp for some support functions or notes.

Im very sorry to my reviewer, @Jarcho for wasting their time. I'm going to close this PR and unnassign myself from the issue. If anyone wants to make another PR about #10108, it's completely free.