Skip to content

Do not generate type-indecisive expressions in unnecessary_cast #14492

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

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from
Draft
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
6 changes: 5 additions & 1 deletion clippy_lints/src/casts/unnecessary_cast.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
use clippy_utils::diagnostics::span_lint_and_sugg;
use clippy_utils::numeric_literal::NumericLiteral;
use clippy_utils::source::{SpanRangeExt, snippet_opt};
use clippy_utils::ty::expr_type_is_certain;
use clippy_utils::visitors::{Visitable, for_each_expr_without_closures};
use clippy_utils::{get_parent_expr, is_hir_ty_cfg_dependant, is_ty_alias, path_to_local};
use rustc_ast::{LitFloatType, LitIntType, LitKind};
Expand Down Expand Up @@ -141,7 +142,10 @@ pub(super) fn check<'tcx>(
}
}

if cast_from.kind() == cast_to.kind() && !expr.span.in_external_macro(cx.sess().source_map()) {
if cast_from.kind() == cast_to.kind()
&& !expr.span.in_external_macro(cx.sess().source_map())
&& expr_type_is_certain(cx, cast_expr)
{
if let Some(id) = path_to_local(cast_expr)
&& !cx.tcx.hir().span(id).eq_ctxt(cast_expr.span)
{
Expand Down
1 change: 1 addition & 0 deletions clippy_utils/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
#![feature(assert_matches)]
#![feature(unwrap_infallible)]
#![feature(array_windows)]
#![feature(try_trait_v2)]
#![recursion_limit = "512"]
#![allow(
clippy::missing_errors_doc,
Expand Down
62 changes: 51 additions & 11 deletions clippy_utils/src/ty/type_certainty/certainty.rs
Original file line number Diff line number Diff line change
@@ -1,15 +1,22 @@
use rustc_hir::def_id::DefId;
use std::fmt::Debug;
use std::ops::{ControlFlow, FromResidual, Try};

#[derive(Clone, Copy, Debug, PartialEq, Eq)]
pub enum TypeKind {
PrimTy,
AdtDef(DefId),
}

#[derive(Clone, Copy, Debug, PartialEq, Eq)]
pub enum Certainty {
/// Determining the type requires contextual information.
Uncertain,

/// The type can be determined purely from subexpressions. If the argument is `Some(..)`, the
/// specific `DefId` is known. Such arguments are needed to handle path segments whose `res` is
/// `Res::Err`.
Certain(Option<DefId>),
/// specific primitive type or `DefId` is known. Such arguments are needed to handle path
/// segments whose `res` is `Res::Err`.
Certain(Option<TypeKind>),

/// The heuristic believes that more than one `DefId` applies to a type---this is a bug.
Contradiction,
Expand All @@ -23,7 +30,7 @@ pub trait TryJoin: Sized {
fn try_join(self, other: Self) -> Option<Self>;
}

impl Meet for Option<DefId> {
impl Meet for Option<TypeKind> {
fn meet(self, other: Self) -> Self {
match (self, other) {
(None, _) | (_, None) => None,
Expand All @@ -32,11 +39,11 @@ impl Meet for Option<DefId> {
}
}

impl TryJoin for Option<DefId> {
impl TryJoin for Option<TypeKind> {
fn try_join(self, other: Self) -> Option<Self> {
match (self, other) {
(Some(lhs), Some(rhs)) => (lhs == rhs).then_some(Some(lhs)),
(Some(def_id), _) | (_, Some(def_id)) => Some(Some(def_id)),
(Some(ty_kind), _) | (_, Some(ty_kind)) => Some(Some(ty_kind)),
(None, None) => Some(None),
}
}
Expand Down Expand Up @@ -79,29 +86,37 @@ impl Certainty {
/// Join two `Certainty`s after clearing their `DefId`s. This method should be used when `self`
/// or `other` do not necessarily refer to types, e.g., when they are aggregations of other
/// `Certainty`s.
pub fn join_clearing_def_ids(self, other: Self) -> Self {
self.clear_def_id().join(other.clear_def_id())
pub fn join_clearing_types(self, other: Self) -> Self {
self.clear_type().join(other.clear_type())
}

pub fn clear_def_id(self) -> Certainty {
pub fn clear_type(self) -> Certainty {
if matches!(self, Certainty::Certain(_)) {
Certainty::Certain(None)
} else {
self
}
}

pub fn with_prim_ty(self) -> Certainty {
if matches!(self, Certainty::Certain(_)) {
Certainty::Certain(Some(TypeKind::PrimTy))
} else {
self
}
}

pub fn with_def_id(self, def_id: DefId) -> Certainty {
if matches!(self, Certainty::Certain(_)) {
Certainty::Certain(Some(def_id))
Certainty::Certain(Some(TypeKind::AdtDef(def_id)))
} else {
self
}
}

pub fn to_def_id(self) -> Option<DefId> {
match self {
Certainty::Certain(inner) => inner,
Certainty::Certain(Some(TypeKind::AdtDef(def_id))) => Some(def_id),
_ => None,
}
}
Expand All @@ -120,3 +135,28 @@ pub fn meet(iter: impl Iterator<Item = Certainty>) -> Certainty {
pub fn join(iter: impl Iterator<Item = Certainty>) -> Certainty {
iter.fold(Certainty::Uncertain, Certainty::join)
}

pub struct NoCertainty(Certainty);

impl FromResidual<NoCertainty> for Certainty {
fn from_residual(residual: NoCertainty) -> Self {
residual.0
}
}

impl Try for Certainty {
type Output = Certainty;

type Residual = NoCertainty;

fn from_output(output: Self::Output) -> Self {
output
}

fn branch(self) -> ControlFlow<Self::Residual, Self::Output> {
match self {
Certainty::Certain(_) => ControlFlow::Continue(self),
_ => ControlFlow::Break(NoCertainty(self)),
}
}
}
93 changes: 64 additions & 29 deletions clippy_utils/src/ty/type_certainty/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
//! be considered a bug.

use crate::def_path_res;
use rustc_ast::{LitFloatType, LitIntType, LitKind};
use rustc_hir::def::{DefKind, Res};
use rustc_hir::def_id::DefId;
use rustc_hir::intravisit::{InferKind, Visitor, VisitorExt, walk_qpath, walk_ty};
Expand All @@ -21,33 +22,36 @@ use rustc_middle::ty::{self, AdtDef, GenericArgKind, Ty};
use rustc_span::{Span, Symbol};

mod certainty;
use certainty::{Certainty, Meet, join, meet};
use certainty::{Certainty, Meet, TypeKind, join, meet};

pub fn expr_type_is_certain(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool {
expr_type_certainty(cx, expr).is_certain()
expr_type_certainty(cx, expr, false).is_certain()
}

fn expr_type_certainty(cx: &LateContext<'_>, expr: &Expr<'_>) -> Certainty {
/// Determine the type certainty of `expr`. `in_arg` indicates that the expression happens within
/// the evaluation of a function or method call argument.
fn expr_type_certainty(cx: &LateContext<'_>, expr: &Expr<'_>, in_arg: bool) -> Certainty {
let certainty = match &expr.kind {
ExprKind::Unary(_, expr)
| ExprKind::Field(expr, _)
| ExprKind::Index(expr, _, _)
| ExprKind::AddrOf(_, _, expr) => expr_type_certainty(cx, expr),
| ExprKind::AddrOf(_, _, expr) => expr_type_certainty(cx, expr, in_arg),

ExprKind::Array(exprs) => join(exprs.iter().map(|expr| expr_type_certainty(cx, expr))),
ExprKind::Array(exprs) => join(exprs.iter().map(|expr| expr_type_certainty(cx, expr, in_arg))),

ExprKind::Call(callee, args) => {
let lhs = expr_type_certainty(cx, callee);
let lhs = expr_type_certainty(cx, callee, false);
let rhs = if type_is_inferable_from_arguments(cx, expr) {
meet(args.iter().map(|arg| expr_type_certainty(cx, arg)))
meet(args.iter().map(|arg| expr_type_certainty(cx, arg, true)))
} else {
Certainty::Uncertain
};
lhs.join_clearing_def_ids(rhs)
lhs.join_clearing_types(rhs)
},

ExprKind::MethodCall(method, receiver, args, _) => {
let mut receiver_type_certainty = expr_type_certainty(cx, receiver);
let mut receiver_type_certainty = expr_type_certainty(cx, receiver, false)?;

// Even if `receiver_type_certainty` is `Certain(Some(..))`, the `Self` type in the method
// identified by `type_dependent_def_id(..)` can differ. This can happen as a result of a `deref`,
// for example. So update the `DefId` in `receiver_type_certainty` (if any).
Expand All @@ -59,39 +63,68 @@ fn expr_type_certainty(cx: &LateContext<'_>, expr: &Expr<'_>) -> Certainty {
let lhs = path_segment_certainty(cx, receiver_type_certainty, method, false);
let rhs = if type_is_inferable_from_arguments(cx, expr) {
meet(
std::iter::once(receiver_type_certainty).chain(args.iter().map(|arg| expr_type_certainty(cx, arg))),
std::iter::once(receiver_type_certainty)
.chain(args.iter().map(|arg| expr_type_certainty(cx, arg, true))),
)
} else {
Certainty::Uncertain
return Certainty::Uncertain;
};
lhs.join(rhs)
},

ExprKind::Tup(exprs) => meet(exprs.iter().map(|expr| expr_type_certainty(cx, expr))),
ExprKind::Tup(exprs) => meet(exprs.iter().map(|expr| expr_type_certainty(cx, expr, in_arg))),

ExprKind::Binary(_, lhs, rhs) => expr_type_certainty(cx, lhs).meet(expr_type_certainty(cx, rhs)),
ExprKind::Binary(_, lhs, rhs) => {
// If one of the side of the expression is uncertain, the certainty will come from the other side,
// with no information on the type.
match (
expr_type_certainty(cx, lhs, in_arg),
expr_type_certainty(cx, rhs, in_arg),
) {
(Certainty::Uncertain, Certainty::Certain(_)) | (Certainty::Certain(_), Certainty::Uncertain) => {
Certainty::Certain(None)
},
(l, r) => l.meet(r),
}
},

ExprKind::Lit(_) => Certainty::Certain(None),
ExprKind::Lit(lit) => {
if !in_arg
&& matches!(
lit.node,
LitKind::Int(_, LitIntType::Unsuffixed) | LitKind::Float(_, LitFloatType::Unsuffixed)
)
{
Certainty::Uncertain
} else {
Certainty::Certain(None)
}
},

ExprKind::Cast(_, ty) => type_certainty(cx, ty),

ExprKind::If(_, if_expr, Some(else_expr)) => {
expr_type_certainty(cx, if_expr).join(expr_type_certainty(cx, else_expr))
expr_type_certainty(cx, if_expr, in_arg).join(expr_type_certainty(cx, else_expr, in_arg))
},

ExprKind::Path(qpath) => qpath_certainty(cx, qpath, false),

ExprKind::Struct(qpath, _, _) => qpath_certainty(cx, qpath, true),

ExprKind::Block(block, _) => block
.expr
.map_or(Certainty::Certain(None), |expr| expr_type_certainty(cx, expr, false)),

_ => Certainty::Uncertain,
};

let expr_ty = cx.typeck_results().expr_ty(expr);
if let Some(def_id) = adt_def_id(expr_ty) {
certainty.with_def_id(def_id)
} else {
certainty.clear_def_id()
}
let result = match cx.typeck_results().expr_ty(expr).kind() {
ty::Adt(adt_def, _) => certainty.with_def_id(adt_def.did()),
ty::Bool | ty::Char | ty::Int(_) | ty::Uint(_) | ty::Float(_) => certainty.with_prim_ty(),
_ => certainty.clear_type(),
};

result
}

struct CertaintyVisitor<'cx, 'tcx> {
Expand Down Expand Up @@ -178,7 +211,11 @@ fn qpath_certainty(cx: &LateContext<'_>, qpath: &QPath<'_>, resolves_to_type: bo
.map_or(Certainty::Uncertain, |def_id| {
let generics = cx.tcx.generics_of(def_id);
if generics.is_empty() {
Certainty::Certain(if resolves_to_type { Some(def_id) } else { None })
Certainty::Certain(if resolves_to_type {
Some(TypeKind::AdtDef(def_id))
} else {
None
})
} else {
Certainty::Uncertain
}
Expand Down Expand Up @@ -218,7 +255,7 @@ fn path_segment_certainty(
.args
.map_or(Certainty::Uncertain, |args| generic_args_certainty(cx, args));
// See the comment preceding `qpath_certainty`. `def_id` could refer to a type or a value.
let certainty = lhs.join_clearing_def_ids(rhs);
let certainty = lhs.join_clearing_types(rhs);
if resolves_to_type {
if let DefKind::TyAlias = cx.tcx.def_kind(def_id) {
adt_def_id(cx.tcx.type_of(def_id).instantiate_identity())
Expand All @@ -234,9 +271,7 @@ fn path_segment_certainty(
}
},

Res::PrimTy(_) | Res::SelfTyParam { .. } | Res::SelfTyAlias { .. } | Res::SelfCtor(_) => {
Certainty::Certain(None)
},
Res::PrimTy(_) => Certainty::Certain(Some(TypeKind::PrimTy)),

// `get_parent` because `hir_id` refers to a `Pat`, and we're interested in the node containing the `Pat`.
Res::Local(hir_id) => match cx.tcx.parent_hir_node(hir_id) {
Expand All @@ -248,18 +283,18 @@ fn path_segment_certainty(
let lhs = local.ty.map_or(Certainty::Uncertain, |ty| type_certainty(cx, ty));
let rhs = local
.init
.map_or(Certainty::Uncertain, |init| expr_type_certainty(cx, init));
.map_or(Certainty::Uncertain, |init| expr_type_certainty(cx, init, false));
let certainty = lhs.join(rhs);
if resolves_to_type {
certainty
} else {
certainty.clear_def_id()
certainty.clear_type()
}
},
_ => Certainty::Uncertain,
},

_ => Certainty::Uncertain,
_ => Certainty::Certain(None),
};
debug_assert!(resolves_to_type || certainty.to_def_id().is_none());
certainty
Expand Down
12 changes: 8 additions & 4 deletions tests/ui/or_fun_call.fixed
Original file line number Diff line number Diff line change
Expand Up @@ -212,7 +212,8 @@ mod issue8239 {
acc.push_str(&f);
acc
})
.unwrap_or(String::new());
.unwrap_or_default();
//~^ unwrap_or_default
}

fn more_to_max_suggestion_highest_lines_1() {
Expand All @@ -225,7 +226,8 @@ mod issue8239 {
acc.push_str(&f);
acc
})
.unwrap_or(String::new());
.unwrap_or_default();
//~^ unwrap_or_default
}

fn equal_to_max_suggestion_highest_lines() {
Expand All @@ -237,7 +239,8 @@ mod issue8239 {
acc.push_str(&f);
acc
})
.unwrap_or(String::new());
.unwrap_or_default();
//~^ unwrap_or_default
}

fn less_than_max_suggestion_highest_lines() {
Expand All @@ -248,7 +251,8 @@ mod issue8239 {
acc.push_str(&f);
acc
})
.unwrap_or(String::new());
.unwrap_or_default();
//~^ unwrap_or_default
}
}

Expand Down
Loading
Loading