-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
add structured suggestions and fix false-positive for elided-lifetimes-in-paths lint #52069
The head ref may contain hidden characters: "elided_states_of_america\u2014and_to_the_re-pub-lic"
add structured suggestions and fix false-positive for elided-lifetimes-in-paths lint #52069
Conversation
(Some would argue that we shouldn't give the lint the increased prominence of the idioms-2018 group (in contrast to allow-by-default, which I fear almost no one turns up) until we fix the false-positive exemplified by |
I'm good with another name, do you have a suggestion? When it comes to the term "elided", there are two schools of thought in any case:
Perhaps best to find another term altogether that is not as contentious. =) It's sort of unclear what to call a
|
Which ones are not addressed? It'd be nice to update the issue to have a current list of known false positives. |
src/librustc/lint/builtin.rs
Outdated
@@ -252,7 +252,7 @@ declare_lint! { | |||
declare_lint! { | |||
pub ELIDED_LIFETIMES_IN_PATHS, | |||
Allow, | |||
"hidden lifetime parameters are deprecated, try `Foo<'_>`" | |||
"implicit lifetime parameters are deprecated" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as names go, my take:
implicit
is ok, but it tends to mean anything that the reader wants it to meanhidden
andinvisible
I think are both ok
I don't think any name will necessarily convey perfectly what we are going for -- but all 3 names are roughly in the right direction. I would summarize as "a lifetime that you can't tell is there from looking at the type". For this, I like "hidden" or "invisible" better than "implicit".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you very much for taking this up! Looks awesome. 💯 I have a few suggestions.
&format!("implicit lifetime parameters in types are deprecated"), | ||
); | ||
|
||
if num_implicit_lifetimes == 1 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why only the case of 1 implicit lifetime?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like the logic below would apply to any number of lifetimes, if we just generalized the string.
(segment_ident.span.shrink_to_hi(), "<'_>") | ||
} else { | ||
// Otherwise—sorry, this is kind of gross—we need to infer the | ||
// replacement point span from the generics that do exist |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see a better way, but I get nervous around macros and things -- e.g., what will happen if somebody does:
Ref<$x>
in a macro? (I'm imagining that the span of $x
might then come from some other place than the type itself...?) Maybe we want a test around that case? I think it'd be fine to disable suggestions there, but it'd be nice to ICE or suggest something mangled?
let mut first_generic_span = None; | ||
for ref arg in &generic_args.args { | ||
match arg { | ||
hir::GenericArg::Lifetime(lt) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there cannot be a lifetime name here... you can totally elide lifetimes (e.g., Foo<T>
) but you cannot write only some of the needed lifetimes (e.g., Foo<'_, T>
when Foo<'_, '_, T>
is needed) -- you'll get an error in the latter case and (probably?) not go down this path. Should test though.
#![deny(elided_lifetimes_in_paths)] | ||
//~^ NOTE lint level defined here | ||
|
||
use std::cell::{RefCell, Ref}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we test:
- some case with more than one parameter (just for fun, though I don't see why it wouldn't work)
- a case where there is more than one lifetime and you only supply some?
- the macro case where the type is constructed in a macro from bits and pieces?
☔ The latest upstream changes (presumably #52275) made this pull request unmergeable. Please resolve the merge conflicts. |
It's only with reluctance and sadness that we rename a lint that has already been renamed once (rust-lang#50879), but it seems worth it to pick the best name now because since the lint is relatively new and has heretofore been allow-by-default, the ecosystem breakage should be minimal. (And—also sadly—the fact that the original implementation was so buggy for so long testifies that not very many people are tuning up the allow-by-default lints. Also, as always, lint capping prevents lint changes from spreading contagiously to dependencies.) The rationales here are that— • "hidden" is less potentially ambiguous than "elided", because this lint is specifically about angle-bracketed lifetime parameters, whereas the term "elided" has a strong precedent for also encompassing omitted lifetime names in reference ('&') types, which is not the concern of this lint, and • "types" is a more specific description of where the lint fires than "paths" (indeed, previous implementations of the lint used to fire on non-type paths in ways that proved to be erroneous false-positives, as evidenced by applications of the suggestion to use an anonymous lifetime (`'_`) resulting in code that didn't even parse) This comes from discussion on rust-lang#52069.
6cc4481
to
681b321
Compare
(When you're back from vacation.)
I couldn't find a situation where inserting #![allow(unused)]
#![warn(elided_lifetimes_in_paths)]
struct DoubleLifetime<'a, 'b> {
one: &'a str,
another: &'b str,
}
fn returns_double_lifetime(one: &'a str, another: &'b str) -> DoubleLifetime {
DoubleLifetime { one, another }
}
fn main() {} The lint does fire on the return type of
Adding
I think this makes sense—if there are two distinct "output" lifetimes (in the terminology of RFC 141), they can't both be anonymous, because they need names to disambiguate which is which, right? And this is a different scenario from when we can legitimately use multiple anonymous input lifetimes
I think I've fixed them now. What was happening is that the lint would fire on the type path in struct construction expressions—
—and in associated-functions/static-methods (the motivating examples being
In both these cases, applying the suggestion results in code that fails to even parse ("expected To avoid this, I first moved the lint check from (a callee of) the
Thanks for clarifying this. However, unless I can convince myself that we error out before we get here, I think I'm inclined to naïvely obey the type system rather than asserting that the
The edition guide calls it an "anonymous" lifetime, which seems appropriate (it's not omitted from the source, it just doesn't have a name). I've followed this terminology in the span label here ("indicate the anonymous lifetime").
After re-consulting RFC 141, I'm not enthusiastic about renaming the lint anymore. (The reason I thought we wanted to rename is because I had mentally associated the word "elision" with |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
(CI error is mysterious; this PR does not touch librustc_metadata) (or, less mysteriously, perhaps some of the changes that I thought were only lint-related have actually broken borrowck 😰 ) |
You definitely broke something, the question is whether this code wasn't broken to begin with: let registrar = unsafe {
let sym = match lib.symbol(&sym) {
Ok(f) => f,
Err(err) => self.sess.span_fatal(span, &err),
};
mem::transmute::<*mut u8, fn(&mut dyn Registry)>(sym)
}; |
Thanks, my code is almost certainly doing the wrong thing on I'll need to rethink this (maybe we can store some extra state on the |
Perhaps this? #![allow(unused)]
#![warn(elided_lifetimes_in_paths)]
struct DoubleLifetime<'a, 'b> {
one: &'a str,
another: &'b str,
}
fn accepts_double_lifetime(dl: DoubleLifetime) -> String {
format!("{} {}", dl.one, dl.another)
}
fn main() {}
Hmm, yes, I can imagine that. I wonder if we are doing this lint in the wrong place. Maybe we should do it during HIR lowering...? After all, that is where we actually insert the various markers in HIR in the first place. I forget why I didn't suggest that route. |
I would say it's not the worst thing ever, but it seems suboptimal. My goal is roughly to make lifetime parameters work the same as types -- that is, whenever you would have to write the type parameters for |
☔ The latest upstream changes (presumably #52264) made this pull request unmergeable. Please resolve the merge conflicts. |
(This is looking like it's going to be much better, but for now my build is mysteriously ICEing and I need to sleep.) |
681b321
to
6fdcc60
Compare
@nikomatsakis ready 🏁 |
@bors r+ -- looks nice |
📌 Commit 6fdcc60d6314d111acf4ea8e4e82cfd20f55f998 has been approved by |
@kennytm @nikomatsakis I forgot that integer overflow panics (in non-release builds). Should be fixed now. |
@bors r+ |
📌 Commit 41d5c0c has been approved by |
…_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
☀️ Test successful - status-appveyor, status-travis |
📣 Toolstate changed by #52069! Tested on commit ffaf3d2. 💔 clippy-driver on windows: test-fail → build-fail (cc @Manishearth @llogiq @mcarton @oli-obk, @rust-lang/infra). |
Tested on commit rust-lang/rust@ffaf3d2. Direct link to PR: <rust-lang/rust#52069> 💔 clippy-driver on windows: test-fail → build-fail (cc @Manishearth @llogiq @mcarton @oli-obk, @rust-lang/infra). 💔 clippy-driver on linux: test-fail → build-fail (cc @Manishearth @llogiq @mcarton @oli-obk, @rust-lang/infra). 💔 rls on windows: test-pass → build-fail (cc @nrc, @rust-lang/infra). 💔 rls on linux: test-pass → build-fail (cc @nrc, @rust-lang/infra).
↑ I'm confused why this would break Clippy given that Clippy is not |
I think we have |
This seemed like a good way to kick the tires on the elided-lifetimes-in-paths lint (rust-lang#52069)—seems to work! This was also pretty tedious—it sure would be nice if `cargo fix` worked on this codebase (rust-lang#53896)!
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) 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) anis_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).r? @nikomatsakis
cc @nrc @petrochenkov