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

edition lint: prefer '_ to lifetime elision in types #52041

Closed
nrc opened this issue Jul 3, 2018 · 1 comment
Closed

edition lint: prefer '_ to lifetime elision in types #52041

nrc opened this issue Jul 3, 2018 · 1 comment
Labels
A-edition-2018-lints Area: Lints supporting the 2018 edition
Milestone

Comments

@nrc
Copy link
Member

nrc commented Jul 3, 2018

E.g., use Ref<'_, T> rather than Ref<T>

@nrc nrc added the A-edition-2018-lints Area: Lints supporting the 2018 edition label Jul 3, 2018
@nrc nrc mentioned this issue Jul 4, 2018
15 tasks
@zackmdavis
Copy link
Member

This looks like a duplicate of #45992, implemented in #46254? Some more work is needed, however. As far as polish goes:

  • elided-lifetimes-in-paths is not currently listed as an Edition lint
  • it should have a structured suggestion (for RLS, rustfix, &c.)

More pressingly, it seems to have a lot of false positives. This code:

#![feature(rust_2018_preview)]
#![warn(elided_lifetimes_in_paths)]

use std::cell::{RefCell, Ref};

fn main() {
    // example from `Ref` documentation:
    // https://doc.rust-lang.org/std/cell/struct.Ref.html#examples
    let c = RefCell::new((5, 'b'));
    let b1: Ref<(u32, char)> = c.borrow();
    let b2: Ref<u32> = Ref::map(b1, |t| &t.0);
    assert_eq!(*b2, 5);
}

results in this output:

zmd@ReflectiveCoherence:~/Code/Misc$ rustc +nightly elide_ref.rs 
warning: hidden lifetime parameters are deprecated, try `Foo<'_>`
  --> elide_ref.rs:10:13
   |
10 |     let b1: Ref<(u32, char)> = c.borrow();
   |             ^^^^^^^^^^^^^^^^
   |
note: lint level defined here
  --> elide_ref.rs:2:9
   |
2  | #![warn(elided_lifetimes_in_paths)]
   |         ^^^^^^^^^^^^^^^^^^^^^^^^^

warning: hidden lifetime parameters are deprecated, try `Foo<'_>`
  --> elide_ref.rs:11:24
   |
11 |     let b2: Ref<u32> = Ref::map(b1, |t| &t.0);
   |                        ^^^^^^^^

warning: hidden lifetime parameters are deprecated, try `Foo<'_>`
  --> elide_ref.rs:11:13
   |
11 |     let b2: Ref<u32> = Ref::map(b1, |t| &t.0);
   |             ^^^^^^^^

warning: hidden lifetime parameters are deprecated, try `Foo<'_>`
  --> elide_ref.rs:12:5
   |
12 |     assert_eq!(*b2, 5);
   |     ^^^^^^^^^^^^^^^^^^^
   |
   = note: this error originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info)

Linting on the assert_eq! seems pretty obviously wrong (what is the user supposed to do about that?), and I don't think we should be linting Ref::map, either—it's just a static method (no self) that we're accessing through Ref's namespace, right?

Also, even when we do add the underscore-lifetime, the lints still go off (and point to the '_)—

zmd@ReflectiveCoherence:~/Code/Misc$ rustc +nightly elide_ref_fixed.rs 
warning: hidden lifetime parameters are deprecated, try `Foo<'_>`
  --> elide_ref_fixed.rs:10:17
   |
10 |     let b1: Ref<'_, (u32, char)> = c.borrow();
   |                 ^^
   |
note: lint level defined here
  --> elide_ref_fixed.rs:2:9
   |
2  | #![warn(elided_lifetimes_in_paths)]
   |         ^^^^^^^^^^^^^^^^^^^^^^^^^

warning: hidden lifetime parameters are deprecated, try `Foo<'_>`
  --> elide_ref_fixed.rs:11:28
   |
11 |     let b2: Ref<'_, u32> = Ref::map(b1, |t| &t.0);
   |                            ^^^^^^^^

warning: hidden lifetime parameters are deprecated, try `Foo<'_>`
  --> elide_ref_fixed.rs:11:17
   |
11 |     let b2: Ref<'_, u32> = Ref::map(b1, |t| &t.0);
   |                 ^^

warning: hidden lifetime parameters are deprecated, try `Foo<'_>`
  --> elide_ref_fixed.rs:12:5
   |
12 |     assert_eq!(*b2, 5);
   |     ^^^^^^^^^^^^^^^^^^^
   |
   = note: this error originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info)

