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

Implementation of the const_static_lifetime lint. #2151

Merged
merged 7 commits into from
Oct 20, 2017

Conversation

gbip
Copy link
Contributor

@gbip gbip commented Oct 18, 2017

Hello,
After a few month, I decided to come back on some unfinished work (previous pr : #1829, issue : #1762).
Currently, the lint has the correct behaviour, but I am struggling setting up the test.

./tests/ui/const_static_lifetime.rs contains some tests, but I can't figure out how to generate a correct .stderr output: ./target/debug/test_build_base/const_static_lifetime.err contains hardcoded strings for file location (tests/ui/const_static_lifetime.rs), whereas other tests contains $DIR/<real_filename>.
Moreover, I have broken two test : ./tests/ui/regex.rs that I fixed (it contained a constant with an explicit static lifetime) and ./tests/ui/for_loop.rs. I can't figure what broke on this done...

Anyway, if some of you want to give some more test cases, I will be glad to add them in the test file.

Thanks for your time !

PS: Please be tolerant, english is not my native language, I am trying my best to avoid mistakes.

@oli-obk
Copy link
Contributor

oli-obk commented Oct 19, 2017

You need to run ./tests/ui/update_all_references.sh after cargo test fails. Then you can commit the changes.

@gbip
Copy link
Contributor Author

gbip commented Oct 19, 2017

Hey, I just fixed the test thing !

Copy link
Contributor

@oli-obk oli-obk left a comment

Choose a reason for hiding this comment

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

Please undo all the unrelated formatting changes or use the most recent rustfmt, which should produce the previous layout.

@@ -33,24 +33,22 @@ declare_lint! {
}

// Tuples are of the form (constant, name, min_digits)
const KNOWN_CONSTS: &'static [(f64, &'static str, usize)] = &[
Copy link
Contributor

Choose a reason for hiding this comment

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

Please don't make unrelated formatting changes

&format!(
"approximate value of `{}::consts::{}` found. \
Consider using it directly",
module,
Copy link
Contributor

Choose a reason for hiding this comment

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

more unrelated formatting changes

if let TyKind::Path(_, _) = borrow_type.ty.node {
// Verify that the path is a str
if lifetime.ident.name == "'static" {
span_help_and_lint(cx,
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you use span_help_and_then and add a suggestion to remove the lifetime? (Search for span_suggestion for examples)

@gbip
Copy link
Contributor Author

gbip commented Oct 20, 2017

I think all your request have been implemented.
If I forgot something, tell me !

Copy link
Contributor

@oli-obk oli-obk left a comment

Choose a reason for hiding this comment

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

Thanks! This is much easier to review.

Just some nits left

OPENING_PAREN,
r"[a-z]+\.(com|org|net)",
]);
let set = RegexSet::new(&[r"[a-z]+@[a-z]+\.(com|org|net)", r"[a-z]+\.(com|org|net)"]);
Copy link
Contributor

Choose a reason for hiding this comment

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

Some leftover formatting changes

let mut sug: String = String::new();
// This should be ok, since we know that the current type is a borrow.
sug.push('&');
sug += &snippet(cx, borrow_type.ty.span, "<expr>").into_owned();
Copy link
Contributor

Choose a reason for hiding this comment

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

The suggestion should be empty. It is the code that is written over 'static, not over &'static T.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, thanks I couldn't figured it out.
Is this for the application that automatically applies suggestions ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yea. In the future rls will probably make use of it, too. So you'll be able to fix these things with a single click on the offending error message.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am currently applying your requests, but cargo test fails because :

error: Unnecessary `!=` operation
   --> clippy_lints/src/loops.rs:754:21
    |
754 | /                     if offset_left.var_name != offset_right.var_name {
755 | |                         Some((offset_left, offset_right))
756 | |                     } else {
757 | |                         None
758 | |                     }
    | |_____________________^
    |
    = note: `-D if-not-else` implied by `-D clippy-pedantic`
    = help: change to `==` and swap the blocks of the if/else
    = help: for further information visit https://rust-lang-nursery.github.io/rust-clippy/v0.0.166/index.html#if_not_else

error: aborting due to previous error

Is this related to the latest nigthly or some error in my changes ?

Copy link
Contributor

Choose a reason for hiding this comment

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

curious. I wonder why travis didn't catch that on the other PRs... I'll push a quick fix that you can rebase over

Copy link
Contributor

Choose a reason for hiding this comment

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

You can rebase over the master branch, I fixed it

@@ -0,0 +1 @@
array
Copy link
Contributor

Choose a reason for hiding this comment

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

Accidentally commited debug output?

@oli-obk
Copy link
Contributor

oli-obk commented Oct 20, 2017

Something really weird happened. you have all of our commits in this PR now.

Try git rebase -i origin/master and remove all commits that aren't yours

@gbip
Copy link
Contributor Author

gbip commented Oct 20, 2017

Thanks, I was looking into it !

@oli-obk oli-obk merged commit f03e12b into rust-lang:master Oct 20, 2017
@oli-obk
Copy link
Contributor

oli-obk commented Oct 20, 2017

Thanks! This lint is so much in the spirit of clippy. Annoying as hell 😆 I will keep doing it wrong until clippy beats 'static out of my muscle memory

@gbip
Copy link
Contributor Author

gbip commented Oct 20, 2017

I really enjoyed implementing this first lint, thanks for your help !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants