-
Notifications
You must be signed in to change notification settings - Fork 1.7k
New lint : "const with a static lifetime" #1829
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
Changes from all commits
a732d8e
0a4099c
75a0732
bc9ee0b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,63 @@ | ||
use syntax::ast::{Item, ItemKind, TyKind, ExprKind}; | ||
use rustc::lint::{LintPass, EarlyLintPass, LintArray, EarlyContext}; | ||
use utils::span_help_and_lint; | ||
|
||
/// **What it does:** Checks for constants with an explicit `'static` lifetime. | ||
/// | ||
/// **Why is this bad?** Adding `'static` to every reference can create very complicated types. | ||
/// | ||
/// **Known problems:** None. | ||
/// | ||
/// **Example:** | ||
/// ```rust | ||
/// const FOO: &'static [(&'static str, &'static str, fn(&Bar) -> bool)] = &[..] | ||
/// ``` | ||
/// This code can be rewritten as | ||
/// ```rust | ||
/// const FOO: &[(&str, &str, fn(&Bar) -> bool)] = &[...] | ||
/// ``` | ||
|
||
declare_lint! { | ||
pub CONST_STATIC_LIFETIME, | ||
Warn, | ||
"Using explicit `'static` lifetime for constants when elision rules would allow omitting them." | ||
} | ||
|
||
pub struct StaticConst; | ||
|
||
impl LintPass for StaticConst { | ||
fn get_lints(&self) -> LintArray { | ||
lint_array!(CONST_STATIC_LIFETIME) | ||
} | ||
} | ||
|
||
impl EarlyLintPass for StaticConst { | ||
fn check_item(&mut self, cx: &EarlyContext, item: &Item) { | ||
// Match only constants... | ||
if let ItemKind::Const(ref var_type, ref expr) = item.node { | ||
// ... which are actually variables, not declaration such as #![feature(clippy)] ... | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't get this check... I don't think attributes create any constants... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If I added it, it is because I was having warning on things such as https://github.com/Manishearth/rust-clippy/blob/master/tests/cc_seme.rs:l1 and l2. I was running cargo test from the root folder if that helps There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Very confusing... I have no clue how those are related. Can you show the exact compiler output you get without this check? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Of course, here is the result : https://pastebin.com/tzWKUcnV There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. O_o that is really weird. Try dumping the expression itself with debug formatting with println so we see what that thing is. https://github.com/Manishearth/rust-clippy/blob/7535afc0fa9cec38f0fdd3a9deafb44fb3e5fcf1/clippy_lints/src/mut_mut.rs#L36 is an example invocation of a visitor. Look below that for the implementation. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nope, this is rustc inserted. But I'm not sure why... I'll look around. You can try to call the in_macro function, maybe it returns true on the item span? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since this problem is fixed, I'll let you dive into it 😉 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh... sorry about that, should have paid more attention. https://github.com/Manishearth/rust-clippy/blob/3b3e47f4518d83ab625d34e93f0855b713ebd1c3/clippy_lints/src/non_expressive_names.rs#L81 is a (convoluted) example of an AST visitor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I tested it. You just need to add if in_macro(item.span) {
return;
} to the beginning of the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I will take care of finishing the implementation after Wednesday, I'm a little bit busy before... Thanks for the tip ! |
||
// if let ExprKind::Lit(_) = expr.node { | ||
// ... which are reference ... | ||
if let TyKind::Rptr(ref optionnal_lifetime, _) = var_type.node { | ||
// ... and have an explicit 'static lifetime. | ||
if let Some(lifetime) = *optionnal_lifetime { | ||
|
||
println!("item : \n {:?} \n item.node : \n {:?}", item, item.node); | ||
println!("var_type : \n {:?} \n var_type.node : \n {:?}", var_type, var_type.node); | ||
println!("expr : \n {:?} \n expr.node : \n {:?}", expr, expr.node); | ||
println!("lifetime : {:?}", lifetime); | ||
|
||
|
||
if lifetime.ident.name == "'static" { | ||
span_help_and_lint(cx, | ||
CONST_STATIC_LIFETIME, | ||
lifetime.span, | ||
"Constants have by default a `'static` lifetime", | ||
"consider removing the `'static`"); | ||
} | ||
} | ||
// } | ||
} | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -69,7 +69,7 @@ struct SimilarNamesLocalVisitor<'a, 'tcx: 'a> { | |
// this list contains lists of names that are allowed to be similar | ||
// the assumption is that no name is ever contained in multiple lists. | ||
#[cfg_attr(rustfmt, rustfmt_skip)] | ||
const WHITELIST: &'static [&'static [&'static str]] = &[ | ||
const WHITELIST: &[&[&str]] = &[ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Wow that's better 😄 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. C'mon, It's my first time implementing a lint, even touching something relating to compiler stuff, please be tolerant. Even if it is disabled by default I would be happy 😄 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This was a positive comment… 😕 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh I'm sorry, I thought it was sarcastic 😞 |
||
&["parsed", "parser"], | ||
&["lhs", "rhs"], | ||
&["tx", "rx"], | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,13 @@ | ||
#![feature(plugin)] | ||
#![plugin(clippy)] | ||
|
||
const VAR_ONE: &'static str = "Test constant #1"; // ERROR Consider removing 'static. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you add a test for a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Of course ! |
||
|
||
const VAR_TWO: &str = "Test constant #2"; // This line should not raise a warning. | ||
|
||
fn main() { | ||
|
||
println!("{}", VAR_ONE); | ||
println!("{}", VAR_TWO); | ||
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,14 @@ | ||
error: Constants have by default a `'static` lifetime | ||
--> const_static_lifetime.rs:4:17 | ||
| | ||
4 | const VAR_ONE: &'static str = "Test constant #1"; // ERROR Consider removing 'static. | ||
| ^^^^^^^ | ||
| | ||
= note: `-D const-static-lifetime` implied by `-D warnings` | ||
= help: consider removing the `'static` | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You could try adding a suggestion for rustfix 😄 |
||
|
||
error: aborting due to previous error(s) | ||
|
||
error: Could not compile `clippy_tests`. | ||
|
||
To learn more, run the command again with --verbose. |
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 know if this should be Warn by default, at least not now. This warns against a syntax that was the only possible syntax for a long time, and lots of projects are using it now. Plus there is little value in updating old code except for "it's possible that way now".
Maybe we should ask for the lint level to be a dynamic parameter to rustc's devs. That way we could have a
minimal_rustc_version
parameter inclippy.toml
, and only have the lint warn if the minimal version of rustc you want to support is higher that whatever version of rustc added that shortcut.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.
Well.. there was no need to update from try macro invocations to the question mark operator, other than readability. But yes, we should delay making it warn by default for the next few releases. I think 3 releases compatibility is what nursery crates require.