Skip to content
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

Rustup #3889

Closed
wants to merge 9 commits into from
Closed

Rustup #3889

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
10 changes: 5 additions & 5 deletions clippy_lints/src/attrs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -352,22 +352,22 @@ fn check_clippy_lint_names(cx: &LateContext<'_, '_>, items: &[NestedMetaItem]) {
}
}

fn is_relevant_item(tcx: TyCtxt<'_, '_, '_>, item: &Item) -> bool {
fn is_relevant_item<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>, item: &Item) -> bool {
flip1995 marked this conversation as resolved.
Show resolved Hide resolved
if let ItemKind::Fn(_, _, _, eid) = item.node {
is_relevant_expr(tcx, tcx.body_tables(eid), &tcx.hir().body(eid).value)
} else {
true
}
}

fn is_relevant_impl(tcx: TyCtxt<'_, '_, '_>, item: &ImplItem) -> bool {
fn is_relevant_impl<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>, item: &ImplItem) -> bool {
match item.node {
ImplItemKind::Method(_, eid) => is_relevant_expr(tcx, tcx.body_tables(eid), &tcx.hir().body(eid).value),
_ => false,
}
}

fn is_relevant_trait(tcx: TyCtxt<'_, '_, '_>, item: &TraitItem) -> bool {
fn is_relevant_trait<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>, item: &TraitItem) -> bool {
match item.node {
TraitItemKind::Method(_, TraitMethod::Required(_)) => true,
TraitItemKind::Method(_, TraitMethod::Provided(eid)) => {
Expand All @@ -377,7 +377,7 @@ fn is_relevant_trait(tcx: TyCtxt<'_, '_, '_>, item: &TraitItem) -> bool {
}
}

fn is_relevant_block(tcx: TyCtxt<'_, '_, '_>, tables: &ty::TypeckTables<'_>, block: &Block) -> bool {
fn is_relevant_block<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>, tables: &ty::TypeckTables<'_>, block: &Block) -> bool {
if let Some(stmt) = block.stmts.first() {
match &stmt.node {
StmtKind::Local(_) => true,
Expand All @@ -389,7 +389,7 @@ fn is_relevant_block(tcx: TyCtxt<'_, '_, '_>, tables: &ty::TypeckTables<'_>, blo
}
}

fn is_relevant_expr(tcx: TyCtxt<'_, '_, '_>, tables: &ty::TypeckTables<'_>, expr: &Expr) -> bool {
fn is_relevant_expr<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>, tables: &ty::TypeckTables<'_>, expr: &Expr) -> bool {
match &expr.node {
ExprKind::Block(block, _) => is_relevant_block(tcx, tables, block),
ExprKind::Ret(Some(e)) => is_relevant_expr(tcx, tables, e),
Expand Down
32 changes: 16 additions & 16 deletions clippy_lints/src/eta_reduction.rs
Original file line number Diff line number Diff line change
Expand Up @@ -129,54 +129,54 @@ fn get_ufcs_type_name(
method_def_id: def_id::DefId,
self_arg: &Expr,
) -> std::option::Option<String> {
let expected_type_of_self = &cx.tcx.fn_sig(method_def_id).inputs_and_output().skip_binder()[0].sty;
let actual_type_of_self = &cx.tables.node_type(self_arg.hir_id).sty;
let expected_type_of_self = &cx.tcx.fn_sig(method_def_id).inputs_and_output().skip_binder()[0];
let actual_type_of_self = &cx.tables.node_type(self_arg.hir_id);

if let Some(trait_id) = cx.tcx.trait_of_item(method_def_id) {
if match_borrow_depth(expected_type_of_self, actual_type_of_self) {
return Some(cx.tcx.item_path_str(trait_id));
if match_borrow_depth(expected_type_of_self, &actual_type_of_self) {
return Some(cx.tcx.def_path_str(trait_id));
}
}

cx.tcx.impl_of_method(method_def_id).and_then(|_| {
//a type may implicitly implement other type's methods (e.g. Deref)
if match_types(expected_type_of_self, actual_type_of_self) {
if match_types(expected_type_of_self, &actual_type_of_self) {
return Some(get_type_name(cx, &actual_type_of_self));
}
None
})
}

fn match_borrow_depth(lhs: &ty::TyKind<'_>, rhs: &ty::TyKind<'_>) -> bool {
match (lhs, rhs) {
(ty::Ref(_, t1, _), ty::Ref(_, t2, _)) => match_borrow_depth(&t1.sty, &t2.sty),
fn match_borrow_depth(lhs: ty::Ty<'_>, rhs: ty::Ty<'_>) -> bool {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should import Ty and always refer to it unqualified - that's at least the rustc convention.

match (&lhs.sty, &rhs.sty) {
(ty::Ref(_, t1, _), ty::Ref(_, t2, _)) => match_borrow_depth(&t1, &t2),
(l, r) => match (l, r) {
(ty::Ref(_, _, _), _) | (_, ty::Ref(_, _, _)) => false,
(_, _) => true,
},
}
}

fn match_types(lhs: &ty::TyKind<'_>, rhs: &ty::TyKind<'_>) -> bool {
match (lhs, rhs) {
fn match_types(lhs: ty::Ty<'_>, rhs: ty::Ty<'_>) -> bool {
match (&lhs.sty, &rhs.sty) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, random note, cc @Manishearth @oli-obk: this looks like it could use the ty::relate infrastructure.

(ty::Bool, ty::Bool)
| (ty::Char, ty::Char)
| (ty::Int(_), ty::Int(_))
| (ty::Uint(_), ty::Uint(_))
| (ty::Str, ty::Str) => true,
(ty::Ref(_, t1, _), ty::Ref(_, t2, _))
| (ty::Array(t1, _), ty::Array(t2, _))
| (ty::Slice(t1), ty::Slice(t2)) => match_types(&t1.sty, &t2.sty),
| (ty::Slice(t1), ty::Slice(t2)) => match_types(&t1, &t2),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

&x.field, when x: &T, is &(*x).field, so when you want the whole thing instead of the field, you can write just x (&x happens to work, but it's because deref coercions stripping the &).

(ty::Adt(def1, _), ty::Adt(def2, _)) => def1 == def2,
(_, _) => false,
}
}

fn get_type_name(cx: &LateContext<'_, '_>, kind: &ty::TyKind<'_>) -> String {
match kind {
ty::Adt(t, _) => cx.tcx.item_path_str(t.did),
ty::Ref(_, r, _) => get_type_name(cx, &r.sty),
_ => kind.to_string(),
fn get_type_name(cx: &LateContext<'_, '_>, ty: ty::Ty<'_>) -> String {
match ty.sty {
ty::Adt(t, _) => cx.tcx.def_path_str(t.did),
ty::Ref(_, r, _) => get_type_name(cx, &r),
_ => ty.to_string(),
}
}

Expand Down
2 changes: 1 addition & 1 deletion clippy_lints/src/fallible_impl_from.rs
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,7 @@ fn lint_impl_body<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, impl_span: Span, impl_it
}
}

fn match_type(tcx: ty::TyCtxt<'_, '_, '_>, ty: ty::Ty<'_>, path: &[&str]) -> bool {
fn match_type<'a, 'tcx>(tcx: ty::TyCtxt<'a, 'tcx, 'tcx>, ty: ty::Ty<'_>, path: &[&str]) -> bool {
match ty.sty {
ty::Adt(adt, _) => match_def_path(tcx, adt.did, path),
_ => false,
Expand Down
1 change: 1 addition & 0 deletions clippy_lints/src/lib.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
// error-pattern:cargo-clippy

#![feature(box_syntax)]
#![feature(never_type)]
#![feature(rustc_private)]
#![feature(slice_patterns)]
#![feature(stmt_expr_attributes)]
Expand Down
2 changes: 1 addition & 1 deletion clippy_lints/src/matches.rs
Original file line number Diff line number Diff line change
Expand Up @@ -539,7 +539,7 @@ fn check_wild_enum_match(cx: &LateContext<'_, '_>, ex: &Expr, arms: &[Arm]) {
String::new()
};
// This path assumes that the enum type is imported into scope.
format!("{}{}{}", ident_str, cx.tcx.item_path_str(v.did), suffix)
format!("{}{}{}", ident_str, cx.tcx.def_path_str(v.did), suffix)
})
.collect();

Expand Down
2 changes: 1 addition & 1 deletion clippy_lints/src/transmute.rs
Original file line number Diff line number Diff line change
Expand Up @@ -347,7 +347,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Transmute {
|db| {
let arg = sugg::Sugg::hir(cx, &args[0], "..");
let arg = if let ty::Int(_) = from_ty.sty {
arg.as_ty(ty::Uint(ast::UintTy::U32))
arg.as_ty(ast::UintTy::U32)
} else {
arg
};
Expand Down
12 changes: 6 additions & 6 deletions clippy_lints/src/types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ use rustc::hir::intravisit::{walk_body, walk_expr, walk_ty, FnKind, NestedVisito
use rustc::hir::*;
use rustc::lint::{in_external_macro, LateContext, LateLintPass, LintArray, LintContext, LintPass};
use rustc::ty::layout::LayoutOf;
use rustc::ty::print::Printer;
use rustc::ty::{self, InferTy, Ty, TyCtxt, TypeckTables};
use rustc::{declare_tool_lint, lint_array};
use rustc_errors::Applicability;
Expand All @@ -24,7 +25,7 @@ use crate::utils::paths;
use crate::utils::{
clip, comparisons, differing_macro_contexts, higher, in_constant, in_macro, int_bits, last_path_segment,
match_def_path, match_path, multispan_sugg, same_tys, sext, snippet, snippet_opt, snippet_with_applicability,
span_help_and_lint, span_lint, span_lint_and_sugg, span_lint_and_then, unsext, AbsolutePathBuffer,
span_help_and_lint, span_lint, span_lint_and_sugg, span_lint_and_then, unsext, AbsolutePathPrinter,
};

/// Handles all the linting of funky types
Expand Down Expand Up @@ -1135,15 +1136,14 @@ impl LintPass for CastPass {

// Check if the given type is either `core::ffi::c_void` or
// one of the platform specific `libc::<platform>::c_void` of libc.
fn is_c_void(tcx: TyCtxt<'_, '_, '_>, ty: Ty<'_>) -> bool {
fn is_c_void<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>, ty: Ty<'_>) -> bool {
if let ty::Adt(adt, _) = ty.sty {
let mut apb = AbsolutePathBuffer { names: vec![] };
tcx.push_item_path(&mut apb, adt.did, false);
let names = AbsolutePathPrinter { tcx }.print_def_path(adt.did, &[]).unwrap();

if apb.names.is_empty() {
if names.is_empty() {
return false;
}
if apb.names[0] == "libc" || apb.names[0] == "core" && *apb.names.last().unwrap() == "c_void" {
if names[0] == "libc" || names[0] == "core" && *names.last().unwrap() == "c_void" {
return true;
}
}
Expand Down
3 changes: 2 additions & 1 deletion clippy_lints/src/use_self.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ use rustc::hir::intravisit::{walk_item, walk_path, walk_ty, NestedVisitorMap, Vi
use rustc::hir::*;
use rustc::lint::{in_external_macro, LateContext, LateLintPass, LintArray, LintContext, LintPass};
use rustc::ty;
use rustc::ty::DefIdTree;
use rustc::{declare_tool_lint, lint_array};
use rustc_errors::Applicability;
use syntax_pos::symbol::keywords::SelfUpper;
Expand Down Expand Up @@ -233,7 +234,7 @@ impl<'a, 'tcx> Visitor<'tcx> for UseSelfVisitor<'a, 'tcx> {
if self.item_path.def == path.def {
span_use_self_lint(self.cx, path);
} else if let Def::StructCtor(ctor_did, CtorKind::Fn) = path.def {
if self.item_path.def.opt_def_id() == self.cx.tcx.parent_def_id(ctor_did) {
if self.item_path.def.opt_def_id() == self.cx.tcx.parent(ctor_did) {
span_use_self_lint(self.cx, path);
}
}
Expand Down
112 changes: 91 additions & 21 deletions clippy_lints/src/utils/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,10 @@ use if_chain::if_chain;
use matches::matches;
use rustc::hir;
use rustc::hir::def::Def;
use rustc::hir::def_id::CrateNum;
use rustc::hir::def_id::{DefId, CRATE_DEF_INDEX, LOCAL_CRATE};
use rustc::hir::intravisit::{NestedVisitorMap, Visitor};
use rustc::hir::map::DisambiguatedDefPathData;
use rustc::hir::Node;
use rustc::hir::*;
use rustc::lint::{LateContext, Level, Lint, LintContext};
Expand All @@ -41,8 +43,7 @@ use rustc_errors::Applicability;
use syntax::ast::{self, LitKind};
use syntax::attr;
use syntax::source_map::{Span, DUMMY_SP};
use syntax::symbol;
use syntax::symbol::{keywords, Symbol};
use syntax::symbol::{keywords, LocalInternedString, Symbol};

use crate::reexport::*;

Expand Down Expand Up @@ -97,19 +98,90 @@ pub fn in_macro(span: Span) -> bool {
/// Used to store the absolute path to a type.
///
/// See `match_def_path` for usage.
#[derive(Debug)]
pub struct AbsolutePathBuffer {
pub names: Vec<symbol::LocalInternedString>,
pub struct AbsolutePathPrinter<'a, 'tcx> {
pub tcx: TyCtxt<'a, 'tcx, 'tcx>,
}

impl ty::item_path::ItemPathBuffer for AbsolutePathBuffer {
fn root_mode(&self) -> &ty::item_path::RootMode {
const ABSOLUTE: &ty::item_path::RootMode = &ty::item_path::RootMode::Absolute;
ABSOLUTE
use rustc::ty::print::Printer;

#[allow(clippy::diverging_sub_expression)]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This sounds like a false positive - is an issue filled?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not really a FP:
Since type Error = ! and we call let mut path = print_prefix(self)?;, the ? is a diverging sub-expression. IMO allowing this here is ok.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I disagree, ? with a diverging error path should not be warned against, the behavior is more like that of let Ok(mut path) = print_prefix(self);.

cc @Manishearth @oli-obk

impl<'tcx> Printer<'tcx, 'tcx> for AbsolutePathPrinter<'_, 'tcx> {
type Error = !;

type Path = Vec<LocalInternedString>;
type Region = ();
type Type = ();
type DynExistential = ();

fn tcx<'a>(&'a self) -> TyCtxt<'a, 'tcx, 'tcx> {
self.tcx
}

fn print_region(self, _region: ty::Region<'_>) -> Result<Self::Region, Self::Error> {
Ok(())
}

fn print_type(self, _ty: Ty<'tcx>) -> Result<Self::Type, Self::Error> {
Ok(())
}

fn print_dyn_existential(
self,
_predicates: &'tcx ty::List<ty::ExistentialPredicate<'tcx>>,
) -> Result<Self::DynExistential, Self::Error> {
Ok(())
}

fn push(&mut self, text: &str) {
self.names.push(symbol::Symbol::intern(text).as_str());
fn path_crate(self, cnum: CrateNum) -> Result<Self::Path, Self::Error> {
Ok(vec![self.tcx.original_crate_name(cnum).as_str()])
}

fn path_qualified(
self,
self_ty: Ty<'tcx>,
trait_ref: Option<ty::TraitRef<'tcx>>,
) -> Result<Self::Path, Self::Error> {
// This shouldn't ever be needed, but just in case:
Ok(vec![match trait_ref {
Some(trait_ref) => Symbol::intern(&format!("{:?}", trait_ref)).as_str(),
None => Symbol::intern(&format!("<{}>", self_ty)).as_str(),
}])
}

fn path_append_impl(
self,
print_prefix: impl FnOnce(Self) -> Result<Self::Path, Self::Error>,
_disambiguated_data: &DisambiguatedDefPathData,
self_ty: Ty<'tcx>,
trait_ref: Option<ty::TraitRef<'tcx>>,
) -> Result<Self::Path, Self::Error> {
let mut path = print_prefix(self)?;

// This shouldn't ever be needed, but just in case:
path.push(match trait_ref {
Some(trait_ref) => Symbol::intern(&format!("<impl {} for {}>", trait_ref, self_ty)).as_str(),
None => Symbol::intern(&format!("<impl {}>", self_ty)).as_str(),
});

Ok(path)
}

fn path_append(
self,
print_prefix: impl FnOnce(Self) -> Result<Self::Path, Self::Error>,
disambiguated_data: &DisambiguatedDefPathData,
) -> Result<Self::Path, Self::Error> {
let mut path = print_prefix(self)?;
path.push(disambiguated_data.data.as_interned_str().as_str());
Ok(path)
}

fn path_generic_args(
self,
print_prefix: impl FnOnce(Self) -> Result<Self::Path, Self::Error>,
_args: &[Kind<'tcx>],
) -> Result<Self::Path, Self::Error> {
print_prefix(self)
}
}

Expand All @@ -121,12 +193,10 @@ impl ty::item_path::ItemPathBuffer for AbsolutePathBuffer {
/// ```
///
/// See also the `paths` module.
pub fn match_def_path(tcx: TyCtxt<'_, '_, '_>, def_id: DefId, path: &[&str]) -> bool {
let mut apb = AbsolutePathBuffer { names: vec![] };

tcx.push_item_path(&mut apb, def_id, false);
pub fn match_def_path<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>, def_id: DefId, path: &[&str]) -> bool {
let names = get_def_path(tcx, def_id);

apb.names.len() == path.len() && apb.names.into_iter().zip(path.iter()).all(|(a, &b)| *a == *b)
names.len() == path.len() && names.into_iter().zip(path.iter()).all(|(a, &b)| *a == *b)
}

/// Gets the absolute path of `def_id` as a vector of `&str`.
Expand All @@ -138,12 +208,12 @@ pub fn match_def_path(tcx: TyCtxt<'_, '_, '_>, def_id: DefId, path: &[&str]) ->
/// // The given `def_id` is that of an `Option` type
/// };
/// ```
pub fn get_def_path(tcx: TyCtxt<'_, '_, '_>, def_id: DefId) -> Vec<&'static str> {
flip1995 marked this conversation as resolved.
Show resolved Hide resolved
let mut apb = AbsolutePathBuffer { names: vec![] };
tcx.push_item_path(&mut apb, def_id, false);
apb.names
pub fn get_def_path<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>, def_id: DefId) -> Vec<&'static str> {
AbsolutePathPrinter { tcx }
.print_def_path(def_id, &[])
.unwrap()
.iter()
.map(syntax_pos::symbol::LocalInternedString::get)
.map(LocalInternedString::get)
.collect()
}

Expand Down
Loading