zackmdavis added a commit to zackmdavis/rust that referenced this issue Jul 4, 2018
`is_elided` is true for both underscore (`'_`) and implicit (no
representation in source code) lifetimes, but we don't want to fire the
lint on the former, because the entire point of the lint is to suggest
changing the latter to the former (see the initial issue rust-lang#45992).

It seems unfortunate for there to be ambiguity on whether the word
"elided" includes underscore lifetimes or not—the mandate of the
elided-lifetimes-in-paths lint seems to suggest it doesn't, whereas
the `is_elided` method seems to suggest it does—but it's beyond us to
resolve that in this commit. For now, let the message say "implicit"
for definiteness.

This relates to rust-lang#52041.
zackmdavis added a commit to zackmdavis/rust that referenced this issue Jul 14, 2018
`is_elided` is true for both underscore (`'_`) and implicit (no
representation in source code) lifetimes, but we don't want to fire the
lint on the former, because the entire point of the lint is to suggest
changing the latter to the former (see the initial issue rust-lang#45992).

It seems unfortunate for there to be ambiguity on whether the word
"elided" includes underscore lifetimes or not—the mandate of the
elided-lifetimes-in-paths lint seems to suggest it doesn't, whereas
the `is_elided` method seems to suggest it does—but it's beyond us to
resolve that in this commit. For now, let the message say "implicit"
for definiteness.

This relates to rust-lang#52041.
zackmdavis added a commit to zackmdavis/rust that referenced this issue Jul 15, 2018
This was a doozy!

We only issue a suggestion when there's just one implicit lifetime,
because there can be situations where there are several (notice how
`elided_path_lifetimes` in librustc/hir/lowering.rs takes a number of
expected lifetimes as an argument), but we don't have an actionable
suggestion to offer (e.g., if there are two implicit lifetimes in a
return type, we hit E0106 "missing lifetime specifiers" as well as
(still) triggering this lint).

This has to do with rust-lang#52041.
zackmdavis added a commit to zackmdavis/rust that referenced this issue Jul 15, 2018
We also conscientiously note where current work has fallen short.

This is for rust-lang#52041.
bors added a commit that referenced this issue Jul 22, 2018
…_re-pub-lic, r=nikomatsakis

add structured suggestions and fix false-positive for elided-lifetimes-in-paths lint

This adds structured suggestions to the elided-lifetimes-in-paths lint (introduced in Nov. 2017's #46254), prevents it from emitting a false-positive on anonymous (underscore) lifetimes (!), and adds it to the idioms-2018 group (#52041).

~~As an aside, "elided-lifetimes-in-paths" seems like an unfortunate name, because it's not clear exactly what "elided" means. The motivation for this lint (see original issue #45992, and [RFC 2115](https://github.com/rust-lang/rfcs/blob/e978a8d3017a01d632f916140c98802505cd1324/text/2115-argument-lifetimes.md#motivation)) seems to be specifically about not supplying angle-bracketed lifetime arguments to non-`&` types, but (1) the phrase "lifetime elision" has historically also referred to the ability to not supply a lifetime name to `&` references, and (2) an `is_elided` method in the HIR returns true for anoymous/underscore lifetimes, which is _not_ what we're trying to lint here. (That naming confusion is almost certainly what led to the false positive addressed here.) Given that the lint is relatively new and is allow-by-default, is it too late to rename it ... um, _again_ (#50879)?~~

~~This does _not_ address a couple of other false positives discovered in #52041 (comment)

![elided_states](https://user-images.githubusercontent.com/1076988/42302137-2bf9479c-7fce-11e8-8bd0-f29aefc802b6.png)

r? @nikomatsakis
cc @nrc @petrochenkov
@bors bors closed this as completed in 41d5c0c Jul 22, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-edition-2018-lints Area: Lints supporting the 2018 edition
Projects
None yet
Development

No branches or pull requests

3 participants