Skip to content

Commit

Permalink
Add wrong_any_coerce lint
Browse files Browse the repository at this point in the history
  • Loading branch information
HMPerson1 committed Jun 27, 2019
1 parent f0edfab commit 8e9c8f6
Show file tree
Hide file tree
Showing 9 changed files with 404 additions and 4 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -1176,6 +1176,7 @@ Released 2018-09-13
[`write_literal`]: https://rust-lang.github.io/rust-clippy/master/index.html#write_literal
[`write_with_newline`]: https://rust-lang.github.io/rust-clippy/master/index.html#write_with_newline
[`writeln_empty_string`]: https://rust-lang.github.io/rust-clippy/master/index.html#writeln_empty_string
[`wrong_any_coerce`]: https://rust-lang.github.io/rust-clippy/master/index.html#wrong_any_coerce
[`wrong_pub_self_convention`]: https://rust-lang.github.io/rust-clippy/master/index.html#wrong_pub_self_convention
[`wrong_self_convention`]: https://rust-lang.github.io/rust-clippy/master/index.html#wrong_self_convention
[`wrong_transmute`]: https://rust-lang.github.io/rust-clippy/master/index.html#wrong_transmute
Expand Down
2 changes: 1 addition & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@

A collection of lints to catch common mistakes and improve your [Rust](https://github.com/rust-lang/rust) code.

[There are 305 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html)
[There are 306 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html)

We have a bunch of lint categories to allow you to choose how much Clippy is supposed to ~~annoy~~ help you:

Expand Down
225 changes: 225 additions & 0 deletions clippy_lints/src/any_coerce.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,225 @@
// Copyright 2014-2018 The Rust Project Developers. See the COPYRIGHT
// file at the top-level directory of this distribution.
//
// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
// option. This file may not be copied, modified, or distributed
// except according to those terms.

use crate::rustc::hir::Expr;
use crate::rustc::infer::InferCtxt;
use crate::rustc::lint::{LateContext, LateLintPass, LintArray, LintPass};
use crate::rustc::traits;
use crate::rustc::ty::adjustment::{Adjust, PointerCast};
use crate::rustc::ty::{self, ToPolyTraitRef, Ty};
use crate::rustc::{declare_lint_pass, declare_tool_lint};
use crate::syntax_pos::symbol::Ident;
use crate::utils::{match_def_path, paths, span_lint_and_then};
use if_chain::if_chain;
use std::collections::VecDeque;

declare_clippy_lint! {
/// **What it does:** Checks for coercing something that already contains a
/// `dyn Any` to `dyn Any` itself.
///
/// **Why is this bad?** It's probably a mistake.
///
/// **Known problems:** None.
///
/// **Example:**
/// ```rust
/// let box_foo: Box<Foo> = Box::new(Foo);
/// let mut box_any: Box<dyn Any> = box_foo;
/// let bad: &mut dyn Any = &mut box_any;
/// // you probably meant
/// let ok: &mut dyn Any = &mut *box_any;
/// ```
pub WRONG_ANY_COERCE,
correctness,
"coercing a type already containing `dyn Any` to `dyn Any` itself"
}

declare_lint_pass!(WrongAnyCoerce => [WRONG_ANY_COERCE]);

struct LintData<'tcx> {
coerced_to_any: Ty<'tcx>,
}

impl<'a, 'tcx> LateLintPass<'a, 'tcx> for WrongAnyCoerce {
fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr) {
let adjustments = cx.tables.expr_adjustments(expr);
for (i, adj) in adjustments.iter().enumerate() {
if let Adjust::Pointer(PointerCast::Unsize) = adj.kind {
let src_ty = if i == 0 {
cx.tables.expr_ty(expr)
} else {
adjustments[i - 1].target
};
cx.tcx.infer_ctxt().enter(|infcx| {
let opt_lint_data = check_unsize_coercion(cx, &infcx, cx.param_env, src_ty, adj.target);
if let Some(lint_data) = opt_lint_data {
// TODO: we might be able to suggest dereferencing in some cases
let cta_str = lint_data.coerced_to_any.to_string();
span_lint_and_then(
cx,
WRONG_ANY_COERCE,
expr.span,
&format!("coercing `{}` to `dyn Any`", cta_str),
|db| {
if !cta_str.contains("Any") {
db.note(&format!("`{}` dereferences to `dyn Any`", cta_str));
}
},
)
}
});
}
}
}
}

