Skip to content

Commit de4f8e2

Browse files
committed
fix: manual_ok_err suggests wrongly with references
1 parent 4ef7529 commit de4f8e2

File tree

4 files changed

+108
-4
lines changed

4 files changed

+108
-4
lines changed

clippy_lints/src/matches/manual_ok_err.rs

Lines changed: 17 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,9 @@
11
use clippy_utils::diagnostics::span_lint_and_sugg;
22
use clippy_utils::source::{indent_of, reindent_multiline};
33
use clippy_utils::sugg::Sugg;
4-
use clippy_utils::ty::option_arg_ty;
4+
use clippy_utils::ty::{option_arg_ty, peel_mid_ty_refs_is_mutable};
55
use clippy_utils::{get_parent_expr, is_res_lang_ctor, path_res, peel_blocks, span_contains_comment};
6-
use rustc_ast::BindingMode;
6+
use rustc_ast::{BindingMode, Mutability};
77
use rustc_errors::Applicability;
88
use rustc_hir::LangItem::{OptionNone, OptionSome, ResultErr};
99
use rustc_hir::def::{DefKind, Res};
@@ -133,7 +133,21 @@ fn apply_lint(cx: &LateContext<'_>, expr: &Expr<'_>, scrutinee: &Expr<'_>, is_ok
133133
Applicability::MachineApplicable
134134
};
135135
let scrut = Sugg::hir_with_applicability(cx, scrutinee, "..", &mut app).maybe_paren();
136-
let sugg = format!("{scrut}.{method}()");
136+
137+
let scrutinee_ty = cx.typeck_results().expr_ty(scrutinee);
138+
let (_, n_ref, mutability) = peel_mid_ty_refs_is_mutable(scrutinee_ty);
139+
let prefix = if n_ref > 0 {
140+
if mutability == Mutability::Mut {
141+
".as_mut()"
142+
} else {
143+
".as_ref()"
144+
}
145+
} else {
146+
""
147+
};
148+
149+
let sugg = format!("{scrut}{prefix}.{method}()");
150+
137151
// If the expression being expanded is the `if …` part of an `else if …`, it must be blockified.
138152
let sugg = if let Some(parent_expr) = get_parent_expr(cx, expr)
139153
&& let ExprKind::If(_, _, Some(else_part)) = parent_expr.kind

tests/ui/manual_ok_err.fixed

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -103,3 +103,27 @@ fn issue14239() {
103103
};
104104
//~^^^^^ manual_ok_err
105105
}
106+
107+
mod issue15051 {
108+
struct Container {
109+
field: Result<bool, ()>,
110+
}
111+
112+
#[allow(clippy::needless_borrow)]
113+
fn with_addr_of(x: &Container) -> Option<&bool> {
114+
(&x.field).as_ref().ok()
115+
}
116+
117+
fn from_fn(x: &Container) -> Option<&bool> {
118+
let result_with_ref = || &x.field;
119+
result_with_ref().as_ref().ok()
120+
}
121+
122+
fn result_with_ref_mut(x: &mut Container) -> &mut Result<bool, ()> {
123+
&mut x.field
124+
}
125+
126+
fn from_fn_mut(x: &mut Container) -> Option<&mut bool> {
127+
result_with_ref_mut(x).as_mut().ok()
128+
}
129+
}

tests/ui/manual_ok_err.rs

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -141,3 +141,39 @@ fn issue14239() {
141141
};
142142
//~^^^^^ manual_ok_err
143143
}
144+
145+
mod issue15051 {
146+
struct Container {
147+
field: Result<bool, ()>,
148+
}
149+
150+
#[allow(clippy::needless_borrow)]
151+
fn with_addr_of(x: &Container) -> Option<&bool> {
152+
match &x.field {
153+
//~^ manual_ok_err
154+
Ok(panel) => Some(panel),
155+
Err(_) => None,
156+
}
157+
}
158+
159+
fn from_fn(x: &Container) -> Option<&bool> {
160+
let result_with_ref = || &x.field;
161+
match result_with_ref() {
162+
//~^ manual_ok_err
163+
Ok(panel) => Some(panel),
164+
Err(_) => None,
165+
}
166+
}
167+
168+
fn result_with_ref_mut(x: &mut Container) -> &mut Result<bool, ()> {
169+
&mut x.field
170+
}
171+
172+
fn from_fn_mut(x: &mut Container) -> Option<&mut bool> {
173+
match result_with_ref_mut(x) {
174+
//~^ manual_ok_err
175+
Ok(panel) => Some(panel),
176+
Err(_) => None,
177+
}
178+
}
179+
}

tests/ui/manual_ok_err.stderr

Lines changed: 31 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -111,5 +111,35 @@ LL + "1".parse::<u8>().ok()
111111
LL ~ };
112112
|
113113

114-
error: aborting due to 9 previous errors
114+
error: manual implementation of `ok`
115+
--> tests/ui/manual_ok_err.rs:152:9
116+
|
117+
LL | / match &x.field {
118+
LL | |
119+
LL | | Ok(panel) => Some(panel),
120+
LL | | Err(_) => None,
121+
LL | | }
122+
| |_________^ help: replace with: `(&x.field).as_ref().ok()`
123+
124+
error: manual implementation of `ok`
125+
--> tests/ui/manual_ok_err.rs:161:9
126+
|
127+
LL | / match result_with_ref() {
128+
LL | |
129+
LL | | Ok(panel) => Some(panel),
130+
LL | | Err(_) => None,
131+
LL | | }
132+
| |_________^ help: replace with: `result_with_ref().as_ref().ok()`
133+
134+
error: manual implementation of `ok`
135+
--> tests/ui/manual_ok_err.rs:173:9
136+
|
137+
LL | / match result_with_ref_mut(x) {
138+
LL | |
139+
LL | | Ok(panel) => Some(panel),
140+
LL | | Err(_) => None,
141+
LL | | }
142+
| |_________^ help: replace with: `result_with_ref_mut(x).as_mut().ok()`
143+
144+
error: aborting due to 12 previous errors
115145

0 commit comments

Comments
 (0)