Skip to content

Commit 7a34143

Browse files
committed
Auto merge of #11135 - smoelius:unwrap_or_else_default-fp, r=Centri3
Fix `unwrap_or_else_default` false positive This PR fixes a false positive in the handling of `unwrap_or_else` with a default value when the value is needed for type inference. An easy example to exhibit the false positive is the following: ```rust let option = None; option.unwrap_or_else(Vec::new).push(1); ``` The following code would not compile, because the fact that the value is a `Vec` has been lost: ```rust let option = None; option.unwrap_or_default().push(1); ``` The fix is to: - implement a heuristic to tell whether an expression's type can be determined purely from its subexpressions, and the arguments and locals they use; - apply the heuristic to `unwrap_or_else`'s receiver. The heuristic returns false when applied to `option` in the above example, but it returns true when applied to `option` in either of the following examples: ```rust let option: Option<Vec<u64>> = None; option.unwrap_or_else(Vec::new).push(1); ``` ```rust let option = None::<Vec<u64>>; option.unwrap_or_else(Vec::new).push(1); ``` (Aside: #10120 unfairly contained multiple changes in one PR. I am trying to break that PR up into smaller pieces.) --- changelog: FP: [`unwrap_or_else_default`]: No longer lints if the default value is needed for type inference
2 parents 0fa1fd3 + f583fd1 commit 7a34143

File tree

7 files changed

+568
-2
lines changed

7 files changed

+568
-2
lines changed

clippy_lints/src/methods/unwrap_or_else_default.rs

+5-1
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@ use super::UNWRAP_OR_ELSE_DEFAULT;
44
use clippy_utils::diagnostics::span_lint_and_sugg;
55
use clippy_utils::is_default_equivalent_call;
66
use clippy_utils::source::snippet_with_applicability;
7-
use clippy_utils::ty::is_type_diagnostic_item;
7+
use clippy_utils::ty::{expr_type_is_certain, is_type_diagnostic_item};
88
use rustc_ast::ast::LitKind;
99
use rustc_errors::Applicability;
1010
use rustc_hir as hir;
@@ -17,6 +17,10 @@ pub(super) fn check<'tcx>(
1717
recv: &'tcx hir::Expr<'_>,
1818
u_arg: &'tcx hir::Expr<'_>,
1919
) {
20+
if !expr_type_is_certain(cx, recv) {
21+
return;
22+
}
23+
2024
// something.unwrap_or_else(Default::default)
2125
// ^^^^^^^^^- recv ^^^^^^^^^^^^^^^^- u_arg
2226
// ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^- expr

clippy_utils/src/ty.rs

+3
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,9 @@ use std::iter;
2828

2929
use crate::{match_def_path, path_res, paths};
3030

31+
mod type_certainty;
32+
pub use type_certainty::expr_type_is_certain;
33+
3134
/// Checks if the given type implements copy.
3235
pub fn is_copy<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> bool {
3336
ty.is_copy_modulo_regions(cx.tcx, cx.param_env)
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,122 @@
1+
use rustc_hir::def_id::DefId;
2+
use std::fmt::Debug;
3+
4+
#[derive(Clone, Copy, Debug, PartialEq, Eq)]
5+
pub enum Certainty {
6+
/// Determining the type requires contextual information.
7+
Uncertain,
8+
9+
/// The type can be determined purely from subexpressions. If the argument is `Some(..)`, the
10+
/// specific `DefId` is known. Such arguments are needed to handle path segments whose `res` is
11+
/// `Res::Err`.
12+
Certain(Option<DefId>),
13+
14+
/// The heuristic believes that more than one `DefId` applies to a type---this is a bug.
15+
Contradiction,
16+
}
17+
18+
pub trait Meet {
19+
fn meet(self, other: Self) -> Self;
20+
}
21+
22+
pub trait TryJoin: Sized {
23+
fn try_join(self, other: Self) -> Option<Self>;
24+
}
25+
26+
impl Meet for Option<DefId> {
27+
fn meet(self, other: Self) -> Self {
28+
match (self, other) {
29+
(None, _) | (_, None) => None,
30+
(Some(lhs), Some(rhs)) => (lhs == rhs).then_some(lhs),
31+
}
32+
}
33+
}
34+
35+
impl TryJoin for Option<DefId> {
36+
fn try_join(self, other: Self) -> Option<Self> {
37+
match (self, other) {
38+
(Some(lhs), Some(rhs)) => (lhs == rhs).then_some(Some(lhs)),
39+
(Some(def_id), _) | (_, Some(def_id)) => Some(Some(def_id)),
40+
(None, None) => Some(None),
41+
}
42+
}
43+
}
44+
45+
impl Meet for Certainty {
46+
fn meet(self, other: Self) -> Self {
47+
match (self, other) {
48+
(Certainty::Uncertain, _) | (_, Certainty::Uncertain) => Certainty::Uncertain,
49+
(Certainty::Certain(lhs), Certainty::Certain(rhs)) => Certainty::Certain(lhs.meet(rhs)),
50+
(Certainty::Certain(inner), _) | (_, Certainty::Certain(inner)) => Certainty::Certain(inner),
51+
(Certainty::Contradiction, Certainty::Contradiction) => Certainty::Contradiction,
52+
}
53+
}
54+
}
55+
56+
impl Certainty {
57+
/// Join two `Certainty`s preserving their `DefId`s (if any). Generally speaking, this method
58+
/// should be used only when `self` and `other` refer directly to types. Otherwise,
59+
/// `join_clearing_def_ids` should be used.
60+
pub fn join(self, other: Self) -> Self {
61+
match (self, other) {
62+
(Certainty::Contradiction, _) | (_, Certainty::Contradiction) => Certainty::Contradiction,
63+
64+
(Certainty::Certain(lhs), Certainty::Certain(rhs)) => {
65+
if let Some(inner) = lhs.try_join(rhs) {
66+
Certainty::Certain(inner)
67+
} else {
68+
debug_assert!(false, "Contradiction with {lhs:?} and {rhs:?}");
69+
Certainty::Contradiction
70+
}
71+
},
72+
73+
(Certainty::Certain(inner), _) | (_, Certainty::Certain(inner)) => Certainty::Certain(inner),
74+
75+
(Certainty::Uncertain, Certainty::Uncertain) => Certainty::Uncertain,
76+
}
77+
}
78+
79+
/// Join two `Certainty`s after clearing their `DefId`s. This method should be used when `self`
80+
/// or `other` do not necessarily refer to types, e.g., when they are aggregations of other
81+
/// `Certainty`s.
82+
pub fn join_clearing_def_ids(self, other: Self) -> Self {
83+
self.clear_def_id().join(other.clear_def_id())
84+
}
85+
86+
pub fn clear_def_id(self) -> Certainty {
87+
if matches!(self, Certainty::Certain(_)) {
88+
Certainty::Certain(None)
89+
} else {
90+
self
91+
}
92+
}
93+
94+
pub fn with_def_id(self, def_id: DefId) -> Certainty {
95+
if matches!(self, Certainty::Certain(_)) {
96+
Certainty::Certain(Some(def_id))
97+
} else {
98+
self
99+
}
100+
}
101+
102+
pub fn to_def_id(self) -> Option<DefId> {
103+
match self {
104+
Certainty::Certain(inner) => inner,
105+
_ => None,
106+
}
107+
}
108+
109+
pub fn is_certain(self) -> bool {
110+
matches!(self, Self::Certain(_))
111+
}
112+
}
113+
114+
/// Think: `iter.all(/* is certain */)`
115+
pub fn meet(iter: impl Iterator<Item = Certainty>) -> Certainty {
116+
iter.fold(Certainty::Certain(None), Certainty::meet)
117+
}
118+
119+
/// Think: `iter.any(/* is certain */)`
120+
pub fn join(iter: impl Iterator<Item = Certainty>) -> Certainty {
121+
iter.fold(Certainty::Uncertain, Certainty::join)
122+
}

0 commit comments

Comments
 (0)