Skip to content

let unnecessary_cast work for trivial non_literal expressions #9576

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
Merged
merged 4 commits into from
Oct 2, 2022
Merged

let unnecessary_cast work for trivial non_literal expressions #9576

merged 4 commits into from
Oct 2, 2022

Conversation

TennyZhuang
Copy link
Contributor

@TennyZhuang TennyZhuang commented Oct 2, 2022 •
edited
Loading

Signed-off-by: TennyZhuang [email protected]

changelog: [unnecessary_cast]: fix for trivial non_literal expressions

Fixes #9562

@rust-highfive
Copy link

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

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Oct 2, 2022
@TennyZhuang TennyZhuang marked this pull request as draft October 2, 2022 09:24
@TennyZhuang
Copy link
Contributor Author

TennyZhuang commented Oct 2, 2022 •
edited
Loading

Some other tests were broken, convert to draft :(

@TennyZhuang
Copy link
Contributor Author

I've added some rules on some test files, but I'm not sure why they doesn't work.

783ee63

@kraktus
Copy link
Contributor

kraktus commented Oct 2, 2022

Those tests need to be blessed (because we also check the code output after clippy --fix, which is stored as my_test.fixed.

You can bless them with cargo dev bless and commit the changes (which normally should just be adding the allow parameter to the corresponding .fixed)

kraktus
kraktus reviewed Oct 2, 2022
View reviewed changes
clippy_lints/src/casts/unnecessary_cast.rs Outdated
Comment on lines 85 to 90
} else if cast_from.kind() == cast_to.kind() && !in_external_macro(cx.sess(), expr.span) {
span_lint_and_sugg(
cx,
UNNECESSARY_CAST,
expr.span,
&format!("casting to the same type is unnecessary (`{cast_from}` -> `{cast_to}`)"),
"try",
cast_str,
Applicability::MachineApplicable,
);
return true;
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like it should already be caught by

rust-clippy/clippy_lints/src/casts/unnecessary_cast.rs

Line 71 in 783ee63

if cast_from.kind() == cast_to.kind() && !in_external_macro(cx.sess(), expr.span) {

no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's under a condition so it will only work for numeric literal.

Copy link
Contributor

Choose a reason for hiding this comment

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

right, you could refactor it by turning your if else into a if and remove the afore mentioned line then I think

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've tried it when I implement it, but the control flow is too complicated to do that. At least, I will try to extract a common function lint_same_type.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated

@TennyZhuang
Copy link
Contributor Author

TennyZhuang commented Oct 2, 2022 •
edited
Loading

@kraktus Hi, in fact, I blessed them using cargo dev bless but found the allow does not work. You can run them locally or check the CI log. The warnings are still produced so I don't push them.

I found that if I use the function level #[allow(clippy::unnecessary_cast)] instead of the mod level one, then everything works. I guess there is another bug but I will use the function level attribute as a workaround.

@TennyZhuang
Copy link
Contributor Author

Sorry, I misunderstood and forget to run cargo uitest before cargo dev bless :(

@kraktus
Copy link
Contributor

kraktus commented Oct 2, 2022

No worries!

@TennyZhuang TennyZhuang force-pushed the unnecessary_cast_for_non_literal_expr branch 2 times, most recently from aeeae48 to 1ca60ac Compare October 2, 2022 10:36
@TennyZhuang TennyZhuang marked this pull request as ready for review October 2, 2022 10:42
@TennyZhuang
Copy link
Contributor Author

@rustbot label +S-waiting-on-author -S-waiting-on-review

@rustbot rustbot 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 Oct 2, 2022
@bors
Copy link
Contributor

bors commented Oct 2, 2022

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

@TennyZhuang TennyZhuang force-pushed the unnecessary_cast_for_non_literal_expr branch from bbe0d7e to c9b9314 Compare October 2, 2022 15:02
@TennyZhuang
Copy link
Contributor Author

@rustbot label -S-waiting-on-author +S-waiting-on-review

@rustbot rustbot 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 Oct 2, 2022
@TennyZhuang TennyZhuang requested a review from kraktus October 2, 2022 15:04
@llogiq
Copy link
Contributor

llogiq commented Oct 2, 2022

Looks good to me.

Thank you!

@bors r+

@bors
Copy link
Contributor

bors commented Oct 2, 2022

📌 Commit c9b9314 has been approved by llogiq

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Oct 2, 2022

⌛ Testing commit c9b9314 with merge f8ba192...

@bors
Copy link
Contributor

bors commented Oct 2, 2022

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

@bors bors merged commit f8ba192 into rust-lang:master Oct 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kraktus kraktus Awaiting requested review from kraktus

Assignees

@flip1995 flip1995

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.

clippy::unnecessary_cast does not warn on basic usage
7 participants
@TennyZhuang @rust-highfive @kraktus @bors @llogiq @flip1995 @rustbot