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 [manual_slice_size_calculation] #10601

Merged
merged 3 commits into from
Apr 7, 2023

Conversation

schubart
Copy link
Contributor

@schubart schubart commented Apr 6, 2023

Fixes: #10518


changelog: new lint [manual_slice_size_calculation]

@rustbot
Copy link
Collaborator

rustbot commented Apr 6, 2023

r? @llogiq

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Apr 6, 2023
@schubart schubart force-pushed the manual_slice_size_calculation branch from b78ba71 to b66aa09 Compare April 6, 2023 10:48
@schubart schubart marked this pull request as ready for review April 6, 2023 11:15
Copy link
Contributor

@llogiq llogiq left a comment

Choose a reason for hiding this comment

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

I'd like to see a test where either the size or length is bound to a temporary variable first. It's ok for now if that isn't linted, but sufficiently easy to change.

Otherwise the code looks good, and I'll gladly merge it with the additional test added.

Comment on lines 58 to 64
fn simplify<'tcx>(
cx: &LateContext<'tcx>,
expr1: &'tcx Expr<'tcx>,
expr2: &'tcx Expr<'tcx>,
) -> Option<&'tcx Expr<'tcx>> {
simplify_half(cx, expr1, expr2).or_else(|| simplify_half(cx, expr2, expr1))
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I swear I've seen this pattern multiple times. Perhaps we should make a generic clippy_utils function of it.

Copy link
Contributor Author

@schubart schubart Apr 6, 2023

Choose a reason for hiding this comment

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

Such a helper could also fix some of the false negatives in my tests, e.g, size_of::<i32>() * 5 * slice.len(). The helper's semantics could be:

Check if this expression is a chain of multiplications (or additions or something more general) where two (or n?) sub-expressions meet some condition.

I don't have the bandwidth to do it as part of this PR, though.

@schubart
Copy link
Contributor Author

schubart commented Apr 6, 2023

I'd like to see a test where either the size or length is bound to a temporary variable first. It's ok for now if that isn't linted, but sufficiently easy to change.

Otherwise the code looks good, and I'll gladly merge it with the additional test added.

Done.

I feel this kind of data flow analysis (where does a value come from, what do we know about it?) could be a whole new thing, applicable to a lot more than just this lint here.

For example, in the following Java code Intellij warns that the if condition is always true:

List<String> strings = Collections.emptyList();
if (strings.isEmpty()) { // "Condition `strings.isEmpty()` is always `true`.
    System.out.println("There are no strings");
}

and in this code it warns that the list access will always fail:

List<String> strings = foo();
if (strings.isEmpty()) {
    System.out.println(strings.get(0)); // "The call to `get` always fails as an argument is out of bounds"
}

Do we have anything like this in Clippy?

@llogiq
Copy link
Contributor

llogiq commented Apr 6, 2023

We have, it's clippy_utils::expr_or_init(..). If you want to add this directly to this PR, feel free, otherwise I'll have this merged and we can do a follow-up.

@schubart
Copy link
Contributor Author

schubart commented Apr 6, 2023

Thanks, I'll give it a shot. Let's not merge this yet.

@schubart
Copy link
Contributor Author

schubart commented Apr 6, 2023

Done. Thanks for the suggestion, this is very nice!

@llogiq
Copy link
Contributor

llogiq commented Apr 7, 2023

Great!

@bors r+

@bors
Copy link
Contributor

bors commented Apr 7, 2023

📌 Commit b1c784d has been approved by llogiq

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Apr 7, 2023

⌛ Testing commit b1c784d with merge b8cbce8...

@bors
Copy link
Contributor

bors commented Apr 7, 2023

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: llogiq
Pushing b8cbce8 to master...

@bors bors merged commit b8cbce8 into rust-lang:master Apr 7, 2023
@schubart schubart deleted the manual_slice_size_calculation branch April 7, 2023 11:52
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Apr 10, 2023
…Denton

Avoid some manual slice length calculation

No need for us to write the multiplication when `size_of_val` does exactly what we need.

(rust-lang/rust-clippy#10601)
thomcc pushed a commit to tcdi/postgrestd that referenced this pull request Jun 1, 2023
Avoid some manual slice length calculation

No need for us to write the multiplication when `size_of_val` does exactly what we need.

(rust-lang/rust-clippy#10601)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Suggest size_of_val(a) for a.len() * size_of::<T>()
4 participants