/// Returns whether or not this coercion should be linted
fn check_unsize_coercion<'tcx>(
cx: &LateContext<'_, 'tcx>,
infcx: &InferCtxt<'_, 'tcx>,
param_env: ty::ParamEnv<'tcx>,
src_ty: Ty<'tcx>,
tgt_ty: Ty<'tcx>,
) -> Option<LintData<'tcx>> {
// redo the typechecking for this coercion to see if it required unsizing something to `dyn Any`
// see https://github.com/rust-lang/rust/blob/cae6efc37d70ab7d353e6ab9ce229d59a65ed643/src/librustc_typeck/check/coercion.rs#L454-L611
let tcx = infcx.tcx;
let coerce_unsized_trait_did = tcx.lang_items().coerce_unsized_trait().unwrap();
let unsize_trait_did = tcx.lang_items().unsize_trait().unwrap();

// don't report overflow errors
let mut selcx = traits::SelectionContext::with_query_mode(&infcx, traits::TraitQueryMode::Canonical);
let mut queue = VecDeque::new();
queue.push_back(
ty::TraitRef::new(
coerce_unsized_trait_did,
tcx.mk_substs_trait(src_ty, &[tgt_ty.into()]),
)
.to_poly_trait_ref(),
);
while let Some(trait_ref) = queue.pop_front() {
if_chain! {
if trait_ref.def_id() == unsize_trait_did;
if is_type_dyn_any(cx, trait_ref.skip_binder().input_types().nth(1).unwrap());
// found something unsizing to `dyn Any`
let coerced_to_any = trait_ref.self_ty();
if type_contains_any(cx, &mut selcx, param_env, coerced_to_any);
then {
return Some(LintData { coerced_to_any });
}
}
let select_result = selcx.select(&traits::Obligation::new(
traits::ObligationCause::dummy(),
param_env,
trait_ref.to_poly_trait_predicate(),
));
if let Ok(Some(vtable)) = select_result {
// we only care about trait predicates for these traits
let traits = [coerce_unsized_trait_did, unsize_trait_did];
queue.extend(
vtable
.nested_obligations()
.into_iter()
.filter_map(|oblig| oblig.predicate.to_opt_poly_trait_ref())
.filter(|tr| traits.contains(&tr.def_id())),
);
}
}
None
}

fn type_contains_any<'tcx>(
cx: &LateContext<'_, 'tcx>,
selcx: &mut traits::SelectionContext<'_, 'tcx>,
param_env: ty::ParamEnv<'tcx>,
ty: Ty<'tcx>,
) -> bool {
// check if it derefs to `dyn Any`
if_chain! {
if let Some((any_src_deref_ty, _deref_count)) = fully_deref_type(selcx, param_env, ty);
if is_type_dyn_any(cx, any_src_deref_ty);
then {
// TODO: use deref_count to make a suggestion
return true;
}
}
// TODO: check for `RefCell<dyn Any>`?
false
}

fn is_type_dyn_any<'tcx>(
cx: &LateContext<'_, 'tcx>,
ty: Ty<'tcx>,
) -> bool {
if_chain! {
if let ty::Dynamic(trait_list, _) = ty.sty;
if let Some(principal_trait) = trait_list.skip_binder().principal();
if match_def_path(cx, principal_trait.def_id, &paths::ANY_TRAIT);
then {
return true;
}
}
false
}

/// Calls [`deref_type`] repeatedly
fn fully_deref_type<'tcx>(
selcx: &mut traits::SelectionContext<'_, 'tcx>,
param_env: ty::ParamEnv<'tcx>,
src_ty: Ty<'tcx>,
) -> Option<(Ty<'tcx>, usize)> {
if let Some(deref_1) = deref_type(selcx, param_env, src_ty) {
let mut deref_count = 1;
let mut cur_ty = deref_1;
while let Some(deref_n) = deref_type(selcx, param_env, cur_ty) {
deref_count += 1;
cur_ty = deref_n;
}
Some((cur_ty, deref_count))
} else {
None
}
}

