From ea639d9559377799c86455285fa15cf82729eb7f Mon Sep 17 00:00:00 2001 From: Aneesh Kadiyala <143342960+ARandomDev99@users.noreply.github.com> Date: Tue, 26 Mar 2024 11:44:13 +0530 Subject: [PATCH 01/22] Check for `Default` trait implementation in initial condition --- clippy_lints/src/manual_unwrap_or_default.rs | 9 +++++++++ tests/ui/manual_unwrap_or_default.fixed | 10 ++++++++++ tests/ui/manual_unwrap_or_default.rs | 10 ++++++++++ tests/ui/manual_unwrap_or_default.stderr | 2 +- 4 files changed, 30 insertions(+), 1 deletion(-) diff --git a/clippy_lints/src/manual_unwrap_or_default.rs b/clippy_lints/src/manual_unwrap_or_default.rs index fc6b0d6f9f6c..6ca535be4642 100644 --- a/clippy_lints/src/manual_unwrap_or_default.rs +++ b/clippy_lints/src/manual_unwrap_or_default.rs @@ -3,6 +3,7 @@ use rustc_errors::Applicability; use rustc_hir::def::Res; use rustc_hir::{Arm, Expr, ExprKind, HirId, LangItem, MatchSource, Pat, PatKind, QPath}; use rustc_lint::{LateContext, LateLintPass}; +use rustc_middle::ty::GenericArgKind; use rustc_session::declare_lint_pass; use rustc_span::sym; @@ -116,6 +117,10 @@ fn handle_match<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) -> bool { && let match_ty = cx.typeck_results().expr_ty(expr) && let Some(default_trait_id) = cx.tcx.get_diagnostic_item(sym::Default) && implements_trait(cx, match_ty, default_trait_id, &[]) + // We also check that the expression in the match condition implements the `Default` trait. + && let Some(match_expr_ty) = cx.typeck_results().expr_ty(match_expr).walk().nth(1) + && let GenericArgKind::Type(match_expr_ty) = match_expr_ty.unpack() + && implements_trait(cx, match_expr_ty, default_trait_id, &[]) // We now get the bodies for both the `Some` and `None` arms. && let Some(((body_some, binding_id), body_none)) = get_some_and_none_bodies(cx, arm1, arm2) // We check that the `Some(x) => x` doesn't do anything apart "returning" the value in `Some`. @@ -149,6 +154,10 @@ fn handle_if_let<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) { && let Some(default_trait_id) = cx.tcx.get_diagnostic_item(sym::Default) && implements_trait(cx, match_ty, default_trait_id, &[]) && let Some(binding_id) = get_some(cx, let_.pat) + // We also check that the expression in the let expression implements the `Default` trait. + && let Some(let_ty) = cx.typeck_results().expr_ty(let_.init).walk().nth(1) + && let GenericArgKind::Type(let_ty) = let_ty.unpack() + && implements_trait(cx, let_ty, default_trait_id, &[]) // We check that the `Some(x) => x` doesn't do anything apart "returning" the value in `Some`. && let ExprKind::Path(QPath::Resolved(_, path)) = if_block.peel_blocks().kind && let Res::Local(local_id) = path.res diff --git a/tests/ui/manual_unwrap_or_default.fixed b/tests/ui/manual_unwrap_or_default.fixed index b24967242ebd..a6e59133f129 100644 --- a/tests/ui/manual_unwrap_or_default.fixed +++ b/tests/ui/manual_unwrap_or_default.fixed @@ -16,6 +16,16 @@ fn main() { let x: Option> = None; x.unwrap_or_default(); + + // Issue #12564 + // No error as &Vec<_> doesn't implement std::default::Default + let mut map = std::collections::HashMap::from([(0, vec![0; 3]), (1, vec![1; 3]), (2, vec![2])]); + let x: &[_] = if let Some(x) = map.get(&0) { x } else { &[] }; + // Same code as above written using match. + let x: &[_] = match map.get(&0) { + Some(x) => x, + None => &[], + }; } // Issue #12531 diff --git a/tests/ui/manual_unwrap_or_default.rs b/tests/ui/manual_unwrap_or_default.rs index ed5e54b4e147..07e6bc555bc2 100644 --- a/tests/ui/manual_unwrap_or_default.rs +++ b/tests/ui/manual_unwrap_or_default.rs @@ -37,6 +37,16 @@ fn main() { } else { Vec::default() }; + + // Issue #12564 + // No error as &Vec<_> doesn't implement std::default::Default + let mut map = std::collections::HashMap::from([(0, vec![0; 3]), (1, vec![1; 3]), (2, vec![2])]); + let x: &[_] = if let Some(x) = map.get(&0) { x } else { &[] }; + // Same code as above written using match. + let x: &[_] = match map.get(&0) { + Some(x) => x, + None => &[], + }; } // Issue #12531 diff --git a/tests/ui/manual_unwrap_or_default.stderr b/tests/ui/manual_unwrap_or_default.stderr index d89212e60459..3f1da444301f 100644 --- a/tests/ui/manual_unwrap_or_default.stderr +++ b/tests/ui/manual_unwrap_or_default.stderr @@ -53,7 +53,7 @@ LL | | }; | |_____^ help: replace it with: `x.unwrap_or_default()` error: match can be simplified with `.unwrap_or_default()` - --> tests/ui/manual_unwrap_or_default.rs:46:20 + --> tests/ui/manual_unwrap_or_default.rs:56:20 | LL | Some(_) => match *b { | ____________________^ From 54791ff0f022e0e4eca9827d32e36adc672184e4 Mon Sep 17 00:00:00 2001 From: xFrednet Date: Mon, 11 Mar 2024 23:32:35 +0100 Subject: [PATCH 02/22] RFC: Document Clippy's teams and team duties I want to be clear: this is just the initial draft outlining what, I think, should be the responsibilities of the team members. It has not yet been discussed with anyone else. --- book/src/SUMMARY.md | 1 + book/src/development/the_team.md | 112 +++++++++++++++++++++++++++++++ 2 files changed, 113 insertions(+) create mode 100644 book/src/development/the_team.md diff --git a/book/src/SUMMARY.md b/book/src/SUMMARY.md index a048fbbd8acf..be13fcbe260f 100644 --- a/book/src/SUMMARY.md +++ b/book/src/SUMMARY.md @@ -32,3 +32,4 @@ - [Proposals](development/proposals/README.md) - [Roadmap 2021](development/proposals/roadmap-2021.md) - [Syntax Tree Patterns](development/proposals/syntax-tree-patterns.md) + - [The Team](development/the_team.md) diff --git a/book/src/development/the_team.md b/book/src/development/the_team.md new file mode 100644 index 000000000000..fa7da80de0c1 --- /dev/null +++ b/book/src/development/the_team.md @@ -0,0 +1,112 @@ +# The team + +Everyone who contributes to Clippy makes the project what it is. Collaboration +and discussions are the lifeblood of every open-source project. Clippy has a +very flat hierarchy. The teams mainly have additional access rights to the repo. + +This document outlines the onboarding process, as well as duties, and access +rights for members of a group. + +## Everyone + +Everyone, including you, is welcome to join discussions and contribute in other +ways, like PRs. + +You also have some triage rights, using `@rustbot` to add labels and claim +issues. See +[labeling with @rustbot](https://forge.rust-lang.org/triagebot/labeling.html) + +A rule for everyone should be to keep a healthy work-life balance. Take a break +when you need one. + +## Clippy-Contributors + +This is a group of regular contributors to Clippy to help with triaging. + +### Duties + +This team exists to make contributing easier for regular members. It doesn't +carry any duties that need to be done. However, we want to encourage members of +this group to help with triaging, which can include: + +1. **Labeling issues** + + For the `good-first-issue` label, it can still be good to use `@rustbot` to + subscribe to the issue and help interested parties, if they post questions + in the comments. + +2. **Closing duplicate or resolved issues** + + When you manually close and issue, it's often a good idea, to add a short + comment explaining the reason. + +3. **Ping people after two weeks of inactivity** + + We try to keep issue assignments and PRs fairly up-to-date. After two weeks, + it can be good to send a friendly ping to the delaying party. + + You might close a PR with the `I-inactive-closed` label if the author is + busy or wants to abandon it. If the reviewer is busy, the PR can be + reassigned to someone else. + + Checkout: https://triage.rust-lang.org/triage/rust-lang/rust-clippy to + monitor PRs. + +### Membership + +If you have been contributing to Clippy for some time, we'll probably ask you if +you want to join this team. Members of this team are also welcome to suggest +people who they think would make a great addition to this group. + +For this group, there is no direct onboarding process. You're welcome to just +continue what you've been doing. If you like, you can ask for someone to mentor +you, either in the Clippy stream on Zulip or privately via a PM. + +If you have been inactive in Clippy for over three months, we'll probably move +you to the alumni group. You're always welcome to come back. + +## The Clippy Team + +[The Clippy team](https://www.rust-lang.org/governance/teams/dev-tools#Clippy%20team) +is responsible for maintaining Clippy. + +### Duties + +1. **Respond to PRs in a timely manner** + + It's totally fine, if you don't have the time for reviews right now. + You can reassign the PR to a random member by commenting `r? clippy`. + +2. **Take a break when you need one.** + + You are valuable! Clippy wouldn't be what it is without you. So take a break + early and recharge some energy when you need to. + +3. **Sync Clippy with the rust-lang/rust repo** + + This should be done roughly every two weeks. This is usually done by our + king @flip1995. + +4. **Update the changelog** + + This needs to be done for every release, every six weeks. This is usually + done by @xFrednet. + +### Membership + +If you have been in the *Clippy-Contributors* team for some time, we'll probably +reach out and ask if you want to help with reviews and eventually join the +Clippy team. + +During the onboarding process, you'll have an active Clippy team member as a +mentor, who assigns PRs to you. They will shadow your reviews, meaning that +they'll keep an eye on your PRs, help you with any questions, and once you're +done, perform a full review. When everything looks good, they'll r+ the PR in +the name of both of you. + +When you've done several reviews and seem confident in the role, you'll be +invited to join the team officially, as long as you're still interested. This +onboarding phase typically takes a couple of weeks to a few months. + +If you have been inactive in Clippy for over three months, we'll probably move +you to the alumni group. You're always welcome to come back. From 123700b67e841caa8d597fc8fbc5bee15b49d8a3 Mon Sep 17 00:00:00 2001 From: xFrednet Date: Mon, 18 Mar 2024 14:31:00 +0100 Subject: [PATCH 03/22] Apply pretty review comments =^.^= --- book/src/development/the_team.md | 54 +++++++++++++++++++++----------- 1 file changed, 36 insertions(+), 18 deletions(-) diff --git a/book/src/development/the_team.md b/book/src/development/the_team.md index fa7da80de0c1..10341791cec4 100644 --- a/book/src/development/the_team.md +++ b/book/src/development/the_team.md @@ -7,14 +7,16 @@ very flat hierarchy. The teams mainly have additional access rights to the repo. This document outlines the onboarding process, as well as duties, and access rights for members of a group. +All regular events mentioned in this chapter are tracked in the [calendar repository]. +The calendar file is also available for download: [clippy.ics] + ## Everyone Everyone, including you, is welcome to join discussions and contribute in other ways, like PRs. You also have some triage rights, using `@rustbot` to add labels and claim -issues. See -[labeling with @rustbot](https://forge.rust-lang.org/triagebot/labeling.html) +issues. See [labeling with @rustbot]. A rule for everyone should be to keep a healthy work-life balance. Take a break when you need one. @@ -37,7 +39,7 @@ this group to help with triaging, which can include: 2. **Closing duplicate or resolved issues** - When you manually close and issue, it's often a good idea, to add a short + When you manually close an issue, it's often a good idea, to add a short comment explaining the reason. 3. **Ping people after two weeks of inactivity** @@ -52,6 +54,9 @@ this group to help with triaging, which can include: Checkout: https://triage.rust-lang.org/triage/rust-lang/rust-clippy to monitor PRs. +While not part of their duties, contributors are encouraged to review PRs +and help on Zulip. The team always appreciates help! + ### Membership If you have been contributing to Clippy for some time, we'll probably ask you if @@ -77,36 +82,49 @@ is responsible for maintaining Clippy. It's totally fine, if you don't have the time for reviews right now. You can reassign the PR to a random member by commenting `r? clippy`. -2. **Take a break when you need one.** +2. **Take a break when you need one** You are valuable! Clippy wouldn't be what it is without you. So take a break early and recharge some energy when you need to. -3. **Sync Clippy with the rust-lang/rust repo** +3. **Be responsive on Zulip** + + This means in a reasonable time frame, so responding within one or two days + is totally fine. - This should be done roughly every two weeks. This is usually done by our - king @flip1995. + It's also good, if you answer threads on Zulip and take part in our Clippy + meetings, every two weeks. The meeting dates are tracked in the [calendar repository]. + -4. **Update the changelog** +4. **Sync Clippy with the rust-lang/rust repo** + + This is done every two weeks, usually by @flip1995. + +5. **Update the changelog** This needs to be done for every release, every six weeks. This is usually done by @xFrednet. ### Membership -If you have been in the *Clippy-Contributors* team for some time, we'll probably -reach out and ask if you want to help with reviews and eventually join the -Clippy team. +If you have been active for some time, we'll probably reach out and ask +if you want to help with reviews and eventually join the Clippy team. -During the onboarding process, you'll have an active Clippy team member as a -mentor, who assigns PRs to you. They will shadow your reviews, meaning that -they'll keep an eye on your PRs, help you with any questions, and once you're -done, perform a full review. When everything looks good, they'll r+ the PR in -the name of both of you. +During the onboarding process, you'll be assigned pull requests to review. +You'll also have an active team member as a mentor who'll stay in contact via +Zulip DMs to provide advice and feedback. If you have questions, you're always +welcome to ask, that is the best way to learn. Once you're done with the review, +you can ping your mentor for a full review and to r+ the PR in both of your names. -When you've done several reviews and seem confident in the role, you'll be -invited to join the team officially, as long as you're still interested. This +When your mentor is confident that you can handle reviews on your own, they'll +start an informal vote among the active team members to officially add you to +the team. This vote is usually accepted unanimously. Then you'll be added to +the team once you've confirmed that you're still interested in joining. The onboarding phase typically takes a couple of weeks to a few months. If you have been inactive in Clippy for over three months, we'll probably move you to the alumni group. You're always welcome to come back. + +[calendar repository]: https://github.com/rust-lang/calendar/blob/main/clippy.toml +[clippy.ics]: https://rust-lang.github.io/calendar/clippy.ics +[labeling with @rustbot]: https://forge.rust-lang.org/triagebot/labeling.html From 74a35ee149e5f7a3ec21428a101ea1e8d0214f12 Mon Sep 17 00:00:00 2001 From: Guillaume Gomez Date: Mon, 25 Mar 2024 12:15:48 +0100 Subject: [PATCH 04/22] Don't emit `duplicated_attribute` lint on "complex" `cfg`s --- .../src/attrs/duplicated_attributes.rs | 13 ++- tests/ui/duplicated_attributes.rs | 13 +-- tests/ui/duplicated_attributes.stderr | 94 +++---------------- 3 files changed, 32 insertions(+), 88 deletions(-) diff --git a/clippy_lints/src/attrs/duplicated_attributes.rs b/clippy_lints/src/attrs/duplicated_attributes.rs index 3c5ac597fd5d..d0ccfc4c36d2 100644 --- a/clippy_lints/src/attrs/duplicated_attributes.rs +++ b/clippy_lints/src/attrs/duplicated_attributes.rs @@ -25,12 +25,16 @@ fn emit_if_duplicated( } } +#[allow(clippy::needless_return)] fn check_duplicated_attr( cx: &EarlyContext<'_>, attr: &MetaItem, attr_paths: &mut FxHashMap, parent: &mut Vec, ) { + if attr.span.from_expansion() { + return; + } let Some(ident) = attr.ident() else { return }; let name = ident.name; if name == sym::doc || name == sym::cfg_attr { @@ -38,7 +42,14 @@ fn check_duplicated_attr( // conditions are the same. return; } - if let Some(value) = attr.value_str() { + if let Some(direct_parent) = parent.last() + && ["cfg", "cfg_attr"].contains(&direct_parent.as_str()) + && [sym::all, sym::not, sym::any].contains(&name) + { + // FIXME: We don't correctly check `cfg`s for now, so if it's more complex than just a one + // level `cfg`, we leave. + return; + } else if let Some(value) = attr.value_str() { emit_if_duplicated(cx, attr, attr_paths, format!("{}:{name}={value}", parent.join(":"))); } else if let Some(sub_attrs) = attr.meta_item_list() { parent.push(name.as_str().to_string()); diff --git a/tests/ui/duplicated_attributes.rs b/tests/ui/duplicated_attributes.rs index 0f036c684c1c..d051c881f15b 100644 --- a/tests/ui/duplicated_attributes.rs +++ b/tests/ui/duplicated_attributes.rs @@ -2,16 +2,17 @@ #![cfg(any(unix, windows))] #![allow(dead_code)] #![allow(dead_code)] //~ ERROR: duplicated attribute -#![cfg(any(unix, windows))] -//~^ ERROR: duplicated attribute -//~| ERROR: duplicated attribute +#![cfg(any(unix, windows))] // Should not warn! #[cfg(any(unix, windows, target_os = "linux"))] #[allow(dead_code)] #[allow(dead_code)] //~ ERROR: duplicated attribute -#[cfg(any(unix, windows, target_os = "linux"))] -//~^ ERROR: duplicated attribute -//~| ERROR: duplicated attribute +#[cfg(any(unix, windows, target_os = "linux"))] // Should not warn! fn foo() {} +#[cfg(unix)] +#[cfg(windows)] +#[cfg(unix)] //~ ERROR: duplicated attribute +fn bar() {} + fn main() {} diff --git a/tests/ui/duplicated_attributes.stderr b/tests/ui/duplicated_attributes.stderr index 1c6578dbb43a..9e26ba990ac1 100644 --- a/tests/ui/duplicated_attributes.stderr +++ b/tests/ui/duplicated_attributes.stderr @@ -18,106 +18,38 @@ LL | #![allow(dead_code)] = help: to override `-D warnings` add `#[allow(clippy::duplicated_attributes)]` error: duplicated attribute - --> tests/ui/duplicated_attributes.rs:5:12 - | -LL | #![cfg(any(unix, windows))] - | ^^^^ - | -note: first defined here - --> tests/ui/duplicated_attributes.rs:2:12 - | -LL | #![cfg(any(unix, windows))] - | ^^^^ -help: remove this attribute - --> tests/ui/duplicated_attributes.rs:5:12 - | -LL | #![cfg(any(unix, windows))] - | ^^^^ - -error: duplicated attribute - --> tests/ui/duplicated_attributes.rs:5:18 - | -LL | #![cfg(any(unix, windows))] - | ^^^^^^^ - | -note: first defined here - --> tests/ui/duplicated_attributes.rs:2:18 - | -LL | #![cfg(any(unix, windows))] - | ^^^^^^^ -help: remove this attribute - --> tests/ui/duplicated_attributes.rs:5:18 - | -LL | #![cfg(any(unix, windows))] - | ^^^^^^^ - -error: duplicated attribute - --> tests/ui/duplicated_attributes.rs:11:9 + --> tests/ui/duplicated_attributes.rs:9:9 | LL | #[allow(dead_code)] | ^^^^^^^^^ | note: first defined here - --> tests/ui/duplicated_attributes.rs:10:9 + --> tests/ui/duplicated_attributes.rs:8:9 | LL | #[allow(dead_code)] | ^^^^^^^^^ help: remove this attribute - --> tests/ui/duplicated_attributes.rs:11:9 + --> tests/ui/duplicated_attributes.rs:9:9 | LL | #[allow(dead_code)] | ^^^^^^^^^ error: duplicated attribute - --> tests/ui/duplicated_attributes.rs:12:11 - | -LL | #[cfg(any(unix, windows, target_os = "linux"))] - | ^^^^ - | -note: first defined here - --> tests/ui/duplicated_attributes.rs:9:11 - | -LL | #[cfg(any(unix, windows, target_os = "linux"))] - | ^^^^ -help: remove this attribute - --> tests/ui/duplicated_attributes.rs:12:11 - | -LL | #[cfg(any(unix, windows, target_os = "linux"))] - | ^^^^ - -error: duplicated attribute - --> tests/ui/duplicated_attributes.rs:12:17 - | -LL | #[cfg(any(unix, windows, target_os = "linux"))] - | ^^^^^^^ - | -note: first defined here - --> tests/ui/duplicated_attributes.rs:9:17 - | -LL | #[cfg(any(unix, windows, target_os = "linux"))] - | ^^^^^^^ -help: remove this attribute - --> tests/ui/duplicated_attributes.rs:12:17 - | -LL | #[cfg(any(unix, windows, target_os = "linux"))] - | ^^^^^^^ - -error: duplicated attribute - --> tests/ui/duplicated_attributes.rs:12:26 + --> tests/ui/duplicated_attributes.rs:15:7 | -LL | #[cfg(any(unix, windows, target_os = "linux"))] - | ^^^^^^^^^^^^^^^^^^^ +LL | #[cfg(unix)] + | ^^^^ | note: first defined here - --> tests/ui/duplicated_attributes.rs:9:26 + --> tests/ui/duplicated_attributes.rs:13:7 | -LL | #[cfg(any(unix, windows, target_os = "linux"))] - | ^^^^^^^^^^^^^^^^^^^ +LL | #[cfg(unix)] + | ^^^^ help: remove this attribute - --> tests/ui/duplicated_attributes.rs:12:26 + --> tests/ui/duplicated_attributes.rs:15:7 | -LL | #[cfg(any(unix, windows, target_os = "linux"))] - | ^^^^^^^^^^^^^^^^^^^ +LL | #[cfg(unix)] + | ^^^^ -error: aborting due to 7 previous errors +error: aborting due to 3 previous errors From 209cb08e1fa693d9d36d30d37d7f33fae76727e7 Mon Sep 17 00:00:00 2001 From: y21 <30553356+y21@users.noreply.github.com> Date: Wed, 27 Mar 2024 13:06:32 +0100 Subject: [PATCH 05/22] move `mixed_attributes_style` to the style category --- clippy_lints/src/attrs/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clippy_lints/src/attrs/mod.rs b/clippy_lints/src/attrs/mod.rs index 6f4575f1b9ac..684ad7de2f05 100644 --- a/clippy_lints/src/attrs/mod.rs +++ b/clippy_lints/src/attrs/mod.rs @@ -496,7 +496,7 @@ declare_clippy_lint! { /// ``` #[clippy::version = "1.78.0"] pub MIXED_ATTRIBUTES_STYLE, - suspicious, + style, "item has both inner and outer attributes" } From 88cd20d4a13b76f0b3178425f71751989e1de920 Mon Sep 17 00:00:00 2001 From: J-ZhengLi Date: Wed, 27 Mar 2024 16:28:15 +0800 Subject: [PATCH 06/22] allow [`manual_unwrap_or_default`] in const function --- clippy_lints/src/manual_unwrap_or_default.rs | 4 ++-- tests/ui/manual_unwrap_or_default.fixed | 7 +++++++ tests/ui/manual_unwrap_or_default.rs | 7 +++++++ 3 files changed, 16 insertions(+), 2 deletions(-) diff --git a/clippy_lints/src/manual_unwrap_or_default.rs b/clippy_lints/src/manual_unwrap_or_default.rs index 6ca535be4642..e292639a6c69 100644 --- a/clippy_lints/src/manual_unwrap_or_default.rs +++ b/clippy_lints/src/manual_unwrap_or_default.rs @@ -8,9 +8,9 @@ use rustc_session::declare_lint_pass; use rustc_span::sym; use clippy_utils::diagnostics::span_lint_and_sugg; -use clippy_utils::is_default_equivalent; use clippy_utils::source::snippet_opt; use clippy_utils::ty::implements_trait; +use clippy_utils::{in_constant, is_default_equivalent}; declare_clippy_lint! { /// ### What it does @@ -181,7 +181,7 @@ fn handle_if_let<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) { impl<'tcx> LateLintPass<'tcx> for ManualUnwrapOrDefault { fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) { - if expr.span.from_expansion() { + if expr.span.from_expansion() || in_constant(cx, expr.hir_id) { return; } if !handle_match(cx, expr) { diff --git a/tests/ui/manual_unwrap_or_default.fixed b/tests/ui/manual_unwrap_or_default.fixed index a6e59133f129..5a9d2d5ab5d7 100644 --- a/tests/ui/manual_unwrap_or_default.fixed +++ b/tests/ui/manual_unwrap_or_default.fixed @@ -36,3 +36,10 @@ unsafe fn no_deref_ptr(a: Option, b: *const Option) -> i32 { _ => 0, } } + +const fn issue_12568(opt: Option) -> bool { + match opt { + Some(s) => s, + None => false, + } +} diff --git a/tests/ui/manual_unwrap_or_default.rs b/tests/ui/manual_unwrap_or_default.rs index 07e6bc555bc2..8d3d7422921d 100644 --- a/tests/ui/manual_unwrap_or_default.rs +++ b/tests/ui/manual_unwrap_or_default.rs @@ -60,3 +60,10 @@ unsafe fn no_deref_ptr(a: Option, b: *const Option) -> i32 { _ => 0, } } + +const fn issue_12568(opt: Option) -> bool { + match opt { + Some(s) => s, + None => false, + } +} From 83e39ede87699cb184c745c52a2f3707e5d9d6ea Mon Sep 17 00:00:00 2001 From: Kevin Reid Date: Wed, 27 Mar 2024 22:52:34 -0700 Subject: [PATCH 07/22] `large_stack_frames`: print total size and largest component. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Instead of just saying “this function's stack frame is big”, report: * the (presumed) size of the frame * the size and type of the largest local contributing to that size * the configurable limit that was exceeded (once) --- clippy_lints/src/large_stack_frames.rs | 106 ++++++++++++++---- .../large_stack_frames/large_stack_frames.rs | 2 +- .../large_stack_frames.stderr | 17 +-- tests/ui/large_stack_frames.rs | 17 ++- tests/ui/large_stack_frames.stderr | 64 +++++------ 5 files changed, 136 insertions(+), 70 deletions(-) diff --git a/clippy_lints/src/large_stack_frames.rs b/clippy_lints/src/large_stack_frames.rs index b397180a69c5..059ba981ef3b 100644 --- a/clippy_lints/src/large_stack_frames.rs +++ b/clippy_lints/src/large_stack_frames.rs @@ -1,10 +1,12 @@ -use std::ops::AddAssign; +use std::{fmt, ops}; -use clippy_utils::diagnostics::span_lint_and_note; +use clippy_utils::diagnostics::span_lint_and_then; use clippy_utils::fn_has_unsatisfiable_preds; +use clippy_utils::source::snippet_opt; use rustc_hir::def_id::LocalDefId; use rustc_hir::intravisit::FnKind; use rustc_hir::{Body, FnDecl}; +use rustc_lexer::is_ident; use rustc_lint::{LateContext, LateLintPass}; use rustc_session::impl_lint_pass; use rustc_span::Span; @@ -108,13 +110,25 @@ impl Space { } } -impl AddAssign for Space { - fn add_assign(&mut self, rhs: u64) { - if let Self::Used(lhs) = self { - match lhs.checked_add(rhs) { - Some(sum) => *self = Self::Used(sum), - None => *self = Self::Overflow, - } +impl fmt::Display for Space { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + match self { + Space::Used(1) => write!(f, "1 byte"), + Space::Used(n) => write!(f, "{n} bytes"), + Space::Overflow => write!(f, "over 2⁶⁴-1 bytes"), + } + } +} + +impl ops::Add for Space { + type Output = Self; + fn add(self, rhs: u64) -> Self { + match self { + Self::Used(lhs) => match lhs.checked_add(rhs) { + Some(sum) => Self::Used(sum), + None => Self::Overflow, + }, + Self::Overflow => self, } } } @@ -123,10 +137,10 @@ impl<'tcx> LateLintPass<'tcx> for LargeStackFrames { fn check_fn( &mut self, cx: &LateContext<'tcx>, - _: FnKind<'tcx>, + fn_kind: FnKind<'tcx>, _: &'tcx FnDecl<'tcx>, _: &'tcx Body<'tcx>, - span: Span, + entire_fn_span: Span, local_def_id: LocalDefId, ) { let def_id = local_def_id.to_def_id(); @@ -138,22 +152,68 @@ impl<'tcx> LateLintPass<'tcx> for LargeStackFrames { let mir = cx.tcx.optimized_mir(def_id); let param_env = cx.tcx.param_env(def_id); - let mut frame_size = Space::Used(0); + let sizes_of_locals = || { + mir.local_decls.iter().filter_map(|local| { + let layout = cx.tcx.layout_of(param_env.and(local.ty)).ok()?; + Some((local, layout.size.bytes())) + }) + }; - for local in &mir.local_decls { - if let Ok(layout) = cx.tcx.layout_of(param_env.and(local.ty)) { - frame_size += layout.size.bytes(); - } - } + let frame_size = sizes_of_locals().fold(Space::Used(0), |sum, (_, size)| sum + size); + + let limit = self.maximum_allowed_size; + if frame_size.exceeds_limit(limit) { + // Point at just the function name if possible, because lints that span + // the entire body and don't have to are less legible. + let fn_span = match fn_kind { + FnKind::ItemFn(ident, _, _) | FnKind::Method(ident, _) => ident.span, + FnKind::Closure => entire_fn_span, + }; - if frame_size.exceeds_limit(self.maximum_allowed_size) { - span_lint_and_note( + span_lint_and_then( cx, LARGE_STACK_FRAMES, - span, - "this function allocates a large amount of stack space", - None, - "allocating large amounts of stack space can overflow the stack", + fn_span, + &format!("this function may allocate {frame_size} on the stack"), + |diag| { + // Point out the largest individual contribution to this size, because + // it is the most likely to be unintentionally large. + if let Some((local, size)) = sizes_of_locals().max_by_key(|&(_, size)| size) { + let local_span: Span = local.source_info.span; + let size = Space::Used(size); // pluralizes for us + let ty = local.ty; + + // TODO: Is there a cleaner, robust way to ask this question? + // The obvious `LocalDecl::is_user_variable()` panics on "unwrapping cross-crate data", + // and that doesn't get us the true name in scope rather than the span text either. + if let Some(name) = snippet_opt(cx, local_span) + && is_ident(&name) + { + // If the local is an ordinary named variable, + // print its name rather than relying solely on the span. + diag.span_label( + local_span, + format!("`{name}` is the largest part, at {size} for type `{ty}`"), + ); + } else { + diag.span_label( + local_span, + format!("this is the largest part, at {size} for type `{ty}`"), + ); + } + } + + // Explain why we are linting this and not other functions. + diag.note(format!( + "{frame_size} is larger than Clippy's configured `stack-size-threshold` of {limit}" + )); + + // Explain why the user should care, briefly. + diag.note_once( + "allocating large amounts of stack space can overflow the stack \ + and cause the program to abort", + ); + }, ); } } diff --git a/tests/ui-toml/large_stack_frames/large_stack_frames.rs b/tests/ui-toml/large_stack_frames/large_stack_frames.rs index 39798ffea494..a612e56570ff 100644 --- a/tests/ui-toml/large_stack_frames/large_stack_frames.rs +++ b/tests/ui-toml/large_stack_frames/large_stack_frames.rs @@ -10,7 +10,7 @@ fn f() { let _x = create_array::<1000>(); } fn f2() { - //~^ ERROR: this function allocates a large amount of stack space + //~^ ERROR: this function may allocate 1001 bytes on the stack let _x = create_array::<1001>(); } diff --git a/tests/ui-toml/large_stack_frames/large_stack_frames.stderr b/tests/ui-toml/large_stack_frames/large_stack_frames.stderr index c23fac145646..19983e2f3e80 100644 --- a/tests/ui-toml/large_stack_frames/large_stack_frames.stderr +++ b/tests/ui-toml/large_stack_frames/large_stack_frames.stderr @@ -1,13 +1,14 @@ -error: this function allocates a large amount of stack space - --> tests/ui-toml/large_stack_frames/large_stack_frames.rs:12:1 +error: this function may allocate 1001 bytes on the stack + --> tests/ui-toml/large_stack_frames/large_stack_frames.rs:12:4 | -LL | / fn f2() { -LL | | -LL | | let _x = create_array::<1001>(); -LL | | } - | |_^ +LL | fn f2() { + | ^^ +LL | +LL | let _x = create_array::<1001>(); + | -- `_x` is the largest part, at 1001 bytes for type `[u8; 1001]` | - = note: allocating large amounts of stack space can overflow the stack + = note: 1001 bytes is larger than Clippy's configured `stack-size-threshold` of 1000 + = note: allocating large amounts of stack space can overflow the stack and cause the program to abort = note: `-D clippy::large-stack-frames` implied by `-D warnings` = help: to override `-D warnings` add `#[allow(clippy::large_stack_frames)]` diff --git a/tests/ui/large_stack_frames.rs b/tests/ui/large_stack_frames.rs index f32368f93975..e6c030b8a9e8 100644 --- a/tests/ui/large_stack_frames.rs +++ b/tests/ui/large_stack_frames.rs @@ -1,3 +1,5 @@ +//@ normalize-stderr-test: "\b10000(08|16|32)\b" -> "100$$PTR" +//@ normalize-stderr-test: "\b2500(060|120)\b" -> "250$$PTR" #![allow(unused, incomplete_features)] #![warn(clippy::large_stack_frames)] #![feature(unsized_locals)] @@ -23,8 +25,7 @@ impl Default for ArrayDefault { } fn many_small_arrays() { - //~^ ERROR: this function allocates a large amount of stack space - //~| NOTE: allocating large amounts of stack space can overflow the stack + //~^ ERROR: this function may allocate let x = [0u8; 500_000]; let x2 = [0u8; 500_000]; let x3 = [0u8; 500_000]; @@ -34,17 +35,21 @@ fn many_small_arrays() { } fn large_return_value() -> ArrayDefault<1_000_000> { - //~^ ERROR: this function allocates a large amount of stack space - //~| NOTE: allocating large amounts of stack space can overflow the stack + //~^ ERROR: this function may allocate 1000000 bytes on the stack Default::default() } fn large_fn_arg(x: ArrayDefault<1_000_000>) { - //~^ ERROR: this function allocates a large amount of stack space - //~| NOTE: allocating large amounts of stack space can overflow the stack + //~^ ERROR: this function may allocate black_box(&x); } +fn has_large_closure() { + let f = || black_box(&[0u8; 1_000_000]); + //~^ ERROR: this function may allocate + f(); +} + fn main() { generic::>(); } diff --git a/tests/ui/large_stack_frames.stderr b/tests/ui/large_stack_frames.stderr index b99500fd9c34..f2e0a127f5f5 100644 --- a/tests/ui/large_stack_frames.stderr +++ b/tests/ui/large_stack_frames.stderr @@ -1,42 +1,42 @@ -error: this function allocates a large amount of stack space - --> tests/ui/large_stack_frames.rs:25:1 - | -LL | / fn many_small_arrays() { -LL | | -LL | | -LL | | let x = [0u8; 500_000]; -... | -LL | | black_box((&x, &x2, &x3, &x4, &x5)); -LL | | } - | |_^ - | - = note: allocating large amounts of stack space can overflow the stack +error: this function may allocate 250$PTR bytes on the stack + --> tests/ui/large_stack_frames.rs:27:4 + | +LL | fn many_small_arrays() { + | ^^^^^^^^^^^^^^^^^ +... +LL | let x5 = [0u8; 500_000]; + | -- `x5` is the largest part, at 500000 bytes for type `[u8; 500000]` + | + = note: 250$PTR bytes is larger than Clippy's configured `stack-size-threshold` of 512000 + = note: allocating large amounts of stack space can overflow the stack and cause the program to abort = note: `-D clippy::large-stack-frames` implied by `-D warnings` = help: to override `-D warnings` add `#[allow(clippy::large_stack_frames)]` -error: this function allocates a large amount of stack space - --> tests/ui/large_stack_frames.rs:36:1 +error: this function may allocate 1000000 bytes on the stack + --> tests/ui/large_stack_frames.rs:37:4 + | +LL | fn large_return_value() -> ArrayDefault<1_000_000> { + | ^^^^^^^^^^^^^^^^^^ ----------------------- this is the largest part, at 1000000 bytes for type `ArrayDefault<1000000>` + | + = note: 1000000 bytes is larger than Clippy's configured `stack-size-threshold` of 512000 + +error: this function may allocate 100$PTR bytes on the stack + --> tests/ui/large_stack_frames.rs:42:4 | -LL | / fn large_return_value() -> ArrayDefault<1_000_000> { -LL | | -LL | | -LL | | Default::default() -LL | | } - | |_^ +LL | fn large_fn_arg(x: ArrayDefault<1_000_000>) { + | ^^^^^^^^^^^^ - `x` is the largest part, at 1000000 bytes for type `ArrayDefault<1000000>` | - = note: allocating large amounts of stack space can overflow the stack + = note: 100$PTR bytes is larger than Clippy's configured `stack-size-threshold` of 512000 -error: this function allocates a large amount of stack space - --> tests/ui/large_stack_frames.rs:42:1 +error: this function may allocate 100$PTR bytes on the stack + --> tests/ui/large_stack_frames.rs:48:13 | -LL | / fn large_fn_arg(x: ArrayDefault<1_000_000>) { -LL | | -LL | | -LL | | black_box(&x); -LL | | } - | |_^ +LL | let f = || black_box(&[0u8; 1_000_000]); + | ^^^^^^^^^^^^^^----------------^ + | | + | this is the largest part, at 1000000 bytes for type `[u8; 1000000]` | - = note: allocating large amounts of stack space can overflow the stack + = note: 100$PTR bytes is larger than Clippy's configured `stack-size-threshold` of 512000 -error: aborting due to 3 previous errors +error: aborting due to 4 previous errors From 30697a5ae651c936515b6b211c78bba1170fca82 Mon Sep 17 00:00:00 2001 From: shandongbinzhou Date: Fri, 29 Mar 2024 16:17:07 +0800 Subject: [PATCH 08/22] Fix typo in comment Signed-off-by: shandongbinzhou --- clippy_lints/src/serde_api.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clippy_lints/src/serde_api.rs b/clippy_lints/src/serde_api.rs index b4278d879e54..f1ec91d7affc 100644 --- a/clippy_lints/src/serde_api.rs +++ b/clippy_lints/src/serde_api.rs @@ -9,7 +9,7 @@ declare_clippy_lint! { /// Checks for misuses of the serde API. /// /// ### Why is this bad? - /// Serde is very finnicky about how its API should be + /// Serde is very finicky about how its API should be /// used, but the type system can't be used to enforce it (yet?). /// /// ### Example From dd4ae6af1cf38fd5f531bcb4933ac2fa18959e6b Mon Sep 17 00:00:00 2001 From: Jacob Kiesel Date: Sat, 23 Mar 2024 20:59:30 -0600 Subject: [PATCH 09/22] restrict manual_clamp to const case, bring it out of nursery --- clippy_lints/src/manual_clamp.rs | 30 ++- tests/ui/manual_clamp.fixed | 279 +++++++++++++++++++------- tests/ui/manual_clamp.rs | 333 ++++++++++++++++++++++--------- tests/ui/manual_clamp.stderr | 188 ++++++++--------- 4 files changed, 570 insertions(+), 260 deletions(-) diff --git a/clippy_lints/src/manual_clamp.rs b/clippy_lints/src/manual_clamp.rs index 830af77968c0..0148e6deb56c 100644 --- a/clippy_lints/src/manual_clamp.rs +++ b/clippy_lints/src/manual_clamp.rs @@ -1,4 +1,5 @@ use clippy_config::msrvs::{self, Msrv}; +use clippy_utils::consts::{constant, Constant}; use clippy_utils::diagnostics::{span_lint_and_then, span_lint_hir_and_then}; use clippy_utils::higher::If; use clippy_utils::sugg::Sugg; @@ -17,6 +18,7 @@ use rustc_middle::ty::Ty; use rustc_session::impl_lint_pass; use rustc_span::symbol::sym; use rustc_span::Span; +use std::cmp::Ordering; use std::ops::Deref; declare_clippy_lint! { @@ -80,7 +82,7 @@ declare_clippy_lint! { /// ``` #[clippy::version = "1.66.0"] pub MANUAL_CLAMP, - nursery, + complexity, "using a clamp pattern instead of the clamp function" } impl_lint_pass!(ManualClamp => [MANUAL_CLAMP]); @@ -103,6 +105,24 @@ struct ClampSuggestion<'tcx> { hir_with_ignore_attr: Option, } +impl<'tcx> ClampSuggestion<'tcx> { + /// This function will return true if and only if you can demonstrate at compile time that min + /// is less than max. + fn min_less_than_max(&self, cx: &LateContext<'tcx>) -> bool { + let max_type = cx.typeck_results().expr_ty(self.params.max); + let min_type = cx.typeck_results().expr_ty(self.params.min); + if max_type != min_type { + return false; + } + let max = constant(cx, cx.typeck_results(), self.params.max); + let min = constant(cx, cx.typeck_results(), self.params.min); + let cmp = max + .zip(min) + .and_then(|(max, min)| Constant::partial_cmp(cx.tcx, max_type, &min, &max)); + cmp.is_some_and(|cmp| cmp != Ordering::Greater) + } +} + #[derive(Debug)] struct InputMinMax<'tcx> { input: &'tcx Expr<'tcx>, @@ -123,7 +143,9 @@ impl<'tcx> LateLintPass<'tcx> for ManualClamp { .or_else(|| is_match_pattern(cx, expr)) .or_else(|| is_if_elseif_pattern(cx, expr)); if let Some(suggestion) = suggestion { - emit_suggestion(cx, &suggestion); + if suggestion.min_less_than_max(cx) { + emit_suggestion(cx, &suggestion); + } } } } @@ -133,7 +155,9 @@ impl<'tcx> LateLintPass<'tcx> for ManualClamp { return; } for suggestion in is_two_if_pattern(cx, block) { - emit_suggestion(cx, &suggestion); + if suggestion.min_less_than_max(cx) { + emit_suggestion(cx, &suggestion); + } } } extract_msrv_attr!(LateContext); diff --git a/tests/ui/manual_clamp.fixed b/tests/ui/manual_clamp.fixed index c5355cce8e21..8d57cbbf51bf 100644 --- a/tests/ui/manual_clamp.fixed +++ b/tests/ui/manual_clamp.fixed @@ -17,48 +17,171 @@ const CONST_F64_MIN: f64 = 4.0; fn main() { let (input, min, max) = (0, -2, 3); - // Lint - let x0 = input.clamp(min, max); + // Min and max are not const, so this shouldn't trigger the lint. + let x0 = if max < input { + max + } else if min > input { + min + } else { + input + }; + + let x1 = if input > max { + max + } else if input < min { + min + } else { + input + }; + + let x2 = if input < min { + min + } else if input > max { + max + } else { + input + }; - let x1 = input.clamp(min, max); + let x3 = if min > input { + min + } else if max < input { + max + } else { + input + }; - let x2 = input.clamp(min, max); + let x4 = input.max(min).min(max); - let x3 = input.clamp(min, max); + let x5 = input.min(max).max(min); + + let x6 = match input { + x if x > max => max, + x if x < min => min, + x => x, + }; - let x4 = input.clamp(min, max); + let x7 = match input { + x if x < min => min, + x if x > max => max, + x => x, + }; + + let x8 = match input { + x if max < x => max, + x if min > x => min, + x => x, + }; + + let mut x9 = input; + if x9 < min { + x9 = min; + } + if x9 > max { + x9 = max; + } + + let x10 = match input { + x if min > x => min, + x if max < x => max, + x => x, + }; + + let mut x11 = input; + let _ = 1; + if x11 > max { + x11 = max; + } + if x11 < min { + x11 = min; + } + + let mut x12 = input; + if min > x12 { + x12 = min; + } + if max < x12 { + x12 = max; + } + + let mut x13 = input; + if max < x13 { + x13 = max; + } + if min > x13 { + x13 = min; + } + + { + let (input, min, max) = (0.0f64, -2.0, 3.0); + let x14 = if input > max { + max + } else if input < min { + min + } else { + input + }; + } + let mut x15 = input; + if x15 < min { + x15 = min; + } else if x15 > max { + x15 = max; + } + + // It's important this be the last set of statements + let mut x16 = input; + if max < x16 { + x16 = max; + } + if min > x16 { + x16 = min; + } +} + +fn const_main() { + let input = 0; + // Min and max are const, so this should trigger the lint. + let x0 = input.clamp(CONST_MIN, CONST_MAX); + + let x1 = input.clamp(CONST_MIN, CONST_MAX); + + let x2 = input.clamp(CONST_MIN, CONST_MAX); + + let x3 = input.clamp(CONST_MIN, CONST_MAX); + + let x4 = input.clamp(CONST_MIN, CONST_MAX); //~^ ERROR: clamp-like pattern without using clamp function //~| NOTE: clamp will panic if max < min - let x5 = input.clamp(min, max); + let x5 = input.clamp(CONST_MIN, CONST_MAX); //~^ ERROR: clamp-like pattern without using clamp function //~| NOTE: clamp will panic if max < min - let x6 = input.clamp(min, max); + let x6 = input.clamp(CONST_MIN, CONST_MAX); - let x7 = input.clamp(min, max); + let x7 = input.clamp(CONST_MIN, CONST_MAX); - let x8 = input.clamp(min, max); + let x8 = input.clamp(CONST_MIN, CONST_MAX); let mut x9 = input; - x9 = x9.clamp(min, max); + x9 = x9.clamp(CONST_MIN, CONST_MAX); - let x10 = input.clamp(min, max); + let x10 = input.clamp(CONST_MIN, CONST_MAX); let mut x11 = input; let _ = 1; - x11 = x11.clamp(min, max); + x11 = x11.clamp(CONST_MIN, CONST_MAX); let mut x12 = input; - x12 = x12.clamp(min, max); + x12 = x12.clamp(CONST_MIN, CONST_MAX); let mut x13 = input; - x13 = x13.clamp(min, max); + x13 = x13.clamp(CONST_MIN, CONST_MAX); let x14 = input.clamp(CONST_MIN, CONST_MAX); { - let (input, min, max) = (0.0f64, -2.0, 3.0); - let x15 = input.clamp(min, max); + let input = 0.0f64; + let x15 = input.clamp(CONST_F64_MIN, CONST_F64_MAX); } { let input: i32 = cmp_min_max(1); @@ -114,108 +237,128 @@ fn main() { //~| NOTE: clamp will panic if max < min, min.is_nan(), or max.is_nan() } let mut x32 = input; - x32 = x32.clamp(min, max); + x32 = x32.clamp(CONST_MIN, CONST_MAX); - // It's important this be the last set of statements + // Flip the script, swap the places of min and max. Make sure this doesn't + // trigger when clamp would be guaranteed to panic. let mut x33 = input; - x33 = x33.clamp(min, max); + if x33 < CONST_MAX { + x33 = CONST_MAX; + } else if x33 > CONST_MIN { + x33 = CONST_MIN; + } + + // Do it again for NaN + #[allow(invalid_nan_comparisons)] + { + let mut x34 = input as f64; + if x34 < f64::NAN { + x34 = f64::NAN; + } else if x34 > CONST_F64_MAX { + x34 = CONST_F64_MAX; + } + } + + // It's important this be the last set of statements + let mut x35 = input; + x35 = x35.clamp(CONST_MIN, CONST_MAX); } // This code intentionally nonsense. fn no_lint() { - let (input, min, max) = (0, -2, 3); - let x0 = if max < input { - max - } else if min > input { - max + let input = 0; + let x0 = if CONST_MAX < input { + CONST_MAX + } else if CONST_MIN > input { + CONST_MAX } else { - min + CONST_MIN }; - let x1 = if input > max { - max - } else if input > min { - min + let x1 = if input > CONST_MAX { + CONST_MAX + } else if input > CONST_MIN { + CONST_MIN } else { - max + CONST_MAX }; - let x2 = if max < min { - min - } else if input > max { + let x2 = if CONST_MAX < CONST_MIN { + CONST_MIN + } else if input > CONST_MAX { input } else { input }; - let x3 = if min > input { + let x3 = if CONST_MIN > input { input - } else if max < input { - max + } else if CONST_MAX < input { + CONST_MAX } else { - max + CONST_MAX }; let x6 = match input { - x if x < max => x, - x if x < min => x, + x if x < CONST_MAX => x, + x if x < CONST_MIN => x, x => x, }; let x7 = match input { - x if x < min => max, - x if x > max => min, + x if x < CONST_MIN => CONST_MAX, + x if x > CONST_MAX => CONST_MIN, x => x, }; let x8 = match input { - x if max > x => max, - x if min > x => min, + x if CONST_MAX > x => CONST_MAX, + x if CONST_MIN > x => CONST_MIN, x => x, }; let mut x9 = input; - if x9 > min { - x9 = min; + if x9 > CONST_MIN { + x9 = CONST_MIN; } - if x9 > max { - x9 = max; + if x9 > CONST_MAX { + x9 = CONST_MAX; } let x10 = match input { - x if min > x => min, - x if max < x => max, - x => min, + x if CONST_MIN > x => CONST_MIN, + x if CONST_MAX < x => CONST_MAX, + x => CONST_MIN, }; let mut x11 = input; - if x11 > max { - x11 = min; + if x11 > CONST_MAX { + x11 = CONST_MIN; } - if x11 < min { - x11 = max; + if x11 < CONST_MIN { + x11 = CONST_MAX; } let mut x12 = input; - if min > x12 { - x12 = max * 3; + if CONST_MIN > x12 { + x12 = CONST_MAX * 3; } - if max < x12 { - x12 = min; + if CONST_MAX < x12 { + x12 = CONST_MIN; } let mut x13 = input; - if max < x13 { - let x13 = max; + if CONST_MAX < x13 { + let x13 = CONST_MAX; } - if min > x13 { - x13 = min; + if CONST_MIN > x13 { + x13 = CONST_MIN; } let mut x14 = input; - if x14 < min { + if x14 < CONST_MIN { x14 = 3; - } else if x14 > max { - x14 = max; + } else if x14 > CONST_MAX { + x14 = CONST_MAX; } { let input: i32 = cmp_min_max(1); @@ -272,8 +415,8 @@ fn msrv_1_49() { #[clippy::msrv = "1.50"] fn msrv_1_50() { - let (input, min, max) = (0, -1, 2); - let _ = input.clamp(min, max); + let input = 0; + let _ = input.clamp(CONST_MIN, CONST_MAX); } const fn _const() { diff --git a/tests/ui/manual_clamp.rs b/tests/ui/manual_clamp.rs index cacb40ae027f..4d2a0c47bbd7 100644 --- a/tests/ui/manual_clamp.rs +++ b/tests/ui/manual_clamp.rs @@ -17,10 +17,8 @@ const CONST_F64_MIN: f64 = 4.0; fn main() { let (input, min, max) = (0, -2, 3); - // Lint + // Min and max are not const, so this shouldn't trigger the lint. let x0 = if max < input { - //~^ ERROR: clamp-like pattern without using clamp function - //~| NOTE: clamp will panic if max < min max } else if min > input { min @@ -29,8 +27,6 @@ fn main() { }; let x1 = if input > max { - //~^ ERROR: clamp-like pattern without using clamp function - //~| NOTE: clamp will panic if max < min max } else if input < min { min @@ -39,8 +35,6 @@ fn main() { }; let x2 = if input < min { - //~^ ERROR: clamp-like pattern without using clamp function - //~| NOTE: clamp will panic if max < min min } else if input > max { max @@ -49,8 +43,6 @@ fn main() { }; let x3 = if min > input { - //~^ ERROR: clamp-like pattern without using clamp function - //~| NOTE: clamp will panic if max < min min } else if max < input { max @@ -59,32 +51,22 @@ fn main() { }; let x4 = input.max(min).min(max); - //~^ ERROR: clamp-like pattern without using clamp function - //~| NOTE: clamp will panic if max < min let x5 = input.min(max).max(min); - //~^ ERROR: clamp-like pattern without using clamp function - //~| NOTE: clamp will panic if max < min let x6 = match input { - //~^ ERROR: clamp-like pattern without using clamp function - //~| NOTE: clamp will panic if max < min x if x > max => max, x if x < min => min, x => x, }; let x7 = match input { - //~^ ERROR: clamp-like pattern without using clamp function - //~| NOTE: clamp will panic if max < min x if x < min => min, x if x > max => max, x => x, }; let x8 = match input { - //~^ ERROR: clamp-like pattern without using clamp function - //~| NOTE: clamp will panic if max < min x if max < x => max, x if min > x => min, x => x, @@ -92,8 +74,6 @@ fn main() { let mut x9 = input; if x9 < min { - //~^ ERROR: clamp-like pattern without using clamp function - //~| NOTE: clamp will panic if max < min x9 = min; } if x9 > max { @@ -101,8 +81,6 @@ fn main() { } let x10 = match input { - //~^ ERROR: clamp-like pattern without using clamp function - //~| NOTE: clamp will panic if max < min x if min > x => min, x if max < x => max, x => x, @@ -111,8 +89,6 @@ fn main() { let mut x11 = input; let _ = 1; if x11 > max { - //~^ ERROR: clamp-like pattern without using clamp function - //~| NOTE: clamp will panic if max < min x11 = max; } if x11 < min { @@ -121,8 +97,6 @@ fn main() { let mut x12 = input; if min > x12 { - //~^ ERROR: clamp-like pattern without using clamp function - //~| NOTE: clamp will panic if max < min x12 = min; } if max < x12 { @@ -131,14 +105,163 @@ fn main() { let mut x13 = input; if max < x13 { - //~^ ERROR: clamp-like pattern without using clamp function - //~| NOTE: clamp will panic if max < min x13 = max; } if min > x13 { x13 = min; } + { + let (input, min, max) = (0.0f64, -2.0, 3.0); + let x14 = if input > max { + max + } else if input < min { + min + } else { + input + }; + } + let mut x15 = input; + if x15 < min { + x15 = min; + } else if x15 > max { + x15 = max; + } + + // It's important this be the last set of statements + let mut x16 = input; + if max < x16 { + x16 = max; + } + if min > x16 { + x16 = min; + } +} + +fn const_main() { + let input = 0; + // Min and max are const, so this should trigger the lint. + let x0 = if CONST_MAX < input { + //~^ ERROR: clamp-like pattern without using clamp function + //~| NOTE: clamp will panic if max < min + CONST_MAX + } else if CONST_MIN > input { + CONST_MIN + } else { + input + }; + + let x1 = if input > CONST_MAX { + //~^ ERROR: clamp-like pattern without using clamp function + //~| NOTE: clamp will panic if max < min + CONST_MAX + } else if input < CONST_MIN { + CONST_MIN + } else { + input + }; + + let x2 = if input < CONST_MIN { + //~^ ERROR: clamp-like pattern without using clamp function + //~| NOTE: clamp will panic if max < min + CONST_MIN + } else if input > CONST_MAX { + CONST_MAX + } else { + input + }; + + let x3 = if CONST_MIN > input { + //~^ ERROR: clamp-like pattern without using clamp function + //~| NOTE: clamp will panic if max < min + CONST_MIN + } else if CONST_MAX < input { + CONST_MAX + } else { + input + }; + + let x4 = input.max(CONST_MIN).min(CONST_MAX); + //~^ ERROR: clamp-like pattern without using clamp function + //~| NOTE: clamp will panic if max < min + + let x5 = input.min(CONST_MAX).max(CONST_MIN); + //~^ ERROR: clamp-like pattern without using clamp function + //~| NOTE: clamp will panic if max < min + + let x6 = match input { + //~^ ERROR: clamp-like pattern without using clamp function + //~| NOTE: clamp will panic if max < min + x if x > CONST_MAX => CONST_MAX, + x if x < CONST_MIN => CONST_MIN, + x => x, + }; + + let x7 = match input { + //~^ ERROR: clamp-like pattern without using clamp function + //~| NOTE: clamp will panic if max < min + x if x < CONST_MIN => CONST_MIN, + x if x > CONST_MAX => CONST_MAX, + x => x, + }; + + let x8 = match input { + //~^ ERROR: clamp-like pattern without using clamp function + //~| NOTE: clamp will panic if max < min + x if CONST_MAX < x => CONST_MAX, + x if CONST_MIN > x => CONST_MIN, + x => x, + }; + + let mut x9 = input; + if x9 < CONST_MIN { + //~^ ERROR: clamp-like pattern without using clamp function + //~| NOTE: clamp will panic if max < min + x9 = CONST_MIN; + } + if x9 > CONST_MAX { + x9 = CONST_MAX; + } + + let x10 = match input { + //~^ ERROR: clamp-like pattern without using clamp function + //~| NOTE: clamp will panic if max < min + x if CONST_MIN > x => CONST_MIN, + x if CONST_MAX < x => CONST_MAX, + x => x, + }; + + let mut x11 = input; + let _ = 1; + if x11 > CONST_MAX { + //~^ ERROR: clamp-like pattern without using clamp function + //~| NOTE: clamp will panic if max < min + x11 = CONST_MAX; + } + if x11 < CONST_MIN { + x11 = CONST_MIN; + } + + let mut x12 = input; + if CONST_MIN > x12 { + //~^ ERROR: clamp-like pattern without using clamp function + //~| NOTE: clamp will panic if max < min + x12 = CONST_MIN; + } + if CONST_MAX < x12 { + x12 = CONST_MAX; + } + + let mut x13 = input; + if CONST_MAX < x13 { + //~^ ERROR: clamp-like pattern without using clamp function + //~| NOTE: clamp will panic if max < min + x13 = CONST_MAX; + } + if CONST_MIN > x13 { + x13 = CONST_MIN; + } + let x14 = if input > CONST_MAX { //~^ ERROR: clamp-like pattern without using clamp function //~| NOTE: clamp will panic if max < min @@ -149,13 +272,13 @@ fn main() { input }; { - let (input, min, max) = (0.0f64, -2.0, 3.0); - let x15 = if input > max { + let input = 0.0f64; + let x15 = if input > CONST_F64_MAX { //~^ ERROR: clamp-like pattern without using clamp function - //~| NOTE: clamp will panic if max < min, min.is_nan(), or max.is_nan() - max - } else if input < min { - min + //~| NOTE: clamp will panic if max < min + CONST_F64_MAX + } else if input < CONST_F64_MIN { + CONST_F64_MIN } else { input }; @@ -214,121 +337,141 @@ fn main() { //~| NOTE: clamp will panic if max < min, min.is_nan(), or max.is_nan() } let mut x32 = input; - if x32 < min { + if x32 < CONST_MIN { //~^ ERROR: clamp-like pattern without using clamp function //~| NOTE: clamp will panic if max < min - x32 = min; - } else if x32 > max { - x32 = max; + x32 = CONST_MIN; + } else if x32 > CONST_MAX { + x32 = CONST_MAX; } - // It's important this be the last set of statements + // Flip the script, swap the places of min and max. Make sure this doesn't + // trigger when clamp would be guaranteed to panic. let mut x33 = input; - if max < x33 { + if x33 < CONST_MAX { + x33 = CONST_MAX; + } else if x33 > CONST_MIN { + x33 = CONST_MIN; + } + + // Do it again for NaN + #[allow(invalid_nan_comparisons)] + { + let mut x34 = input as f64; + if x34 < f64::NAN { + x34 = f64::NAN; + } else if x34 > CONST_F64_MAX { + x34 = CONST_F64_MAX; + } + } + + // It's important this be the last set of statements + let mut x35 = input; + if CONST_MAX < x35 { //~^ ERROR: clamp-like pattern without using clamp function //~| NOTE: clamp will panic if max < min - x33 = max; + x35 = CONST_MAX; } - if min > x33 { - x33 = min; + if CONST_MIN > x35 { + x35 = CONST_MIN; } } // This code intentionally nonsense. fn no_lint() { - let (input, min, max) = (0, -2, 3); - let x0 = if max < input { - max - } else if min > input { - max + let input = 0; + let x0 = if CONST_MAX < input { + CONST_MAX + } else if CONST_MIN > input { + CONST_MAX } else { - min + CONST_MIN }; - let x1 = if input > max { - max - } else if input > min { - min + let x1 = if input > CONST_MAX { + CONST_MAX + } else if input > CONST_MIN { + CONST_MIN } else { - max + CONST_MAX }; - let x2 = if max < min { - min - } else if input > max { + let x2 = if CONST_MAX < CONST_MIN { + CONST_MIN + } else if input > CONST_MAX { input } else { input }; - let x3 = if min > input { + let x3 = if CONST_MIN > input { input - } else if max < input { - max + } else if CONST_MAX < input { + CONST_MAX } else { - max + CONST_MAX }; let x6 = match input { - x if x < max => x, - x if x < min => x, + x if x < CONST_MAX => x, + x if x < CONST_MIN => x, x => x, }; let x7 = match input { - x if x < min => max, - x if x > max => min, + x if x < CONST_MIN => CONST_MAX, + x if x > CONST_MAX => CONST_MIN, x => x, }; let x8 = match input { - x if max > x => max, - x if min > x => min, + x if CONST_MAX > x => CONST_MAX, + x if CONST_MIN > x => CONST_MIN, x => x, }; let mut x9 = input; - if x9 > min { - x9 = min; + if x9 > CONST_MIN { + x9 = CONST_MIN; } - if x9 > max { - x9 = max; + if x9 > CONST_MAX { + x9 = CONST_MAX; } let x10 = match input { - x if min > x => min, - x if max < x => max, - x => min, + x if CONST_MIN > x => CONST_MIN, + x if CONST_MAX < x => CONST_MAX, + x => CONST_MIN, }; let mut x11 = input; - if x11 > max { - x11 = min; + if x11 > CONST_MAX { + x11 = CONST_MIN; } - if x11 < min { - x11 = max; + if x11 < CONST_MIN { + x11 = CONST_MAX; } let mut x12 = input; - if min > x12 { - x12 = max * 3; + if CONST_MIN > x12 { + x12 = CONST_MAX * 3; } - if max < x12 { - x12 = min; + if CONST_MAX < x12 { + x12 = CONST_MIN; } let mut x13 = input; - if max < x13 { - let x13 = max; + if CONST_MAX < x13 { + let x13 = CONST_MAX; } - if min > x13 { - x13 = min; + if CONST_MIN > x13 { + x13 = CONST_MIN; } let mut x14 = input; - if x14 < min { + if x14 < CONST_MIN { x14 = 3; - } else if x14 > max { - x14 = max; + } else if x14 > CONST_MAX { + x14 = CONST_MAX; } { let input: i32 = cmp_min_max(1); @@ -385,13 +528,13 @@ fn msrv_1_49() { #[clippy::msrv = "1.50"] fn msrv_1_50() { - let (input, min, max) = (0, -1, 2); - let _ = if input < min { + let input = 0; + let _ = if input > CONST_MAX { //~^ ERROR: clamp-like pattern without using clamp function //~| NOTE: clamp will panic if max < min - min - } else if input > max { - max + CONST_MAX + } else if input < CONST_MIN { + CONST_MIN } else { input }; diff --git a/tests/ui/manual_clamp.stderr b/tests/ui/manual_clamp.stderr index 52c816f2b347..459d46796d87 100644 --- a/tests/ui/manual_clamp.stderr +++ b/tests/ui/manual_clamp.stderr @@ -1,213 +1,213 @@ error: clamp-like pattern without using clamp function - --> tests/ui/manual_clamp.rs:94:5 + --> tests/ui/manual_clamp.rs:217:5 | -LL | / if x9 < min { +LL | / if x9 < CONST_MIN { LL | | LL | | -LL | | x9 = min; +LL | | x9 = CONST_MIN; ... | -LL | | x9 = max; +LL | | x9 = CONST_MAX; LL | | } - | |_____^ help: replace with clamp: `x9 = x9.clamp(min, max);` + | |_____^ help: replace with clamp: `x9 = x9.clamp(CONST_MIN, CONST_MAX);` | = note: clamp will panic if max < min = note: `-D clippy::manual-clamp` implied by `-D warnings` = help: to override `-D warnings` add `#[allow(clippy::manual_clamp)]` error: clamp-like pattern without using clamp function - --> tests/ui/manual_clamp.rs:113:5 + --> tests/ui/manual_clamp.rs:236:5 | -LL | / if x11 > max { +LL | / if x11 > CONST_MAX { LL | | LL | | -LL | | x11 = max; +LL | | x11 = CONST_MAX; ... | -LL | | x11 = min; +LL | | x11 = CONST_MIN; LL | | } - | |_____^ help: replace with clamp: `x11 = x11.clamp(min, max);` + | |_____^ help: replace with clamp: `x11 = x11.clamp(CONST_MIN, CONST_MAX);` | = note: clamp will panic if max < min error: clamp-like pattern without using clamp function - --> tests/ui/manual_clamp.rs:123:5 + --> tests/ui/manual_clamp.rs:246:5 | -LL | / if min > x12 { +LL | / if CONST_MIN > x12 { LL | | LL | | -LL | | x12 = min; +LL | | x12 = CONST_MIN; ... | -LL | | x12 = max; +LL | | x12 = CONST_MAX; LL | | } - | |_____^ help: replace with clamp: `x12 = x12.clamp(min, max);` + | |_____^ help: replace with clamp: `x12 = x12.clamp(CONST_MIN, CONST_MAX);` | = note: clamp will panic if max < min error: clamp-like pattern without using clamp function - --> tests/ui/manual_clamp.rs:133:5 + --> tests/ui/manual_clamp.rs:256:5 | -LL | / if max < x13 { +LL | / if CONST_MAX < x13 { LL | | LL | | -LL | | x13 = max; +LL | | x13 = CONST_MAX; ... | -LL | | x13 = min; +LL | | x13 = CONST_MIN; LL | | } - | |_____^ help: replace with clamp: `x13 = x13.clamp(min, max);` + | |_____^ help: replace with clamp: `x13 = x13.clamp(CONST_MIN, CONST_MAX);` | = note: clamp will panic if max < min error: clamp-like pattern without using clamp function - --> tests/ui/manual_clamp.rs:227:5 + --> tests/ui/manual_clamp.rs:370:5 | -LL | / if max < x33 { +LL | / if CONST_MAX < x35 { LL | | LL | | -LL | | x33 = max; +LL | | x35 = CONST_MAX; ... | -LL | | x33 = min; +LL | | x35 = CONST_MIN; LL | | } - | |_____^ help: replace with clamp: `x33 = x33.clamp(min, max);` + | |_____^ help: replace with clamp: `x35 = x35.clamp(CONST_MIN, CONST_MAX);` | = note: clamp will panic if max < min error: clamp-like pattern without using clamp function - --> tests/ui/manual_clamp.rs:21:14 + --> tests/ui/manual_clamp.rs:144:14 | -LL | let x0 = if max < input { +LL | let x0 = if CONST_MAX < input { | ______________^ LL | | LL | | -LL | | max +LL | | CONST_MAX ... | LL | | input LL | | }; - | |_____^ help: replace with clamp: `input.clamp(min, max)` + | |_____^ help: replace with clamp: `input.clamp(CONST_MIN, CONST_MAX)` | = note: clamp will panic if max < min error: clamp-like pattern without using clamp function - --> tests/ui/manual_clamp.rs:31:14 + --> tests/ui/manual_clamp.rs:154:14 | -LL | let x1 = if input > max { +LL | let x1 = if input > CONST_MAX { | ______________^ LL | | LL | | -LL | | max +LL | | CONST_MAX ... | LL | | input LL | | }; - | |_____^ help: replace with clamp: `input.clamp(min, max)` + | |_____^ help: replace with clamp: `input.clamp(CONST_MIN, CONST_MAX)` | = note: clamp will panic if max < min error: clamp-like pattern without using clamp function - --> tests/ui/manual_clamp.rs:41:14 + --> tests/ui/manual_clamp.rs:164:14 | -LL | let x2 = if input < min { +LL | let x2 = if input < CONST_MIN { | ______________^ LL | | LL | | -LL | | min +LL | | CONST_MIN ... | LL | | input LL | | }; - | |_____^ help: replace with clamp: `input.clamp(min, max)` + | |_____^ help: replace with clamp: `input.clamp(CONST_MIN, CONST_MAX)` | = note: clamp will panic if max < min error: clamp-like pattern without using clamp function - --> tests/ui/manual_clamp.rs:51:14 + --> tests/ui/manual_clamp.rs:174:14 | -LL | let x3 = if min > input { +LL | let x3 = if CONST_MIN > input { | ______________^ LL | | LL | | -LL | | min +LL | | CONST_MIN ... | LL | | input LL | | }; - | |_____^ help: replace with clamp: `input.clamp(min, max)` + | |_____^ help: replace with clamp: `input.clamp(CONST_MIN, CONST_MAX)` | = note: clamp will panic if max < min error: clamp-like pattern without using clamp function - --> tests/ui/manual_clamp.rs:61:14 + --> tests/ui/manual_clamp.rs:184:14 | -LL | let x4 = input.max(min).min(max); - | ^^^^^^^^^^^^^^^^^^^^^^^ help: replace with clamp: `input.clamp(min, max)` +LL | let x4 = input.max(CONST_MIN).min(CONST_MAX); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace with clamp: `input.clamp(CONST_MIN, CONST_MAX)` | = note: clamp will panic if max < min error: clamp-like pattern without using clamp function - --> tests/ui/manual_clamp.rs:65:14 + --> tests/ui/manual_clamp.rs:188:14 | -LL | let x5 = input.min(max).max(min); - | ^^^^^^^^^^^^^^^^^^^^^^^ help: replace with clamp: `input.clamp(min, max)` +LL | let x5 = input.min(CONST_MAX).max(CONST_MIN); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace with clamp: `input.clamp(CONST_MIN, CONST_MAX)` | = note: clamp will panic if max < min error: clamp-like pattern without using clamp function - --> tests/ui/manual_clamp.rs:69:14 + --> tests/ui/manual_clamp.rs:192:14 | LL | let x6 = match input { | ______________^ LL | | LL | | -LL | | x if x > max => max, -LL | | x if x < min => min, +LL | | x if x > CONST_MAX => CONST_MAX, +LL | | x if x < CONST_MIN => CONST_MIN, LL | | x => x, LL | | }; - | |_____^ help: replace with clamp: `input.clamp(min, max)` + | |_____^ help: replace with clamp: `input.clamp(CONST_MIN, CONST_MAX)` | = note: clamp will panic if max < min error: clamp-like pattern without using clamp function - --> tests/ui/manual_clamp.rs:77:14 + --> tests/ui/manual_clamp.rs:200:14 | LL | let x7 = match input { | ______________^ LL | | LL | | -LL | | x if x < min => min, -LL | | x if x > max => max, +LL | | x if x < CONST_MIN => CONST_MIN, +LL | | x if x > CONST_MAX => CONST_MAX, LL | | x => x, LL | | }; - | |_____^ help: replace with clamp: `input.clamp(min, max)` + | |_____^ help: replace with clamp: `input.clamp(CONST_MIN, CONST_MAX)` | = note: clamp will panic if max < min error: clamp-like pattern without using clamp function - --> tests/ui/manual_clamp.rs:85:14 + --> tests/ui/manual_clamp.rs:208:14 | LL | let x8 = match input { | ______________^ LL | | LL | | -LL | | x if max < x => max, -LL | | x if min > x => min, +LL | | x if CONST_MAX < x => CONST_MAX, +LL | | x if CONST_MIN > x => CONST_MIN, LL | | x => x, LL | | }; - | |_____^ help: replace with clamp: `input.clamp(min, max)` + | |_____^ help: replace with clamp: `input.clamp(CONST_MIN, CONST_MAX)` | = note: clamp will panic if max < min error: clamp-like pattern without using clamp function - --> tests/ui/manual_clamp.rs:103:15 + --> tests/ui/manual_clamp.rs:226:15 | LL | let x10 = match input { | _______________^ LL | | LL | | -LL | | x if min > x => min, -LL | | x if max < x => max, +LL | | x if CONST_MIN > x => CONST_MIN, +LL | | x if CONST_MAX < x => CONST_MAX, LL | | x => x, LL | | }; - | |_____^ help: replace with clamp: `input.clamp(min, max)` + | |_____^ help: replace with clamp: `input.clamp(CONST_MIN, CONST_MAX)` | = note: clamp will panic if max < min error: clamp-like pattern without using clamp function - --> tests/ui/manual_clamp.rs:142:15 + --> tests/ui/manual_clamp.rs:265:15 | LL | let x14 = if input > CONST_MAX { | _______________^ @@ -222,23 +222,23 @@ LL | | }; = note: clamp will panic if max < min error: clamp-like pattern without using clamp function - --> tests/ui/manual_clamp.rs:153:19 + --> tests/ui/manual_clamp.rs:276:19 | -LL | let x15 = if input > max { +LL | let x15 = if input > CONST_F64_MAX { | ___________________^ LL | | LL | | -LL | | max +LL | | CONST_F64_MAX ... | LL | | input LL | | }; - | |_________^ help: replace with clamp: `input.clamp(min, max)` + | |_________^ help: replace with clamp: `input.clamp(CONST_F64_MIN, CONST_F64_MAX)` | = note: clamp will panic if max < min, min.is_nan(), or max.is_nan() = note: clamp returns NaN if the input is NaN error: clamp-like pattern without using clamp function - --> tests/ui/manual_clamp.rs:166:19 + --> tests/ui/manual_clamp.rs:289:19 | LL | let x16 = cmp_max(cmp_min(input, CONST_MAX), CONST_MIN); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace with clamp: `input.clamp(CONST_MIN, CONST_MAX)` @@ -246,7 +246,7 @@ LL | let x16 = cmp_max(cmp_min(input, CONST_MAX), CONST_MIN); = note: clamp will panic if max < min error: clamp-like pattern without using clamp function - --> tests/ui/manual_clamp.rs:169:19 + --> tests/ui/manual_clamp.rs:292:19 | LL | let x17 = cmp_min(cmp_max(input, CONST_MIN), CONST_MAX); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace with clamp: `input.clamp(CONST_MIN, CONST_MAX)` @@ -254,7 +254,7 @@ LL | let x17 = cmp_min(cmp_max(input, CONST_MIN), CONST_MAX); = note: clamp will panic if max < min error: clamp-like pattern without using clamp function - --> tests/ui/manual_clamp.rs:172:19 + --> tests/ui/manual_clamp.rs:295:19 | LL | let x18 = cmp_max(CONST_MIN, cmp_min(input, CONST_MAX)); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace with clamp: `input.clamp(CONST_MIN, CONST_MAX)` @@ -262,7 +262,7 @@ LL | let x18 = cmp_max(CONST_MIN, cmp_min(input, CONST_MAX)); = note: clamp will panic if max < min error: clamp-like pattern without using clamp function - --> tests/ui/manual_clamp.rs:175:19 + --> tests/ui/manual_clamp.rs:298:19 | LL | let x19 = cmp_min(CONST_MAX, cmp_max(input, CONST_MIN)); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace with clamp: `input.clamp(CONST_MIN, CONST_MAX)` @@ -270,7 +270,7 @@ LL | let x19 = cmp_min(CONST_MAX, cmp_max(input, CONST_MIN)); = note: clamp will panic if max < min error: clamp-like pattern without using clamp function - --> tests/ui/manual_clamp.rs:178:19 + --> tests/ui/manual_clamp.rs:301:19 | LL | let x20 = cmp_max(cmp_min(CONST_MAX, input), CONST_MIN); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace with clamp: `input.clamp(CONST_MIN, CONST_MAX)` @@ -278,7 +278,7 @@ LL | let x20 = cmp_max(cmp_min(CONST_MAX, input), CONST_MIN); = note: clamp will panic if max < min error: clamp-like pattern without using clamp function - --> tests/ui/manual_clamp.rs:181:19 + --> tests/ui/manual_clamp.rs:304:19 | LL | let x21 = cmp_min(cmp_max(CONST_MIN, input), CONST_MAX); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace with clamp: `input.clamp(CONST_MIN, CONST_MAX)` @@ -286,7 +286,7 @@ LL | let x21 = cmp_min(cmp_max(CONST_MIN, input), CONST_MAX); = note: clamp will panic if max < min error: clamp-like pattern without using clamp function - --> tests/ui/manual_clamp.rs:184:19 + --> tests/ui/manual_clamp.rs:307:19 | LL | let x22 = cmp_max(CONST_MIN, cmp_min(CONST_MAX, input)); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace with clamp: `input.clamp(CONST_MIN, CONST_MAX)` @@ -294,7 +294,7 @@ LL | let x22 = cmp_max(CONST_MIN, cmp_min(CONST_MAX, input)); = note: clamp will panic if max < min error: clamp-like pattern without using clamp function - --> tests/ui/manual_clamp.rs:187:19 + --> tests/ui/manual_clamp.rs:310:19 | LL | let x23 = cmp_min(CONST_MAX, cmp_max(CONST_MIN, input)); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace with clamp: `input.clamp(CONST_MIN, CONST_MAX)` @@ -302,7 +302,7 @@ LL | let x23 = cmp_min(CONST_MAX, cmp_max(CONST_MIN, input)); = note: clamp will panic if max < min error: clamp-like pattern without using clamp function - --> tests/ui/manual_clamp.rs:191:19 + --> tests/ui/manual_clamp.rs:314:19 | LL | let x24 = f64::max(f64::min(input, CONST_F64_MAX), CONST_F64_MIN); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace with clamp: `input.clamp(CONST_F64_MIN, CONST_F64_MAX)` @@ -311,7 +311,7 @@ LL | let x24 = f64::max(f64::min(input, CONST_F64_MAX), CONST_F64_MIN); = note: clamp returns NaN if the input is NaN error: clamp-like pattern without using clamp function - --> tests/ui/manual_clamp.rs:194:19 + --> tests/ui/manual_clamp.rs:317:19 | LL | let x25 = f64::min(f64::max(input, CONST_F64_MIN), CONST_F64_MAX); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace with clamp: `input.clamp(CONST_F64_MIN, CONST_F64_MAX)` @@ -320,7 +320,7 @@ LL | let x25 = f64::min(f64::max(input, CONST_F64_MIN), CONST_F64_MAX); = note: clamp returns NaN if the input is NaN error: clamp-like pattern without using clamp function - --> tests/ui/manual_clamp.rs:197:19 + --> tests/ui/manual_clamp.rs:320:19 | LL | let x26 = f64::max(CONST_F64_MIN, f64::min(input, CONST_F64_MAX)); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace with clamp: `input.clamp(CONST_F64_MIN, CONST_F64_MAX)` @@ -329,7 +329,7 @@ LL | let x26 = f64::max(CONST_F64_MIN, f64::min(input, CONST_F64_MAX)); = note: clamp returns NaN if the input is NaN error: clamp-like pattern without using clamp function - --> tests/ui/manual_clamp.rs:200:19 + --> tests/ui/manual_clamp.rs:323:19 | LL | let x27 = f64::min(CONST_F64_MAX, f64::max(input, CONST_F64_MIN)); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace with clamp: `input.clamp(CONST_F64_MIN, CONST_F64_MAX)` @@ -338,7 +338,7 @@ LL | let x27 = f64::min(CONST_F64_MAX, f64::max(input, CONST_F64_MIN)); = note: clamp returns NaN if the input is NaN error: clamp-like pattern without using clamp function - --> tests/ui/manual_clamp.rs:203:19 + --> tests/ui/manual_clamp.rs:326:19 | LL | let x28 = f64::max(f64::min(CONST_F64_MAX, input), CONST_F64_MIN); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace with clamp: `input.clamp(CONST_F64_MIN, CONST_F64_MAX)` @@ -347,7 +347,7 @@ LL | let x28 = f64::max(f64::min(CONST_F64_MAX, input), CONST_F64_MIN); = note: clamp returns NaN if the input is NaN error: clamp-like pattern without using clamp function - --> tests/ui/manual_clamp.rs:206:19 + --> tests/ui/manual_clamp.rs:329:19 | LL | let x29 = f64::min(f64::max(CONST_F64_MIN, input), CONST_F64_MAX); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace with clamp: `input.clamp(CONST_F64_MIN, CONST_F64_MAX)` @@ -356,7 +356,7 @@ LL | let x29 = f64::min(f64::max(CONST_F64_MIN, input), CONST_F64_MAX); = note: clamp returns NaN if the input is NaN error: clamp-like pattern without using clamp function - --> tests/ui/manual_clamp.rs:209:19 + --> tests/ui/manual_clamp.rs:332:19 | LL | let x30 = f64::max(CONST_F64_MIN, f64::min(CONST_F64_MAX, input)); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace with clamp: `input.clamp(CONST_F64_MIN, CONST_F64_MAX)` @@ -365,7 +365,7 @@ LL | let x30 = f64::max(CONST_F64_MIN, f64::min(CONST_F64_MAX, input)); = note: clamp returns NaN if the input is NaN error: clamp-like pattern without using clamp function - --> tests/ui/manual_clamp.rs:212:19 + --> tests/ui/manual_clamp.rs:335:19 | LL | let x31 = f64::min(CONST_F64_MAX, f64::max(CONST_F64_MIN, input)); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace with clamp: `input.clamp(CONST_F64_MIN, CONST_F64_MAX)` @@ -374,31 +374,31 @@ LL | let x31 = f64::min(CONST_F64_MAX, f64::max(CONST_F64_MIN, input)); = note: clamp returns NaN if the input is NaN error: clamp-like pattern without using clamp function - --> tests/ui/manual_clamp.rs:217:5 + --> tests/ui/manual_clamp.rs:340:5 | -LL | / if x32 < min { +LL | / if x32 < CONST_MIN { LL | | LL | | -LL | | x32 = min; -LL | | } else if x32 > max { -LL | | x32 = max; +LL | | x32 = CONST_MIN; +LL | | } else if x32 > CONST_MAX { +LL | | x32 = CONST_MAX; LL | | } - | |_____^ help: replace with clamp: `x32 = x32.clamp(min, max);` + | |_____^ help: replace with clamp: `x32 = x32.clamp(CONST_MIN, CONST_MAX);` | = note: clamp will panic if max < min error: clamp-like pattern without using clamp function - --> tests/ui/manual_clamp.rs:389:13 + --> tests/ui/manual_clamp.rs:532:13 | -LL | let _ = if input < min { +LL | let _ = if input > CONST_MAX { | _____________^ LL | | LL | | -LL | | min +LL | | CONST_MAX ... | LL | | input LL | | }; - | |_____^ help: replace with clamp: `input.clamp(min, max)` + | |_____^ help: replace with clamp: `input.clamp(CONST_MIN, CONST_MAX)` | = note: clamp will panic if max < min From 3912f35a0c17ed6c58b7b6609060f7ada786302c Mon Sep 17 00:00:00 2001 From: Jacob Kiesel Date: Tue, 26 Mar 2024 10:45:27 -0600 Subject: [PATCH 10/22] short circuit logic better --- clippy_lints/src/manual_clamp.rs | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/clippy_lints/src/manual_clamp.rs b/clippy_lints/src/manual_clamp.rs index 0148e6deb56c..f484672bc3aa 100644 --- a/clippy_lints/src/manual_clamp.rs +++ b/clippy_lints/src/manual_clamp.rs @@ -114,12 +114,13 @@ impl<'tcx> ClampSuggestion<'tcx> { if max_type != min_type { return false; } - let max = constant(cx, cx.typeck_results(), self.params.max); - let min = constant(cx, cx.typeck_results(), self.params.min); - let cmp = max - .zip(min) - .and_then(|(max, min)| Constant::partial_cmp(cx.tcx, max_type, &min, &max)); - cmp.is_some_and(|cmp| cmp != Ordering::Greater) + if let Some(max) = constant(cx, cx.typeck_results(), self.params.max) + && let Some(min) = constant(cx, cx.typeck_results(), self.params.min) + { + Constant::partial_cmp(cx.tcx, max_type, &min, &max).is_some_and(|o| o != Ordering::Greater) + } else { + false + } } } From 5aa419467658e80a42e56fe452f88e269da63f16 Mon Sep 17 00:00:00 2001 From: Jacob Kiesel Date: Tue, 26 Mar 2024 10:53:41 -0600 Subject: [PATCH 11/22] the power of if let chain compels you --- clippy_lints/src/manual_clamp.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/clippy_lints/src/manual_clamp.rs b/clippy_lints/src/manual_clamp.rs index f484672bc3aa..59fc8948c2f0 100644 --- a/clippy_lints/src/manual_clamp.rs +++ b/clippy_lints/src/manual_clamp.rs @@ -116,8 +116,9 @@ impl<'tcx> ClampSuggestion<'tcx> { } if let Some(max) = constant(cx, cx.typeck_results(), self.params.max) && let Some(min) = constant(cx, cx.typeck_results(), self.params.min) + && let Some(ord) = Constant::partial_cmp(cx.tcx, max_type, &min, &max) { - Constant::partial_cmp(cx.tcx, max_type, &min, &max).is_some_and(|o| o != Ordering::Greater) + ord != Ordering::Greater } else { false } From dae986810939ce1dd48dbba60c26c75783c10dad Mon Sep 17 00:00:00 2001 From: Jacob Kiesel Date: Fri, 29 Mar 2024 09:38:59 -0600 Subject: [PATCH 12/22] Add limitations section, move check --- clippy_lints/src/manual_clamp.rs | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/clippy_lints/src/manual_clamp.rs b/clippy_lints/src/manual_clamp.rs index 59fc8948c2f0..1eadc200bedc 100644 --- a/clippy_lints/src/manual_clamp.rs +++ b/clippy_lints/src/manual_clamp.rs @@ -28,6 +28,11 @@ declare_clippy_lint! { /// ### Why is this bad? /// clamp is much shorter, easier to read, and doesn't use any control flow. /// + /// ### Limitations + /// + /// This lint will only trigger if max and min are known at compile time, and max is + /// greater than min. + /// /// ### Known issue(s) /// If the clamped variable is NaN this suggestion will cause the code to propagate NaN /// rather than returning either `max` or `min`. @@ -145,9 +150,7 @@ impl<'tcx> LateLintPass<'tcx> for ManualClamp { .or_else(|| is_match_pattern(cx, expr)) .or_else(|| is_if_elseif_pattern(cx, expr)); if let Some(suggestion) = suggestion { - if suggestion.min_less_than_max(cx) { - emit_suggestion(cx, &suggestion); - } + maybe_emit_suggestion(cx, &suggestion); } } } @@ -157,15 +160,16 @@ impl<'tcx> LateLintPass<'tcx> for ManualClamp { return; } for suggestion in is_two_if_pattern(cx, block) { - if suggestion.min_less_than_max(cx) { - emit_suggestion(cx, &suggestion); - } + maybe_emit_suggestion(cx, &suggestion); } } extract_msrv_attr!(LateContext); } -fn emit_suggestion<'tcx>(cx: &LateContext<'tcx>, suggestion: &ClampSuggestion<'tcx>) { +fn maybe_emit_suggestion<'tcx>(cx: &LateContext<'tcx>, suggestion: &ClampSuggestion<'tcx>) { + if !suggestion.min_less_than_max(cx) { + return; + } let ClampSuggestion { params: InputMinMax { input, From fe61e91c1a6c4bf75b9026366b12550daffad1a5 Mon Sep 17 00:00:00 2001 From: J-ZhengLi Date: Sat, 30 Mar 2024 09:18:53 +0800 Subject: [PATCH 13/22] fix [`manual_unwrap_or_default`] suggestion ignoring side-effects --- clippy_lints/src/manual_unwrap_or_default.rs | 25 +++++++++++------ tests/ui/manual_unwrap_or_default.fixed | 29 ++++++++++++++++++++ tests/ui/manual_unwrap_or_default.rs | 29 ++++++++++++++++++++ 3 files changed, 75 insertions(+), 8 deletions(-) diff --git a/clippy_lints/src/manual_unwrap_or_default.rs b/clippy_lints/src/manual_unwrap_or_default.rs index e292639a6c69..ce7d2d2c9323 100644 --- a/clippy_lints/src/manual_unwrap_or_default.rs +++ b/clippy_lints/src/manual_unwrap_or_default.rs @@ -3,14 +3,13 @@ use rustc_errors::Applicability; use rustc_hir::def::Res; use rustc_hir::{Arm, Expr, ExprKind, HirId, LangItem, MatchSource, Pat, PatKind, QPath}; use rustc_lint::{LateContext, LateLintPass}; -use rustc_middle::ty::GenericArgKind; use rustc_session::declare_lint_pass; use rustc_span::sym; use clippy_utils::diagnostics::span_lint_and_sugg; use clippy_utils::source::snippet_opt; use clippy_utils::ty::implements_trait; -use clippy_utils::{in_constant, is_default_equivalent}; +use clippy_utils::{in_constant, is_default_equivalent, peel_blocks, span_contains_comment}; declare_clippy_lint! { /// ### What it does @@ -124,14 +123,19 @@ fn handle_match<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) -> bool { // We now get the bodies for both the `Some` and `None` arms. && let Some(((body_some, binding_id), body_none)) = get_some_and_none_bodies(cx, arm1, arm2) // We check that the `Some(x) => x` doesn't do anything apart "returning" the value in `Some`. - && let ExprKind::Path(QPath::Resolved(_, path)) = body_some.peel_blocks().kind + && let ExprKind::Path(QPath::Resolved(_, path)) = peel_blocks(body_some).kind && let Res::Local(local_id) = path.res && local_id == binding_id // We now check the `None` arm is calling a method equivalent to `Default::default`. - && let body_none = body_none.peel_blocks() + && let body_none = peel_blocks(body_none) && is_default_equivalent(cx, body_none) && let Some(receiver) = Sugg::hir_opt(cx, match_expr).map(Sugg::maybe_par) { + let applicability = if span_contains_comment(cx.sess().source_map(), expr.span) { + Applicability::MaybeIncorrect + } else { + Applicability::MachineApplicable + }; span_lint_and_sugg( cx, MANUAL_UNWRAP_OR_DEFAULT, @@ -139,7 +143,7 @@ fn handle_match<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) -> bool { "match can be simplified with `.unwrap_or_default()`", "replace it with", format!("{receiver}.unwrap_or_default()"), - Applicability::MachineApplicable, + applicability, ); } true @@ -159,14 +163,19 @@ fn handle_if_let<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) { && let GenericArgKind::Type(let_ty) = let_ty.unpack() && implements_trait(cx, let_ty, default_trait_id, &[]) // We check that the `Some(x) => x` doesn't do anything apart "returning" the value in `Some`. - && let ExprKind::Path(QPath::Resolved(_, path)) = if_block.peel_blocks().kind + && let ExprKind::Path(QPath::Resolved(_, path)) = peel_blocks(if_block).kind && let Res::Local(local_id) = path.res && local_id == binding_id // We now check the `None` arm is calling a method equivalent to `Default::default`. - && let body_else = else_expr.peel_blocks() + && let body_else = peel_blocks(else_expr) && is_default_equivalent(cx, body_else) && let Some(if_let_expr_snippet) = snippet_opt(cx, let_.init.span) { + let applicability = if span_contains_comment(cx.sess().source_map(), expr.span) { + Applicability::MaybeIncorrect + } else { + Applicability::MachineApplicable + }; span_lint_and_sugg( cx, MANUAL_UNWRAP_OR_DEFAULT, @@ -174,7 +183,7 @@ fn handle_if_let<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) { "if let can be simplified with `.unwrap_or_default()`", "replace it with", format!("{if_let_expr_snippet}.unwrap_or_default()"), - Applicability::MachineApplicable, + applicability, ); } } diff --git a/tests/ui/manual_unwrap_or_default.fixed b/tests/ui/manual_unwrap_or_default.fixed index 5a9d2d5ab5d7..d6e736ba9cc2 100644 --- a/tests/ui/manual_unwrap_or_default.fixed +++ b/tests/ui/manual_unwrap_or_default.fixed @@ -43,3 +43,32 @@ const fn issue_12568(opt: Option) -> bool { None => false, } } + +fn issue_12569() { + let match_none_se = match 1u32.checked_div(0) { + Some(v) => v, + None => { + println!("important"); + 0 + }, + }; + let match_some_se = match 1u32.checked_div(0) { + Some(v) => { + println!("important"); + v + }, + None => 0, + }; + let iflet_else_se = if let Some(v) = 1u32.checked_div(0) { + v + } else { + println!("important"); + 0 + }; + let iflet_then_se = if let Some(v) = 1u32.checked_div(0) { + println!("important"); + v + } else { + 0 + }; +} diff --git a/tests/ui/manual_unwrap_or_default.rs b/tests/ui/manual_unwrap_or_default.rs index 8d3d7422921d..462d5d90ee77 100644 --- a/tests/ui/manual_unwrap_or_default.rs +++ b/tests/ui/manual_unwrap_or_default.rs @@ -67,3 +67,32 @@ const fn issue_12568(opt: Option) -> bool { None => false, } } + +fn issue_12569() { + let match_none_se = match 1u32.checked_div(0) { + Some(v) => v, + None => { + println!("important"); + 0 + }, + }; + let match_some_se = match 1u32.checked_div(0) { + Some(v) => { + println!("important"); + v + }, + None => 0, + }; + let iflet_else_se = if let Some(v) = 1u32.checked_div(0) { + v + } else { + println!("important"); + 0 + }; + let iflet_then_se = if let Some(v) = 1u32.checked_div(0) { + println!("important"); + v + } else { + 0 + }; +} From 5c73fb77a785b71eb4c1a2ea62e93ad5ed243348 Mon Sep 17 00:00:00 2001 From: J-ZhengLi Date: Wed, 27 Mar 2024 08:36:08 +0800 Subject: [PATCH 14/22] check for init expr when linting [`question_mark`] --- clippy_lints/src/question_mark.rs | 21 +++++++++++++++++- tests/ui/question_mark.fixed | 34 +++++++++++++++++++++++++++++ tests/ui/question_mark.rs | 36 +++++++++++++++++++++++++++++++ tests/ui/question_mark.stderr | 10 ++++++++- 4 files changed, 99 insertions(+), 2 deletions(-) diff --git a/clippy_lints/src/question_mark.rs b/clippy_lints/src/question_mark.rs index 09bbcdb9c792..771ea33d1bf9 100644 --- a/clippy_lints/src/question_mark.rs +++ b/clippy_lints/src/question_mark.rs @@ -4,7 +4,7 @@ use clippy_config::msrvs::Msrv; use clippy_config::types::MatchLintBehaviour; use clippy_utils::diagnostics::span_lint_and_sugg; use clippy_utils::source::snippet_with_applicability; -use clippy_utils::ty::is_type_diagnostic_item; +use clippy_utils::ty::{implements_trait, is_type_diagnostic_item}; use clippy_utils::{ eq_expr_value, higher, in_constant, is_else_clause, is_lint_allowed, is_path_lang_item, is_res_lang_ctor, pat_and_expr_can_be_question_mark, path_to_local, path_to_local_id, peel_blocks, peel_blocks_with_stmt, @@ -109,12 +109,31 @@ fn find_let_else_ret_expression<'hir>(block: &'hir Block<'hir>) -> Option<&'hir } fn check_let_some_else_return_none(cx: &LateContext<'_>, stmt: &Stmt<'_>) { + /// Make sure the init expr implements try trait so a valid suggestion could be given. + /// + /// Because the init expr could have the type of `&Option` which does not implements `Try`. + /// + /// NB: This conveniently prevents the cause of + /// issue [#12412](https://github.com/rust-lang/rust-clippy/issues/12412), + /// since accessing an `Option` field from a borrowed struct requires borrow, such as + /// `&some_struct.opt`, which is type of `&Option`. And we can't suggest `&some_struct.opt?` + /// or `(&some_struct.opt)?` since the first one has different semantics and the later does + /// not implements `Try`. + fn init_expr_can_use_question_mark(cx: &LateContext<'_>, init_expr: &Expr<'_>) -> bool { + let init_ty = cx.typeck_results().expr_ty_adjusted(init_expr); + cx.tcx + .lang_items() + .try_trait() + .map_or(false, |did| implements_trait(cx, init_ty, did, &[])) + } + if let StmtKind::Let(Local { pat, init: Some(init_expr), els: Some(els), .. }) = stmt.kind + && init_expr_can_use_question_mark(cx, init_expr) && let Some(ret) = find_let_else_ret_expression(els) && let Some(inner_pat) = pat_and_expr_can_be_question_mark(cx, pat, ret) && !span_contains_comment(cx.tcx.sess.source_map(), els.span) diff --git a/tests/ui/question_mark.fixed b/tests/ui/question_mark.fixed index 567472a8af22..679388372e61 100644 --- a/tests/ui/question_mark.fixed +++ b/tests/ui/question_mark.fixed @@ -283,3 +283,37 @@ fn issue12337() -> Option { }; Some(42) } + +fn issue11983(option: &Option) -> Option<()> { + // Don't lint, `&Option` dose not impl `Try`. + let Some(v) = option else { return None }; + + let opt = Some(String::new()); + // Don't lint, `branch` method in `Try` takes ownership of `opt`, + // and `(&opt)?` also doesn't work since it's `&Option`. + let Some(v) = &opt else { return None }; + let mov = opt; + + Some(()) +} + +struct Foo { + owned: Option, +} +struct Bar { + foo: Foo, +} +#[allow(clippy::disallowed_names)] +fn issue12412(foo: &Foo, bar: &Bar) -> Option<()> { + // Don't lint, `owned` is behind a shared reference. + let Some(v) = &foo.owned else { + return None; + }; + // Don't lint, `owned` is behind a shared reference. + let Some(v) = &bar.foo.owned else { + return None; + }; + // lint + let v = bar.foo.owned.clone()?; + Some(()) +} diff --git a/tests/ui/question_mark.rs b/tests/ui/question_mark.rs index abf8c270de83..601ab78bf5aa 100644 --- a/tests/ui/question_mark.rs +++ b/tests/ui/question_mark.rs @@ -323,3 +323,39 @@ fn issue12337() -> Option { }; Some(42) } + +fn issue11983(option: &Option) -> Option<()> { + // Don't lint, `&Option` dose not impl `Try`. + let Some(v) = option else { return None }; + + let opt = Some(String::new()); + // Don't lint, `branch` method in `Try` takes ownership of `opt`, + // and `(&opt)?` also doesn't work since it's `&Option`. + let Some(v) = &opt else { return None }; + let mov = opt; + + Some(()) +} + +struct Foo { + owned: Option, +} +struct Bar { + foo: Foo, +} +#[allow(clippy::disallowed_names)] +fn issue12412(foo: &Foo, bar: &Bar) -> Option<()> { + // Don't lint, `owned` is behind a shared reference. + let Some(v) = &foo.owned else { + return None; + }; + // Don't lint, `owned` is behind a shared reference. + let Some(v) = &bar.foo.owned else { + return None; + }; + // lint + let Some(v) = bar.foo.owned.clone() else { + return None; + }; + Some(()) +} diff --git a/tests/ui/question_mark.stderr b/tests/ui/question_mark.stderr index 4fcccdf5512f..5f26a7ea2c3e 100644 --- a/tests/ui/question_mark.stderr +++ b/tests/ui/question_mark.stderr @@ -141,5 +141,13 @@ LL | | // https://github.com/rust-lang/rust-clippy/pull/11001#is LL | | } | |_____________^ help: replace it with: `a?;` -error: aborting due to 16 previous errors +error: this `let...else` may be rewritten with the `?` operator + --> tests/ui/question_mark.rs:357:5 + | +LL | / let Some(v) = bar.foo.owned.clone() else { +LL | | return None; +LL | | }; + | |______^ help: replace it with: `let v = bar.foo.owned.clone()?;` + +error: aborting due to 17 previous errors From 4abb1021870d58804f5b07e4048843cdfb3f9ec6 Mon Sep 17 00:00:00 2001 From: Catherine <114838443+Centri3@users.noreply.github.com> Date: Tue, 20 Jun 2023 16:34:24 -0500 Subject: [PATCH 15/22] new lint `legacy_numeric_constants` --- CHANGELOG.md | 1 + book/src/lint_configuration.md | 1 + clippy_config/src/conf.rs | 2 +- clippy_config/src/msrvs.rs | 2 +- .../src/casts/cast_possible_truncation.rs | 4 +- clippy_lints/src/declared_lints.rs | 1 + clippy_lints/src/legacy_numeric_constants.rs | 290 ++++++++++++++++++ clippy_lints/src/lib.rs | 2 + clippy_utils/src/check_proc_macro.rs | 12 +- .../ui-toml/absolute_paths/absolute_paths.rs | 2 +- tests/ui/checked_conversions.fixed | 1 + tests/ui/checked_conversions.rs | 1 + tests/ui/checked_conversions.stderr | 34 +- tests/ui/legacy_numeric_constants.fixed | 118 +++++++ tests/ui/legacy_numeric_constants.rs | 118 +++++++ tests/ui/legacy_numeric_constants.stderr | 184 +++++++++++ .../ui/legacy_numeric_constants_unfixable.rs | 79 +++++ .../legacy_numeric_constants_unfixable.stderr | 83 +++++ tests/ui/manual_saturating_arithmetic.fixed | 2 +- tests/ui/manual_saturating_arithmetic.rs | 2 +- tests/ui/suspicious_arithmetic_impl.rs | 1 + tests/ui/suspicious_arithmetic_impl.stderr | 18 +- 22 files changed, 921 insertions(+), 37 deletions(-) create mode 100644 clippy_lints/src/legacy_numeric_constants.rs create mode 100644 tests/ui/legacy_numeric_constants.fixed create mode 100644 tests/ui/legacy_numeric_constants.rs create mode 100644 tests/ui/legacy_numeric_constants.stderr create mode 100644 tests/ui/legacy_numeric_constants_unfixable.rs create mode 100644 tests/ui/legacy_numeric_constants_unfixable.stderr diff --git a/CHANGELOG.md b/CHANGELOG.md index 87bffe7f74d6..f7e7ed86eed8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5379,6 +5379,7 @@ Released 2018-09-13 [`large_stack_arrays`]: https://rust-lang.github.io/rust-clippy/master/index.html#large_stack_arrays [`large_stack_frames`]: https://rust-lang.github.io/rust-clippy/master/index.html#large_stack_frames [`large_types_passed_by_value`]: https://rust-lang.github.io/rust-clippy/master/index.html#large_types_passed_by_value +[`legacy_numeric_constants`]: https://rust-lang.github.io/rust-clippy/master/index.html#legacy_numeric_constants [`len_without_is_empty`]: https://rust-lang.github.io/rust-clippy/master/index.html#len_without_is_empty [`len_zero`]: https://rust-lang.github.io/rust-clippy/master/index.html#len_zero [`let_and_return`]: https://rust-lang.github.io/rust-clippy/master/index.html#let_and_return diff --git a/book/src/lint_configuration.md b/book/src/lint_configuration.md index a92348997464..4a2727c5197f 100644 --- a/book/src/lint_configuration.md +++ b/book/src/lint_configuration.md @@ -616,6 +616,7 @@ The minimum rust version that the project supports. Defaults to the `rust-versio * [`if_then_some_else_none`](https://rust-lang.github.io/rust-clippy/master/index.html#if_then_some_else_none) * [`index_refutable_slice`](https://rust-lang.github.io/rust-clippy/master/index.html#index_refutable_slice) * [`iter_kv_map`](https://rust-lang.github.io/rust-clippy/master/index.html#iter_kv_map) +* [`legacy_numeric_constants`](https://rust-lang.github.io/rust-clippy/master/index.html#legacy_numeric_constants) * [`manual_bits`](https://rust-lang.github.io/rust-clippy/master/index.html#manual_bits) * [`manual_c_str_literals`](https://rust-lang.github.io/rust-clippy/master/index.html#manual_c_str_literals) * [`manual_clamp`](https://rust-lang.github.io/rust-clippy/master/index.html#manual_clamp) diff --git a/clippy_config/src/conf.rs b/clippy_config/src/conf.rs index 3218fe7f4562..3b5de2c16ffa 100644 --- a/clippy_config/src/conf.rs +++ b/clippy_config/src/conf.rs @@ -262,7 +262,7 @@ define_Conf! { /// /// Suppress lints whenever the suggested change would cause breakage for other crates. (avoid_breaking_exported_api: bool = true), - /// Lint: MANUAL_SPLIT_ONCE, MANUAL_STR_REPEAT, CLONED_INSTEAD_OF_COPIED, REDUNDANT_FIELD_NAMES, OPTION_MAP_UNWRAP_OR, REDUNDANT_STATIC_LIFETIMES, FILTER_MAP_NEXT, CHECKED_CONVERSIONS, MANUAL_RANGE_CONTAINS, USE_SELF, MEM_REPLACE_WITH_DEFAULT, MANUAL_NON_EXHAUSTIVE, OPTION_AS_REF_DEREF, MAP_UNWRAP_OR, MATCH_LIKE_MATCHES_MACRO, MANUAL_STRIP, MISSING_CONST_FOR_FN, UNNESTED_OR_PATTERNS, FROM_OVER_INTO, PTR_AS_PTR, IF_THEN_SOME_ELSE_NONE, APPROX_CONSTANT, DEPRECATED_CFG_ATTR, INDEX_REFUTABLE_SLICE, MAP_CLONE, BORROW_AS_PTR, MANUAL_BITS, ERR_EXPECT, CAST_ABS_TO_UNSIGNED, UNINLINED_FORMAT_ARGS, MANUAL_CLAMP, MANUAL_LET_ELSE, UNCHECKED_DURATION_SUBTRACTION, COLLAPSIBLE_STR_REPLACE, SEEK_FROM_CURRENT, SEEK_REWIND, UNNECESSARY_LAZY_EVALUATIONS, TRANSMUTE_PTR_TO_REF, ALMOST_COMPLETE_RANGE, NEEDLESS_BORROW, DERIVABLE_IMPLS, MANUAL_IS_ASCII_CHECK, MANUAL_REM_EUCLID, MANUAL_RETAIN, TYPE_REPETITION_IN_BOUNDS, TUPLE_ARRAY_CONVERSIONS, MANUAL_TRY_FOLD, MANUAL_HASH_ONE, ITER_KV_MAP, MANUAL_C_STR_LITERALS, ASSIGNING_CLONES. + /// Lint: MANUAL_SPLIT_ONCE, MANUAL_STR_REPEAT, CLONED_INSTEAD_OF_COPIED, REDUNDANT_FIELD_NAMES, OPTION_MAP_UNWRAP_OR, REDUNDANT_STATIC_LIFETIMES, FILTER_MAP_NEXT, CHECKED_CONVERSIONS, MANUAL_RANGE_CONTAINS, USE_SELF, MEM_REPLACE_WITH_DEFAULT, MANUAL_NON_EXHAUSTIVE, OPTION_AS_REF_DEREF, MAP_UNWRAP_OR, MATCH_LIKE_MATCHES_MACRO, MANUAL_STRIP, MISSING_CONST_FOR_FN, UNNESTED_OR_PATTERNS, FROM_OVER_INTO, PTR_AS_PTR, IF_THEN_SOME_ELSE_NONE, APPROX_CONSTANT, DEPRECATED_CFG_ATTR, INDEX_REFUTABLE_SLICE, MAP_CLONE, BORROW_AS_PTR, MANUAL_BITS, ERR_EXPECT, CAST_ABS_TO_UNSIGNED, UNINLINED_FORMAT_ARGS, MANUAL_CLAMP, MANUAL_LET_ELSE, UNCHECKED_DURATION_SUBTRACTION, COLLAPSIBLE_STR_REPLACE, SEEK_FROM_CURRENT, SEEK_REWIND, UNNECESSARY_LAZY_EVALUATIONS, TRANSMUTE_PTR_TO_REF, ALMOST_COMPLETE_RANGE, NEEDLESS_BORROW, DERIVABLE_IMPLS, MANUAL_IS_ASCII_CHECK, MANUAL_REM_EUCLID, MANUAL_RETAIN, TYPE_REPETITION_IN_BOUNDS, TUPLE_ARRAY_CONVERSIONS, MANUAL_TRY_FOLD, MANUAL_HASH_ONE, ITER_KV_MAP, MANUAL_C_STR_LITERALS, ASSIGNING_CLONES, LEGACY_NUMERIC_CONSTANTS. /// /// The minimum rust version that the project supports. Defaults to the `rust-version` field in `Cargo.toml` #[default_text = ""] diff --git a/clippy_config/src/msrvs.rs b/clippy_config/src/msrvs.rs index bf4da5f14fe0..32107ded305e 100644 --- a/clippy_config/src/msrvs.rs +++ b/clippy_config/src/msrvs.rs @@ -36,7 +36,7 @@ msrv_aliases! { 1,47,0 { TAU, IS_ASCII_DIGIT_CONST, ARRAY_IMPL_ANY_LEN } 1,46,0 { CONST_IF_MATCH } 1,45,0 { STR_STRIP_PREFIX } - 1,43,0 { LOG2_10, LOG10_2 } + 1,43,0 { LOG2_10, LOG10_2, NUMERIC_ASSOCIATED_CONSTANTS } 1,42,0 { MATCHES_MACRO, SLICE_PATTERNS, PTR_SLICE_RAW_PARTS } 1,41,0 { RE_REBALANCING_COHERENCE, RESULT_MAP_OR_ELSE } 1,40,0 { MEM_TAKE, NON_EXHAUSTIVE, OPTION_AS_DEREF } diff --git a/clippy_lints/src/casts/cast_possible_truncation.rs b/clippy_lints/src/casts/cast_possible_truncation.rs index 2c0a3d482960..32e45bd0a795 100644 --- a/clippy_lints/src/casts/cast_possible_truncation.rs +++ b/clippy_lints/src/casts/cast_possible_truncation.rs @@ -41,7 +41,7 @@ fn apply_reductions(cx: &LateContext<'_>, nbits: u64, expr: &Expr<'_>, signed: b }) }, BinOpKind::Rem | BinOpKind::BitAnd => get_constant_bits(cx, right) - .unwrap_or(u64::max_value()) + .unwrap_or(u64::MAX) .min(apply_reductions(cx, nbits, left, signed)), BinOpKind::Shr => apply_reductions(cx, nbits, left, signed) .saturating_sub(constant_int(cx, right).map_or(0, |s| u64::try_from(s).unwrap_or_default())), @@ -56,7 +56,7 @@ fn apply_reductions(cx: &LateContext<'_>, nbits: u64, expr: &Expr<'_>, signed: b } else { None }; - apply_reductions(cx, nbits, left, signed).min(max_bits.unwrap_or(u64::max_value())) + apply_reductions(cx, nbits, left, signed).min(max_bits.unwrap_or(u64::MAX)) }, ExprKind::MethodCall(method, _, [lo, hi], _) => { if method.ident.as_str() == "clamp" { diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index eba048ad90be..5ff7d8e51343 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -254,6 +254,7 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[ crate::large_include_file::LARGE_INCLUDE_FILE_INFO, crate::large_stack_arrays::LARGE_STACK_ARRAYS_INFO, crate::large_stack_frames::LARGE_STACK_FRAMES_INFO, + crate::legacy_numeric_constants::LEGACY_NUMERIC_CONSTANTS_INFO, crate::len_zero::COMPARISON_TO_EMPTY_INFO, crate::len_zero::LEN_WITHOUT_IS_EMPTY_INFO, crate::len_zero::LEN_ZERO_INFO, diff --git a/clippy_lints/src/legacy_numeric_constants.rs b/clippy_lints/src/legacy_numeric_constants.rs new file mode 100644 index 000000000000..a325b4d54937 --- /dev/null +++ b/clippy_lints/src/legacy_numeric_constants.rs @@ -0,0 +1,290 @@ +use clippy_config::msrvs::{Msrv, NUMERIC_ASSOCIATED_CONSTANTS}; +use clippy_utils::diagnostics::{span_lint_and_then, span_lint_hir_and_then}; +use clippy_utils::{get_parent_expr, is_from_proc_macro}; +use hir::def_id::DefId; +use rustc_errors::{Applicability, SuggestionStyle}; +use rustc_hir as hir; +use rustc_hir::{ExprKind, Item, ItemKind, QPath, UseKind}; +use rustc_lint::{LateContext, LateLintPass, LintContext}; +use rustc_middle::lint::in_external_macro; +use rustc_session::impl_lint_pass; +use rustc_span::symbol::kw; +use rustc_span::{sym, Symbol}; + +declare_clippy_lint! { + /// ### What it does + /// Checks for usage of `::max_value()`, `std::::MAX`, + /// `std::::EPSILON`, etc. + /// + /// ### Why is this bad? + /// All of these have been superceded by the associated constants on their respective types, + /// such as `i128::MAX`. These legacy items may be deprecated in a future version of rust. + /// + /// ### Example + /// ```rust + /// let eps = std::f32::EPSILON; + /// ``` + /// Use instead: + /// ```rust + /// let eps = f32::EPSILON; + /// ``` + #[clippy::version = "1.72.0"] + pub LEGACY_NUMERIC_CONSTANTS, + style, + "checks for usage of legacy std numeric constants and methods" +} +pub struct LegacyNumericConstants { + msrv: Msrv, +} + +impl LegacyNumericConstants { + #[must_use] + pub fn new(msrv: Msrv) -> Self { + Self { msrv } + } +} + +impl_lint_pass!(LegacyNumericConstants => [LEGACY_NUMERIC_CONSTANTS]); + +impl<'tcx> LateLintPass<'tcx> for LegacyNumericConstants { + fn check_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx Item<'tcx>) { + let Self { msrv } = self; + + if !msrv.meets(NUMERIC_ASSOCIATED_CONSTANTS) || in_external_macro(cx.sess(), item.span) { + return; + } + + // Integer modules are "TBD" deprecated, and the contents are too, + // so lint on the `use` statement directly. + if let ItemKind::Use(path, kind @ (UseKind::Single | UseKind::Glob)) = item.kind + && let Some(def_id) = path.res[0].opt_def_id() + { + let module = if is_integer_module(cx, def_id) { + true + } else if is_numeric_const(cx, def_id) { + false + } else { + return; + }; + + span_lint_and_then( + cx, + LEGACY_NUMERIC_CONSTANTS, + path.span, + if module { + "importing legacy numeric constants" + } else { + "importing a legacy numeric constant" + }, + |diag| { + if item.ident.name == kw::Underscore { + diag.help("remove this import"); + return; + } + + let def_path = cx.get_def_path(def_id); + + if module && let [.., module_name] = &*def_path { + if kind == UseKind::Glob { + diag.help(format!("remove this import and use associated constants `{module_name}::` from the primitive type instead")); + } else { + diag.help("remove this import").note(format!( + "then `{module_name}::` will resolve to the respective associated constant" + )); + } + } else if let [.., module_name, name] = &*def_path { + diag.help( + format!("remove this import and use the associated constant `{module_name}::{name}` from the primitive type instead") + ); + } + }, + ); + } + } + + fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx rustc_hir::Expr<'tcx>) { + let Self { msrv } = self; + + if !msrv.meets(NUMERIC_ASSOCIATED_CONSTANTS) || in_external_macro(cx.sess(), expr.span) { + return; + } + let ExprKind::Path(qpath) = expr.kind else { + return; + }; + + // `std::::` check + let (span, sugg, msg) = if let QPath::Resolved(None, path) = qpath + && let Some(def_id) = path.res.opt_def_id() + && is_numeric_const(cx, def_id) + && let def_path = cx.get_def_path(def_id) + && let [.., mod_name, name] = &*def_path + // Skip linting if this usage looks identical to the associated constant, + // since this would only require removing a `use` import (which is already linted). + && !is_numeric_const_path_canonical(path, [*mod_name, *name]) + { + ( + expr.span, + format!("{mod_name}::{name}"), + "usage of a legacy numeric constant", + ) + // `::xxx_value` check + } else if let QPath::TypeRelative(_, last_segment) = qpath + && let Some(def_id) = cx.qpath_res(&qpath, expr.hir_id).opt_def_id() + && is_integer_method(cx, def_id) + && let Some(par_expr) = get_parent_expr(cx, expr) + && let ExprKind::Call(_, _) = par_expr.kind + { + let name = last_segment.ident.name.as_str(); + + ( + last_segment.ident.span.with_hi(par_expr.span.hi()), + name[..=2].to_ascii_uppercase(), + "usage of a legacy numeric method", + ) + } else { + return; + }; + + if is_from_proc_macro(cx, expr) { + return; + } + + span_lint_hir_and_then(cx, LEGACY_NUMERIC_CONSTANTS, expr.hir_id, span, msg, |diag| { + diag.span_suggestion_with_style( + span, + "use the associated constant instead", + sugg, + Applicability::MaybeIncorrect, + SuggestionStyle::ShowAlways, + ); + }); + } + + extract_msrv_attr!(LateContext); +} + +fn is_integer_module(cx: &LateContext<'_>, did: DefId) -> bool { + [ + sym::isize_legacy_mod, + sym::i128_legacy_mod, + sym::i64_legacy_mod, + sym::i32_legacy_mod, + sym::i16_legacy_mod, + sym::i8_legacy_mod, + sym::usize_legacy_mod, + sym::u128_legacy_mod, + sym::u64_legacy_mod, + sym::u32_legacy_mod, + sym::u16_legacy_mod, + sym::u8_legacy_mod, + ] + .iter() + .any(|&name| cx.tcx.is_diagnostic_item(name, did)) +} + +fn is_numeric_const(cx: &LateContext<'_>, did: DefId) -> bool { + [ + sym::isize_legacy_const_max, + sym::isize_legacy_const_min, + sym::i128_legacy_const_max, + sym::i128_legacy_const_min, + sym::i16_legacy_const_max, + sym::i16_legacy_const_min, + sym::i32_legacy_const_max, + sym::i32_legacy_const_min, + sym::i64_legacy_const_max, + sym::i64_legacy_const_min, + sym::i8_legacy_const_max, + sym::i8_legacy_const_min, + sym::usize_legacy_const_max, + sym::usize_legacy_const_min, + sym::u128_legacy_const_max, + sym::u128_legacy_const_min, + sym::u16_legacy_const_max, + sym::u16_legacy_const_min, + sym::u32_legacy_const_max, + sym::u32_legacy_const_min, + sym::u64_legacy_const_max, + sym::u64_legacy_const_min, + sym::u8_legacy_const_max, + sym::u8_legacy_const_min, + sym::f32_legacy_const_digits, + sym::f32_legacy_const_epsilon, + sym::f32_legacy_const_infinity, + sym::f32_legacy_const_mantissa_dig, + sym::f32_legacy_const_max, + sym::f32_legacy_const_max_10_exp, + sym::f32_legacy_const_max_exp, + sym::f32_legacy_const_min, + sym::f32_legacy_const_min_10_exp, + sym::f32_legacy_const_min_exp, + sym::f32_legacy_const_min_positive, + sym::f32_legacy_const_nan, + sym::f32_legacy_const_neg_infinity, + sym::f32_legacy_const_radix, + sym::f64_legacy_const_digits, + sym::f64_legacy_const_epsilon, + sym::f64_legacy_const_infinity, + sym::f64_legacy_const_mantissa_dig, + sym::f64_legacy_const_max, + sym::f64_legacy_const_max_10_exp, + sym::f64_legacy_const_max_exp, + sym::f64_legacy_const_min, + sym::f64_legacy_const_min_10_exp, + sym::f64_legacy_const_min_exp, + sym::f64_legacy_const_min_positive, + sym::f64_legacy_const_nan, + sym::f64_legacy_const_neg_infinity, + sym::f64_legacy_const_radix, + ] + .iter() + .any(|&name| cx.tcx.is_diagnostic_item(name, did)) +} + +// Whether path expression looks like `i32::MAX` +fn is_numeric_const_path_canonical(expr_path: &hir::Path<'_>, [mod_name, name]: [Symbol; 2]) -> bool { + let [ + hir::PathSegment { + ident: one, args: None, .. + }, + hir::PathSegment { + ident: two, args: None, .. + }, + ] = expr_path.segments + else { + return false; + }; + + one.name == mod_name && two.name == name +} + +fn is_integer_method(cx: &LateContext<'_>, did: DefId) -> bool { + [ + sym::isize_legacy_fn_max_value, + sym::isize_legacy_fn_min_value, + sym::i128_legacy_fn_max_value, + sym::i128_legacy_fn_min_value, + sym::i16_legacy_fn_max_value, + sym::i16_legacy_fn_min_value, + sym::i32_legacy_fn_max_value, + sym::i32_legacy_fn_min_value, + sym::i64_legacy_fn_max_value, + sym::i64_legacy_fn_min_value, + sym::i8_legacy_fn_max_value, + sym::i8_legacy_fn_min_value, + sym::usize_legacy_fn_max_value, + sym::usize_legacy_fn_min_value, + sym::u128_legacy_fn_max_value, + sym::u128_legacy_fn_min_value, + sym::u16_legacy_fn_max_value, + sym::u16_legacy_fn_min_value, + sym::u32_legacy_fn_max_value, + sym::u32_legacy_fn_min_value, + sym::u64_legacy_fn_max_value, + sym::u64_legacy_fn_min_value, + sym::u8_legacy_fn_max_value, + sym::u8_legacy_fn_min_value, + ] + .iter() + .any(|&name| cx.tcx.is_diagnostic_item(name, did)) +} diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 169e51e2cb93..b92364a9d147 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -189,6 +189,7 @@ mod large_futures; mod large_include_file; mod large_stack_arrays; mod large_stack_frames; +mod legacy_numeric_constants; mod len_zero; mod let_if_seq; mod let_underscore; @@ -1083,6 +1084,7 @@ pub fn register_lints(store: &mut rustc_lint::LintStore, conf: &'static Conf) { allow_one_hash_in_raw_strings, }) }); + store.register_late_pass(move |_| Box::new(legacy_numeric_constants::LegacyNumericConstants::new(msrv()))); store.register_late_pass(|_| Box::new(manual_range_patterns::ManualRangePatterns)); store.register_early_pass(|| Box::new(visibility::Visibility)); store.register_late_pass(move |_| Box::new(tuple_array_conversions::TupleArrayConversions { msrv: msrv() })); diff --git a/clippy_utils/src/check_proc_macro.rs b/clippy_utils/src/check_proc_macro.rs index d751aeaf9022..422673105136 100644 --- a/clippy_utils/src/check_proc_macro.rs +++ b/clippy_utils/src/check_proc_macro.rs @@ -24,7 +24,7 @@ use rustc_hir::{ use rustc_lint::{LateContext, LintContext}; use rustc_middle::ty::TyCtxt; use rustc_session::Session; -use rustc_span::symbol::Ident; +use rustc_span::symbol::{kw, Ident}; use rustc_span::{Span, Symbol}; use rustc_target::spec::abi::Abi; @@ -99,9 +99,13 @@ fn qpath_search_pat(path: &QPath<'_>) -> (Pat, Pat) { let start = if ty.is_some() { Pat::Str("<") } else { - path.segments - .first() - .map_or(Pat::Str(""), |seg| Pat::Sym(seg.ident.name)) + path.segments.first().map_or(Pat::Str(""), |seg| { + if seg.ident.name == kw::PathRoot { + Pat::Str("::") + } else { + Pat::Sym(seg.ident.name) + } + }) }; let end = path.segments.last().map_or(Pat::Str(""), |seg| { if seg.args.is_some() { diff --git a/tests/ui-toml/absolute_paths/absolute_paths.rs b/tests/ui-toml/absolute_paths/absolute_paths.rs index 0e6a54452ee8..a828701bcee9 100644 --- a/tests/ui-toml/absolute_paths/absolute_paths.rs +++ b/tests/ui-toml/absolute_paths/absolute_paths.rs @@ -3,7 +3,7 @@ //@revisions: allow_crates disallow_crates //@[allow_crates] rustc-env:CLIPPY_CONF_DIR=tests/ui-toml/absolute_paths/allow_crates //@[disallow_crates] rustc-env:CLIPPY_CONF_DIR=tests/ui-toml/absolute_paths/disallow_crates -#![allow(clippy::no_effect, unused)] +#![allow(clippy::no_effect, clippy::legacy_numeric_constants, unused)] #![warn(clippy::absolute_paths)] #![feature(decl_macro)] diff --git a/tests/ui/checked_conversions.fixed b/tests/ui/checked_conversions.fixed index 0e05a27429b0..1e8da3316141 100644 --- a/tests/ui/checked_conversions.fixed +++ b/tests/ui/checked_conversions.fixed @@ -1,5 +1,6 @@ #![allow( clippy::cast_lossless, + clippy::legacy_numeric_constants, unused, // Int::max_value will be deprecated in the future deprecated, diff --git a/tests/ui/checked_conversions.rs b/tests/ui/checked_conversions.rs index ac7826992653..67a9adc049ea 100644 --- a/tests/ui/checked_conversions.rs +++ b/tests/ui/checked_conversions.rs @@ -1,5 +1,6 @@ #![allow( clippy::cast_lossless, + clippy::legacy_numeric_constants, unused, // Int::max_value will be deprecated in the future deprecated, diff --git a/tests/ui/checked_conversions.stderr b/tests/ui/checked_conversions.stderr index 223e379cce96..453cd7fcf016 100644 --- a/tests/ui/checked_conversions.stderr +++ b/tests/ui/checked_conversions.stderr @@ -1,5 +1,5 @@ error: checked cast can be simplified - --> tests/ui/checked_conversions.rs:14:13 + --> tests/ui/checked_conversions.rs:15:13 | LL | let _ = value <= (u32::max_value() as i64) && value >= 0; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `u32::try_from(value).is_ok()` @@ -8,97 +8,97 @@ LL | let _ = value <= (u32::max_value() as i64) && value >= 0; = help: to override `-D warnings` add `#[allow(clippy::checked_conversions)]` error: checked cast can be simplified - --> tests/ui/checked_conversions.rs:15:13 + --> tests/ui/checked_conversions.rs:16:13 | LL | let _ = value <= (u32::MAX as i64) && value >= 0; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `u32::try_from(value).is_ok()` error: checked cast can be simplified - --> tests/ui/checked_conversions.rs:19:13 + --> tests/ui/checked_conversions.rs:20:13 | LL | let _ = value <= i64::from(u16::max_value()) && value >= 0; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `u16::try_from(value).is_ok()` error: checked cast can be simplified - --> tests/ui/checked_conversions.rs:20:13 + --> tests/ui/checked_conversions.rs:21:13 | LL | let _ = value <= i64::from(u16::MAX) && value >= 0; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `u16::try_from(value).is_ok()` error: checked cast can be simplified - --> tests/ui/checked_conversions.rs:24:13 + --> tests/ui/checked_conversions.rs:25:13 | LL | let _ = value <= (u8::max_value() as isize) && value >= 0; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `u8::try_from(value).is_ok()` error: checked cast can be simplified - --> tests/ui/checked_conversions.rs:25:13 + --> tests/ui/checked_conversions.rs:26:13 | LL | let _ = value <= (u8::MAX as isize) && value >= 0; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `u8::try_from(value).is_ok()` error: checked cast can be simplified - --> tests/ui/checked_conversions.rs:31:13 + --> tests/ui/checked_conversions.rs:32:13 | LL | let _ = value <= (i32::max_value() as i64) && value >= (i32::min_value() as i64); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `i32::try_from(value).is_ok()` error: checked cast can be simplified - --> tests/ui/checked_conversions.rs:32:13 + --> tests/ui/checked_conversions.rs:33:13 | LL | let _ = value <= (i32::MAX as i64) && value >= (i32::MIN as i64); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `i32::try_from(value).is_ok()` error: checked cast can be simplified - --> tests/ui/checked_conversions.rs:36:13 + --> tests/ui/checked_conversions.rs:37:13 | LL | let _ = value <= i64::from(i16::max_value()) && value >= i64::from(i16::min_value()); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `i16::try_from(value).is_ok()` error: checked cast can be simplified - --> tests/ui/checked_conversions.rs:37:13 + --> tests/ui/checked_conversions.rs:38:13 | LL | let _ = value <= i64::from(i16::MAX) && value >= i64::from(i16::MIN); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `i16::try_from(value).is_ok()` error: checked cast can be simplified - --> tests/ui/checked_conversions.rs:43:13 + --> tests/ui/checked_conversions.rs:44:13 | LL | let _ = value <= i32::max_value() as u32; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `i32::try_from(value).is_ok()` error: checked cast can be simplified - --> tests/ui/checked_conversions.rs:44:13 + --> tests/ui/checked_conversions.rs:45:13 | LL | let _ = value <= i32::MAX as u32; | ^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `i32::try_from(value).is_ok()` error: checked cast can be simplified - --> tests/ui/checked_conversions.rs:48:13 + --> tests/ui/checked_conversions.rs:49:13 | LL | let _ = value <= isize::max_value() as usize && value as i32 == 5; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `isize::try_from(value).is_ok()` error: checked cast can be simplified - --> tests/ui/checked_conversions.rs:49:13 + --> tests/ui/checked_conversions.rs:50:13 | LL | let _ = value <= isize::MAX as usize && value as i32 == 5; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `isize::try_from(value).is_ok()` error: checked cast can be simplified - --> tests/ui/checked_conversions.rs:53:13 + --> tests/ui/checked_conversions.rs:54:13 | LL | let _ = value <= u16::max_value() as u32 && value as i32 == 5; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `u16::try_from(value).is_ok()` error: checked cast can be simplified - --> tests/ui/checked_conversions.rs:54:13 + --> tests/ui/checked_conversions.rs:55:13 | LL | let _ = value <= u16::MAX as u32 && value as i32 == 5; | ^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `u16::try_from(value).is_ok()` error: checked cast can be simplified - --> tests/ui/checked_conversions.rs:87:13 + --> tests/ui/checked_conversions.rs:88:13 | LL | let _ = value <= (u32::MAX as i64) && value >= 0; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `u32::try_from(value).is_ok()` diff --git a/tests/ui/legacy_numeric_constants.fixed b/tests/ui/legacy_numeric_constants.fixed new file mode 100644 index 000000000000..31a6c1b3944e --- /dev/null +++ b/tests/ui/legacy_numeric_constants.fixed @@ -0,0 +1,118 @@ +//@aux-build:proc_macros.rs +#![allow(clippy::no_effect, deprecated, unused)] +#![allow(clippy::legacy_numeric_constants)] // For imports. +#![feature(lint_reasons)] + +#[macro_use] +extern crate proc_macros; + +pub mod a { + pub use std::u128; +} + +macro_rules! b { + () => { + mod b { + #[warn(clippy::legacy_numeric_constants)] + fn b() { + let x = u64::MAX; + //~^ ERROR: usage of a legacy numeric constant + //~| HELP: use the associated constant instead + } + } + }; +} + +use std::u32::MAX; +use std::u8::MIN; +use std::{f64, u32}; + +#[warn(clippy::legacy_numeric_constants)] +fn main() { + f32::EPSILON; + //~^ ERROR: usage of a legacy numeric constant + //~| HELP: use the associated constant instead + u8::MIN; + //~^ ERROR: usage of a legacy numeric constant + //~| HELP: use the associated constant instead + usize::MIN; + //~^ ERROR: usage of a legacy numeric constant + //~| HELP: use the associated constant instead + u32::MAX; + //~^ ERROR: usage of a legacy numeric constant + //~| HELP: use the associated constant instead + u32::MAX; + //~^ ERROR: usage of a legacy numeric constant + //~| HELP: use the associated constant instead + u32::MAX; + //~^ ERROR: usage of a legacy numeric constant + //~| HELP: use the associated constant instead + i32::MAX; + //~^ ERROR: usage of a legacy numeric method + //~| HELP: use the associated constant instead + u8::MAX; + //~^ ERROR: usage of a legacy numeric method + //~| HELP: use the associated constant instead + u8::MIN; + //~^ ERROR: usage of a legacy numeric method + //~| HELP: use the associated constant instead + u8::MIN; + //~^ ERROR: usage of a legacy numeric constant + //~| HELP: use the associated constant instead + ::std::primitive::u8::MIN; + //~^ ERROR: usage of a legacy numeric method + //~| HELP: use the associated constant instead + std::primitive::i32::MAX; + //~^ ERROR: usage of a legacy numeric method + //~| HELP: use the associated constant instead + u128::MAX; + //~^ ERROR: usage of a legacy numeric constant + //~| HELP: use the associated constant instead + u32::MAX; + u128::MAX; + f32::EPSILON; + ::std::primitive::u8::MIN; + std::f32::consts::E; + f64::consts::E; + u8::MIN; + std::f32::consts::E; + f64::consts::E; + b!(); + + [(0, "", i128::MAX)]; + //~^ ERROR: usage of a legacy numeric constant + //~| HELP: use the associated constant instead +} + +#[warn(clippy::legacy_numeric_constants)] +fn ext() { + external! { + ::std::primitive::u8::MIN; + ::std::u8::MIN; + ::std::primitive::u8::min_value(); + use std::u64; + use std::u8::MIN; + } +} + +#[allow(clippy::legacy_numeric_constants)] +fn allow() { + ::std::primitive::u8::MIN; + ::std::u8::MIN; + ::std::primitive::u8::min_value(); + use std::u64; + use std::u8::MIN; +} + +#[warn(clippy::legacy_numeric_constants)] +#[clippy::msrv = "1.42.0"] +fn msrv_too_low() { + std::u32::MAX; +} + +#[warn(clippy::legacy_numeric_constants)] +#[clippy::msrv = "1.43.0"] +fn msrv_juust_right() { + u32::MAX; + //~^ ERROR: usage of a legacy numeric constant +} diff --git a/tests/ui/legacy_numeric_constants.rs b/tests/ui/legacy_numeric_constants.rs new file mode 100644 index 000000000000..e4dd3d5896f5 --- /dev/null +++ b/tests/ui/legacy_numeric_constants.rs @@ -0,0 +1,118 @@ +//@aux-build:proc_macros.rs +#![allow(clippy::no_effect, deprecated, unused)] +#![allow(clippy::legacy_numeric_constants)] // For imports. +#![feature(lint_reasons)] + +#[macro_use] +extern crate proc_macros; + +pub mod a { + pub use std::u128; +} + +macro_rules! b { + () => { + mod b { + #[warn(clippy::legacy_numeric_constants)] + fn b() { + let x = std::u64::MAX; + //~^ ERROR: usage of a legacy numeric constant + //~| HELP: use the associated constant instead + } + } + }; +} + +use std::u32::MAX; +use std::u8::MIN; +use std::{f64, u32}; + +#[warn(clippy::legacy_numeric_constants)] +fn main() { + std::f32::EPSILON; + //~^ ERROR: usage of a legacy numeric constant + //~| HELP: use the associated constant instead + std::u8::MIN; + //~^ ERROR: usage of a legacy numeric constant + //~| HELP: use the associated constant instead + std::usize::MIN; + //~^ ERROR: usage of a legacy numeric constant + //~| HELP: use the associated constant instead + std::u32::MAX; + //~^ ERROR: usage of a legacy numeric constant + //~| HELP: use the associated constant instead + core::u32::MAX; + //~^ ERROR: usage of a legacy numeric constant + //~| HELP: use the associated constant instead + MAX; + //~^ ERROR: usage of a legacy numeric constant + //~| HELP: use the associated constant instead + i32::max_value(); + //~^ ERROR: usage of a legacy numeric method + //~| HELP: use the associated constant instead + u8::max_value(); + //~^ ERROR: usage of a legacy numeric method + //~| HELP: use the associated constant instead + u8::min_value(); + //~^ ERROR: usage of a legacy numeric method + //~| HELP: use the associated constant instead + ::std::u8::MIN; + //~^ ERROR: usage of a legacy numeric constant + //~| HELP: use the associated constant instead + ::std::primitive::u8::min_value(); + //~^ ERROR: usage of a legacy numeric method + //~| HELP: use the associated constant instead + std::primitive::i32::max_value(); + //~^ ERROR: usage of a legacy numeric method + //~| HELP: use the associated constant instead + self::a::u128::MAX; + //~^ ERROR: usage of a legacy numeric constant + //~| HELP: use the associated constant instead + u32::MAX; + u128::MAX; + f32::EPSILON; + ::std::primitive::u8::MIN; + std::f32::consts::E; + f64::consts::E; + u8::MIN; + std::f32::consts::E; + f64::consts::E; + b!(); + + [(0, "", std::i128::MAX)]; + //~^ ERROR: usage of a legacy numeric constant + //~| HELP: use the associated constant instead +} + +#[warn(clippy::legacy_numeric_constants)] +fn ext() { + external! { + ::std::primitive::u8::MIN; + ::std::u8::MIN; + ::std::primitive::u8::min_value(); + use std::u64; + use std::u8::MIN; + } +} + +#[allow(clippy::legacy_numeric_constants)] +fn allow() { + ::std::primitive::u8::MIN; + ::std::u8::MIN; + ::std::primitive::u8::min_value(); + use std::u64; + use std::u8::MIN; +} + +#[warn(clippy::legacy_numeric_constants)] +#[clippy::msrv = "1.42.0"] +fn msrv_too_low() { + std::u32::MAX; +} + +#[warn(clippy::legacy_numeric_constants)] +#[clippy::msrv = "1.43.0"] +fn msrv_juust_right() { + std::u32::MAX; + //~^ ERROR: usage of a legacy numeric constant +} diff --git a/tests/ui/legacy_numeric_constants.stderr b/tests/ui/legacy_numeric_constants.stderr new file mode 100644 index 000000000000..af9b003ed540 --- /dev/null +++ b/tests/ui/legacy_numeric_constants.stderr @@ -0,0 +1,184 @@ +error: usage of a legacy numeric constant + --> tests/ui/legacy_numeric_constants.rs:32:5 + | +LL | std::f32::EPSILON; + | ^^^^^^^^^^^^^^^^^ + | + = note: `-D clippy::legacy-numeric-constants` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(clippy::legacy_numeric_constants)]` +help: use the associated constant instead + | +LL | f32::EPSILON; + | ~~~~~~~~~~~~ + +error: usage of a legacy numeric constant + --> tests/ui/legacy_numeric_constants.rs:35:5 + | +LL | std::u8::MIN; + | ^^^^^^^^^^^^ + | +help: use the associated constant instead + | +LL | u8::MIN; + | ~~~~~~~ + +error: usage of a legacy numeric constant + --> tests/ui/legacy_numeric_constants.rs:38:5 + | +LL | std::usize::MIN; + | ^^^^^^^^^^^^^^^ + | +help: use the associated constant instead + | +LL | usize::MIN; + | ~~~~~~~~~~ + +error: usage of a legacy numeric constant + --> tests/ui/legacy_numeric_constants.rs:41:5 + | +LL | std::u32::MAX; + | ^^^^^^^^^^^^^ + | +help: use the associated constant instead + | +LL | u32::MAX; + | ~~~~~~~~ + +error: usage of a legacy numeric constant + --> tests/ui/legacy_numeric_constants.rs:44:5 + | +LL | core::u32::MAX; + | ^^^^^^^^^^^^^^ + | +help: use the associated constant instead + | +LL | u32::MAX; + | ~~~~~~~~ + +error: usage of a legacy numeric constant + --> tests/ui/legacy_numeric_constants.rs:47:5 + | +LL | MAX; + | ^^^ + | +help: use the associated constant instead + | +LL | u32::MAX; + | ~~~~~~~~ + +error: usage of a legacy numeric method + --> tests/ui/legacy_numeric_constants.rs:50:10 + | +LL | i32::max_value(); + | ^^^^^^^^^^^ + | +help: use the associated constant instead + | +LL | i32::MAX; + | ~~~ + +error: usage of a legacy numeric method + --> tests/ui/legacy_numeric_constants.rs:53:9 + | +LL | u8::max_value(); + | ^^^^^^^^^^^ + | +help: use the associated constant instead + | +LL | u8::MAX; + | ~~~ + +error: usage of a legacy numeric method + --> tests/ui/legacy_numeric_constants.rs:56:9 + | +LL | u8::min_value(); + | ^^^^^^^^^^^ + | +help: use the associated constant instead + | +LL | u8::MIN; + | ~~~ + +error: usage of a legacy numeric constant + --> tests/ui/legacy_numeric_constants.rs:59:5 + | +LL | ::std::u8::MIN; + | ^^^^^^^^^^^^^^ + | +help: use the associated constant instead + | +LL | u8::MIN; + | ~~~~~~~ + +error: usage of a legacy numeric method + --> tests/ui/legacy_numeric_constants.rs:62:27 + | +LL | ::std::primitive::u8::min_value(); + | ^^^^^^^^^^^ + | +help: use the associated constant instead + | +LL | ::std::primitive::u8::MIN; + | ~~~ + +error: usage of a legacy numeric method + --> tests/ui/legacy_numeric_constants.rs:65:26 + | +LL | std::primitive::i32::max_value(); + | ^^^^^^^^^^^ + | +help: use the associated constant instead + | +LL | std::primitive::i32::MAX; + | ~~~ + +error: usage of a legacy numeric constant + --> tests/ui/legacy_numeric_constants.rs:68:5 + | +LL | self::a::u128::MAX; + | ^^^^^^^^^^^^^^^^^^ + | +help: use the associated constant instead + | +LL | u128::MAX; + | ~~~~~~~~~ + +error: usage of a legacy numeric constant + --> tests/ui/legacy_numeric_constants.rs:18:25 + | +LL | let x = std::u64::MAX; + | ^^^^^^^^^^^^^ +... +LL | b!(); + | ---- in this macro invocation + | + = note: this error originates in the macro `b` (in Nightly builds, run with -Z macro-backtrace for more info) +help: use the associated constant instead + | +LL | let x = u64::MAX; + | ~~~~~~~~ + +error: usage of a legacy numeric constant + --> tests/ui/legacy_numeric_constants.rs:82:14 + | +LL | [(0, "", std::i128::MAX)]; + | ^^^^^^^^^^^^^^ + | +help: use the associated constant instead + | +LL | [(0, "", i128::MAX)]; + | ~~~~~~~~~ + +error: usage of a legacy numeric constant + --> tests/ui/legacy_numeric_constants.rs:116:5 + | +LL | std::u32::MAX; + | ^^^^^^^^^^^^^ + | +help: use the associated constant instead + | +LL | u32::MAX; + | ~~~~~~~~ + +error: aborting due to 16 previous errors + diff --git a/tests/ui/legacy_numeric_constants_unfixable.rs b/tests/ui/legacy_numeric_constants_unfixable.rs new file mode 100644 index 000000000000..228ebdd6a6c5 --- /dev/null +++ b/tests/ui/legacy_numeric_constants_unfixable.rs @@ -0,0 +1,79 @@ +//@no-rustfix +//@aux-build:proc_macros.rs +#![allow(clippy::no_effect, deprecated, unused)] +#![warn(clippy::legacy_numeric_constants)] +#![feature(lint_reasons)] + +#[macro_use] +extern crate proc_macros; + +use std::u128 as _; +//~^ ERROR: importing legacy numeric constants +//~| HELP: remove this import +pub mod a { + pub use std::{mem, u128}; + //~^ ERROR: importing legacy numeric constants + //~| HELP: remove this import +} + +macro_rules! b { + () => { + mod b { + use std::u32; + //~^ ERROR: importing legacy numeric constants + //~| HELP: remove this import + } + }; +} + +fn main() { + use std::u32::MAX; + //~^ ERROR: importing a legacy numeric constant + //~| HELP: remove this import and use the associated constant `u32::MAX` + use std::u8::MIN; + //~^ ERROR: importing a legacy numeric constant + //~| HELP: remove this import and use the associated constant `u8::MIN` + f64::MAX; + use std::u32; + //~^ ERROR: importing legacy numeric constants + //~| HELP: remove this import + u32::MAX; + use std::f32::MIN_POSITIVE; + //~^ ERROR: importing a legacy numeric constant + //~| HELP: remove this import and use the associated constant `f32::MIN_POSITIVE` + use std::f64; + use std::i16::*; + //~^ ERROR: importing legacy numeric constants + //~| HELP: remove this import and use associated constants `i16::` + u128::MAX; + f32::EPSILON; + f64::EPSILON; + ::std::primitive::u8::MIN; + std::f32::consts::E; + f64::consts::E; + u8::MIN; + std::f32::consts::E; + f64::consts::E; + b!(); +} + +fn ext() { + external! { + ::std::primitive::u8::MIN; + ::std::u8::MIN; + ::std::primitive::u8::min_value(); + use std::u64; + use std::u8::MIN; + } +} + +#[clippy::msrv = "1.42.0"] +fn msrv_too_low() { + use std::u32::MAX; +} + +#[clippy::msrv = "1.43.0"] +fn msrv_juust_right() { + use std::u32::MAX; + //~^ ERROR: importing a legacy numeric constant +} diff --git a/tests/ui/legacy_numeric_constants_unfixable.stderr b/tests/ui/legacy_numeric_constants_unfixable.stderr new file mode 100644 index 000000000000..96e6c9ef9759 --- /dev/null +++ b/tests/ui/legacy_numeric_constants_unfixable.stderr @@ -0,0 +1,83 @@ +error: importing legacy numeric constants + --> tests/ui/legacy_numeric_constants_unfixable.rs:10:5 + | +LL | use std::u128 as _; + | ^^^^^^^^^ + | + = help: remove this import + = note: `-D clippy::legacy-numeric-constants` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(clippy::legacy_numeric_constants)]` + +error: importing legacy numeric constants + --> tests/ui/legacy_numeric_constants_unfixable.rs:14:24 + | +LL | pub use std::{mem, u128}; + | ^^^^ + | + = help: remove this import + = note: then `u128::` will resolve to the respective associated constant + +error: importing a legacy numeric constant + --> tests/ui/legacy_numeric_constants_unfixable.rs:30:9 + | +LL | use std::u32::MAX; + | ^^^^^^^^^^^^^ + | + = help: remove this import and use the associated constant `u32::MAX` from the primitive type instead + +error: importing a legacy numeric constant + --> tests/ui/legacy_numeric_constants_unfixable.rs:33:9 + | +LL | use std::u8::MIN; + | ^^^^^^^^^^^^ + | + = help: remove this import and use the associated constant `u8::MIN` from the primitive type instead + +error: importing legacy numeric constants + --> tests/ui/legacy_numeric_constants_unfixable.rs:37:9 + | +LL | use std::u32; + | ^^^^^^^^ + | + = help: remove this import + = note: then `u32::` will resolve to the respective associated constant + +error: importing a legacy numeric constant + --> tests/ui/legacy_numeric_constants_unfixable.rs:41:9 + | +LL | use std::f32::MIN_POSITIVE; + | ^^^^^^^^^^^^^^^^^^^^^^ + | + = help: remove this import and use the associated constant `f32::MIN_POSITIVE` from the primitive type instead + +error: importing legacy numeric constants + --> tests/ui/legacy_numeric_constants_unfixable.rs:45:9 + | +LL | use std::i16::*; + | ^^^^^^^^ + | + = help: remove this import and use associated constants `i16::` from the primitive type instead + +error: importing legacy numeric constants + --> tests/ui/legacy_numeric_constants_unfixable.rs:22:17 + | +LL | use std::u32; + | ^^^^^^^^ +... +LL | b!(); + | ---- in this macro invocation + | + = help: remove this import + = note: then `u32::` will resolve to the respective associated constant + = note: this error originates in the macro `b` (in Nightly builds, run with -Z macro-backtrace for more info) + +error: importing a legacy numeric constant + --> tests/ui/legacy_numeric_constants_unfixable.rs:77:9 + | +LL | use std::u32::MAX; + | ^^^^^^^^^^^^^ + | + = help: remove this import and use the associated constant `u32::MAX` from the primitive type instead + +error: aborting due to 9 previous errors + diff --git a/tests/ui/manual_saturating_arithmetic.fixed b/tests/ui/manual_saturating_arithmetic.fixed index 8218f10881a7..41a32df32ee4 100644 --- a/tests/ui/manual_saturating_arithmetic.fixed +++ b/tests/ui/manual_saturating_arithmetic.fixed @@ -1,4 +1,4 @@ -#![allow(unused_imports)] +#![allow(clippy::legacy_numeric_constants, unused_imports)] use std::{i128, i32, u128, u32}; diff --git a/tests/ui/manual_saturating_arithmetic.rs b/tests/ui/manual_saturating_arithmetic.rs index 60022b54b02d..3a6b32d80690 100644 --- a/tests/ui/manual_saturating_arithmetic.rs +++ b/tests/ui/manual_saturating_arithmetic.rs @@ -1,4 +1,4 @@ -#![allow(unused_imports)] +#![allow(clippy::legacy_numeric_constants, unused_imports)] use std::{i128, i32, u128, u32}; diff --git a/tests/ui/suspicious_arithmetic_impl.rs b/tests/ui/suspicious_arithmetic_impl.rs index 1bd4cd5fb50d..07280351e76c 100644 --- a/tests/ui/suspicious_arithmetic_impl.rs +++ b/tests/ui/suspicious_arithmetic_impl.rs @@ -1,3 +1,4 @@ +#![allow(clippy::legacy_numeric_constants)] #![warn(clippy::suspicious_arithmetic_impl)] use std::ops::{ Add, AddAssign, BitAnd, BitOr, BitOrAssign, BitXor, Div, DivAssign, Mul, MulAssign, Rem, Shl, Shr, Sub, diff --git a/tests/ui/suspicious_arithmetic_impl.stderr b/tests/ui/suspicious_arithmetic_impl.stderr index 193cd64149b1..1bfca49a635d 100644 --- a/tests/ui/suspicious_arithmetic_impl.stderr +++ b/tests/ui/suspicious_arithmetic_impl.stderr @@ -1,5 +1,5 @@ error: suspicious use of `-` in `Add` impl - --> tests/ui/suspicious_arithmetic_impl.rs:13:20 + --> tests/ui/suspicious_arithmetic_impl.rs:14:20 | LL | Foo(self.0 - other.0) | ^ @@ -8,7 +8,7 @@ LL | Foo(self.0 - other.0) = help: to override `-D warnings` add `#[allow(clippy::suspicious_arithmetic_impl)]` error: suspicious use of `-` in `AddAssign` impl - --> tests/ui/suspicious_arithmetic_impl.rs:21:23 + --> tests/ui/suspicious_arithmetic_impl.rs:22:23 | LL | *self = *self - other; | ^ @@ -17,43 +17,43 @@ LL | *self = *self - other; = help: to override `-D warnings` add `#[allow(clippy::suspicious_op_assign_impl)]` error: suspicious use of `/` in `MulAssign` impl - --> tests/ui/suspicious_arithmetic_impl.rs:36:16 + --> tests/ui/suspicious_arithmetic_impl.rs:37:16 | LL | self.0 /= other.0; | ^^ error: suspicious use of `/` in `Rem` impl - --> tests/ui/suspicious_arithmetic_impl.rs:75:20 + --> tests/ui/suspicious_arithmetic_impl.rs:76:20 | LL | Foo(self.0 / other.0) | ^ error: suspicious use of `|` in `BitAnd` impl - --> tests/ui/suspicious_arithmetic_impl.rs:84:20 + --> tests/ui/suspicious_arithmetic_impl.rs:85:20 | LL | Foo(self.0 | other.0) | ^ error: suspicious use of `^` in `BitOr` impl - --> tests/ui/suspicious_arithmetic_impl.rs:93:20 + --> tests/ui/suspicious_arithmetic_impl.rs:94:20 | LL | Foo(self.0 ^ other.0) | ^ error: suspicious use of `&` in `BitXor` impl - --> tests/ui/suspicious_arithmetic_impl.rs:102:20 + --> tests/ui/suspicious_arithmetic_impl.rs:103:20 | LL | Foo(self.0 & other.0) | ^ error: suspicious use of `>>` in `Shl` impl - --> tests/ui/suspicious_arithmetic_impl.rs:111:20 + --> tests/ui/suspicious_arithmetic_impl.rs:112:20 | LL | Foo(self.0 >> other.0) | ^^ error: suspicious use of `<<` in `Shr` impl - --> tests/ui/suspicious_arithmetic_impl.rs:120:20 + --> tests/ui/suspicious_arithmetic_impl.rs:121:20 | LL | Foo(self.0 << other.0) | ^^ From 3aa37e0c3e1fc48a47fe43d12b3589cc65ec94ca Mon Sep 17 00:00:00 2001 From: Peter Jaszkowiak Date: Sun, 24 Mar 2024 10:16:45 -0600 Subject: [PATCH 16/22] match syms, remove lint_reasons --- clippy_lints/src/legacy_numeric_constants.rs | 203 +++++++++--------- tests/ui/legacy_numeric_constants.fixed | 1 - tests/ui/legacy_numeric_constants.rs | 1 - tests/ui/legacy_numeric_constants.stderr | 32 +-- .../ui/legacy_numeric_constants_unfixable.rs | 1 - .../legacy_numeric_constants_unfixable.stderr | 18 +- 6 files changed, 128 insertions(+), 128 deletions(-) diff --git a/clippy_lints/src/legacy_numeric_constants.rs b/clippy_lints/src/legacy_numeric_constants.rs index a325b4d54937..c5f1afe68c3f 100644 --- a/clippy_lints/src/legacy_numeric_constants.rs +++ b/clippy_lints/src/legacy_numeric_constants.rs @@ -164,81 +164,83 @@ impl<'tcx> LateLintPass<'tcx> for LegacyNumericConstants { } fn is_integer_module(cx: &LateContext<'_>, did: DefId) -> bool { - [ - sym::isize_legacy_mod, - sym::i128_legacy_mod, - sym::i64_legacy_mod, - sym::i32_legacy_mod, - sym::i16_legacy_mod, - sym::i8_legacy_mod, - sym::usize_legacy_mod, - sym::u128_legacy_mod, - sym::u64_legacy_mod, - sym::u32_legacy_mod, - sym::u16_legacy_mod, - sym::u8_legacy_mod, - ] - .iter() - .any(|&name| cx.tcx.is_diagnostic_item(name, did)) + matches!( + cx.tcx.get_diagnostic_name(did), + Some( + sym::isize_legacy_mod + | sym::i128_legacy_mod + | sym::i64_legacy_mod + | sym::i32_legacy_mod + | sym::i16_legacy_mod + | sym::i8_legacy_mod + | sym::usize_legacy_mod + | sym::u128_legacy_mod + | sym::u64_legacy_mod + | sym::u32_legacy_mod + | sym::u16_legacy_mod + | sym::u8_legacy_mod + ) + ) } fn is_numeric_const(cx: &LateContext<'_>, did: DefId) -> bool { - [ - sym::isize_legacy_const_max, - sym::isize_legacy_const_min, - sym::i128_legacy_const_max, - sym::i128_legacy_const_min, - sym::i16_legacy_const_max, - sym::i16_legacy_const_min, - sym::i32_legacy_const_max, - sym::i32_legacy_const_min, - sym::i64_legacy_const_max, - sym::i64_legacy_const_min, - sym::i8_legacy_const_max, - sym::i8_legacy_const_min, - sym::usize_legacy_const_max, - sym::usize_legacy_const_min, - sym::u128_legacy_const_max, - sym::u128_legacy_const_min, - sym::u16_legacy_const_max, - sym::u16_legacy_const_min, - sym::u32_legacy_const_max, - sym::u32_legacy_const_min, - sym::u64_legacy_const_max, - sym::u64_legacy_const_min, - sym::u8_legacy_const_max, - sym::u8_legacy_const_min, - sym::f32_legacy_const_digits, - sym::f32_legacy_const_epsilon, - sym::f32_legacy_const_infinity, - sym::f32_legacy_const_mantissa_dig, - sym::f32_legacy_const_max, - sym::f32_legacy_const_max_10_exp, - sym::f32_legacy_const_max_exp, - sym::f32_legacy_const_min, - sym::f32_legacy_const_min_10_exp, - sym::f32_legacy_const_min_exp, - sym::f32_legacy_const_min_positive, - sym::f32_legacy_const_nan, - sym::f32_legacy_const_neg_infinity, - sym::f32_legacy_const_radix, - sym::f64_legacy_const_digits, - sym::f64_legacy_const_epsilon, - sym::f64_legacy_const_infinity, - sym::f64_legacy_const_mantissa_dig, - sym::f64_legacy_const_max, - sym::f64_legacy_const_max_10_exp, - sym::f64_legacy_const_max_exp, - sym::f64_legacy_const_min, - sym::f64_legacy_const_min_10_exp, - sym::f64_legacy_const_min_exp, - sym::f64_legacy_const_min_positive, - sym::f64_legacy_const_nan, - sym::f64_legacy_const_neg_infinity, - sym::f64_legacy_const_radix, - ] - .iter() - .any(|&name| cx.tcx.is_diagnostic_item(name, did)) + matches!( + cx.tcx.get_diagnostic_name(did), + Some( + sym::isize_legacy_const_max + | sym::isize_legacy_const_min + | sym::i128_legacy_const_max + | sym::i128_legacy_const_min + | sym::i16_legacy_const_max + | sym::i16_legacy_const_min + | sym::i32_legacy_const_max + | sym::i32_legacy_const_min + | sym::i64_legacy_const_max + | sym::i64_legacy_const_min + | sym::i8_legacy_const_max + | sym::i8_legacy_const_min + | sym::usize_legacy_const_max + | sym::usize_legacy_const_min + | sym::u128_legacy_const_max + | sym::u128_legacy_const_min + | sym::u16_legacy_const_max + | sym::u16_legacy_const_min + | sym::u32_legacy_const_max + | sym::u32_legacy_const_min + | sym::u64_legacy_const_max + | sym::u64_legacy_const_min + | sym::u8_legacy_const_max + | sym::u8_legacy_const_min + | sym::f32_legacy_const_digits + | sym::f32_legacy_const_epsilon + | sym::f32_legacy_const_infinity + | sym::f32_legacy_const_mantissa_dig + | sym::f32_legacy_const_max + | sym::f32_legacy_const_max_10_exp + | sym::f32_legacy_const_max_exp + | sym::f32_legacy_const_min + | sym::f32_legacy_const_min_10_exp + | sym::f32_legacy_const_min_exp + | sym::f32_legacy_const_min_positive + | sym::f32_legacy_const_nan + | sym::f32_legacy_const_neg_infinity + | sym::f32_legacy_const_radix + | sym::f64_legacy_const_digits + | sym::f64_legacy_const_epsilon + | sym::f64_legacy_const_infinity + | sym::f64_legacy_const_mantissa_dig + | sym::f64_legacy_const_max + | sym::f64_legacy_const_max_10_exp + | sym::f64_legacy_const_max_exp + | sym::f64_legacy_const_min + | sym::f64_legacy_const_min_10_exp + | sym::f64_legacy_const_min_exp + | sym::f64_legacy_const_min_positive + | sym::f64_legacy_const_nan + | sym::f64_legacy_const_neg_infinity + | sym::f64_legacy_const_radix + ) + ) } // Whether path expression looks like `i32::MAX` @@ -259,32 +261,33 @@ fn is_numeric_const_path_canonical(expr_path: &hir::Path<'_>, [mod_name, name]: } fn is_integer_method(cx: &LateContext<'_>, did: DefId) -> bool { - [ - sym::isize_legacy_fn_max_value, - sym::isize_legacy_fn_min_value, - sym::i128_legacy_fn_max_value, - sym::i128_legacy_fn_min_value, - sym::i16_legacy_fn_max_value, - sym::i16_legacy_fn_min_value, - sym::i32_legacy_fn_max_value, - sym::i32_legacy_fn_min_value, - sym::i64_legacy_fn_max_value, - sym::i64_legacy_fn_min_value, - sym::i8_legacy_fn_max_value, - sym::i8_legacy_fn_min_value, - sym::usize_legacy_fn_max_value, - sym::usize_legacy_fn_min_value, - sym::u128_legacy_fn_max_value, - sym::u128_legacy_fn_min_value, - sym::u16_legacy_fn_max_value, - sym::u16_legacy_fn_min_value, - sym::u32_legacy_fn_max_value, - sym::u32_legacy_fn_min_value, - sym::u64_legacy_fn_max_value, - sym::u64_legacy_fn_min_value, - sym::u8_legacy_fn_max_value, - sym::u8_legacy_fn_min_value, - ] - .iter() - .any(|&name| cx.tcx.is_diagnostic_item(name, did)) + matches!( + cx.tcx.get_diagnostic_name(did), + Some( + sym::isize_legacy_fn_max_value + | sym::isize_legacy_fn_min_value + | sym::i128_legacy_fn_max_value + | sym::i128_legacy_fn_min_value + | sym::i16_legacy_fn_max_value + | sym::i16_legacy_fn_min_value + | sym::i32_legacy_fn_max_value + | sym::i32_legacy_fn_min_value + | sym::i64_legacy_fn_max_value + | sym::i64_legacy_fn_min_value + | sym::i8_legacy_fn_max_value + | sym::i8_legacy_fn_min_value + | sym::usize_legacy_fn_max_value + | sym::usize_legacy_fn_min_value + | sym::u128_legacy_fn_max_value + | sym::u128_legacy_fn_min_value + | sym::u16_legacy_fn_max_value + | sym::u16_legacy_fn_min_value + | sym::u32_legacy_fn_max_value + | sym::u32_legacy_fn_min_value + | sym::u64_legacy_fn_max_value + | sym::u64_legacy_fn_min_value + | sym::u8_legacy_fn_max_value + | sym::u8_legacy_fn_min_value + ) + ) } diff --git a/tests/ui/legacy_numeric_constants.fixed b/tests/ui/legacy_numeric_constants.fixed index 31a6c1b3944e..a6ef8f8c119a 100644 --- a/tests/ui/legacy_numeric_constants.fixed +++ b/tests/ui/legacy_numeric_constants.fixed @@ -1,7 +1,6 @@ //@aux-build:proc_macros.rs #![allow(clippy::no_effect, deprecated, unused)] #![allow(clippy::legacy_numeric_constants)] // For imports. -#![feature(lint_reasons)] #[macro_use] extern crate proc_macros; diff --git a/tests/ui/legacy_numeric_constants.rs b/tests/ui/legacy_numeric_constants.rs index e4dd3d5896f5..cd633545372c 100644 --- a/tests/ui/legacy_numeric_constants.rs +++ b/tests/ui/legacy_numeric_constants.rs @@ -1,7 +1,6 @@ //@aux-build:proc_macros.rs #![allow(clippy::no_effect, deprecated, unused)] #![allow(clippy::legacy_numeric_constants)] // For imports. -#![feature(lint_reasons)] #[macro_use] extern crate proc_macros; diff --git a/tests/ui/legacy_numeric_constants.stderr b/tests/ui/legacy_numeric_constants.stderr index af9b003ed540..267b9ac8e4d1 100644 --- a/tests/ui/legacy_numeric_constants.stderr +++ b/tests/ui/legacy_numeric_constants.stderr @@ -1,5 +1,5 @@ error: usage of a legacy numeric constant - --> tests/ui/legacy_numeric_constants.rs:32:5 + --> tests/ui/legacy_numeric_constants.rs:31:5 | LL | std::f32::EPSILON; | ^^^^^^^^^^^^^^^^^ @@ -12,7 +12,7 @@ LL | f32::EPSILON; | ~~~~~~~~~~~~ error: usage of a legacy numeric constant - --> tests/ui/legacy_numeric_constants.rs:35:5 + --> tests/ui/legacy_numeric_constants.rs:34:5 | LL | std::u8::MIN; | ^^^^^^^^^^^^ @@ -23,7 +23,7 @@ LL | u8::MIN; | ~~~~~~~ error: usage of a legacy numeric constant - --> tests/ui/legacy_numeric_constants.rs:38:5 + --> tests/ui/legacy_numeric_constants.rs:37:5 | LL | std::usize::MIN; | ^^^^^^^^^^^^^^^ @@ -34,7 +34,7 @@ LL | usize::MIN; | ~~~~~~~~~~ error: usage of a legacy numeric constant - --> tests/ui/legacy_numeric_constants.rs:41:5 + --> tests/ui/legacy_numeric_constants.rs:40:5 | LL | std::u32::MAX; | ^^^^^^^^^^^^^ @@ -45,7 +45,7 @@ LL | u32::MAX; | ~~~~~~~~ error: usage of a legacy numeric constant - --> tests/ui/legacy_numeric_constants.rs:44:5 + --> tests/ui/legacy_numeric_constants.rs:43:5 | LL | core::u32::MAX; | ^^^^^^^^^^^^^^ @@ -56,7 +56,7 @@ LL | u32::MAX; | ~~~~~~~~ error: usage of a legacy numeric constant - --> tests/ui/legacy_numeric_constants.rs:47:5 + --> tests/ui/legacy_numeric_constants.rs:46:5 | LL | MAX; | ^^^ @@ -67,7 +67,7 @@ LL | u32::MAX; | ~~~~~~~~ error: usage of a legacy numeric method - --> tests/ui/legacy_numeric_constants.rs:50:10 + --> tests/ui/legacy_numeric_constants.rs:49:10 | LL | i32::max_value(); | ^^^^^^^^^^^ @@ -78,7 +78,7 @@ LL | i32::MAX; | ~~~ error: usage of a legacy numeric method - --> tests/ui/legacy_numeric_constants.rs:53:9 + --> tests/ui/legacy_numeric_constants.rs:52:9 | LL | u8::max_value(); | ^^^^^^^^^^^ @@ -89,7 +89,7 @@ LL | u8::MAX; | ~~~ error: usage of a legacy numeric method - --> tests/ui/legacy_numeric_constants.rs:56:9 + --> tests/ui/legacy_numeric_constants.rs:55:9 | LL | u8::min_value(); | ^^^^^^^^^^^ @@ -100,7 +100,7 @@ LL | u8::MIN; | ~~~ error: usage of a legacy numeric constant - --> tests/ui/legacy_numeric_constants.rs:59:5 + --> tests/ui/legacy_numeric_constants.rs:58:5 | LL | ::std::u8::MIN; | ^^^^^^^^^^^^^^ @@ -111,7 +111,7 @@ LL | u8::MIN; | ~~~~~~~ error: usage of a legacy numeric method - --> tests/ui/legacy_numeric_constants.rs:62:27 + --> tests/ui/legacy_numeric_constants.rs:61:27 | LL | ::std::primitive::u8::min_value(); | ^^^^^^^^^^^ @@ -122,7 +122,7 @@ LL | ::std::primitive::u8::MIN; | ~~~ error: usage of a legacy numeric method - --> tests/ui/legacy_numeric_constants.rs:65:26 + --> tests/ui/legacy_numeric_constants.rs:64:26 | LL | std::primitive::i32::max_value(); | ^^^^^^^^^^^ @@ -133,7 +133,7 @@ LL | std::primitive::i32::MAX; | ~~~ error: usage of a legacy numeric constant - --> tests/ui/legacy_numeric_constants.rs:68:5 + --> tests/ui/legacy_numeric_constants.rs:67:5 | LL | self::a::u128::MAX; | ^^^^^^^^^^^^^^^^^^ @@ -144,7 +144,7 @@ LL | u128::MAX; | ~~~~~~~~~ error: usage of a legacy numeric constant - --> tests/ui/legacy_numeric_constants.rs:18:25 + --> tests/ui/legacy_numeric_constants.rs:17:25 | LL | let x = std::u64::MAX; | ^^^^^^^^^^^^^ @@ -159,7 +159,7 @@ LL | let x = u64::MAX; | ~~~~~~~~ error: usage of a legacy numeric constant - --> tests/ui/legacy_numeric_constants.rs:82:14 + --> tests/ui/legacy_numeric_constants.rs:81:14 | LL | [(0, "", std::i128::MAX)]; | ^^^^^^^^^^^^^^ @@ -170,7 +170,7 @@ LL | [(0, "", i128::MAX)]; | ~~~~~~~~~ error: usage of a legacy numeric constant - --> tests/ui/legacy_numeric_constants.rs:116:5 + --> tests/ui/legacy_numeric_constants.rs:115:5 | LL | std::u32::MAX; | ^^^^^^^^^^^^^ diff --git a/tests/ui/legacy_numeric_constants_unfixable.rs b/tests/ui/legacy_numeric_constants_unfixable.rs index 228ebdd6a6c5..86738ede210c 100644 --- a/tests/ui/legacy_numeric_constants_unfixable.rs +++ b/tests/ui/legacy_numeric_constants_unfixable.rs @@ -2,7 +2,6 @@ //@aux-build:proc_macros.rs #![allow(clippy::no_effect, deprecated, unused)] #![warn(clippy::legacy_numeric_constants)] -#![feature(lint_reasons)] #[macro_use] extern crate proc_macros; diff --git a/tests/ui/legacy_numeric_constants_unfixable.stderr b/tests/ui/legacy_numeric_constants_unfixable.stderr index 96e6c9ef9759..2edcf7188362 100644 --- a/tests/ui/legacy_numeric_constants_unfixable.stderr +++ b/tests/ui/legacy_numeric_constants_unfixable.stderr @@ -1,5 +1,5 @@ error: importing legacy numeric constants - --> tests/ui/legacy_numeric_constants_unfixable.rs:10:5 + --> tests/ui/legacy_numeric_constants_unfixable.rs:9:5 | LL | use std::u128 as _; | ^^^^^^^^^ @@ -9,7 +9,7 @@ LL | use std::u128 as _; = help: to override `-D warnings` add `#[allow(clippy::legacy_numeric_constants)]` error: importing legacy numeric constants - --> tests/ui/legacy_numeric_constants_unfixable.rs:14:24 + --> tests/ui/legacy_numeric_constants_unfixable.rs:13:24 | LL | pub use std::{mem, u128}; | ^^^^ @@ -18,7 +18,7 @@ LL | pub use std::{mem, u128}; = note: then `u128::` will resolve to the respective associated constant error: importing a legacy numeric constant - --> tests/ui/legacy_numeric_constants_unfixable.rs:30:9 + --> tests/ui/legacy_numeric_constants_unfixable.rs:29:9 | LL | use std::u32::MAX; | ^^^^^^^^^^^^^ @@ -26,7 +26,7 @@ LL | use std::u32::MAX; = help: remove this import and use the associated constant `u32::MAX` from the primitive type instead error: importing a legacy numeric constant - --> tests/ui/legacy_numeric_constants_unfixable.rs:33:9 + --> tests/ui/legacy_numeric_constants_unfixable.rs:32:9 | LL | use std::u8::MIN; | ^^^^^^^^^^^^ @@ -34,7 +34,7 @@ LL | use std::u8::MIN; = help: remove this import and use the associated constant `u8::MIN` from the primitive type instead error: importing legacy numeric constants - --> tests/ui/legacy_numeric_constants_unfixable.rs:37:9 + --> tests/ui/legacy_numeric_constants_unfixable.rs:36:9 | LL | use std::u32; | ^^^^^^^^ @@ -43,7 +43,7 @@ LL | use std::u32; = note: then `u32::` will resolve to the respective associated constant error: importing a legacy numeric constant - --> tests/ui/legacy_numeric_constants_unfixable.rs:41:9 + --> tests/ui/legacy_numeric_constants_unfixable.rs:40:9 | LL | use std::f32::MIN_POSITIVE; | ^^^^^^^^^^^^^^^^^^^^^^ @@ -51,7 +51,7 @@ LL | use std::f32::MIN_POSITIVE; = help: remove this import and use the associated constant `f32::MIN_POSITIVE` from the primitive type instead error: importing legacy numeric constants - --> tests/ui/legacy_numeric_constants_unfixable.rs:45:9 + --> tests/ui/legacy_numeric_constants_unfixable.rs:44:9 | LL | use std::i16::*; | ^^^^^^^^ @@ -59,7 +59,7 @@ LL | use std::i16::*; = help: remove this import and use associated constants `i16::` from the primitive type instead error: importing legacy numeric constants - --> tests/ui/legacy_numeric_constants_unfixable.rs:22:17 + --> tests/ui/legacy_numeric_constants_unfixable.rs:21:17 | LL | use std::u32; | ^^^^^^^^ @@ -72,7 +72,7 @@ LL | b!(); = note: this error originates in the macro `b` (in Nightly builds, run with -Z macro-backtrace for more info) error: importing a legacy numeric constant - --> tests/ui/legacy_numeric_constants_unfixable.rs:77:9 + --> tests/ui/legacy_numeric_constants_unfixable.rs:76:9 | LL | use std::u32::MAX; | ^^^^^^^^^^^^^ From 03ded2944f0d408c90df0eceed3f96d34bb54e6c Mon Sep 17 00:00:00 2001 From: y21 <30553356+y21@users.noreply.github.com> Date: Fri, 18 Aug 2023 09:25:18 +0200 Subject: [PATCH 17/22] [`type_id_on_box`]: lint of `Any` subtraits --- clippy_lints/src/methods/type_id_on_box.rs | 46 ++++++++++++++++------ tests/ui/type_id_on_box.fixed | 24 +++++++++++ tests/ui/type_id_on_box.rs | 24 +++++++++++ tests/ui/type_id_on_box.stderr | 31 ++++++++++----- 4 files changed, 103 insertions(+), 22 deletions(-) diff --git a/clippy_lints/src/methods/type_id_on_box.rs b/clippy_lints/src/methods/type_id_on_box.rs index 4917936a9322..cec5d3b2b6dd 100644 --- a/clippy_lints/src/methods/type_id_on_box.rs +++ b/clippy_lints/src/methods/type_id_on_box.rs @@ -1,3 +1,5 @@ +use std::borrow::Cow; + use crate::methods::TYPE_ID_ON_BOX; use clippy_utils::diagnostics::span_lint_and_then; use clippy_utils::source::snippet; @@ -5,17 +7,37 @@ use rustc_errors::Applicability; use rustc_hir::Expr; use rustc_lint::LateContext; use rustc_middle::ty::adjustment::{Adjust, Adjustment}; +use rustc_middle::ty::print::with_forced_trimmed_paths; use rustc_middle::ty::{self, ExistentialPredicate, Ty}; use rustc_span::{sym, Span}; -fn is_dyn_any(cx: &LateContext<'_>, ty: Ty<'_>) -> bool { +/// Checks if a [`Ty`] is a `dyn Any` or a `dyn Trait` where `Trait: Any` +/// and returns the name of the trait object. +fn is_dyn_any(cx: &LateContext<'_>, ty: Ty<'_>) -> Option> { if let ty::Dynamic(preds, ..) = ty.kind() { - preds.iter().any(|p| match p.skip_binder() { - ExistentialPredicate::Trait(tr) => cx.tcx.is_diagnostic_item(sym::Any, tr.def_id), - _ => false, + preds.iter().find_map(|p| match p.skip_binder() { + ExistentialPredicate::Trait(tr) => { + if cx.tcx.is_diagnostic_item(sym::Any, tr.def_id) { + Some(Cow::Borrowed("Any")) + } else if cx + .tcx + .super_predicates_of(tr.def_id) + .predicates + .iter() + .any(|(clause, _)| { + matches!(clause.kind().skip_binder(), ty::ClauseKind::Trait(super_tr) + if cx.tcx.is_diagnostic_item(sym::Any, super_tr.def_id())) + }) + { + Some(Cow::Owned(with_forced_trimmed_paths!(cx.tcx.def_path_str(tr.def_id)))) + } else { + None + } + }, + _ => None, }) } else { - false + None } } @@ -26,13 +48,13 @@ pub(super) fn check(cx: &LateContext<'_>, receiver: &Expr<'_>, call_span: Span) && let ty::Ref(_, ty, _) = recv_ty.kind() && let ty::Adt(adt, args) = ty.kind() && adt.is_box() - && is_dyn_any(cx, args.type_at(0)) + && let Some(trait_path) = is_dyn_any(cx, args.type_at(0)) { span_lint_and_then( cx, TYPE_ID_ON_BOX, call_span, - "calling `.type_id()` on a `Box`", + &format!("calling `.type_id()` on `Box`"), |diag| { let derefs = recv_adjusts .iter() @@ -43,13 +65,13 @@ pub(super) fn check(cx: &LateContext<'_>, receiver: &Expr<'_>, call_span: Span) sugg += &snippet(cx, receiver.span, ""); diag.note( - "this returns the type id of the literal type `Box` instead of the \ + "this returns the type id of the literal type `Box<_>` instead of the \ type id of the boxed value, which is most likely not what you want", ) - .note( - "if this is intentional, use `TypeId::of::>()` instead, \ - which makes it more clear", - ) + .note(format!( + "if this is intentional, use `TypeId::of::>()` instead, \ + which makes it more clear" + )) .span_suggestion( receiver.span, "consider dereferencing first", diff --git a/tests/ui/type_id_on_box.fixed b/tests/ui/type_id_on_box.fixed index 538c38b70e6b..bdc45a93e714 100644 --- a/tests/ui/type_id_on_box.fixed +++ b/tests/ui/type_id_on_box.fixed @@ -19,6 +19,21 @@ fn existential() -> impl Any { Box::new(1) as Box } +trait AnySubTrait: Any {} +impl AnySubTrait for T {} + +// `Any` is an indirect supertrait +trait AnySubSubTrait: AnySubTrait {} +impl AnySubSubTrait for T {} + +// This trait mentions `Any` in its predicates, but it is not a subtrait of `Any`. +trait NormalTrait +where + i32: Any, +{ +} +impl NormalTrait for T {} + fn main() { let any_box: Box = Box::new(0usize); let _ = (*any_box).type_id(); @@ -35,4 +50,13 @@ fn main() { let b = BadBox(Box::new(0usize)); let _ = b.type_id(); // Don't lint. This is a call to `::type_id`. Not `std::boxed::Box`! + + let b: Box = Box::new(1); + let _ = (*b).type_id(); // Lint if calling `type_id` on a `dyn Trait` where `Trait: Any` + + let b: Box = Box::new(1); + let _ = b.type_id(); // Known FN - Any is not an "immediate" supertrait + + let b: Box = Box::new(1); + let _ = b.type_id(); // `NormalTrait` does not have `Any` as its supertrait (even though it mentions it in `i32: Any`) } diff --git a/tests/ui/type_id_on_box.rs b/tests/ui/type_id_on_box.rs index f224d273bc23..e087d9989d89 100644 --- a/tests/ui/type_id_on_box.rs +++ b/tests/ui/type_id_on_box.rs @@ -19,6 +19,21 @@ fn existential() -> impl Any { Box::new(1) as Box } +trait AnySubTrait: Any {} +impl AnySubTrait for T {} + +// `Any` is an indirect supertrait +trait AnySubSubTrait: AnySubTrait {} +impl AnySubSubTrait for T {} + +// This trait mentions `Any` in its predicates, but it is not a subtrait of `Any`. +trait NormalTrait +where + i32: Any, +{ +} +impl NormalTrait for T {} + fn main() { let any_box: Box = Box::new(0usize); let _ = any_box.type_id(); @@ -35,4 +50,13 @@ fn main() { let b = BadBox(Box::new(0usize)); let _ = b.type_id(); // Don't lint. This is a call to `::type_id`. Not `std::boxed::Box`! + + let b: Box = Box::new(1); + let _ = b.type_id(); // Lint if calling `type_id` on a `dyn Trait` where `Trait: Any` + + let b: Box = Box::new(1); + let _ = b.type_id(); // Known FN - Any is not an "immediate" supertrait + + let b: Box = Box::new(1); + let _ = b.type_id(); // `NormalTrait` does not have `Any` as its supertrait (even though it mentions it in `i32: Any`) } diff --git a/tests/ui/type_id_on_box.stderr b/tests/ui/type_id_on_box.stderr index 0fce6a37c004..8edecc47c746 100644 --- a/tests/ui/type_id_on_box.stderr +++ b/tests/ui/type_id_on_box.stderr @@ -1,37 +1,48 @@ -error: calling `.type_id()` on a `Box` - --> tests/ui/type_id_on_box.rs:24:13 +error: calling `.type_id()` on `Box` + --> tests/ui/type_id_on_box.rs:39:13 | LL | let _ = any_box.type_id(); | -------^^^^^^^^^^ | | | help: consider dereferencing first: `(*any_box)` | - = note: this returns the type id of the literal type `Box` instead of the type id of the boxed value, which is most likely not what you want + = note: this returns the type id of the literal type `Box<_>` instead of the type id of the boxed value, which is most likely not what you want = note: if this is intentional, use `TypeId::of::>()` instead, which makes it more clear = note: `-D clippy::type-id-on-box` implied by `-D warnings` = help: to override `-D warnings` add `#[allow(clippy::type_id_on_box)]` -error: calling `.type_id()` on a `Box` - --> tests/ui/type_id_on_box.rs:28:13 +error: calling `.type_id()` on `Box` + --> tests/ui/type_id_on_box.rs:43:13 | LL | let _ = any_box.type_id(); // 2 derefs are needed here to get to the `dyn Any` | -------^^^^^^^^^^ | | | help: consider dereferencing first: `(**any_box)` | - = note: this returns the type id of the literal type `Box` instead of the type id of the boxed value, which is most likely not what you want + = note: this returns the type id of the literal type `Box<_>` instead of the type id of the boxed value, which is most likely not what you want = note: if this is intentional, use `TypeId::of::>()` instead, which makes it more clear -error: calling `.type_id()` on a `Box` - --> tests/ui/type_id_on_box.rs:34:13 +error: calling `.type_id()` on `Box` + --> tests/ui/type_id_on_box.rs:49:13 | LL | let _ = b.type_id(); | -^^^^^^^^^^ | | | help: consider dereferencing first: `(*b)` | - = note: this returns the type id of the literal type `Box` instead of the type id of the boxed value, which is most likely not what you want + = note: this returns the type id of the literal type `Box<_>` instead of the type id of the boxed value, which is most likely not what you want = note: if this is intentional, use `TypeId::of::>()` instead, which makes it more clear -error: aborting due to 3 previous errors +error: calling `.type_id()` on `Box` + --> tests/ui/type_id_on_box.rs:55:13 + | +LL | let _ = b.type_id(); // Lint if calling `type_id` on a `dyn Trait` where `Trait: Any` + | -^^^^^^^^^^ + | | + | help: consider dereferencing first: `(*b)` + | + = note: this returns the type id of the literal type `Box<_>` instead of the type id of the boxed value, which is most likely not what you want + = note: if this is intentional, use `TypeId::of::>()` instead, which makes it more clear + +error: aborting due to 4 previous errors From 542687ef5cc5b9e23e4957896a32c1b9c9f94b7c Mon Sep 17 00:00:00 2001 From: y21 <30553356+y21@users.noreply.github.com> Date: Sat, 30 Mar 2024 20:52:53 +0100 Subject: [PATCH 18/22] lint on any `Box`, but provide a suggestion for subtypes of `dyn Any` --- clippy_lints/src/methods/mod.rs | 21 ++++-- clippy_lints/src/methods/type_id_on_box.rs | 76 ++++++++++++---------- 2 files changed, 55 insertions(+), 42 deletions(-) diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index df5dad0423cf..3ad92a91ea75 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -3002,13 +3002,22 @@ declare_clippy_lint! { declare_clippy_lint! { /// ### What it does - /// Looks for calls to ` as Any>::type_id`. + /// Looks for calls to `.type_id()` on a `Box`. /// /// ### Why is this bad? - /// This most certainly does not do what the user expects and is very easy to miss. - /// Calling `type_id` on a `Box` calls `type_id` on the `Box<..>` itself, - /// so this will return the `TypeId` of the `Box` type (not the type id - /// of the value referenced by the box!). + /// This almost certainly does not do what the user expects and can lead to subtle bugs. + /// Calling `.type_id()` on a `Box` returns a fixed `TypeId` of the `Box` itself, + /// rather than returning the `TypeId` of the underlying type behind the trait object. + /// + /// For `Box` specifically (and trait objects that have `Any` as its supertrait), + /// this lint will provide a suggestion, which is to dereference the receiver explicitly + /// to go from `Box` to `dyn Any`. + /// This makes sure that `.type_id()` resolves to a dynamic call on the trait object + /// and not on the box. + /// + /// If the fixed `TypeId` of the `Box` is the intended behavior, it's better to be explicit about it + /// and write `TypeId::of::>()`: + /// this makes it clear that a fixed `TypeId` is returned and not the `TypeId` of the implementor. /// /// ### Example /// ```rust,ignore @@ -3028,7 +3037,7 @@ declare_clippy_lint! { #[clippy::version = "1.73.0"] pub TYPE_ID_ON_BOX, suspicious, - "calling `.type_id()` on `Box`" + "calling `.type_id()` on a boxed trait object" } declare_clippy_lint! { diff --git a/clippy_lints/src/methods/type_id_on_box.rs b/clippy_lints/src/methods/type_id_on_box.rs index cec5d3b2b6dd..31e6ccb950ad 100644 --- a/clippy_lints/src/methods/type_id_on_box.rs +++ b/clippy_lints/src/methods/type_id_on_box.rs @@ -1,5 +1,3 @@ -use std::borrow::Cow; - use crate::methods::TYPE_ID_ON_BOX; use clippy_utils::diagnostics::span_lint_and_then; use clippy_utils::source::snippet; @@ -11,33 +9,33 @@ use rustc_middle::ty::print::with_forced_trimmed_paths; use rustc_middle::ty::{self, ExistentialPredicate, Ty}; use rustc_span::{sym, Span}; -/// Checks if a [`Ty`] is a `dyn Any` or a `dyn Trait` where `Trait: Any` -/// and returns the name of the trait object. -fn is_dyn_any(cx: &LateContext<'_>, ty: Ty<'_>) -> Option> { +/// Checks if the given type is `dyn Any`, or a trait object that has `Any` as a supertrait. +/// Only in those cases will its vtable have a `type_id` method that returns the implementor's +/// `TypeId`, and only in those cases can we give a proper suggestion to dereference the box. +/// +/// If this returns false, then `.type_id()` likely (this may have FNs) will not be what the user +/// expects in any case and dereferencing it won't help either. It will likely require some +/// other changes, but it is still worth emitting a lint. +/// See for more details. +fn is_subtrait_of_any(cx: &LateContext<'_>, ty: Ty<'_>) -> bool { if let ty::Dynamic(preds, ..) = ty.kind() { - preds.iter().find_map(|p| match p.skip_binder() { + preds.iter().any(|p| match p.skip_binder() { ExistentialPredicate::Trait(tr) => { - if cx.tcx.is_diagnostic_item(sym::Any, tr.def_id) { - Some(Cow::Borrowed("Any")) - } else if cx - .tcx - .super_predicates_of(tr.def_id) - .predicates - .iter() - .any(|(clause, _)| { - matches!(clause.kind().skip_binder(), ty::ClauseKind::Trait(super_tr) + cx.tcx.is_diagnostic_item(sym::Any, tr.def_id) + || cx + .tcx + .super_predicates_of(tr.def_id) + .predicates + .iter() + .any(|(clause, _)| { + matches!(clause.kind().skip_binder(), ty::ClauseKind::Trait(super_tr) if cx.tcx.is_diagnostic_item(sym::Any, super_tr.def_id())) - }) - { - Some(Cow::Owned(with_forced_trimmed_paths!(cx.tcx.def_path_str(tr.def_id)))) - } else { - None - } + }) }, - _ => None, + _ => false, }) } else { - None + false } } @@ -48,36 +46,42 @@ pub(super) fn check(cx: &LateContext<'_>, receiver: &Expr<'_>, call_span: Span) && let ty::Ref(_, ty, _) = recv_ty.kind() && let ty::Adt(adt, args) = ty.kind() && adt.is_box() - && let Some(trait_path) = is_dyn_any(cx, args.type_at(0)) + && let inner_box_ty = args.type_at(0) + && let ty::Dynamic(..) = inner_box_ty.kind() { + let ty_name = with_forced_trimmed_paths!(ty.to_string()); + span_lint_and_then( cx, TYPE_ID_ON_BOX, call_span, - &format!("calling `.type_id()` on `Box`"), + &format!("calling `.type_id()` on `{ty_name}`"), |diag| { let derefs = recv_adjusts .iter() .filter(|adj| matches!(adj.kind, Adjust::Deref(None))) .count(); - let mut sugg = "*".repeat(derefs + 1); - sugg += &snippet(cx, receiver.span, ""); - diag.note( "this returns the type id of the literal type `Box<_>` instead of the \ type id of the boxed value, which is most likely not what you want", ) .note(format!( - "if this is intentional, use `TypeId::of::>()` instead, \ + "if this is intentional, use `TypeId::of::<{ty_name}>()` instead, \ which makes it more clear" - )) - .span_suggestion( - receiver.span, - "consider dereferencing first", - format!("({sugg})"), - Applicability::MaybeIncorrect, - ); + )); + + if is_subtrait_of_any(cx, inner_box_ty) { + let mut sugg = "*".repeat(derefs + 1); + sugg += &snippet(cx, receiver.span, ""); + + diag.span_suggestion( + receiver.span, + "consider dereferencing first", + format!("({sugg})"), + Applicability::MaybeIncorrect, + ); + } }, ); } From 56c91a47688bce29c7d61dc8c8ba78a10bfa9be8 Mon Sep 17 00:00:00 2001 From: y21 <30553356+y21@users.noreply.github.com> Date: Sat, 30 Mar 2024 20:54:16 +0100 Subject: [PATCH 19/22] split up tests into fixable and unfixable now and add annotations --- tests/ui/type_id_on_box.fixed | 42 ++++++++++-------------- tests/ui/type_id_on_box.rs | 42 ++++++++++-------------- tests/ui/type_id_on_box.stderr | 20 +++++------ tests/ui/type_id_on_box_unfixable.rs | 31 +++++++++++++++++ tests/ui/type_id_on_box_unfixable.stderr | 22 +++++++++++++ 5 files changed, 99 insertions(+), 58 deletions(-) create mode 100644 tests/ui/type_id_on_box_unfixable.rs create mode 100644 tests/ui/type_id_on_box_unfixable.stderr diff --git a/tests/ui/type_id_on_box.fixed b/tests/ui/type_id_on_box.fixed index bdc45a93e714..3656043700fa 100644 --- a/tests/ui/type_id_on_box.fixed +++ b/tests/ui/type_id_on_box.fixed @@ -22,41 +22,35 @@ fn existential() -> impl Any { trait AnySubTrait: Any {} impl AnySubTrait for T {} -// `Any` is an indirect supertrait -trait AnySubSubTrait: AnySubTrait {} -impl AnySubSubTrait for T {} - -// This trait mentions `Any` in its predicates, but it is not a subtrait of `Any`. -trait NormalTrait -where - i32: Any, -{ -} -impl NormalTrait for T {} - fn main() { + // Don't lint, calling `.type_id()` on a `&dyn Any` does the expected thing + let ref_dyn: &dyn Any = &42; + let _ = ref_dyn.type_id(); + let any_box: Box = Box::new(0usize); let _ = (*any_box).type_id(); - let _ = TypeId::of::>(); // Don't lint. We explicitly say "do this instead" if this is intentional + //~^ ERROR: calling `.type_id()` on + + // Don't lint. We explicitly say "do this instead" if this is intentional + let _ = TypeId::of::>(); let _ = (*any_box).type_id(); + + // 2 derefs are needed here to get to the `dyn Any` let any_box: &Box = &(Box::new(0usize) as Box); - let _ = (**any_box).type_id(); // 2 derefs are needed here to get to the `dyn Any` + let _ = (**any_box).type_id(); + //~^ ERROR: calling `.type_id()` on let b = existential(); - let _ = b.type_id(); // Don't lint. + let _ = b.type_id(); // Don't + + let b: Box = Box::new(1); + let _ = (*b).type_id(); + //~^ ERROR: calling `.type_id()` on let b: SomeBox = Box::new(0usize); let _ = (*b).type_id(); + //~^ ERROR: calling `.type_id()` on let b = BadBox(Box::new(0usize)); let _ = b.type_id(); // Don't lint. This is a call to `::type_id`. Not `std::boxed::Box`! - - let b: Box = Box::new(1); - let _ = (*b).type_id(); // Lint if calling `type_id` on a `dyn Trait` where `Trait: Any` - - let b: Box = Box::new(1); - let _ = b.type_id(); // Known FN - Any is not an "immediate" supertrait - - let b: Box = Box::new(1); - let _ = b.type_id(); // `NormalTrait` does not have `Any` as its supertrait (even though it mentions it in `i32: Any`) } diff --git a/tests/ui/type_id_on_box.rs b/tests/ui/type_id_on_box.rs index e087d9989d89..4bd9e73f2da0 100644 --- a/tests/ui/type_id_on_box.rs +++ b/tests/ui/type_id_on_box.rs @@ -22,41 +22,35 @@ fn existential() -> impl Any { trait AnySubTrait: Any {} impl AnySubTrait for T {} -// `Any` is an indirect supertrait -trait AnySubSubTrait: AnySubTrait {} -impl AnySubSubTrait for T {} - -// This trait mentions `Any` in its predicates, but it is not a subtrait of `Any`. -trait NormalTrait -where - i32: Any, -{ -} -impl NormalTrait for T {} - fn main() { + // Don't lint, calling `.type_id()` on a `&dyn Any` does the expected thing + let ref_dyn: &dyn Any = &42; + let _ = ref_dyn.type_id(); + let any_box: Box = Box::new(0usize); let _ = any_box.type_id(); - let _ = TypeId::of::>(); // Don't lint. We explicitly say "do this instead" if this is intentional + //~^ ERROR: calling `.type_id()` on + + // Don't lint. We explicitly say "do this instead" if this is intentional + let _ = TypeId::of::>(); let _ = (*any_box).type_id(); + + // 2 derefs are needed here to get to the `dyn Any` let any_box: &Box = &(Box::new(0usize) as Box); - let _ = any_box.type_id(); // 2 derefs are needed here to get to the `dyn Any` + let _ = any_box.type_id(); + //~^ ERROR: calling `.type_id()` on let b = existential(); - let _ = b.type_id(); // Don't lint. + let _ = b.type_id(); // Don't + + let b: Box = Box::new(1); + let _ = b.type_id(); + //~^ ERROR: calling `.type_id()` on let b: SomeBox = Box::new(0usize); let _ = b.type_id(); + //~^ ERROR: calling `.type_id()` on let b = BadBox(Box::new(0usize)); let _ = b.type_id(); // Don't lint. This is a call to `::type_id`. Not `std::boxed::Box`! - - let b: Box = Box::new(1); - let _ = b.type_id(); // Lint if calling `type_id` on a `dyn Trait` where `Trait: Any` - - let b: Box = Box::new(1); - let _ = b.type_id(); // Known FN - Any is not an "immediate" supertrait - - let b: Box = Box::new(1); - let _ = b.type_id(); // `NormalTrait` does not have `Any` as its supertrait (even though it mentions it in `i32: Any`) } diff --git a/tests/ui/type_id_on_box.stderr b/tests/ui/type_id_on_box.stderr index 8edecc47c746..4528195607da 100644 --- a/tests/ui/type_id_on_box.stderr +++ b/tests/ui/type_id_on_box.stderr @@ -1,5 +1,5 @@ error: calling `.type_id()` on `Box` - --> tests/ui/type_id_on_box.rs:39:13 + --> tests/ui/type_id_on_box.rs:31:13 | LL | let _ = any_box.type_id(); | -------^^^^^^^^^^ @@ -12,9 +12,9 @@ LL | let _ = any_box.type_id(); = help: to override `-D warnings` add `#[allow(clippy::type_id_on_box)]` error: calling `.type_id()` on `Box` - --> tests/ui/type_id_on_box.rs:43:13 + --> tests/ui/type_id_on_box.rs:40:13 | -LL | let _ = any_box.type_id(); // 2 derefs are needed here to get to the `dyn Any` +LL | let _ = any_box.type_id(); | -------^^^^^^^^^^ | | | help: consider dereferencing first: `(**any_box)` @@ -22,8 +22,8 @@ LL | let _ = any_box.type_id(); // 2 derefs are needed here to get to the `d = note: this returns the type id of the literal type `Box<_>` instead of the type id of the boxed value, which is most likely not what you want = note: if this is intentional, use `TypeId::of::>()` instead, which makes it more clear -error: calling `.type_id()` on `Box` - --> tests/ui/type_id_on_box.rs:49:13 +error: calling `.type_id()` on `Box` + --> tests/ui/type_id_on_box.rs:47:13 | LL | let _ = b.type_id(); | -^^^^^^^^^^ @@ -31,18 +31,18 @@ LL | let _ = b.type_id(); | help: consider dereferencing first: `(*b)` | = note: this returns the type id of the literal type `Box<_>` instead of the type id of the boxed value, which is most likely not what you want - = note: if this is intentional, use `TypeId::of::>()` instead, which makes it more clear + = note: if this is intentional, use `TypeId::of::>()` instead, which makes it more clear -error: calling `.type_id()` on `Box` - --> tests/ui/type_id_on_box.rs:55:13 +error: calling `.type_id()` on `Box` + --> tests/ui/type_id_on_box.rs:51:13 | -LL | let _ = b.type_id(); // Lint if calling `type_id` on a `dyn Trait` where `Trait: Any` +LL | let _ = b.type_id(); | -^^^^^^^^^^ | | | help: consider dereferencing first: `(*b)` | = note: this returns the type id of the literal type `Box<_>` instead of the type id of the boxed value, which is most likely not what you want - = note: if this is intentional, use `TypeId::of::>()` instead, which makes it more clear + = note: if this is intentional, use `TypeId::of::>()` instead, which makes it more clear error: aborting due to 4 previous errors diff --git a/tests/ui/type_id_on_box_unfixable.rs b/tests/ui/type_id_on_box_unfixable.rs new file mode 100644 index 000000000000..f6d09834adb1 --- /dev/null +++ b/tests/ui/type_id_on_box_unfixable.rs @@ -0,0 +1,31 @@ +#![warn(clippy::type_id_on_box)] + +use std::any::{Any, TypeId}; +use std::ops::Deref; + +trait AnySubTrait: Any {} +impl AnySubTrait for T {} + +// `Any` is an indirect supertrait +trait AnySubSubTrait: AnySubTrait {} +impl AnySubSubTrait for T {} + +// This trait mentions `Any` in its predicates, but it is not a subtrait of `Any`. +trait NormalTrait +where + i32: Any, +{ +} +impl NormalTrait for T {} + +fn main() { + // (currently we don't look deeper than one level into the supertrait hierachy, but we probably + // could) + let b: Box = Box::new(1); + let _ = b.type_id(); + //~^ ERROR: calling `.type_id()` on + + let b: Box = Box::new(1); + let _ = b.type_id(); + //~^ ERROR: calling `.type_id()` on +} diff --git a/tests/ui/type_id_on_box_unfixable.stderr b/tests/ui/type_id_on_box_unfixable.stderr new file mode 100644 index 000000000000..539ed481ec10 --- /dev/null +++ b/tests/ui/type_id_on_box_unfixable.stderr @@ -0,0 +1,22 @@ +error: calling `.type_id()` on `Box` + --> tests/ui/type_id_on_box_unfixable.rs:25:13 + | +LL | let _ = b.type_id(); + | ^^^^^^^^^^^ + | + = note: this returns the type id of the literal type `Box<_>` instead of the type id of the boxed value, which is most likely not what you want + = note: if this is intentional, use `TypeId::of::>()` instead, which makes it more clear + = note: `-D clippy::type-id-on-box` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(clippy::type_id_on_box)]` + +error: calling `.type_id()` on `Box` + --> tests/ui/type_id_on_box_unfixable.rs:29:13 + | +LL | let _ = b.type_id(); + | ^^^^^^^^^^^ + | + = note: this returns the type id of the literal type `Box<_>` instead of the type id of the boxed value, which is most likely not what you want + = note: if this is intentional, use `TypeId::of::>()` instead, which makes it more clear + +error: aborting due to 2 previous errors + From 0f72ce117aa0dcb7bf4ecbf4625affdb85013823 Mon Sep 17 00:00:00 2001 From: Quinn Sinclair Date: Mon, 25 Mar 2024 23:29:34 +0100 Subject: [PATCH 20/22] Allow `filter_map_identity` when the closure is typed This extends the `filter_map_identity` lint to support typed closures. For untyped closures, we know that the program compiles, and therefore we can safely suggest using flatten. For typed closures, they may participate in type resolution. In this case we use `Applicability::MaybeIncorrect`. Details: https://rust-lang.zulipchat.com/#narrow/stream/257328-clippy/topic/Should.20.60filter_map_identity.60.20lint.20when.20closures.20are.20typed.3F --- .../src/methods/filter_map_identity.rs | 18 ++- tests/ui/filter_map_identity.fixed | 86 +++++++++-- tests/ui/filter_map_identity.rs | 86 +++++++++-- tests/ui/filter_map_identity.stderr | 134 ++++++++++++++++-- 4 files changed, 288 insertions(+), 36 deletions(-) diff --git a/clippy_lints/src/methods/filter_map_identity.rs b/clippy_lints/src/methods/filter_map_identity.rs index 8291c373f371..15b11a731c98 100644 --- a/clippy_lints/src/methods/filter_map_identity.rs +++ b/clippy_lints/src/methods/filter_map_identity.rs @@ -1,5 +1,5 @@ use clippy_utils::diagnostics::span_lint_and_sugg; -use clippy_utils::{is_expr_untyped_identity_function, is_trait_method}; +use clippy_utils::{is_expr_identity_function, is_expr_untyped_identity_function, is_trait_method}; use rustc_errors::Applicability; use rustc_hir as hir; use rustc_lint::LateContext; @@ -7,8 +7,20 @@ use rustc_span::{sym, Span}; use super::FILTER_MAP_IDENTITY; +fn is_identity(cx: &LateContext<'_>, expr: &hir::Expr<'_>) -> Option { + if is_expr_untyped_identity_function(cx, expr) { + return Some(Applicability::MachineApplicable); + } + if is_expr_identity_function(cx, expr) { + return Some(Applicability::MaybeIncorrect); + } + None +} + pub(super) fn check(cx: &LateContext<'_>, expr: &hir::Expr<'_>, filter_map_arg: &hir::Expr<'_>, filter_map_span: Span) { - if is_trait_method(cx, expr, sym::Iterator) && is_expr_untyped_identity_function(cx, filter_map_arg) { + if is_trait_method(cx, expr, sym::Iterator) + && let Some(applicability) = is_identity(cx, filter_map_arg) + { span_lint_and_sugg( cx, FILTER_MAP_IDENTITY, @@ -16,7 +28,7 @@ pub(super) fn check(cx: &LateContext<'_>, expr: &hir::Expr<'_>, filter_map_arg: "use of `filter_map` with an identity function", "try", "flatten()".to_string(), - Applicability::MachineApplicable, + applicability, ); } } diff --git a/tests/ui/filter_map_identity.fixed b/tests/ui/filter_map_identity.fixed index ad438afaca77..f3f6848e5f92 100644 --- a/tests/ui/filter_map_identity.fixed +++ b/tests/ui/filter_map_identity.fixed @@ -1,17 +1,83 @@ -#![allow(unused_imports, clippy::needless_return)] +#![allow(unused_imports, clippy::needless_return, clippy::useless_vec)] #![warn(clippy::filter_map_identity)] +#![feature(stmt_expr_attributes)] + +use std::option::Option; +struct NonCopy; +use std::convert::identity; + +fn non_copy_vec() -> Vec> { + todo!() +} + +fn copy_vec() -> Vec> { + todo!() +} + +fn copy_vec_non_inferred() -> Vec> { + todo!() +} + +fn opaque() -> impl IntoIterator> { + vec![Some(T::default())] +} fn main() { - let iterator = vec![Some(1), None, Some(2)].into_iter(); - let _ = iterator.flatten(); + { + // into_iter + copy_vec_non_inferred().into_iter().flatten(); + //~^ ERROR: use of + copy_vec_non_inferred().into_iter().flatten(); + //~^ ERROR: use of + copy_vec_non_inferred().into_iter().flatten(); + //~^ ERROR: use of + copy_vec_non_inferred().into_iter().flatten(); + //~^ ERROR: use of + copy_vec_non_inferred().into_iter().flatten(); + //~^ ERROR: use of + + non_copy_vec().into_iter().flatten(); + //~^ ERROR: use of + non_copy_vec().into_iter().flatten(); + //~^ ERROR: use of + + non_copy_vec().into_iter().flatten(); + //~^ ERROR: use of + non_copy_vec().into_iter().flatten(); + //~^ ERROR: use of + non_copy_vec().into_iter().flatten(); + //~^ ERROR: use of + non_copy_vec().into_iter().flatten(); + //~^ ERROR: use of - let iterator = vec![Some(1), None, Some(2)].into_iter(); - let _ = iterator.flatten(); + copy_vec::().into_iter().flatten(); + //~^ ERROR: use of + copy_vec::().into_iter().flatten(); + //~^ ERROR: use of + copy_vec::().into_iter().flatten(); + //~^ ERROR: use of + copy_vec::().into_iter().flatten(); + //~^ ERROR: use of - use std::convert::identity; - let iterator = vec![Some(1), None, Some(2)].into_iter(); - let _ = iterator.flatten(); + // we are forced to pass the type in the call. + copy_vec::().into_iter().flatten(); + //~^ ERROR: use of + copy_vec::().into_iter().flatten(); + //~^ ERROR: use of + copy_vec::().into_iter().flatten(); + //~^ ERROR: use of + copy_vec::().into_iter().flatten(); + //~^ ERROR: use of + #[rustfmt::skip] + copy_vec::().into_iter().flatten(); + //~^ ERROR: use of + #[rustfmt::skip] + copy_vec::().into_iter().flatten(); + //~^ ERROR: use of - let iterator = vec![Some(1), None, Some(2)].into_iter(); - let _ = iterator.flatten(); + // note, the compiler requires that we pass the type to `opaque`. This is mostly for reference, + // it behaves the same as copy_vec. + opaque::().into_iter().flatten(); + //~^ ERROR: use of + } } diff --git a/tests/ui/filter_map_identity.rs b/tests/ui/filter_map_identity.rs index d7423276872a..b9aa9c05be89 100644 --- a/tests/ui/filter_map_identity.rs +++ b/tests/ui/filter_map_identity.rs @@ -1,17 +1,83 @@ -#![allow(unused_imports, clippy::needless_return)] +#![allow(unused_imports, clippy::needless_return, clippy::useless_vec)] #![warn(clippy::filter_map_identity)] +#![feature(stmt_expr_attributes)] + +use std::option::Option; +struct NonCopy; +use std::convert::identity; + +fn non_copy_vec() -> Vec> { + todo!() +} + +fn copy_vec() -> Vec> { + todo!() +} + +fn copy_vec_non_inferred() -> Vec> { + todo!() +} + +fn opaque() -> impl IntoIterator> { + vec![Some(T::default())] +} fn main() { - let iterator = vec![Some(1), None, Some(2)].into_iter(); - let _ = iterator.filter_map(|x| x); + { + // into_iter + copy_vec_non_inferred().into_iter().filter_map(|x| x); + //~^ ERROR: use of + copy_vec_non_inferred().into_iter().filter_map(std::convert::identity); + //~^ ERROR: use of + copy_vec_non_inferred().into_iter().filter_map(identity); + //~^ ERROR: use of + copy_vec_non_inferred().into_iter().filter_map(|x| return x); + //~^ ERROR: use of + copy_vec_non_inferred().into_iter().filter_map(|x| return x); + //~^ ERROR: use of + + non_copy_vec().into_iter().filter_map(|x| x); + //~^ ERROR: use of + non_copy_vec().into_iter().filter_map(|x| x); + //~^ ERROR: use of + + non_copy_vec().into_iter().filter_map(std::convert::identity); + //~^ ERROR: use of + non_copy_vec().into_iter().filter_map(identity); + //~^ ERROR: use of + non_copy_vec().into_iter().filter_map(|x| return x); + //~^ ERROR: use of + non_copy_vec().into_iter().filter_map(|x| return x); + //~^ ERROR: use of - let iterator = vec![Some(1), None, Some(2)].into_iter(); - let _ = iterator.filter_map(std::convert::identity); + copy_vec::().into_iter().filter_map(|x: Option<_>| x); + //~^ ERROR: use of + copy_vec::().into_iter().filter_map(|x: Option<_>| x); + //~^ ERROR: use of + copy_vec::().into_iter().filter_map(|x: Option<_>| return x); + //~^ ERROR: use of + copy_vec::().into_iter().filter_map(|x: Option<_>| return x); + //~^ ERROR: use of - use std::convert::identity; - let iterator = vec![Some(1), None, Some(2)].into_iter(); - let _ = iterator.filter_map(identity); + // we are forced to pass the type in the call. + copy_vec::().into_iter().filter_map(|x: Option| x); + //~^ ERROR: use of + copy_vec::().into_iter().filter_map(|x: Option| x); + //~^ ERROR: use of + copy_vec::().into_iter().filter_map(|x: Option| return x); + //~^ ERROR: use of + copy_vec::().into_iter().filter_map(|x: Option| return x); + //~^ ERROR: use of + #[rustfmt::skip] + copy_vec::().into_iter().filter_map(|x: Option| -> Option {{ x }}); + //~^ ERROR: use of + #[rustfmt::skip] + copy_vec::().into_iter().filter_map(|x: Option| -> Option {{ return x }}); + //~^ ERROR: use of - let iterator = vec![Some(1), None, Some(2)].into_iter(); - let _ = iterator.filter_map(|x| return x); + // note, the compiler requires that we pass the type to `opaque`. This is mostly for reference, + // it behaves the same as copy_vec. + opaque::().into_iter().filter_map(|x| x); + //~^ ERROR: use of + } } diff --git a/tests/ui/filter_map_identity.stderr b/tests/ui/filter_map_identity.stderr index 5aa46ad6d237..55068db4e9d0 100644 --- a/tests/ui/filter_map_identity.stderr +++ b/tests/ui/filter_map_identity.stderr @@ -1,29 +1,137 @@ error: use of `filter_map` with an identity function - --> tests/ui/filter_map_identity.rs:6:22 + --> tests/ui/filter_map_identity.rs:28:45 | -LL | let _ = iterator.filter_map(|x| x); - | ^^^^^^^^^^^^^^^^^ help: try: `flatten()` +LL | copy_vec_non_inferred().into_iter().filter_map(|x| x); + | ^^^^^^^^^^^^^^^^^ help: try: `flatten()` | = note: `-D clippy::filter-map-identity` implied by `-D warnings` = help: to override `-D warnings` add `#[allow(clippy::filter_map_identity)]` error: use of `filter_map` with an identity function - --> tests/ui/filter_map_identity.rs:9:22 + --> tests/ui/filter_map_identity.rs:30:45 | -LL | let _ = iterator.filter_map(std::convert::identity); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `flatten()` +LL | copy_vec_non_inferred().into_iter().filter_map(std::convert::identity); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `flatten()` error: use of `filter_map` with an identity function - --> tests/ui/filter_map_identity.rs:13:22 + --> tests/ui/filter_map_identity.rs:32:45 | -LL | let _ = iterator.filter_map(identity); - | ^^^^^^^^^^^^^^^^^^^^ help: try: `flatten()` +LL | copy_vec_non_inferred().into_iter().filter_map(identity); + | ^^^^^^^^^^^^^^^^^^^^ help: try: `flatten()` error: use of `filter_map` with an identity function - --> tests/ui/filter_map_identity.rs:16:22 + --> tests/ui/filter_map_identity.rs:34:45 | -LL | let _ = iterator.filter_map(|x| return x); - | ^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `flatten()` +LL | copy_vec_non_inferred().into_iter().filter_map(|x| return x); + | ^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `flatten()` -error: aborting due to 4 previous errors +error: use of `filter_map` with an identity function + --> tests/ui/filter_map_identity.rs:36:45 + | +LL | copy_vec_non_inferred().into_iter().filter_map(|x| return x); + | ^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `flatten()` + +error: use of `filter_map` with an identity function + --> tests/ui/filter_map_identity.rs:39:36 + | +LL | non_copy_vec().into_iter().filter_map(|x| x); + | ^^^^^^^^^^^^^^^^^ help: try: `flatten()` + +error: use of `filter_map` with an identity function + --> tests/ui/filter_map_identity.rs:41:36 + | +LL | non_copy_vec().into_iter().filter_map(|x| x); + | ^^^^^^^^^^^^^^^^^ help: try: `flatten()` + +error: use of `filter_map` with an identity function + --> tests/ui/filter_map_identity.rs:44:36 + | +LL | non_copy_vec().into_iter().filter_map(std::convert::identity); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `flatten()` + +error: use of `filter_map` with an identity function + --> tests/ui/filter_map_identity.rs:46:36 + | +LL | non_copy_vec().into_iter().filter_map(identity); + | ^^^^^^^^^^^^^^^^^^^^ help: try: `flatten()` + +error: use of `filter_map` with an identity function + --> tests/ui/filter_map_identity.rs:48:36 + | +LL | non_copy_vec().into_iter().filter_map(|x| return x); + | ^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `flatten()` + +error: use of `filter_map` with an identity function + --> tests/ui/filter_map_identity.rs:50:36 + | +LL | non_copy_vec().into_iter().filter_map(|x| return x); + | ^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `flatten()` + +error: use of `filter_map` with an identity function + --> tests/ui/filter_map_identity.rs:53:39 + | +LL | copy_vec::().into_iter().filter_map(|x: Option<_>| x); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `flatten()` + +error: use of `filter_map` with an identity function + --> tests/ui/filter_map_identity.rs:55:39 + | +LL | copy_vec::().into_iter().filter_map(|x: Option<_>| x); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `flatten()` + +error: use of `filter_map` with an identity function + --> tests/ui/filter_map_identity.rs:57:39 + | +LL | copy_vec::().into_iter().filter_map(|x: Option<_>| return x); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `flatten()` + +error: use of `filter_map` with an identity function + --> tests/ui/filter_map_identity.rs:59:39 + | +LL | copy_vec::().into_iter().filter_map(|x: Option<_>| return x); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `flatten()` + +error: use of `filter_map` with an identity function + --> tests/ui/filter_map_identity.rs:63:39 + | +LL | copy_vec::().into_iter().filter_map(|x: Option| x); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `flatten()` + +error: use of `filter_map` with an identity function + --> tests/ui/filter_map_identity.rs:65:39 + | +LL | copy_vec::().into_iter().filter_map(|x: Option| x); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `flatten()` + +error: use of `filter_map` with an identity function + --> tests/ui/filter_map_identity.rs:67:39 + | +LL | copy_vec::().into_iter().filter_map(|x: Option| return x); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `flatten()` + +error: use of `filter_map` with an identity function + --> tests/ui/filter_map_identity.rs:69:39 + | +LL | copy_vec::().into_iter().filter_map(|x: Option| return x); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `flatten()` + +error: use of `filter_map` with an identity function + --> tests/ui/filter_map_identity.rs:72:43 + | +LL | copy_vec::().into_iter().filter_map(|x: Option| -> Option {{ x }}); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `flatten()` + +error: use of `filter_map` with an identity function + --> tests/ui/filter_map_identity.rs:75:43 + | +LL | copy_vec::().into_iter().filter_map(|x: Option| -> Option {{ return x }}); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `flatten()` + +error: use of `filter_map` with an identity function + --> tests/ui/filter_map_identity.rs:80:37 + | +LL | opaque::().into_iter().filter_map(|x| x); + | ^^^^^^^^^^^^^^^^^ help: try: `flatten()` + +error: aborting due to 22 previous errors From 0201332ea3e82034553a5daa1e863532b5a7859b Mon Sep 17 00:00:00 2001 From: Quinn Sinclair Date: Tue, 26 Mar 2024 18:59:09 +0100 Subject: [PATCH 21/22] :adjust applicability for typed identity closures in `filter_map_identity` --- clippy_lints/src/methods/filter_map_identity.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clippy_lints/src/methods/filter_map_identity.rs b/clippy_lints/src/methods/filter_map_identity.rs index 15b11a731c98..999df875c753 100644 --- a/clippy_lints/src/methods/filter_map_identity.rs +++ b/clippy_lints/src/methods/filter_map_identity.rs @@ -12,7 +12,7 @@ fn is_identity(cx: &LateContext<'_>, expr: &hir::Expr<'_>) -> Option Date: Sun, 31 Mar 2024 13:22:04 +0000 Subject: [PATCH 22/22] Move box_default to style, do not suggest turbofishes `Box::default()` had its `#[rustc_box]` attribute removed in 1.69 so is no longer a perf related lint The lint is moved to style but no longer produces suggestions containing turbofishes, as they're often longer/more annoying to type --- clippy_lints/src/box_default.rs | 51 +++------------- tests/ui/box_default.fixed | 85 ++++++++++++++------------ tests/ui/box_default.rs | 75 +++++++++++++---------- tests/ui/box_default.stderr | 104 ++++++++------------------------ 4 files changed, 122 insertions(+), 193 deletions(-) diff --git a/clippy_lints/src/box_default.rs b/clippy_lints/src/box_default.rs index e83f5c2a74c8..c7b0006c8cbd 100644 --- a/clippy_lints/src/box_default.rs +++ b/clippy_lints/src/box_default.rs @@ -1,6 +1,5 @@ use clippy_utils::diagnostics::span_lint_and_sugg; use clippy_utils::macros::macro_backtrace; -use clippy_utils::source::snippet_opt; use clippy_utils::ty::expr_sig; use clippy_utils::{is_default_equivalent, path_def_id}; use rustc_errors::Applicability; @@ -9,20 +8,16 @@ use rustc_hir::intravisit::{walk_ty, Visitor}; use rustc_hir::{Block, Expr, ExprKind, Local, Node, QPath, Ty, TyKind}; use rustc_lint::{LateContext, LateLintPass, LintContext}; use rustc_middle::lint::in_external_macro; -use rustc_middle::ty::print::with_forced_trimmed_paths; -use rustc_middle::ty::IsSuggestable; use rustc_session::declare_lint_pass; use rustc_span::sym; declare_clippy_lint! { /// ### What it does - /// checks for `Box::new(T::default())`, which is better written as - /// `Box::::default()`. + /// checks for `Box::new(Default::default())`, which can be written as + /// `Box::default()`. /// /// ### Why is this bad? - /// First, it's more complex, involving two calls instead of one. - /// Second, `Box::default()` can be faster - /// [in certain cases](https://nnethercote.github.io/perf-book/standard-library-types.html#box). + /// `Box::default()` is equivalent and more concise. /// /// ### Example /// ```no_run @@ -34,7 +29,7 @@ declare_clippy_lint! { /// ``` #[clippy::version = "1.66.0"] pub BOX_DEFAULT, - perf, + style, "Using Box::new(T::default()) instead of Box::default()" } @@ -53,14 +48,14 @@ impl LateLintPass<'_> for BoxDefault { && path_def_id(cx, ty).map_or(false, |id| Some(id) == cx.tcx.lang_items().owned_box()) // And the single argument to the call is another function call // This is the `T::default()` of `Box::new(T::default())` - && let ExprKind::Call(arg_path, inner_call_args) = arg.kind + && let ExprKind::Call(arg_path, _) = arg.kind // And we are not in a foreign crate's macro && !in_external_macro(cx.sess(), expr.span) // And the argument expression has the same context as the outer call expression // or that we are inside a `vec!` macro expansion && (expr.span.eq_ctxt(arg.span) || is_local_vec_expn(cx, arg, expr)) - // And the argument is equivalent to `Default::default()` - && is_default_equivalent(cx, arg) + // And the argument is `Default::default()` or the type is specified + && (is_plain_default(cx, arg_path) || (given_type(cx, expr) && is_default_equivalent(cx, arg))) { span_lint_and_sugg( cx, @@ -68,23 +63,7 @@ impl LateLintPass<'_> for BoxDefault { expr.span, "`Box::new(_)` of default value", "try", - if is_plain_default(cx, arg_path) || given_type(cx, expr) { - "Box::default()".into() - } else if let Some(arg_ty) = cx.typeck_results().expr_ty(arg).make_suggestable(cx.tcx, true) { - // Check if we can copy from the source expression in the replacement. - // We need the call to have no argument (see `explicit_default_type`). - if inner_call_args.is_empty() - && let Some(ty) = explicit_default_type(arg_path) - && let Some(s) = snippet_opt(cx, ty.span) - { - format!("Box::<{s}>::default()") - } else { - // Otherwise, use the inferred type's formatting. - with_forced_trimmed_paths!(format!("Box::<{arg_ty}>::default()")) - } - } else { - return; - }, + "Box::default()".into(), Applicability::MachineApplicable, ); } @@ -103,20 +82,6 @@ fn is_plain_default(cx: &LateContext<'_>, arg_path: &Expr<'_>) -> bool { } } -// Checks whether the call is of the form `A::B::f()`. Returns `A::B` if it is. -// -// In the event we have this kind of construct, it's easy to use `A::B` as a replacement in the -// quickfix. `f` must however have no parameter. Should `f` have some, then some of the type of -// `A::B` may be inferred from the arguments. This would be the case for `Vec::from([0; false])`, -// where the argument to `from` allows inferring this is a `Vec` -fn explicit_default_type<'a>(arg_path: &'a Expr<'_>) -> Option<&'a Ty<'a>> { - if let ExprKind::Path(QPath::TypeRelative(ty, _)) = &arg_path.kind { - Some(ty) - } else { - None - } -} - fn is_local_vec_expn(cx: &LateContext<'_>, expr: &Expr<'_>, ref_expr: &Expr<'_>) -> bool { macro_backtrace(expr.span).next().map_or(false, |call| { cx.tcx.is_diagnostic_item(sym::vec_macro, call.def_id) && call.span.eq_ctxt(ref_expr.span) diff --git a/tests/ui/box_default.fixed b/tests/ui/box_default.fixed index fea7405c6858..6c2896b3aa0f 100644 --- a/tests/ui/box_default.fixed +++ b/tests/ui/box_default.fixed @@ -1,5 +1,5 @@ #![warn(clippy::box_default)] -#![allow(clippy::default_constructed_unit_structs)] +#![allow(clippy::boxed_local, clippy::default_constructed_unit_structs)] #[derive(Default)] struct ImplementsDefault; @@ -12,26 +12,50 @@ impl OwnDefault { } } -macro_rules! outer { - ($e: expr) => { - $e +macro_rules! default { + () => { + Default::default() + }; +} + +macro_rules! string_new { + () => { + String::new() + }; +} + +macro_rules! box_new { + ($e:expr) => { + Box::new($e) }; } fn main() { - let _string: Box = Box::default(); - let _byte = Box::::default(); - let _vec = Box::>::default(); - let _impl = Box::::default(); - let _impl2 = Box::::default(); - let _impl3: Box = Box::default(); - let _own = Box::new(OwnDefault::default()); // should not lint - let _in_macro = outer!(Box::::default()); - let _string_default = outer!(Box::::default()); - let _vec2: Box> = Box::default(); - let _vec3: Box> = Box::default(); - let _vec4: Box<_> = Box::>::default(); - let _more = ret_ty_fn(); + let string1: Box = Box::default(); + let string2: Box = Box::default(); + let impl1: Box = Box::default(); + let vec: Box> = Box::default(); + let byte: Box = Box::default(); + let vec2: Box> = Box::default(); + let vec3: Box> = Box::default(); + + let plain_default = Box::default(); + let _: Box = plain_default; + + let _: Box = Box::new(default!()); + let _: Box = Box::new(string_new!()); + let _: Box = box_new!(Default::default()); + let _: Box = box_new!(String::new()); + let _: Box = box_new!(default!()); + let _: Box = box_new!(string_new!()); + + let own: Box = Box::new(OwnDefault::default()); // should not lint + + // Do not suggest where a turbofish would be required + let impl2 = Box::new(ImplementsDefault::default()); + let impl3 = Box::new(::default()); + let vec4: Box<_> = Box::new(Vec::from([false; 0])); + let more = ret_ty_fn(); call_ty_fn(Box::default()); issue_10381(); @@ -44,10 +68,9 @@ fn main() { } fn ret_ty_fn() -> Box { - Box::::default() + Box::new(bool::default()) // Could lint, currently doesn't } -#[allow(clippy::boxed_local)] fn call_ty_fn(_b: Box) { issue_9621_dyn_trait(); } @@ -61,7 +84,7 @@ impl Read for ImplementsDefault { } fn issue_9621_dyn_trait() { - let _: Box = Box::::default(); + let _: Box = Box::new(ImplementsDefault::default()); issue_10089(); } @@ -70,7 +93,7 @@ fn issue_10089() { #[derive(Default)] struct WeirdPathed; - let _ = Box::::default(); + let _ = Box::new(WeirdPathed::default()); }; } @@ -82,7 +105,7 @@ fn issue_10381() { fn maybe_get_bar(i: u32) -> Option> { if i % 2 == 0 { - Some(Box::::default()) + Some(Box::new(Foo::default())) } else { None } @@ -91,20 +114,6 @@ fn issue_10381() { assert!(maybe_get_bar(2).is_some()); } -#[allow(unused)] -fn issue_11868() { - fn foo(_: &mut Vec) {} - - macro_rules! bar { - ($baz:expr) => { - Box::leak(Box::new($baz)) - }; - } - - foo(bar!(vec![])); - foo(bar!(vec![1])); -} - // Issue #11927: The quickfix for the `Box::new` suggests replacing with `Box::::default()`, // removing the `outer::` segment. fn issue_11927() { @@ -116,7 +125,7 @@ fn issue_11927() { } fn foo() { - let _b = Box::::default(); - let _b = Box::>::default(); + let _b = Box::new(outer::Inner::default()); + let _b = Box::new(std::collections::HashSet::::new()); } } diff --git a/tests/ui/box_default.rs b/tests/ui/box_default.rs index eecba7464ec2..e19a62a90221 100644 --- a/tests/ui/box_default.rs +++ b/tests/ui/box_default.rs @@ -1,5 +1,5 @@ #![warn(clippy::box_default)] -#![allow(clippy::default_constructed_unit_structs)] +#![allow(clippy::boxed_local, clippy::default_constructed_unit_structs)] #[derive(Default)] struct ImplementsDefault; @@ -12,26 +12,50 @@ impl OwnDefault { } } -macro_rules! outer { - ($e: expr) => { - $e +macro_rules! default { + () => { + Default::default() + }; +} + +macro_rules! string_new { + () => { + String::new() + }; +} + +macro_rules! box_new { + ($e:expr) => { + Box::new($e) }; } fn main() { - let _string: Box = Box::new(Default::default()); - let _byte = Box::new(u8::default()); - let _vec = Box::new(Vec::::new()); - let _impl = Box::new(ImplementsDefault::default()); - let _impl2 = Box::new(::default()); - let _impl3: Box = Box::new(Default::default()); - let _own = Box::new(OwnDefault::default()); // should not lint - let _in_macro = outer!(Box::new(String::new())); - let _string_default = outer!(Box::new(String::from(""))); - let _vec2: Box> = Box::new(vec![]); - let _vec3: Box> = Box::new(Vec::from([])); - let _vec4: Box<_> = Box::new(Vec::from([false; 0])); - let _more = ret_ty_fn(); + let string1: Box = Box::new(Default::default()); + let string2: Box = Box::new(String::new()); + let impl1: Box = Box::new(Default::default()); + let vec: Box> = Box::new(Vec::new()); + let byte: Box = Box::new(u8::default()); + let vec2: Box> = Box::new(vec![]); + let vec3: Box> = Box::new(Vec::from([])); + + let plain_default = Box::new(Default::default()); + let _: Box = plain_default; + + let _: Box = Box::new(default!()); + let _: Box = Box::new(string_new!()); + let _: Box = box_new!(Default::default()); + let _: Box = box_new!(String::new()); + let _: Box = box_new!(default!()); + let _: Box = box_new!(string_new!()); + + let own: Box = Box::new(OwnDefault::default()); // should not lint + + // Do not suggest where a turbofish would be required + let impl2 = Box::new(ImplementsDefault::default()); + let impl3 = Box::new(::default()); + let vec4: Box<_> = Box::new(Vec::from([false; 0])); + let more = ret_ty_fn(); call_ty_fn(Box::new(u8::default())); issue_10381(); @@ -44,10 +68,9 @@ fn main() { } fn ret_ty_fn() -> Box { - Box::new(bool::default()) + Box::new(bool::default()) // Could lint, currently doesn't } -#[allow(clippy::boxed_local)] fn call_ty_fn(_b: Box) { issue_9621_dyn_trait(); } @@ -91,20 +114,6 @@ fn issue_10381() { assert!(maybe_get_bar(2).is_some()); } -#[allow(unused)] -fn issue_11868() { - fn foo(_: &mut Vec) {} - - macro_rules! bar { - ($baz:expr) => { - Box::leak(Box::new($baz)) - }; - } - - foo(bar!(vec![])); - foo(bar!(vec![1])); -} - // Issue #11927: The quickfix for the `Box::new` suggests replacing with `Box::::default()`, // removing the `outer::` segment. fn issue_11927() { diff --git a/tests/ui/box_default.stderr b/tests/ui/box_default.stderr index 8bb5917a627b..f172a875dce4 100644 --- a/tests/ui/box_default.stderr +++ b/tests/ui/box_default.stderr @@ -1,113 +1,59 @@ error: `Box::new(_)` of default value - --> tests/ui/box_default.rs:22:32 + --> tests/ui/box_default.rs:34:32 | -LL | let _string: Box = Box::new(Default::default()); +LL | let string1: Box = Box::new(Default::default()); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `Box::default()` | = note: `-D clippy::box-default` implied by `-D warnings` = help: to override `-D warnings` add `#[allow(clippy::box_default)]` error: `Box::new(_)` of default value - --> tests/ui/box_default.rs:23:17 + --> tests/ui/box_default.rs:35:32 | -LL | let _byte = Box::new(u8::default()); - | ^^^^^^^^^^^^^^^^^^^^^^^ help: try: `Box::::default()` +LL | let string2: Box = Box::new(String::new()); + | ^^^^^^^^^^^^^^^^^^^^^^^ help: try: `Box::default()` error: `Box::new(_)` of default value - --> tests/ui/box_default.rs:24:16 + --> tests/ui/box_default.rs:36:41 | -LL | let _vec = Box::new(Vec::::new()); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `Box::>::default()` +LL | let impl1: Box = Box::new(Default::default()); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `Box::default()` error: `Box::new(_)` of default value - --> tests/ui/box_default.rs:25:17 + --> tests/ui/box_default.rs:37:29 | -LL | let _impl = Box::new(ImplementsDefault::default()); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `Box::::default()` +LL | let vec: Box> = Box::new(Vec::new()); + | ^^^^^^^^^^^^^^^^^^^^ help: try: `Box::default()` error: `Box::new(_)` of default value - --> tests/ui/box_default.rs:26:18 + --> tests/ui/box_default.rs:38:25 | -LL | let _impl2 = Box::new(::default()); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `Box::::default()` +LL | let byte: Box = Box::new(u8::default()); + | ^^^^^^^^^^^^^^^^^^^^^^^ help: try: `Box::default()` error: `Box::new(_)` of default value - --> tests/ui/box_default.rs:27:42 + --> tests/ui/box_default.rs:39:45 | -LL | let _impl3: Box = Box::new(Default::default()); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `Box::default()` +LL | let vec2: Box> = Box::new(vec![]); + | ^^^^^^^^^^^^^^^^ help: try: `Box::default()` error: `Box::new(_)` of default value - --> tests/ui/box_default.rs:29:28 + --> tests/ui/box_default.rs:40:32 | -LL | let _in_macro = outer!(Box::new(String::new())); - | ^^^^^^^^^^^^^^^^^^^^^^^ help: try: `Box::::default()` +LL | let vec3: Box> = Box::new(Vec::from([])); + | ^^^^^^^^^^^^^^^^^^^^^^^ help: try: `Box::default()` error: `Box::new(_)` of default value - --> tests/ui/box_default.rs:30:34 + --> tests/ui/box_default.rs:42:25 | -LL | let _string_default = outer!(Box::new(String::from(""))); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `Box::::default()` +LL | let plain_default = Box::new(Default::default()); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `Box::default()` error: `Box::new(_)` of default value - --> tests/ui/box_default.rs:31:46 - | -LL | let _vec2: Box> = Box::new(vec![]); - | ^^^^^^^^^^^^^^^^ help: try: `Box::default()` - -error: `Box::new(_)` of default value - --> tests/ui/box_default.rs:32:33 - | -LL | let _vec3: Box> = Box::new(Vec::from([])); - | ^^^^^^^^^^^^^^^^^^^^^^^ help: try: `Box::default()` - -error: `Box::new(_)` of default value - --> tests/ui/box_default.rs:33:25 - | -LL | let _vec4: Box<_> = Box::new(Vec::from([false; 0])); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `Box::>::default()` - -error: `Box::new(_)` of default value - --> tests/ui/box_default.rs:35:16 + --> tests/ui/box_default.rs:59:16 | LL | call_ty_fn(Box::new(u8::default())); | ^^^^^^^^^^^^^^^^^^^^^^^ help: try: `Box::default()` -error: `Box::new(_)` of default value - --> tests/ui/box_default.rs:47:5 - | -LL | Box::new(bool::default()) - | ^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `Box::::default()` - -error: `Box::new(_)` of default value - --> tests/ui/box_default.rs:64:28 - | -LL | let _: Box = Box::new(ImplementsDefault::default()); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `Box::::default()` - -error: `Box::new(_)` of default value - --> tests/ui/box_default.rs:73:17 - | -LL | let _ = Box::new(WeirdPathed::default()); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `Box::::default()` - -error: `Box::new(_)` of default value - --> tests/ui/box_default.rs:85:18 - | -LL | Some(Box::new(Foo::default())) - | ^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `Box::::default()` - -error: `Box::new(_)` of default value - --> tests/ui/box_default.rs:119:18 - | -LL | let _b = Box::new(outer::Inner::default()); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `Box::::default()` - -error: `Box::new(_)` of default value - --> tests/ui/box_default.rs:120:18 - | -LL | let _b = Box::new(std::collections::HashSet::::new()); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `Box::>::default()` - -error: aborting due to 18 previous errors +error: aborting due to 9 previous errors