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

lint against "silent elision" in structs/enums/etc #45992

Closed
nikomatsakis opened this issue Nov 14, 2017 · 6 comments
Closed

lint against "silent elision" in structs/enums/etc #45992

nikomatsakis opened this issue Nov 14, 2017 · 6 comments
Labels
T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@nikomatsakis
Copy link
Contributor

Now that support for '_ has landed, as part of #44524, it would be good to start linting for cases where it ought to be used. In particular, we wish to warn if there is a struct with lifetime parameters that are elided:

struct Foo<'a> { x: &'a u32 }
fn foo(x: &Foo) {
//         ^^^ warning: hidden lifetime parameters are deprecated, try `Foo<'_>`
}

This should be fairly straightforward to do during resolve_lifetimes.

@nikomatsakis nikomatsakis added E-needs-mentor T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Nov 14, 2017
@nikomatsakis nikomatsakis changed the title lint against "silent elision" in structs lint against "silent elision" in structs/enums/etc Nov 14, 2017
@Dylan-DPC-zz
Copy link

Interested in taking this up 😄

@nikomatsakis
Copy link
Contributor Author

@Dylan-DPC great =)

Sorry, I didn't get a chance to write up any instructions yesterday. I just took a look here and I think this should be fairly straightforward.

The first thing is that we need to add a lint. There are no perfect directions for adding a new lint that I can find. The best that I know of are in this forge document, under the section "Issuing future compatibility warnings". This section describes how to add a future compatibility lint. We don't want a future compatibility lint, just a regular lint, but most of the steps are the same. The one that does not apply is step 3 ,"Register the lint in src/librustc_lint/lib.rs". You can just skip that step.

Let's call the lint elided_lifetime_in_path. For now, let's set the default level to Allow, since we don't want to bothering people until the whole in-band lifetimes RFC is in place.

Once we have the lint, we want to modify the resolve_elided_lifetimes function in resolve_lifetimes.rs. We should add a deprecated: bool argument to it that indicates whether this elided lifetime occurs in a deprecated position.

Most of the callers can use false. The only one that is deprecated is this caller right here:

self.resolve_elided_lifetimes(&params.lifetimes);

Then we just need to add some tests and we're done! To add tests, just make some ui tests that use lifetimes in the deprecated places (as well as places that are not deprecated) and write #![deny(elided_lifetime_in_path)] at the top.

@nikomatsakis
Copy link
Contributor Author

We're also going to want to feature-gate this, though I don't think we have a good mechanism for that. We should figure that out. But let's do this first part first. =)

@nikomatsakis
Copy link
Contributor Author

@Dylan-DPC how goes? Did you get a chance to take a look?

@Dylan-DPC-zz
Copy link

Hey Niko. Was busy. Will try it by this weekend.

bors added a commit that referenced this issue Dec 13, 2017
Implement impl Trait lifetime elision

Fixes #43396.

There's one weird ICE in the interaction with argument-position `impl Trait`. I'm still debugging it-- I've left a test for it commented out with a FIXME.

Also included a FIXME to ensure that `impl Trait` traits are caught under the lint in #45992.

r? @nikomatsakis
bors added a commit that referenced this issue Feb 3, 2018
elided lifetime

Closes #45992

Hey
Having a problem with my config so decided to make a WIP PR nevertheless. Will add some more tests.
@nikomatsakis
Copy link
Contributor Author

fixed in #46254

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.
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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

2 participants