Implement "Use last" lint #3832

HarrisonMc555 · 2019-02-28T21:51:25Z

This lint checks the use of x.get(x.len() - 1) and suggests x.last() (where x is a Vec).

There's at least one more thing that needs to happen here. I believe I am correctly checking most of the scenarios and avoiding false positives. However, if different Vecs are used for the x.get and y.len, then it will emit a false positive. In other words, I need to check to make sure the Vecs are the same.

Also, a lot of these commits were temporary and not helpful to the project history...am I supposed to squash on merge? If so, how do I do that?

changelog: New lint: get_last_with_len

oli-obk

You can squash commits by using

git rebase -i origin/master

(assuming the clippy repo is "origin" in your setup). That should open an editor which includes instructions. Mostly it's changing all but the first commit to "fixup" instead of "pick"

oli-obk · 2019-03-05T14:34:48Z

clippy_lints/src/lib.rs

@@ -6,7 +6,8 @@
 #![feature(stmt_expr_attributes)]
 #![feature(range_contains)]
 #![allow(clippy::missing_docs_in_private_items)]
-#![recursion_limit = "256"]
+// #![recursion_limit = "256"]


remove this comment

oli-obk · 2019-03-05T14:37:47Z

clippy_lints/src/use_last.rs

+            // let _ = println!("LHS of sub method has an arg");
+
+            // TODO: Is this a valid way to check if they reference the same vector?
+            // if arg_lhs_struct.hir_id == struct_calling_on.hir_id;


you need to check whether the two expressions are equal by structure. I think we have an expr_eq function or similar code somewhere in clippy.

oli-obk · 2019-03-05T14:38:33Z

tests/ui/use_last.rs

