Skip to content

fix(or_fun_call): respect MSRV for unwrap_or_default suggestion #14885

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -46,3 +46,8 @@ helper.txt

# mdbook generated output
/book/book
.cursorrules
.cursor/rules/data-flow-analysis.mdc
.giga/specifications.json
.cursor/rules
.cursor/rules/lint-implementation-patterns.mdc
4 changes: 2 additions & 2 deletions clippy_lints/src/methods/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4412,7 +4412,7 @@ declare_clippy_lint! {
/// Checks for calls to `Read::bytes` on types which don't implement `BufRead`.
///
/// ### Why is this bad?
/// The default implementation calls `read` for each byte, which can be very inefficient for data thats not in memory, such as `File`.
/// The default implementation calls `read` for each byte, which can be very inefficient for data that's not in memory, such as `File`.
///
/// ### Example
/// ```no_run
Expand Down Expand Up @@ -4741,7 +4741,7 @@ impl<'tcx> LateLintPass<'tcx> for Methods {
},
ExprKind::MethodCall(method_call, receiver, args, _) => {
let method_span = method_call.ident.span;
or_fun_call::check(cx, expr, method_span, method_call.ident.name, receiver, args);
or_fun_call::check(cx, expr, method_span, method_call.ident.name, receiver, args, self.msrv);
expect_fun_call::check(
cx,
&self.format_args,
Expand Down
19 changes: 15 additions & 4 deletions clippy_lints/src/methods/or_fun_call.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
#![allow(clippy::too_many_arguments)]
use std::ops::ControlFlow;

use clippy_utils::diagnostics::span_lint_and_sugg;
use clippy_utils::eager_or_lazy::switch_to_lazy_eval;
use clippy_utils::msrvs::{self, Msrv};
use clippy_utils::source::snippet_with_context;
use clippy_utils::ty::{expr_type_is_certain, implements_trait, is_type_diagnostic_item};
use clippy_utils::visitors::for_each_expr;
Expand All @@ -26,11 +28,13 @@ pub(super) fn check<'tcx>(
name: Symbol,
receiver: &'tcx hir::Expr<'_>,
args: &'tcx [hir::Expr<'_>],
msrv: Msrv,
Copy link
Member

Choose a reason for hiding this comment

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

If the lint now checks MSRV, it must be added to the list in the clippy_conf/src/conf.rs and the tests reblessed to include the new metadata.

) {
/// Checks for `unwrap_or(T::new())`, `unwrap_or(T::default())`,
/// `or_insert(T::new())` or `or_insert(T::default())`.
/// Similarly checks for `unwrap_or_else(T::new)`, `unwrap_or_else(T::default)`,
/// `or_insert_with(T::new)` or `or_insert_with(T::default)`.
#[allow(clippy::too_many_arguments)]
fn check_unwrap_or_default(
cx: &LateContext<'_>,
name: Symbol,
Expand All @@ -39,7 +43,14 @@ pub(super) fn check<'tcx>(
call_expr: Option<&hir::Expr<'_>>,
span: Span,
method_span: Span,
msrv: Msrv,
) -> bool {
// Only check MSRV for Result type
let receiver_ty = cx.typeck_results().expr_ty_adjusted(receiver).peel_refs();
if is_type_diagnostic_item(cx, receiver_ty, sym::Result) && !msrv.meets(cx, msrvs::RESULT_UNWRAP_OR_DEFAULT) {
return false;
}

if !expr_type_is_certain(cx, receiver) {
return false;
}
Expand All @@ -55,8 +66,8 @@ pub(super) fn check<'tcx>(

let output_type_implements_default = |fun| {
let fun_ty = cx.typeck_results().expr_ty(fun);
if let ty::FnDef(def_id, args) = fun_ty.kind() {
let output_ty = cx.tcx.fn_sig(def_id).instantiate(cx.tcx, args).skip_binder().output();
if let ty::FnDef(def_id, substs) = fun_ty.kind() {
let output_ty = cx.tcx.fn_sig(def_id).instantiate(cx.tcx, substs).skip_binder().output();
Comment on lines +69 to +70
Copy link
Member

Choose a reason for hiding this comment

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

Why the gratuitous name change? What was wrong with args, since there are the generic arguments of the FnDef?

cx.tcx
.get_diagnostic_item(sym::Default)
.is_some_and(|default_trait_id| implements_trait(cx, output_ty, default_trait_id, &[]))
Expand Down Expand Up @@ -215,11 +226,11 @@ pub(super) fn check<'tcx>(
};
(!inner_fun_has_args
&& !is_nested_expr
&& check_unwrap_or_default(cx, name, receiver, fun, Some(ex), expr.span, method_span))
&& check_unwrap_or_default(cx, name, receiver, fun, Some(ex), expr.span, method_span, msrv))
|| check_or_fn_call(cx, name, method_span, receiver, arg, None, expr.span, fun_span)
},
hir::ExprKind::Path(..) | hir::ExprKind::Closure(..) if !is_nested_expr => {
check_unwrap_or_default(cx, name, receiver, ex, None, expr.span, method_span)
check_unwrap_or_default(cx, name, receiver, ex, None, expr.span, method_span, msrv)
},
hir::ExprKind::Index(..) | hir::ExprKind::MethodCall(..) => {
check_or_fn_call(cx, name, method_span, receiver, arg, None, expr.span, None)
Expand Down
2 changes: 1 addition & 1 deletion clippy_utils/src/msrvs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ msrv_aliases! {
1,24,0 { IS_ASCII_DIGIT }
1,18,0 { HASH_MAP_RETAIN, HASH_SET_RETAIN }
1,17,0 { FIELD_INIT_SHORTHAND, STATIC_IN_CONST, EXPECT_ERR }
1,16,0 { STR_REPEAT }
1,16,0 { STR_REPEAT, RESULT_UNWRAP_OR_DEFAULT }
1,15,0 { MAYBE_BOUND_IN_WHERE }
1,13,0 { QUESTION_MARK_OPERATOR }
}
Expand Down
30 changes: 30 additions & 0 deletions tests/ui/or_fun_call.rs
Original file line number Diff line number Diff line change
Expand Up @@ -409,4 +409,34 @@ fn fn_call_in_nested_expr() {
//~^ or_fun_call
}

mod msrv_tests {
#[clippy::msrv = "1.15"]
fn test_option_below_msrv() {
let opt = Some(1);
let _ = opt.unwrap_or(0);
//~^ unwrap_or_default
}

#[clippy::msrv = "1.15"]
fn test_result_below_msrv() {
let res: Result<i32, ()> = Ok(1);
let _ = res.unwrap_or(0);
//~^ or_fun_call
}

#[clippy::msrv = "1.16"]
fn test_option_above_msrv() {
let opt = Some(1);
let _ = opt.unwrap_or(0);
//~^ unwrap_or_default
}

#[clippy::msrv = "1.16"]
fn test_result_above_msrv() {
let res: Result<i32, ()> = Ok(1);
let _ = res.unwrap_or(0);
//~^ unwrap_or_default
}
}

fn main() {}
Loading