/// Returns the type of `*expr`, where `expr` has type `src_ty`.
/// This will go through `Deref` `impl`s if necessary.
/// Returns `None` if `*expr` would not typecheck.
fn deref_type<'tcx>(
selcx: &mut traits::SelectionContext<'_, 'tcx>,
param_env: ty::ParamEnv<'tcx>,
src_ty: Ty<'tcx>,
) -> Option<Ty<'tcx>> {
if let Some(ty::TypeAndMut { ty, .. }) = src_ty.builtin_deref(true) {
Some(ty)
} else {
// compute `<T as Deref>::Target`
let infcx = selcx.infcx();
let tcx = selcx.tcx();
let src_deref = ty::TraitRef::new(
tcx.lang_items().deref_trait().unwrap(),
tcx.mk_substs_trait(src_ty, &[]),
);
let mut obligations = Vec::new();
let src_deref_ty = traits::normalize_projection_type(
selcx,
param_env,
ty::ProjectionTy::from_ref_and_name(tcx, src_deref, Ident::from_str("Target")),
traits::ObligationCause::dummy(),
0,
&mut obligations,
);
// only return something if all the obligations definitely hold
let obligations_ok = obligations.iter().all(|oblig| infcx.predicate_must_hold_considering_regions(oblig));
if obligations_ok {
Some(infcx.resolve_vars_if_possible(&src_deref_ty))
} else {
None
}
}
}
4 changes: 4 additions & 0 deletions clippy_lints/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,7 @@ mod consts;
mod utils;

// begin lints modules, do not remove this comment, it’s used in `update_lints`
pub mod any_coerce;
pub mod approx_const;
pub mod arithmetic;
pub mod assertions_on_constants;
Expand Down Expand Up @@ -582,6 +583,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry<'_>, conf: &Conf) {
reg.register_late_lint_pass(box path_buf_push_overwrite::PathBufPushOverwrite);
reg.register_late_lint_pass(box checked_conversions::CheckedConversions);
reg.register_late_lint_pass(box integer_division::IntegerDivision);
reg.register_late_lint_pass(box any_coerce::WrongAnyCoerce);

reg.register_lint_group("clippy::restriction", Some("clippy_restriction"), vec![
arithmetic::FLOAT_ARITHMETIC,
Expand Down Expand Up @@ -667,6 +669,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry<'_>, conf: &Conf) {
]);

reg.register_lint_group("clippy::all", Some("clippy"), vec![
any_coerce::WRONG_ANY_COERCE,
approx_const::APPROX_CONSTANT,
assertions_on_constants::ASSERTIONS_ON_CONSTANTS,
assign_ops::ASSIGN_OP_PATTERN,
Expand Down Expand Up @@ -1049,6 +1052,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry<'_>, conf: &Conf) {
]);