+fn dont_use_last() -> Option<i32> {
+    let x = vec![2, 3, 5];
+    let last_element = x.get(x.len() - 1); // ~ERROR Use x.last()
+    last_element.map(|val| val + 1) // To avoid warnings


you could just use let _ = x.get ..... The underscore silences the warnings, too

phansch · 2019-04-08T06:35:03Z

friendly ping @HarrisonMc555 would you like to continue with this? Feel free to ask questions if you're stuck somewhere.

bors · 2019-04-08T07:46:09Z

☔ The latest upstream changes (presumably #3848) made this pull request unmergeable. Please resolve the merge conflicts.

HarrisonMc555 · 2019-04-10T00:58:19Z

Yes, sorry. I've been caught up with other things. I'll try to get this done within the next week or two. Thanks for the ping!

HarrisonMc555 · 2019-04-16T22:39:08Z

I believe I finished everything, sorry it took a while. I can squash if you want.

Also, I couldn't run rustfmt. When I run cargo fmt I get:

error: 'cargo-fmt' is not installed for the toolchain 'master'

When I run rustup component add rustfmt, I get:

error: toolchain 'master' does not support components

I looked for an issue and I saw a merged pull request about switching to nightly (#3586) but I couldn't get that to work either. Can we add something to adding_lints.md?

oli-obk · 2019-04-17T07:36:32Z

You can use cargo +nightly fmt

bors · 2019-04-18T07:50:16Z

☔ The latest upstream changes (presumably #3968) made this pull request unmergeable. Please resolve the merge conflicts.

HarrisonMc555 · 2019-04-18T15:08:26Z

I was wondering why the CI build failed...I forgot how fast things can change :-)

I just merged the changes, updated the README, and ran rustfmt (actually I ran rustfmt afterwards but it didn't change anything, thank goodness).

I think we should be good, we'll see in a few minutes if the CI build passes now. Thanks for the help everyone!

HarrisonMc555 · 2019-04-18T15:55:29Z

So Travis CI failed, and I'm not sure why. I just re-ran git pull upstream master into my master branch and git merge master into my use_last branch.

I then ran cargo test, and all the tests pass. Any tips on resolving this? I'm not sure what's going on.

andrehjr · 2019-04-18T16:08:37Z

@HarrisonMc555 Seems like it's still failing because of cargo fmt 🤔 maybe you need to update rustfmt? Anyway, if you fix the following, you should be good to go.

Diff in /Users/travis/build/rust-lang/rust-clippy/clippy_lints/src/use_last.rs at line 1:
 //! lint on using `x.get(x.len() - 1)` instead of `x.last()`
 
-use crate::utils::{match_type, paths, span_lint_and_sugg,
-                   snippet_with_applicability, SpanlessEq};
+use crate::utils::{match_type, paths, snippet_with_applicability, span_lint_and_sugg, SpanlessEq};
+use if_chain::if_chain;
+use rustc::hir::{Expr, ExprKind};
 use rustc::lint::{LateContext, LateLintPass, LintArray, LintPass};
 use rustc::{declare_tool_lint, lint_array};
-use rustc::hir::{Expr, ExprKind};
 use rustc_errors::Applicability;
-use syntax::ast::{LitKind};
-use if_chain::if_chain;
+use syntax::ast::LitKind;
 
 /// **What it does:** Checks for using `x.get(x.len() - 1)` instead of `x.last()`.
 ///
The command "if [ -z ${INTEGRATION} ]; then```

clippy_lints/src/use_last.rs

HarrisonMc555 · 2019-04-18T18:48:31Z

Ok I finally figured out how to run rustfmt. This is how I did it:

rustup component add --toolchain=nightly rustfmt
rustup run nightly rustfmt path/to/file.rs

Also I used the new declare_lint_pass! macro. Hopefully this passes CI now :-)

HarrisonMc555 · 2019-04-19T15:24:01Z

@andrehjr everything passed now, looks like we should be able to merge. Let me know if there's anything else that needs to be done!

HarrisonMc555 · 2019-04-19T22:00:13Z

Hmm so I had a passing CI build, but some other commits were pushed before this was merged. I pulled the new commits and everything merged on my machine fine.

Now, the Linux CI build passes but the macOS build fails. How do I go about fixing this? I don't have a macOS machine available.

tests/ui/use_last.rs

phansch · 2019-04-21T07:39:26Z

Now, the Linux CI build passes but the macOS build fails.

Woops, I forgot to submit the comment yesterday. It was only a spurious network issue and a restart of the Travis build fixed it.

clippy_lints/src/use_last.rs

bors · 2019-04-30T21:09:29Z

☔ The latest upstream changes (presumably #4039) made this pull request unmergeable. Please resolve the merge conflicts.

HarrisonMc555 · 2019-05-14T16:04:38Z

I have some more time this week, so I should get it done within the next day or two. However, I end up working on a computer both at school and at home, so I'm pushing and pulling to keep them in sync...which I believe will trigger Travis builds, even when I don't intend it to. Is there any way to stop this? I just feel guilty causing an unnecessary build takes time away from necessary builds.

flip1995 · 2019-05-14T19:18:37Z

You could push to a branch unrelated to this PR and when you're finished merge that branch into the branch of this PR.

HarrisonMc555 · 2019-05-14T21:33:43Z

Ok, I think the pull request is ready. All the tests pass, and I believe I've addressed all the concerns. I've left the "conversations" open that I didn't take as literal suggestions. Sorry for not using the new GitHub built-in suggestion inserter thing.

flip1995

rustfmt doesn't format code inside macros. Please format the code by hand, so it more or less matches the style of the codebase.

Please also remove the tests/ui/get_last_with_len.stdout file

clippy_lints/src/get_last_with_len.rs

tests/ui/get_last_with_len.stderr

flip1995

This LGTM now. Thanks for all the work!

Just 2 formatting things and squashing the commits and this is good to go.

clippy_lints/src/get_last_with_len.rs

bors · 2019-05-20T10:00:55Z

☔ The latest upstream changes (presumably #4088) made this pull request unmergeable. Please resolve the merge conflicts.

Manishearth · 2019-05-20T15:31:09Z

Can you rebase over master instead of merging?

Manishearth · 2019-05-20T15:31:40Z

It may be easier to create a fresh commit with the diff from the PR

HarrisonMc555 · 2019-05-20T15:58:51Z

@Manishearth sure thing. What is the recommended way to do that? Do I create a patch with all the changes and add it to a new branch or delete the old commits from this branch or what?

HarrisonMc555 · 2019-05-20T17:19:31Z

Did I do this right? I started doing git rebase -i but there were like 37 steps and they seemed redundant. So I just created a patch from master, reset back to master, applied the patch, and did git push --force-with-lease. Let me know if that's the right idea or if I need to do the tedious git rebase -i stuff 😄

Manishearth · 2019-05-20T17:34:15Z

That's fine! That's what I meant by creating a fresh commit.

HarrisonMc555 · 2019-05-20T20:13:59Z

Ok great! So is everything done now? I think GitHub is saying there's still a change requested from you @Manishearth.

flip1995 · 2019-05-21T07:35:42Z

Yes, that's all. Thanks for all the work!

@bors r+

bors · 2019-05-21T07:35:43Z

📌 Commit 3271491 has been approved by flip1995

Implement "Use last" lint Closes #3673 This lint checks the use of `x.get(x.len() - 1)` and suggests `x.last()` (where `x` is a `Vec`). There's at least one more thing that needs to happen here. I believe I am correctly checking most of the scenarios and avoiding false positives. However, if different `Vec`s are used for the `x.get` and `y.len`, then it will emit a false positive. In other words, I need to check to make sure the `Vec`s are the same. Also, a lot of these commits were temporary and not helpful to the project history...am I supposed to squash on merge? If so, how do I do that? changelog: New lint: `get_last_with_len`

bors · 2019-05-21T07:35:52Z

⌛ Testing commit 3271491 with merge 4a2ff35...

bors · 2019-05-21T07:36:20Z

💡 This pull request was already approved, no need to approve it again.

This pull request is currently being tested. If there's no response from the continuous integration service, you may use retry to trigger a build again.

bors · 2019-05-21T07:36:21Z

📌 Commit 3271491 has been approved by flip1995

bors · 2019-05-21T07:36:29Z

⌛ Testing commit 3271491 with merge 64f4f62...

Implement "Use last" lint Closes #3673 This lint checks the use of `x.get(x.len() - 1)` and suggests `x.last()` (where `x` is a `Vec`). There's at least one more thing that needs to happen here. I believe I am correctly checking most of the scenarios and avoiding false positives. However, if different `Vec`s are used for the `x.get` and `y.len`, then it will emit a false positive. In other words, I need to check to make sure the `Vec`s are the same. Also, a lot of these commits were temporary and not helpful to the project history...am I supposed to squash on merge? If so, how do I do that? changelog: New lint: `get_last_with_len`

bors · 2019-05-21T08:15:32Z

💔 Test failed - checks-travis

flip1995 · 2019-05-21T08:47:37Z

https://travis-ci.com/rust-lang/rust-clippy/jobs/201732318#L1010

cc #4121

HarrisonMc555 · 2019-05-22T23:44:28Z

There was a minor hiccup when attempting to merge formatting suggestions but it's been resolved. All tests are passing and all changes are approved. Are we ready to merge?

There have been a few commits since I squashed, do you want me to merge, re-squash, and update the branch again?

flip1995 · 2019-05-27T11:04:31Z

do you want me to merge, re-squash, and update the branch again?

No it should be good as is, so this should work:

@bors r+

(Sorry, I was busy the past 4 days)

bors · 2019-05-27T11:04:33Z

📌 Commit 42d849c has been approved by flip1995

bors · 2019-05-27T11:04:46Z

⌛ Testing commit 42d849c with merge cf81a3b...

Implement "Use last" lint Closes #3673 This lint checks the use of `x.get(x.len() - 1)` and suggests `x.last()` (where `x` is a `Vec`). There's at least one more thing that needs to happen here. I believe I am correctly checking most of the scenarios and avoiding false positives. However, if different `Vec`s are used for the `x.get` and `y.len`, then it will emit a false positive. In other words, I need to check to make sure the `Vec`s are the same. Also, a lot of these commits were temporary and not helpful to the project history...am I supposed to squash on merge? If so, how do I do that? changelog: New lint: `get_last_with_len`

bors · 2019-05-27T11:33:26Z

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

HarrisonMc555 · 2019-05-28T03:49:58Z

Thanks @flip1995! No problem :-)

phansch added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Mar 1, 2019

oli-obk reviewed Mar 5, 2019

View reviewed changes

flip1995 added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Mar 9, 2019

andrehjr reviewed Apr 18, 2019

View reviewed changes

clippy_lints/src/use_last.rs Outdated Show resolved Hide resolved

flip1995 reviewed Apr 20, 2019

View reviewed changes

tests/ui/use_last.rs Outdated Show resolved Hide resolved

flip1995 requested changes Apr 23, 2019

View reviewed changes

flip1995 requested changes May 15, 2019

View reviewed changes

clippy_lints/src/get_last_with_len.rs Outdated Show resolved Hide resolved

clippy_lints/src/get_last_with_len.rs Outdated Show resolved Hide resolved

clippy_lints/src/get_last_with_len.rs Outdated Show resolved Hide resolved

tests/ui/get_last_with_len.stderr Outdated Show resolved Hide resolved

flip1995 reviewed May 20, 2019

View reviewed changes

clippy_lints/src/get_last_with_len.rs Outdated Show resolved Hide resolved

clippy_lints/src/get_last_with_len.rs Outdated Show resolved Hide resolved

HarrisonMc555 force-pushed the use_last branch from 60ddb60 to e4f66a8 Compare May 20, 2019 17:16

Implement get_last_with_len lint

f32c2fc

HarrisonMc555 force-pushed the use_last branch from e4f66a8 to f32c2fc Compare May 21, 2019 00:01

flip1995 approved these changes May 21, 2019

View reviewed changes

Formatting inside if_chain! macro

42d849c

flip1995 force-pushed the use_last branch from 3271491 to 42d849c Compare May 21, 2019 08:45

flip1995 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties and removed S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels May 22, 2019

bors merged commit 42d849c into rust-lang:master May 27, 2019

HarrisonMc555 deleted the use_last branch May 28, 2019 03:48

Implement "Use last" lint #3832

Implement "Use last" lint #3832

Conversation

HarrisonMc555 commented Feb 28, 2019 • edited by flip1995 Loading

oli-obk left a comment

Choose a reason for hiding this comment

oli-obk Mar 5, 2019

Choose a reason for hiding this comment

oli-obk Mar 5, 2019

Choose a reason for hiding this comment

oli-obk Mar 5, 2019

Choose a reason for hiding this comment

phansch commented Apr 8, 2019

bors commented Apr 8, 2019

HarrisonMc555 commented Apr 10, 2019

HarrisonMc555 commented Apr 16, 2019 • edited Loading

oli-obk commented Apr 17, 2019

bors commented Apr 18, 2019

HarrisonMc555 commented Apr 18, 2019

HarrisonMc555 commented Apr 18, 2019

andrehjr commented Apr 18, 2019

HarrisonMc555 commented Apr 18, 2019

HarrisonMc555 commented Apr 19, 2019

HarrisonMc555 commented Apr 19, 2019

phansch commented Apr 21, 2019 • edited Loading

bors commented Apr 30, 2019

HarrisonMc555 commented May 14, 2019

flip1995 commented May 14, 2019

HarrisonMc555 commented May 14, 2019

flip1995 left a comment

Choose a reason for hiding this comment

flip1995 left a comment

Choose a reason for hiding this comment

bors commented May 20, 2019

Manishearth commented May 20, 2019

Manishearth commented May 20, 2019

HarrisonMc555 commented May 20, 2019

HarrisonMc555 commented May 20, 2019

Manishearth commented May 20, 2019

HarrisonMc555 commented May 20, 2019

flip1995 commented May 21, 2019 • edited Loading

bors commented May 21, 2019

bors commented May 21, 2019

bors commented May 21, 2019

bors commented May 21, 2019

bors commented May 21, 2019

bors commented May 21, 2019

flip1995 commented May 21, 2019

HarrisonMc555 commented May 22, 2019

flip1995 commented May 27, 2019

bors commented May 27, 2019

bors commented May 27, 2019

bors commented May 27, 2019

HarrisonMc555 commented May 28, 2019

HarrisonMc555 commented Feb 28, 2019 •

edited by flip1995

Loading

HarrisonMc555 commented Apr 16, 2019 •

edited

Loading

phansch commented Apr 21, 2019 •

edited

Loading

flip1995 commented May 21, 2019 •

edited

Loading