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 Oct 29, 2018
1 parent 00ed705 commit ca7408b
Show file tree
Hide file tree
Showing 8 changed files with 379 additions and 3 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -899,6 +899,7 @@ All notable changes to this project will be documented in this file.
[`write_literal`]: https://rust-lang-nursery.github.io/rust-clippy/master/index.html#write_literal
[`write_with_newline`]: https://rust-lang-nursery.github.io/rust-clippy/master/index.html#write_with_newline
[`writeln_empty_string`]: https://rust-lang-nursery.github.io/rust-clippy/master/index.html#writeln_empty_string
[`wrong_any_coerce`]: https://rust-lang-nursery.github.io/rust-clippy/master/index.html#wrong_any_coerce
[`wrong_pub_self_convention`]: https://rust-lang-nursery.github.io/rust-clippy/master/index.html#wrong_pub_self_convention
[`wrong_self_convention`]: https://rust-lang-nursery.github.io/rust-clippy/master/index.html#wrong_self_convention
[`wrong_transmute`]: https://rust-lang-nursery.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 @@ -9,7 +9,7 @@ We are currently in the process of discussing Clippy 1.0 via the RFC process in

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

[There are 283 lints included in this crate!](https://rust-lang-nursery.github.io/rust-clippy/master/index.html)
[There are 284 lints included in this crate!](https://rust-lang-nursery.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
208 changes: 208 additions & 0 deletions clippy_lints/src/any_coerce.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,208 @@
// 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;
use crate::rustc::ty::{self, ToPolyTraitRef, Ty};
use crate::rustc::{declare_tool_lint, lint_array};
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;

/// **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;
/// ```
declare_clippy_lint! {
pub WRONG_ANY_COERCE,
correctness,
"coercing a type already containing `dyn Any` to `dyn Any` itself"
}

#[derive(Default)]
pub struct WrongAnyCoerce;

impl LintPass for WrongAnyCoerce {
fn get_lints(&self) -> LintArray {
lint_array!(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::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(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>(
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;
// 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(
tcx.lang_items().coerce_unsized_trait().unwrap(),
tcx.mk_substs_trait(src_ty, &[tgt_ty.into()]),
)
.to_poly_trait_ref(),
);
while let Some(trait_ref) = queue.pop_front() {
if match_def_path(tcx, trait_ref.def_id(), &paths::ANY_TRAIT) {
// found something unsizing to `dyn Any`
let coerced_to_any = trait_ref.self_ty();
if type_contains_any(&mut selcx, param_env, coerced_to_any) {
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
queue.extend(
vtable
.nested_obligations()
.into_iter()
.filter_map(|oblig| oblig.predicate.to_opt_poly_trait_ref()),
);
}
}
None
}

fn type_contains_any<'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 let ty::TyKind::Dynamic(trait_list, _) = any_src_deref_ty.sty;
if match_def_path(selcx.tcx(), trait_list.skip_binder().principal().def_id, &paths::ANY_TRAIT);
then {
// TODO: use deref_count to make a suggestion
return true;
}
}
// TODO: check for `RefCell<dyn Any>`?
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(oblig));
if obligations_ok {
Some(infcx.resolve_type_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 @@ -86,6 +86,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 assign_ops;
Expand Down Expand Up @@ -454,6 +455,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry<'_>, conf: &Conf) {
reg.register_late_lint_pass(box non_copy_const::NonCopyConst);
reg.register_late_lint_pass(box ptr_offset_with_cast::Pass);
reg.register_late_lint_pass(box redundant_clone::RedundantClone);
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 @@ -527,6 +529,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,
assign_ops::ASSIGN_OP_PATTERN,
assign_ops::MISREFACTORED_ASSIGN_OP,
Expand Down Expand Up @@ -900,6 +903,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 @@ -362,7 +362,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
Expand Up @@ -11,7 +11,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
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> = 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 ca7408b

Please sign in to comment.