New lint : "const with a static lifetime" #1829

gbip · 2017-06-14T10:58:28Z

Hello,

This is the implementation for a new lint: it warns against 'static lifetimes when declaring constants, as discussed in #1762.
Since all the tests work with this implementation, I guess you can now merge it.
Please take a look at all English sentences, since English is not my native language it might be full of mistakes even if I was very careful.
Thanks !

with a 'static lifetime".

All tests now passe. Added the .stderr file that I have forgotten.

oli-obk

Great initial impl.

oli-obk · 2017-06-14T12:02:45Z

clippy_tests/examples/const_static_lifetime.rs

+#![feature(plugin)]
+#![plugin(clippy)]
+
+const VAR_ONE: &'static str = "Test constant #1"; // ERROR Consider removing 'static.


Can you add a test for a &[&'static str]?

Of course !
I'll also add tests for tuples, I forgot about them.

oli-obk · 2017-06-14T12:05:22Z

clippy_lints/src/const_static_lifetime.rs

+    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)] ...


I don't get this check... I don't think attributes create any constants...

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
I don't know if this was a side effect from my code, but when I inspected the AST there was nodes which where constants without being Lit.

Very confusing... I have no clue how those are related. Can you show the exact compiler output you get without this check?

Of course, here is the result : https://pastebin.com/tzWKUcnV
Can you point me where some piece of code that implement a visitor ? I know about serde's visitor for serializing/deserializing structs, but I am not familiar with this concept...

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.

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?

Since this problem is fixed, I'll let you dive into it 😉
I prefer to spend time trying to implement a visitor.
I am not sure where to start : your example uses stuff from the hir module, but this is an early lint pass, so I'm not sure what to do. I think I will implement a recursive visitor manually.

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

I tested it. You just need to add

if in_macro(item.span) { return; }

to the beginning of the check_item method

I will take care of finishing the implementation after Wednesday, I'm a little bit busy before... Thanks for the tip !

oli-obk · 2017-06-14T12:08:13Z

clippy_lints/src/const_static_lifetime.rs

+            // ... which are actually variables, not declaration such as #![feature(clippy)] ...
+            if let ExprKind::Lit(_) = expr.node {
+                // ... which are reference ...
+                if let TyKind::Rptr(ref optionnal_lifetime, _) = var_type.node {


This only checks for &'static T, but not (&'static T, U). You need to use a visitor to find all 'static lifetimes that are contained in the type.

oli-obk · 2017-06-14T12:09:45Z

clippy_lints/src/const_static_lifetime.rs

+                    // ... and have an explicit 'static lifetime ...
+                    if let Some(lifetime) = *optionnal_lifetime {
+                        if lifetime.ident.name == "'static" {
+                            span_help_and_lint(cx,


You can use span_lint_and_then to add a suggestion to replace the lifetime with the empty string

mcarton · 2017-06-15T20:24:40Z

clippy_lints/src/const_static_lifetime.rs

+
+declare_lint! {
+    pub CONST_STATIC_LIFETIME, 
+    Warn,


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 in clippy.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.

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.

mcarton · 2017-06-15T20:24:46Z

clippy_lints/src/non_expressive_names.rs

@@ -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]] = &[


Wow that's better 😄

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 😄

This was a positive comment… 😕

Oh I'm sorry, I thought it was sarcastic 😞

mcarton · 2017-06-15T20:26:38Z

clippy_tests/examples/const_static_lifetime.stderr

+  |                 ^^^^^^^
+  |
+  = note: `-D const-static-lifetime` implied by `-D warnings`
+  = help: consider removing the `'static`


You could try adding a suggestion for rustfix 😄

gbip · 2017-06-28T16:01:13Z

Since I am now in holiday, I currently have no motivation to try to finish this implementation. I will do it by the end of the summer !

gbip · 2017-10-18T23:22:22Z

I made a new fork (to start fresh since I was a few month late on upstream) and a new pull request at #2151.
I think this PR can be closed.

gbip added 3 commits June 13, 2017 21:54

I have written all the boilerplate for implementing the new lint "const

a732d8e

with a 'static lifetime".

Implementation of the new lint "const with a static lifetime".

0a4099c

Fixed the implementation of the "const_static_lifetime" lint.

75a0732

All tests now passe. Added the .stderr file that I have forgotten.

gbip mentioned this pull request Jun 14, 2017

Lint 'static lifetimes in the type of statics/consts #1762

Closed

oli-obk suggested changes Jun 14, 2017

View reviewed changes

Debug output.

bc9ee0b

mcarton reviewed Jun 15, 2017

View reviewed changes

gbip mentioned this pull request Oct 18, 2017

Implementation of the const_static_lifetime lint. #2151

Merged

oli-obk closed this Oct 19, 2017

New lint : "const with a static lifetime" #1829

New lint : "const with a static lifetime" #1829

Uh oh!

Conversation

gbip commented Jun 14, 2017

Uh oh!

oli-obk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gbip commented Jun 28, 2017

Uh oh!

gbip commented Oct 18, 2017

Uh oh!

Uh oh!