Skip to content

New lint: concealed_obvious_default #15037

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 2 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
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -5694,6 +5694,7 @@ Released 2018-09-13
[`collection_is_never_read`]: https://rust-lang.github.io/rust-clippy/master/index.html#collection_is_never_read
[`comparison_chain`]: https://rust-lang.github.io/rust-clippy/master/index.html#comparison_chain
[`comparison_to_empty`]: https://rust-lang.github.io/rust-clippy/master/index.html#comparison_to_empty
[`concealed_obvious_default`]: https://rust-lang.github.io/rust-clippy/master/index.html#concealed_obvious_default
[`confusing_method_to_numeric_cast`]: https://rust-lang.github.io/rust-clippy/master/index.html#confusing_method_to_numeric_cast
[`const_is_empty`]: https://rust-lang.github.io/rust-clippy/master/index.html#const_is_empty
[`const_static_lifetime`]: https://rust-lang.github.io/rust-clippy/master/index.html#const_static_lifetime
Expand Down
4 changes: 2 additions & 2 deletions clippy_config/src/conf.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1097,7 +1097,7 @@ impl serde::de::Error for FieldError {
writeln!(msg).unwrap();
for (column, column_width) in column_widths.iter().copied().enumerate() {
let index = column * rows + row;
let field = expected.get(index).copied().unwrap_or_default();
let field = expected.get(index).copied().unwrap_or("");
write!(msg, "{:SEPARATOR_WIDTH$}{field:column_width$}", " ").unwrap();
}
}
Expand Down Expand Up @@ -1128,7 +1128,7 @@ fn calculate_dimensions(fields: &[&str]) -> (usize, Vec<usize>) {
(0..rows)
.map(|row| {
let index = column * rows + row;
let field = fields.get(index).copied().unwrap_or_default();
let field = fields.get(index).copied().unwrap_or("");
field.len()
})
.max()
Expand Down
2 changes: 1 addition & 1 deletion clippy_lints/src/arbitrary_source_item_ordering.rs
Original file line number Diff line number Diff line change
Expand Up @@ -414,7 +414,7 @@ impl<'tcx> LateLintPass<'tcx> for ArbitrarySourceItemOrdering {
let module_level_order = self
.module_item_order_groupings
.module_level_order_of(&item_kind)
.unwrap_or_default();
.unwrap_or(0);

if let Some(cur_t) = cur_t.as_ref() {
use std::cmp::Ordering; // Better legibility.
Expand Down
2 changes: 1 addition & 1 deletion clippy_lints/src/casts/cast_possible_truncation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ fn apply_reductions(cx: &LateContext<'_>, nbits: u64, expr: &Expr<'_>, signed: b
.min(apply_reductions(cx, nbits, right, signed))
.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())),
.saturating_sub(constant_int(cx, right).map_or(0, |s| u64::try_from(s).unwrap_or(0))),
_ => nbits,
},
ExprKind::MethodCall(method, left, [right], _) => {
Expand Down
1 change: 1 addition & 0 deletions clippy_lints/src/declared_lints.rs
Original file line number Diff line number Diff line change
Expand Up @@ -359,6 +359,7 @@ pub static LINTS: &[&crate::LintInfo] = &[
crate::methods::CLONE_ON_COPY_INFO,
crate::methods::CLONE_ON_REF_PTR_INFO,
crate::methods::COLLAPSIBLE_STR_REPLACE_INFO,
crate::methods::CONCEALED_OBVIOUS_DEFAULT_INFO,
crate::methods::CONST_IS_EMPTY_INFO,
crate::methods::DOUBLE_ENDED_ITERATOR_LAST_INFO,
crate::methods::DRAIN_COLLECT_INFO,
Expand Down
2 changes: 1 addition & 1 deletion clippy_lints/src/manual_strip.rs
Original file line number Diff line number Diff line change
Expand Up @@ -269,7 +269,7 @@ fn find_stripping<'tcx>(

fn visit_pat(&mut self, pat: &'tcx rustc_hir::Pat<'tcx>) -> Self::Result {
if let PatKind::Binding(_, _, ident, _) = pat.kind {
*self.bindings.entry(ident.name).or_default() += 1;
*self.bindings.entry(ident.name).or_insert(0) += 1;
}
walk_pat(self, pat);
}
Expand Down
140 changes: 140 additions & 0 deletions clippy_lints/src/methods/concealed_obvious_default.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,140 @@
use rustc_span::Symbol;

use clippy_utils::diagnostics::span_lint_and_sugg;
use clippy_utils::sym;
use rustc_errors::Applicability;
use rustc_hir as hir;
use rustc_lint::LateContext;
use rustc_middle::ty::{self, Ty};
use rustc_span::Span;

use super::CONCEALED_OBVIOUS_DEFAULT;

pub(super) fn check(cx: &LateContext<'_>, recv: &hir::Expr<'_>, method_name: Symbol, call_span: Span) {
// Type of the expression which invoked the method
let recv_ty = cx.typeck_results().expr_ty(recv);

// Option::<bool>::unwrap_or_default()
// ^^^^^^^^^^^^^^^^^^^
// if the call comes from expansion, bail
if call_span.from_expansion() {
return;
}

// Only consider algebraic data types e.g. an `Option`.
// Their generics are represented by `generic_args`
if let ty::Adt(adt, generic_args) = recv_ty.kind()
// `name_of_generic`, is e.g. a `sym::Option`
&& let Some(name_of_generic) = cx.tcx.get_diagnostic_name(adt.did())
&& let Some((message, suggestion)) = CONCEALING_METHODS.into_iter().find(|concealing| {
name_of_generic == concealing.ty && method_name == concealing.method
}).and_then(|concealing| {
let ty = generic_args.type_at(concealing.generic_index);
extract_obvious_default(cx, ty).map(|(default, ty)| {
let method = (concealing.fmt_msg)(ty);
(
format!("method {method} conceals the underlying type"),
(concealing.fmt_sugg)(default),
)
})
})
{
span_lint_and_sugg(
cx,
CONCEALED_OBVIOUS_DEFAULT,
call_span,
message,
"write the default type explicitly".to_string(),
suggestion,
Applicability::MachineApplicable,
);
}
}

/// Method which conceals an underlying type of a generic
struct ConcealingMethod {
/// Generic which contains the concealing method, e.g. `Option<T>`
ty: Symbol,
/// Index of the concealed generic type, e.g. `0` for `Option<T>`
generic_index: usize,
/// The concealing method, e.g. `unwrap_or_default`
method: Symbol,
/// Format the lint's message
///
/// Receives the concealed type, e.g. for `Option<bool>` receives `bool`
fmt_msg: fn(&'static str) -> String,
/// Format the lint's suggestion
///
/// Receives the default of the concealed type, e.g. for `Option<bool>` receives `false`,
/// as `bool::default()` is `false`
fmt_sugg: fn(&'static str) -> String,
}

/// List of methods which use `Default` trait under the hood,
/// but they have an alternative non-`Default` method
///
/// For example, there is `Option::unwrap_or_default` which is almost the same
/// as `Option::unwrap_or`, but the type does not have to be provided and the
/// `Default` implementation is used.
const CONCEALING_METHODS: [ConcealingMethod; 4] = [
ConcealingMethod {
ty: sym::Result,
// Result<T, E>
// ^ want
generic_index: 0,
method: sym::unwrap_or_default,
fmt_msg: |ty| format!("Result::<{ty}, _>::unwrap_or_default()"),
fmt_sugg: |val| format!("unwrap_or({val})"),
},
ConcealingMethod {
ty: sym::Option,
// Option<T>
// ^ want
generic_index: 0,
method: sym::unwrap_or_default,
fmt_msg: |ty| format!("Option::<{ty}>::unwrap_or_default()"),
fmt_sugg: |val| format!("unwrap_or({val})"),
},
ConcealingMethod {
ty: sym::HashMapEntry,
// Entry<'a, K, V, A = Global>
// ^ want
generic_index: 2,
method: sym::or_default,
fmt_msg: |ty| format!("hash_map::Entry::<'_, _, {ty}>::or_default()"),
fmt_sugg: |val| format!("or_insert({val})"),
},
ConcealingMethod {
ty: sym::BTreeEntry,
// Entry<'a, K, V, A = Global>
// ^ want
generic_index: 2,
method: sym::or_default,
fmt_msg: |ty| format!("btree_map::Entry::<'_, _, {ty}>::or_default()"),
fmt_sugg: |val| format!("or_insert({val})"),
},
];

/// Get default value of a type with an obvious default.
///
/// # Returns
///
/// If the type has an obvious default:
///
/// - Default for the type
/// - The type as it should be displayed in the lint message
///
/// If the type is not considered to have an obvious default, return `None`.
fn extract_obvious_default(cx: &LateContext<'_>, ty: Ty<'_>) -> Option<(&'static str, &'static str)> {
match ty.peel_refs().kind() {
ty::Int(ty) => Some(("0", ty.name_str())),
ty::Uint(ty) => Some(("0", ty.name_str())),
ty::Float(ty) => Some(("0.0", ty.name_str())),
ty::Char => Some((r"'\0'", "char")),
ty::Str => Some((r#""""#, "&str")),
ty::Bool => Some(("false", "bool")),
ty::Tuple(tys) if tys.is_empty() => Some(("()", "()")),
ty::Adt(def, _) if cx.tcx.get_diagnostic_name(def.did()) == Some(sym::Option) => Some(("None", "Option<_>")),
_ => None,
}
}
47 changes: 46 additions & 1 deletion clippy_lints/src/methods/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ mod clone_on_copy;
mod clone_on_ref_ptr;
mod cloned_instead_of_copied;
mod collapsible_str_replace;
mod concealed_obvious_default;
mod double_ended_iterator_last;
mod drain_collect;
mod err_expect;
Expand Down Expand Up @@ -4565,6 +4566,46 @@ declare_clippy_lint! {
"hardcoded localhost IP address"
}

declare_clippy_lint! {
/// ### What it does
///
/// Checks for usages of methods like `Option::unwrap_or_default`,
/// for types where the `Default` implementation is obvious and can be written literally
///
/// ### Why is this bad?
///
/// You have to know what the underlying type is, instead of it being obvious from a glance
///
/// ### Example
/// ```no_run
/// fn f(
/// a: Option<&str>,
/// b: Option<usize>,
/// c: Option<Option<bool>>
/// ) {
/// let a = a.unwrap_or_default();
/// let b = b.unwrap_or_default();
/// let c = c.unwrap_or_default();
/// }
/// ```
/// Use instead:
/// ```no_run
/// fn f(
/// a: Option<&str>,
/// b: Option<usize>,
/// c: Option<Option<bool>>
/// ) {
/// let a = a.unwrap_or("");
/// let b = b.unwrap_or(0);
/// let c = c.unwrap_or(None);
/// }
/// ```
#[clippy::version = "1.89.0"]
pub CONCEALED_OBVIOUS_DEFAULT,
complexity,
".*or_default() obscures the underlying type"
}

#[expect(clippy::struct_excessive_bools)]
pub struct Methods {
avoid_breaking_exported_api: bool,
Expand Down Expand Up @@ -4744,6 +4785,7 @@ impl_lint_pass!(Methods => [
IO_OTHER_ERROR,
SWAP_WITH_TEMPORARY,
IP_CONSTANT,
CONCEALED_OBVIOUS_DEFAULT
]);

/// Extracts a method call name, args, and `Span` of the method name.
Expand Down Expand Up @@ -5544,7 +5586,7 @@ impl Methods {
}
}
// Handle method calls whose receiver and arguments may come from expansion
if let ExprKind::MethodCall(path, recv, args, _call_span) = expr.kind {
if let ExprKind::MethodCall(path, recv, args, call_span) = expr.kind {
match (path.ident.name, args) {
(sym::expect, [_]) if !matches!(method_call(recv), Some((sym::ok | sym::err, _, [], _, _))) => {
unwrap_expect_used::check(
Expand Down Expand Up @@ -5579,6 +5621,9 @@ impl Methods {
unwrap_expect_used::Variant::Unwrap,
);
},
(sym::or_default | sym::unwrap_or_default, []) => {
concealed_obvious_default::check(cx, recv, path.ident.name, call_span);
},
(sym::unwrap_err, []) => {
unwrap_expect_used::check(
cx,
Expand Down
6 changes: 3 additions & 3 deletions clippy_lints/src/operators/arithmetic_side_effects.rs
Original file line number Diff line number Diff line change
Expand Up @@ -64,9 +64,9 @@ impl ArithmeticSideEffects {
/// "multiplication" are present in the inner set of allowed types.
fn has_allowed_binary(&self, lhs_ty: Ty<'_>, rhs_ty: Ty<'_>) -> bool {
let lhs_ty_string = lhs_ty.to_string();
let lhs_ty_string_elem = lhs_ty_string.split('<').next().unwrap_or_default();
let lhs_ty_string_elem = lhs_ty_string.split('<').next().unwrap_or("");
let rhs_ty_string = rhs_ty.to_string();
let rhs_ty_string_elem = rhs_ty_string.split('<').next().unwrap_or_default();
let rhs_ty_string_elem = rhs_ty_string.split('<').next().unwrap_or("");
if let Some(rhs_from_specific) = self.allowed_binary.get(lhs_ty_string_elem)
&& {
let rhs_has_allowed_ty = rhs_from_specific.contains(rhs_ty_string_elem);
Expand All @@ -85,7 +85,7 @@ impl ArithmeticSideEffects {
/// allowed types.
fn has_allowed_unary(&self, ty: Ty<'_>) -> bool {
let ty_string = ty.to_string();
let ty_string_elem = ty_string.split('<').next().unwrap_or_default();
let ty_string_elem = ty_string.split('<').next().unwrap_or("");
self.allowed_unary.contains(ty_string_elem)
}

Expand Down
2 changes: 1 addition & 1 deletion clippy_lints/src/utils/author.rs
Original file line number Diff line number Diff line change
Expand Up @@ -216,7 +216,7 @@ impl<'a, 'tcx> PrintVisitor<'a, 'tcx> {

fn next(&self, s: &'static str) -> String {
let mut ids = self.ids.take();
let out = match *ids.entry(s).and_modify(|n| *n += 1).or_default() {
let out = match *ids.entry(s).and_modify(|n| *n += 1).or_insert(0) {
// first usage of the name, use it as is
0 => s.to_string(),
// append a number starting with 1
Expand Down
2 changes: 1 addition & 1 deletion clippy_utils/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2770,7 +2770,7 @@ pub fn tokenize_with_text(s: &str) -> impl Iterator<Item = (TokenKind, &str, Inn
let range = pos as usize..end as usize;
let inner = InnerSpan::new(range.start, range.end);
pos = end;
(t.kind, s.get(range).unwrap_or_default(), inner)
(t.kind, s.get(range).unwrap_or(""), inner)
})
}

Expand Down
8 changes: 4 additions & 4 deletions clippy_utils/src/ty/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -934,7 +934,7 @@ pub fn approx_ty_size<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> u64 {
match (cx.layout_of(ty).map(|layout| layout.size.bytes()), ty.kind()) {
(Ok(size), _) => size,
(Err(_), ty::Tuple(list)) => list.iter().map(|t| approx_ty_size(cx, t)).sum(),
(Err(_), ty::Array(t, n)) => n.try_to_target_usize(cx.tcx).unwrap_or_default() * approx_ty_size(cx, *t),
(Err(_), ty::Array(t, n)) => n.try_to_target_usize(cx.tcx).unwrap_or(0) * approx_ty_size(cx, *t),
(Err(_), ty::Adt(def, subst)) if def.is_struct() => def
.variants()
.iter()
Expand All @@ -955,7 +955,7 @@ pub fn approx_ty_size<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> u64 {
.sum::<u64>()
})
.max()
.unwrap_or_default(),
.unwrap_or(0),
(Err(_), ty::Adt(def, subst)) if def.is_union() => def
.variants()
.iter()
Expand All @@ -964,10 +964,10 @@ pub fn approx_ty_size<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> u64 {
.iter()
.map(|field| approx_ty_size(cx, field.ty(cx.tcx, subst)))
.max()
.unwrap_or_default()
.unwrap_or(0)
})
.max()
.unwrap_or_default(),
.unwrap_or(0),
(Err(_), _) => 0,
}
}
Expand Down
2 changes: 1 addition & 1 deletion src/driver.rs
Original file line number Diff line number Diff line change
Expand Up @@ -267,7 +267,7 @@ pub fn main() {
let clippy_args_var = env::var("CLIPPY_ARGS").ok();
let clippy_args = clippy_args_var
.as_deref()
.unwrap_or_default()
.unwrap_or("")
.split("__CLIPPY_HACKERY__")
.filter_map(|s| match s {
"" => None,
Expand Down
Loading