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.

Sign up for GitHub

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

Jump to bottom

implemented unnecessary min #12061

Closed
wants to merge 4 commits into from
Closed

implemented unnecessary min #12061

wants to merge 4 commits into from

Conversation

FelixMaetzler
Copy link

@FelixMaetzler FelixMaetzler commented Dec 30, 2023 •
edited
Loading

fixes #11924
new version of #11951 because i'm to stupid to figure it out how to fix it

changelog: [`unnecessary_min`]: added lint

@rustbot
Copy link
Collaborator

rustbot commented Dec 30, 2023

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @giraffate (or someone else) soon.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

  • @rustbot author: the review is finished, PR author should check the comments and take action accordingly
  • @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Dec 30, 2023
@FelixMaetzler FelixMaetzler mentioned this pull request Dec 30, 2023
Closed
@bors
Copy link
Contributor

bors commented Dec 30, 2023

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

@bors
Copy link
Contributor

bors commented Dec 31, 2023

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

asquared31415
asquared31415 reviewed Jan 1, 2024
View reviewed changes
clippy_lints/src/methods/mod.rs Outdated Show resolved Hide resolved
tests/ui/unnecessary_min.rs
Comment on lines +8 to +9
let _ = (A * B).min(B); // Both are constants
let _ = C.min(B); // Both are constants
let _ = B.min(C); // Both are constants
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know whether clippy has a policy for these sorts of things, but linting using the values of constants makes me wary. It would be possible to change A, B, or C in such a way that the use of min or max do actually do something, especially if it's something like min(CONST_LIMIT, user_input).

Copy link
Author

Choose a reason for hiding this comment

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

I don't know whether clippy has a policy for these sorts of things, but linting using the values of constants makes me wary. It would be possible to change A, B, or C in such a way that the use of min or max do actually do something

I don't think i get your point.
In your commented block of code all "variables" are constants so afaik it is not possible that one of them can change.
In the case of

min(CONST_LIMIT, user_input)

there should be only a lint emitted if CONST_LIMIT is either the minimum or maximum value. If it is the minimum value, then the result will always be the minumum regardless of the user_input and if it is the maximum value then the result will always be the user_input.

Or did you mean something like that?:

let mut m = u32::MAX;
for _ in 0..1000{
   m = m.min(user_input);
}

In this situation nothing should be linted.
I'll add a test to confirm it.

thank you for your feedback. I appreciate it.

Copy link
Author

Choose a reason for hiding this comment

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

i added the test

tests/ui/unnecessary_min.rs

let _ = i64::MAX.min(test_i64()); // signed with MAX and function
let _ = test_i64().min(i64::MAX); // signed with MAX and function
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I would like to see tests using the u32::min(a, b) syntax to be sure that the suggestions are correct.

It might be worth adding a test for things like u32::min(CONST, { some_math * abc - 3 }) or other complex expressions that could be placed in the call.

Copy link
Author

Choose a reason for hiding this comment

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

i'll add that.
Thank you.

Copy link
Author

Choose a reason for hiding this comment

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

i tried your suggestion and it doesn't lint it.
It seems like it currently only works as a method with the . like a.min(b).

FelixMaetzler
FelixMaetzler commented Jan 4, 2024
View reviewed changes
Copy link
Author

@FelixMaetzler FelixMaetzler left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I changed only one character and now the CI is failing.
i did cargo dev update_lints.
What can i do?

@y21
Copy link
Member

y21 commented Jan 4, 2024

I think you should be able to fix this by rebasing on master and then running cargo dev update_lints.
There are probably some changes on the master branch that your local branch doesn't yet have

@FelixMaetzler
Copy link
Author

thank you. that worked!
should i rebase every time before making a new commit to my feature branch?

@y21
Copy link
Member

y21 commented Jan 4, 2024

I don't think you need to do that for every commit. I usually only rebase when conflicts happen or to squash commits when the PR is about to be merged or to clean up history/remove some commits that don't really add any value.

Looks like it was only needed here because there are currently 699 lints on master but your branch was behind a bit and didn't, so running cargo dev update_lints on the branch didn't update the "lint count" to 700, but on master it wants it updated. That doesn't happen very often ^^

@vohoanglong0107
Copy link
Contributor

Hi @FelixMaetzler. Are you still working on this? Would you mind if I continue your work to combine issue #11901 to this?

@FelixMaetzler
Copy link
Author

FelixMaetzler commented Feb 24, 2024 •
edited
Loading

yes i'm working on it. But imho the feature is finished. I just have to resolve the conflicts and wait on further review.
I don't mind if you continue my work. Currently i don't have time because i have exams at my university.
Feel free to contribute in any way you want :-)
(If you like you can resolve the conflicts)
@vohoanglong0107

@vohoanglong0107
Copy link
Contributor

Thanks @FelixMaetzler. Then I will start by fixing the conflict in this PR

@vohoanglong0107 vohoanglong0107 mentioned this pull request Feb 28, 2024
Merged
@xFrednet
Copy link
Member

xFrednet commented Apr 1, 2024

Hey @FelixMaetzler can this PR be closed in favor of #12368?

@FelixMaetzler
Copy link
Author

Hey @xFrednet, yes this PR can be closed. Thank you.

bors added a commit that referenced this pull request Jun 20, 2024
@bors
Auto merge of #12368 - vohoanglong0107:unnecessary-min, r=Alexendoo
3e84ca8 
Unnecessary call to min/max method

Continuation of #12061
Fix #11901 and #11924

This implementation only deal with literal int, like `i32::MAX`, `-6_i32`, `0`

changelog: added lint [`unnecessary_min_max`]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@asquared31415 asquared31415 asquared31415 left review comments

Assignees

@giraffate giraffate

Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

unnecessary call to min function
8 participants
@FelixMaetzler @rustbot @bors @y21 @vohoanglong0107 @xFrednet @asquared31415 @giraffate