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

Warn on unused lifetime parameters #41960

Closed
clarfonthey opened this issue May 12, 2017 · 15 comments
Closed

Warn on unused lifetime parameters #41960

clarfonthey opened this issue May 12, 2017 · 15 comments
Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. C-feature-request Category: A feature request, i.e: not implemented / a PR. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@clarfonthey
Copy link
Contributor

Somehow this made it into master without being warned:

#[stable(feature = "os_string_from_box", since = "1.17.0")]
impl<'a> From<Box<OsStr>> for OsString {
    fn from(boxed: Box<OsStr>) -> OsString {
        boxed.into_os_string()
    }
}

This should definitely trigger a warning.

@Thiez
Copy link
Contributor

Thiez commented May 12, 2017

Yikes, no warnings at all.

fn main() {
    trait T {}
    impl<'a> T for u8 {}
}

Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this issue May 20, 2017
Remove unused lifetimes.

This was a typo that made it onto master. Noted by @dtolnay in rust-lang#42127.

Also note rust-lang#41960 which suggests warning these.
@Mark-Simulacrum Mark-Simulacrum added the A-diagnostics Area: Messages for errors, warnings, and lints label Jun 22, 2017
@Mark-Simulacrum Mark-Simulacrum added C-enhancement Category: An issue proposing an enhancement or a PR with one. C-feature-request Category: A feature request, i.e: not implemented / a PR. and removed C-enhancement Category: An issue proposing an enhancement or a PR with one. labels Jul 26, 2017
@estebank
Copy link
Contributor

Triage: no change.

@varkor
Copy link
Member

varkor commented Mar 22, 2018

Note that this is currently intentional behaviour:

// Disallow unconstrained lifetimes, but only if they appear in assoc types.

// (*) This is a horrible concession to reality. I think it'd be
// better to just ban unconstrianed lifetimes outright, but in
// practice people do non-hygenic macros like:
//
// ```
// macro_rules! __impl_slice_eq1 {
// ($Lhs: ty, $Rhs: ty, $Bound: ident) => {
// impl<'a, 'b, A: $Bound, B> PartialEq<$Rhs> for $Lhs where A: PartialEq<B> {
// ....
// }
// }
// }
// ```
//
// In a concession to backwards compatbility, we continue to
// permit those, so long as the lifetimes aren't used in
// associated types. I believe this is sound, because lifetimes
// used elsewhere are not projected back out.

@clarfonthey
Copy link
Contributor Author

@varkor they should be accepted, just linted.

@scottmcm
Copy link
Member

scottmcm commented Feb 18, 2019

I think this still needs a warn-by-default lint, because I just spotted this in liballoc:

#[stable(feature = "collection_debug", since = "1.17.0")]
impl<'a, T: fmt::Debug> fmt::Debug for IterMut<'_, T> {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
let (front, back) = RingSlices::ring_slices(&*self.ring, self.head, self.tail);
f.debug_tuple("IterMut")
.field(&front)
.field(&back)
.finish()
}
}

Edit: found another one:

rust/src/libstd/panic.rs

Lines 322 to 330 in 9a3392e

#[unstable(feature = "futures_api", issue = "50547")]
impl<'a, F: Future> Future for AssertUnwindSafe<F> {
type Output = F::Output;
fn poll(self: Pin<&mut Self>, waker: &Waker) -> Poll<Self::Output> {
let pinned_field = unsafe { Pin::map_unchecked_mut(self, |x| &mut x.0) };
F::poll(pinned_field, waker)
}
}

kennytm added a commit to kennytm/rust that referenced this issue Feb 20, 2019
Use more impl header lifetime elision

Inspired by seeing explicit lifetimes on these two:

- https://doc.rust-lang.org/nightly/std/slice/struct.Iter.html#impl-FusedIterator
- https://doc.rust-lang.org/nightly/std/primitive.u32.html#impl-Not

And a follow-up to rust-lang#54687, that started using IHLE in libcore.

Most of the changes in here fall into two big categories:

- Removing lifetimes from common traits that can essentially never user a lifetime from an input (particularly `Drop`, `Debug`, and `Clone`)

- Forwarding impls that are only possible because the lifetime doesn't matter (like `impl<R: Read + ?Sized> Read for &mut R`)

I omitted things that seemed like they could be more controversial, like the handful of iterators that have a `Item: 'static` despite the iterator having a lifetime or the `PartialEq` implementations [where the flipped one cannot elide the lifetime](https://internals.rust-lang.org/t/impl-type-parameter-aliases/9403/2?u=scottmcm).

I also removed two lifetimes that turned out to be completely unused; see rust-lang#41960 (comment)
cuviper pushed a commit to cuviper/rayon-hash that referenced this issue Apr 25, 2019
Use more impl header lifetime elision

Inspired by seeing explicit lifetimes on these two:

- https://doc.rust-lang.org/nightly/std/slice/struct.Iter.html#impl-FusedIterator
- https://doc.rust-lang.org/nightly/std/primitive.u32.html#impl-Not

And a follow-up to rust-lang/rust#54687, that started using IHLE in libcore.

Most of the changes in here fall into two big categories:

- Removing lifetimes from common traits that can essentially never user a lifetime from an input (particularly `Drop`, `Debug`, and `Clone`)

- Forwarding impls that are only possible because the lifetime doesn't matter (like `impl<R: Read + ?Sized> Read for &mut R`)

I omitted things that seemed like they could be more controversial, like the handful of iterators that have a `Item: 'static` despite the iterator having a lifetime or the `PartialEq` implementations [where the flipped one cannot elide the lifetime](https://internals.rust-lang.org/t/impl-type-parameter-aliases/9403/2?u=scottmcm).

I also removed two lifetimes that turned out to be completely unused; see rust-lang/rust#41960 (comment)
@JustAPerson
Copy link
Contributor

pub type UnusedHrtb = for<'a> fn();

Found something like this in my codebase today. No warning from rustc or clippy.

@clarfonthey
Copy link
Contributor Author

Any plans to add a warn-by-default lint?

@Mark-Simulacrum
Copy link
Member

We have an unused_lifetimes lint now.

@estebank
Copy link
Contributor

@Mark-Simulacrum it is not warn by default and I think it probably should be (gated may be on ecosystem prevalence).

@Mark-Simulacrum
Copy link
Member

Hm, yeah, that makes sense. Reopening. I personally think we can probably "just" land this as a PR with FCP T-lang.

@varkor
Copy link
Member

varkor commented Sep 15, 2019

#64493 should be considered blocking for making this warn-by-default.

@estebank estebank added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. labels Oct 15, 2019
@tmandry
Copy link
Member

tmandry commented Jun 25, 2021

#86615 should also be considered blocking.

@Aaron1011
Copy link
Member

With #86615 now fixed on the latest nightly, is there anything blocking this?

@obi1kenobi
Copy link
Member

I was assuming there aren't any blockers so I started looking into it a little bit, but I'm new to the codebase so it might take me a while especially given the holidays. If you'd like to take it over, be my guest!

There's no shortage of open issues, and I'm sure I can both find something else to work on and also learn from reading the PR that fixes this issue 😃

@Mark-Simulacrum
Copy link
Member

Closing this in favor of #94038, which is tracking the enabling of this by default -- I opted to file a new issue so that we have a cleaner history given its tracking status.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. C-feature-request Category: A feature request, i.e: not implemented / a PR. 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

10 participants