reg.register_lint_group("clippy::correctness", Some("clippy_correctness"), vec![
any_coerce::WRONG_ANY_COERCE,
approx_const::APPROX_CONSTANT,
attrs::DEPRECATED_SEMVER,
attrs::USELESS_ATTRIBUTE,
Expand Down
2 changes: 1 addition & 1 deletion clippy_lints/src/types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -427,7 +427,7 @@ fn is_any_trait(t: &hir::Ty) -> bool {
if traits.len() >= 1;
// Only Send/Sync can be used as additional traits, so it is enough to
// check only the first trait.
if match_path(&traits[0].trait_ref.path, &paths::ANY_TRAIT);
if match_path(&traits[0].trait_ref.path, &paths::ANY_TRAIT_STD);
then {
return true;
}
Expand Down
3 changes: 2 additions & 1 deletion clippy_lints/src/utils/paths.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
//! This module contains paths to types and functions Clippy needs to know
//! about.
pub const ANY_TRAIT: [&str; 3] = ["std", "any", "Any"];
pub const ANY_TRAIT: [&str; 3] = ["core", "any", "Any"];
pub const ANY_TRAIT_STD: [&str; 3] = ["std", "any", "Any"];
pub const ARC: [&str; 3] = ["alloc", "sync", "Arc"];
pub const ASMUT_TRAIT: [&str; 3] = ["core", "convert", "AsMut"];
pub const ASREF_TRAIT: [&str; 3] = ["core", "convert", "AsRef"];
Expand Down
9 changes: 8 additions & 1 deletion src/lintlist/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ pub use lint::Lint;
pub use lint::LINT_LEVELS;

// begin lint list, do not remove this comment, it’s used in `update_lints`
pub const ALL_LINTS: [Lint; 305] = [
pub const ALL_LINTS: [Lint; 306] = [
Lint {
name: "absurd_extreme_comparisons",
group: "correctness",
Expand Down Expand Up @@ -2093,6 +2093,13 @@ pub const ALL_LINTS: [Lint; 305] = [
deprecation: None,
module: "write",
},
Lint {
name: "wrong_any_coerce",
group: "correctness",
desc: "coercing a type already containing `dyn Any` to `dyn Any` itself",
deprecation: None,
module: "any_coerce",
},
Lint {
name: "wrong_pub_self_convention",
group: "restriction",
Expand Down
74 changes: 74 additions & 0 deletions tests/ui/any_coerce.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,74 @@
// Copyright 2014-2018 The Rust Project Developers. See the COPYRIGHT
// file at the top-level directory of this distribution.
//
// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
// option. This file may not be copied, modified, or distributed
// except according to those terms.

#![feature(unsize, coerce_unsized)]
#![deny(clippy::wrong_any_coerce)]
#![deny(bare_trait_objects)]

use std::any::Any;
use std::cell::RefCell;
use std::fmt::Debug;
use std::iter::Iterator;
use std::marker::{Send, Unsize};
use std::ops::CoerceUnsized;
use std::ops::Deref;
use std::rc::Rc;

struct Foo;

struct Unsizeable<T: ?Sized, U: ?Sized, V: ?Sized> {
box_v: Box<V>,
rc_t: Rc<T>,
u: U,
}

fn main() {
let mut box_any: Box<dyn Any + Send> = Box::new(Foo);
let _: *mut dyn Any = &mut box_any; // LINT
let _: *mut dyn Any = &mut *box_any; // ok

let rc_rc_any: Rc<Rc<dyn Any>> = Rc::new(Rc::new(Foo));
let _: &dyn Any = &rc_rc_any; // LINT
let _: &dyn Any = &*rc_rc_any; // LINT
let _: &dyn Any = &**rc_rc_any; // ok
let _: &Rc<dyn Any> = &*rc_rc_any; // ok

let refcell_box_any: RefCell<Box<dyn Any>> = RefCell::new(Box::new(Foo));
let _: &RefCell<dyn Any> = &refcell_box_any; // LINT

let rc_unsizable_rc_any: Rc<Unsizeable<i32, Rc<dyn Any>, i32>> = Rc::new(Unsizeable {
box_v: Box::new(0),
rc_t: Rc::new(0),
u: Rc::new(Foo),
});
let _: Rc<Unsizeable<i32, dyn Any, i32>> = rc_unsizable_rc_any.clone(); // LINT
let _: &Unsizeable<i32, dyn Any, i32> = &*rc_unsizable_rc_any; // LINT
let _: &Rc<Unsizeable<i32, Rc<dyn Any>, i32>> = &rc_unsizable_rc_any; // ok
let _: &Unsizeable<i32, Rc<dyn Any>, i32> = &*rc_unsizable_rc_any; // ok

let ref_any: &dyn Any = &Foo;
let _: &dyn Any = &ref_any; // LINT
let _: &dyn Any = &*ref_any; // ok

let ref_refcell_any: &'static RefCell<dyn Any> = Box::leak(Box::new(RefCell::new(Foo)));
let _: &dyn Any = &ref_refcell_any.borrow(); // LINT
let _: &dyn Any = &*ref_refcell_any.borrow(); // ok
}

fn very_generic<T, U>(t: &'static T)
where
T: Deref<Target = U> + 'static,
U: Deref<Target = dyn Any + Send> + 'static,
{
let _: &dyn Any = t; // LINT
let _: &dyn Any = &t; // LINT
let _: &dyn Any = &*t; // LINT
let _: &dyn Any = &**t; // LINT
let _: &dyn Any = &***t; // ok
}
Loading

0 comments on commit 8e9c8f6

Please sign in to comment.