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

Add lint for statics with explicit static lifetime. #4162

Merged
merged 9 commits into from
Jun 14, 2019

Conversation

krk
Copy link
Contributor

@krk krk commented Jun 1, 2019

changelog: Add lint for statics with explicit static lifetime, fixes #4138.

@flip1995 flip1995 added the S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) label Jun 3, 2019
@flip1995
Copy link
Member

flip1995 commented Jun 3, 2019

Please run util/dev update_lints

@oli-obk
Copy link
Contributor

oli-obk commented Jun 3, 2019

does anything speak against just extending the lint for constants to cover statics, too? https://rust-lang.github.io/rust-clippy/master/index.html#const_static_lifetime

@krk
Copy link
Contributor Author

krk commented Jun 10, 2019

@flip1995 Running util/dev update_lints did not introduce any changes.

@flip1995
Copy link
Member

Travis disagrees: https://travis-ci.com/rust-lang/rust-clippy/jobs/204695122#L1283

Not all lints defined properly. Please run `util/dev update_lints` to make sure all lints are defined properly.

CHANGELOG and README need updates.

But I agree with #4162 (comment). At least you should be able reuse the logic there?

@krk krk force-pushed the static-static branch from dec4f33 to 9b09257 Compare June 10, 2019 16:09
@krk
Copy link
Contributor Author

krk commented Jun 10, 2019

I have rebased and pushed, travis is green now, will try to reuse the logic.

@flip1995
Copy link
Member

Thanks! Can you put the code of the lints into the same file (and into the same EarlyLintPass)? You can rename the file, if you want (If you do, please rename in a separate commit).

@flip1995
Copy link
Member

You should also be able to use the Visitor provided by rustc: https://doc.rust-lang.org/nightly/nightly-rustc/syntax/visit/trait.Visitor.html?search=#method.visit_ty, instead of rewriting portions of it.

@krk
Copy link
Contributor Author

krk commented Jun 11, 2019

You should also be able to use the Visitor provided by rustc: https://doc.rust-lang.org/nightly/nightly-rustc/syntax/visit/trait.Visitor.html?search=#method.visit_ty, instead of rewriting portions of it.

visit_type method in clippy_lints/src/const_static_lifetime.rs existed before this PR and I am reluctant to include changing it to the rustc visitor here.

@flip1995
Copy link
Member

That's ok, you can use the existing function!

LGTM, just one NIT and this can get merged.

Copy link
Member

@flip1995 flip1995 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just realized, that you also renamed the lint and not only the file. That's fine IMO, but renaming a lint has two extra steps.

  1. You have to register the renaming here:
    pub fn register_renamed(ls: &mut rustc::lint::LintStore) {
    ls.register_renamed("clippy::stutter", "clippy::module_name_repetitions");
    ls.register_renamed("clippy::new_without_default_derive", "clippy::new_without_default");
    ls.register_renamed("clippy::cyclomatic_complexity", "clippy::cognitive_complexity");
    }
  2. You have to add a test for the renamed lint here:
    https://github.com/rust-lang/rust-clippy/blob/master/tests/ui/rename.rs

Can you rename the lint to REDUNDANT_STATIC_LIFETIMES, that follows the naming conventions.

@flip1995
Copy link
Member

Thanks!

@bors r+

@bors
Copy link
Contributor

bors commented Jun 14, 2019

📌 Commit 38d5e71 has been approved by flip1995

@bors
Copy link
Contributor

bors commented Jun 14, 2019

⌛ Testing commit 38d5e71 with merge a505984...

bors added a commit that referenced this pull request Jun 14, 2019
Add lint for statics with explicit static lifetime.

changelog: Add lint for statics with explicit static lifetime, fixes #4138.
@flip1995
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Jun 14, 2019

📌 Commit 7e07d1b has been approved by flip1995

@bors
Copy link
Contributor

bors commented Jun 14, 2019

⌛ Testing commit 7e07d1b with merge 7a95c20...

bors added a commit that referenced this pull request Jun 14, 2019
Add lint for statics with explicit static lifetime.

changelog: Add lint for statics with explicit static lifetime, fixes #4138.
@bors
Copy link
Contributor

bors commented Jun 14, 2019

☀️ Test successful - checks-travis, status-appveyor
Approved by: flip1995
Pushing 7a95c20 to master...

@bors bors merged commit 7e07d1b into rust-lang:master Jun 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Check for static items with 'static lifetime
4 participants