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

New lint: replace x.get(x.len()-1) with x.last() #3673

Closed
detrumi opened this issue Jan 19, 2019 · 7 comments · Fixed by #3832
Closed

New lint: replace x.get(x.len()-1) with x.last() #3673

detrumi opened this issue Jan 19, 2019 · 7 comments · Fixed by #3832
Labels
A-lint Area: New lints good-first-issue These issues are a good way to get started with Clippy L-complexity Lint: Belongs in the complexity lint group

Comments

@detrumi
Copy link
Member

detrumi commented Jan 19, 2019

The following can be replaced with x.last():

let x = vec![1,2,3];
let _ = x.get(x.len() - 1);
@matthiaskrgr
Copy link
Member

For a vector, x.last() returns Option<T> while x[idx] returns just T so it's not quite the same?

@detrumi
Copy link
Member Author

detrumi commented Jan 19, 2019

Oh right, so it doesn't work for x[x.len() - 1] then.

@detrumi detrumi changed the title New lint: replace x[x.len()-1] with x.last() New lint: replace x.get(x.len()-1) with x.last() Jan 19, 2019
@flip1995 flip1995 added good-first-issue These issues are a good way to get started with Clippy A-lint Area: New lints L-complexity Lint: Belongs in the complexity lint group labels Jan 19, 2019
@HarrisonMc555
Copy link
Contributor

I think I could try to do this one (if nobody else is working on it). I'll try to be following the instructions in CONTRIBUTING.md, but I assume I can ask here if I need more help?

@oli-obk
Copy link
Contributor

oli-obk commented Feb 13, 2019

I assume I can ask here if I need more help?

Yes always. Or on irc, or on discord.

@HarrisonMc555
Copy link
Contributor

Ok so I think I have something working, but I can't manage to get the UI tests to run. Every time I try, it says error: unknown clippy lint: clippy::use_last. I ran utils/dev update_lints and I can see the changes in clippy_lints/src/lib.rs, but it still doesn't seem to be working.

I've tried looking around and haven't found anything. Any idea what's going on?

Here is the branch:

https://github.com/HarrisonMc555/rust-clippy/tree/use_last

Here is the UI test (link):

#![warn(clippy::use_last)] // This gives an error

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

fn index_into_last() -> i32 {
    let x = vec![2, 3, 5];
    let last_element = x[x.len() - 1];
    last_element + 1 // To avoid warnings
}

fn main() {
    let expected_value: i32 = 5;
    assert_eq!(dont_use_last(), Some(expected_value));
    assert_eq!(index_into_last(), expected_value);
}

@flip1995
Copy link
Member

flip1995 commented Feb 28, 2019

You forgot to add

reg.register_early_lint_pass(box foo_functions::FooFunctionsPass);

to clippy_lints/src/lib.rs.

After registering the lint it should produce output.

Currently in the making, but might be useful: #3824

@HarrisonMc555
Copy link
Contributor

Awesome, thanks for your help! I used the automatic update script which added it to the complexity lists, etc., but didn't do this. Whoops.

I have one more question: how do I check to make sure that the vector that they're calling get on is the same as the one they're calling len on? I tried comparing the hir_id values and that didn't work. That makes sense, since that's an Expr, and the expressions are in different places.

However, when I matched on my_struct_expr.node, it was a ExprKind::Path. Can I compare these directly to see if they are the same things? I don't know how to check for identical "objects".

Thanks again for the help!

bors added a commit that referenced this issue May 21, 2019
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 added a commit that referenced this issue May 21, 2019
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 added a commit that referenced this issue May 27, 2019
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`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-lint Area: New lints good-first-issue These issues are a good way to get started with Clippy L-complexity Lint: Belongs in the complexity lint group
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants