Lint against single-use lifetime names #46441
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
kennytm
added
the
S-waiting-on-author
oli-obk
reviewed
Dec 2, 2017
src/librustc/lint/builtin.rs
Outdated
| /// Does nothing as a lint pass, but registers some `Lint`s | ||
| /// which are used by other parts of the compiler. | ||
| #[derive(Copy, Clone)] | ||
| pub struct HardwiredLints; | ||
|
|
||
| impl LintPass for HardwiredLints { | ||
| fn get_lints(&self) -> LintArray { | ||
| lint_array!( | ||
| UNUSED_IMPORTS, |
There was a problem hiding this comment.
fixed formatting changes @oli-obk
| LateBound(ty::DebruijnIndex, /* lifetime decl */ DefId), | ||
| LateBoundAnon(ty::DebruijnIndex, /* anon index */ u32), | ||
| Free(DefId, /* lifetime decl */ DefId), | ||
| EarlyBound(/* index */ |
There was a problem hiding this comment.
Please move the formatting changes into their own commit or undo them. It's very hard to see the non-formatting changes among them
|
Regarding the formatting changes, what I usually find helpful is to have the first commit just rustfmt's the whole file, and then start from there. Then when you rebase, if you get conflicts in this commit, you can just take the "master" version of the code indiscriminately and then re-run rustfmt afterwards. (This...mostly works. Sometimes I find code gets duplicated.) |
gaurikholkar-zz
force-pushed
the
single_lifetimes
branch
from
0546196
to
fc727d6
Compare
|
I guess most of it is undone. Will take care of the rest in a separate commit |
|
@nikomatsakis @oli-obk I get this error |
|
Will fix the ui tests |
|
They are failing with an ICE: |
|
Afk for a while. Will check as asap |
|
☔ The latest upstream changes (presumably #46305) made this pull request unmergeable. Please resolve the merge conflicts. |
gaurikholkar-zz
commented
Dec 5, 2017
|
You can write |
gaurikholkar-zz
force-pushed
the
single_lifetimes
branch
from
3cb47ef
to
1cfe610
Compare
Merged
|
So I feel like the new code is generating unexpected errors. I'm going to poke around a bit. |
|
I'm going to open a PR with just the resolve-lifetimes query work, so we can isolate what is affecting what. |
|
Blocked on #46657 |
gaurikholkar-zz
changed the title
|
☔ The latest upstream changes (presumably #46657) made this pull request unmergeable. Please resolve the merge conflicts. |
|
OK #46657 landed -- @gaurikholkar, can you rebase atop that? |
|
@nikomatsakis at it |
gaurikholkar-zz
force-pushed
the
single_lifetimes
branch
3 times, most recently
from
affef5e
to
c84e49c
Compare
|
@nikomatsakis build passed :) |
nikomatsakis
requested changes
Dec 18, 2017
| self.visit_early_late(None, | ||
| decl, | ||
| generics, | ||
| |this| { intravisit::walk_item(this, item); }); |
| self.visit_early_late(None, | ||
| decl, | ||
| generics, | ||
| |this| { intravisit::walk_foreign_item(this, item); }) |
| @@ -1132,8 +1145,8 @@ impl<'a, 'tcx> LifetimeContext<'a, 'tcx> { | |||
| tcx, ref mut map, .. | |||
| } = *self; | |||
| let labels_in_fn = replace(&mut self.labels_in_fn, vec![]); | |||
| let xcrate_object_lifetime_defaults = | |||
| replace(&mut self.xcrate_object_lifetime_defaults, DefIdMap()); | |||
| let xcrate_object_lifetime_defaults = replace(&mut self.xcrate_object_lifetime_defaults, | |||
| &LifetimeUseSet::One(_) => { | ||
| let node_id = this.tcx.hir.as_local_node_id(*def_id).unwrap(); | ||
| debug!("nod id first={:?}", node_id); | ||
| let hir_lifetime: Option<&hir::Lifetime> = match this.tcx.hir.get(node_id) { |
There was a problem hiding this comment.
would do:
if let Some(hir_lifetime) = this.tcx.hir.get(node_id) {
let span = hir_lifetime.span;
let id = hir_lifetime.span;
// existing code:
debug!("id ={:?} span = {:?} hir_lifetime = {:?}",
node_id,
span,
hir_lifetime);
this.tcx
.struct_span_lint_node(lint::builtin::SINGLE_USE_LIFETIME,
id,
span,
&format!("lifetime name `{}` only used once",
hir_lifetime.unwrap().name.name()))
.emit();
}| @@ -1239,9 +1289,7 @@ impl<'a, 'tcx> LifetimeContext<'a, 'tcx> { | |||
| } | |||
| } | |||
|
|
|||
| fn resolve_lifetime_ref(&mut self, lifetime_ref: &hir::Lifetime) { | |||
| debug!("resolve_lifetime_ref(lifetime_ref={:?})", lifetime_ref); | |||
There was a problem hiding this comment.
Nnit: removed handy debug output
| hir::map::NodeForeignItem(_) | hir::map::NodeTy(_) | hir::map::NodeTraitRef(_) => None, | ||
| hir::map::NodeForeignItem(_) | | ||
| hir::map::NodeTy(_) | | ||
| hir::map::NodeTraitRef(_) => None, |
| for i in 0..lifetimes.len() { | ||
| let lifetime_i = &lifetimes[i]; | ||
|
|
||
| for lifetime in lifetimes { | ||
| match lifetime.lifetime.name { | ||
| hir::LifetimeName::Static | hir::LifetimeName::Underscore => { | ||
| hir::LifetimeName::Static | | ||
| hir::LifetimeName::Underscore => { |
| @@ -1909,7 +1960,8 @@ impl<'a, 'tcx> LifetimeContext<'a, 'tcx> { | |||
| ); | |||
| err.emit(); | |||
| } | |||
| hir::LifetimeName::Implicit | hir::LifetimeName::Name(_) => {} | |||
| hir::LifetimeName::Implicit | | |||
| hir::LifetimeName::Name(_) => {} | |||
| // except according to those terms. | ||
| #![deny(single_use_lifetime)] | ||
|
|
||
| fn deref<'x>(v: &'x u32) -> u32 { //~ ERROR lifetime name `'x` only used once |
There was a problem hiding this comment.
can you add a test
fn deref<'x>() -> &'x u32 {
22
}Ideally, this would not warn, but I suspect your code will. I think that's ok, we can just add a comment like:
// FIXME(#44752) -- this scenario should not be warned
Here are some other scenarios. I will put down the behavior I think we eventually want, but it may not be the behavior that we currently have.
struct Foo<'x> {
x: &'x u32 // no warning!
}
// Once #44524 is fixed, this should issue a warning. Hence, it's ok if it does so now, though perhaps a bit premature.
impl<'y> Foo<'y> {
fn method() { }
}// Neither should issue a warning, as explicit lifetimes are mandatory in these cases:
struct Foo<'x> {
x: &'x u32
}
enum Bar<'x> {
Variant(&'x u32)
}// Should not issue a warning, as explicit lifetimes are mandatory in these cases:
trait Foo<'x> {
fn foo(&self, arg: &'x u32);
}
|
Oh, in my review, I realize I wasn't clear. The "Nit: formatting" bits was just meant to say that these changes weren't necessary, though also not harmful. Ideally they would be isolated into a separate commit (or else pulled out). |
gaurikholkar-zz
force-pushed
the
single_lifetimes
branch
2 times, most recently
from
001aa98
to
cfbff3d
Compare
nikomatsakis
approved these changes
Dec 19, 2017
gaurikholkar-zz
force-pushed
the
single_lifetimes
branch
from
cfbff3d
to
e741dad
Compare
|
@bors r+ |
|
📌 Commit e741dad has been approved by |
kennytm
added
S-waiting-on-bors
bors
added a commit
that referenced
this pull request
Dec 20, 2017
Lint against single-use lifetime names This is a fix for #44752 TO-DO - [x] change lint message - [x] add ui tests r? @nikomatsakis
|
☀️ Test successful - status-appveyor, status-travis |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a fix for #44752
TO-DO
r? @nikomatsakis