Skip to content

Commit

Permalink
rework use_self impl based on ty::Ty comparison rust-lang#3410
Browse files Browse the repository at this point in the history
  • Loading branch information
@tnielens
tnielens committed Apr 26, 2020
1 parent 02c9435 commit 13508cf
Show file tree
Hide file tree
Showing 7 changed files with 291 additions and 198 deletions.
1 change: 1 addition & 0 deletions clippy_lints/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
#![cfg_attr(feature = "deny-warnings", deny(warnings))]
#![feature(crate_visibility_modifier)]
#![feature(concat_idents)]
#![feature(bindings_after_at)]

// FIXME: switch to something more ergonomic here, once available.
// (Currently there is no way to opt into sysroot crates without `extern crate`.)
Expand Down
328 changes: 172 additions & 156 deletions clippy_lints/src/use_self.rs
View file
Original file line number Diff line number Diff line change
@@ -1,22 +1,22 @@
use if_chain::if_chain;
use rustc_errors::Applicability;
use rustc_hir as hir;
use rustc_hir::def::{DefKind, Res};
use rustc_hir::intravisit::{walk_item, walk_path, walk_ty, NestedVisitorMap, Visitor};
use rustc_hir::def::DefKind;
use rustc_hir::intravisit::{walk_expr, walk_impl_item, walk_ty, NestedVisitorMap, Visitor};
use rustc_hir::{
def, FnDecl, FnRetTy, FnSig, GenericArg, HirId, ImplItem, ImplItemKind, Item, ItemKind, Path, PathSegment, QPath,
def, Expr, ExprKind, FnDecl, FnRetTy, FnSig, GenericArg, ImplItem, ImplItemKind, Item, ItemKind, Node, Path, QPath,
TyKind,
};
use rustc_lint::{LateContext, LateLintPass, LintContext};
use rustc_middle::hir::map::Map;
use rustc_middle::lint::in_external_macro;
use rustc_middle::ty;
use rustc_middle::ty::{DefIdTree, Ty};
use rustc_middle::ty::Ty;
use rustc_session::{declare_lint_pass, declare_tool_lint};
use rustc_span::symbol::kw;
use rustc_span::{BytePos, Span};
use rustc_typeck::hir_ty_to_ty;

use crate::utils::{differing_macro_contexts, span_lint_and_sugg};
use crate::utils::span_lint_and_sugg;

declare_clippy_lint! {
/// **What it does:** Checks for unnecessary repetition of structure name when a
Expand Down Expand Up @@ -57,19 +57,7 @@ declare_lint_pass!(UseSelf => [USE_SELF]);

const SEGMENTS_MSG: &str = "segments should be composed of at least 1 element";

fn span_use_self_lint(cx: &LateContext<'_, '_>, path: &Path<'_>, last_segment: Option<&PathSegment<'_>>) {
let last_segment = last_segment.unwrap_or_else(|| path.segments.last().expect(SEGMENTS_MSG));

// Path segments only include actual path, no methods or fields.
let last_path_span = last_segment.ident.span;

if differing_macro_contexts(path.span, last_path_span) {
return;
}

// Only take path up to the end of last_path_span.
let span = path.span.with_hi(last_path_span.hi());

fn span_lint<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, span: Span) {
span_lint_and_sugg(
cx,
USE_SELF,
Expand All @@ -81,78 +69,182 @@ fn span_use_self_lint(cx: &LateContext<'_, '_>, path: &Path<'_>, last_segment: O
);
}

// FIXME: always use this (more correct) visitor, not just in method signatures.
struct SemanticUseSelfVisitor<'a, 'tcx> {
fn span_lint_ignore_last_segment<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, path: &'tcx Path<'tcx>) {
if path.segments.len() > 1 {
span_lint(
cx,
path.span
.with_hi(path.segments[path.segments.len() - 1].ident.span.lo() - BytePos(2)),
)
}
}

fn span_lint_on_qpath_resolved<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, qpath: &'tcx QPath<'tcx>, enum_variant: bool) {
match qpath {
QPath::Resolved(_, path) => {
if enum_variant {
span_lint_ignore_last_segment(cx, path);
} else {
span_lint(cx, path.span);
}
},
_ => (),
};
}

struct ImplVisitor<'a, 'tcx> {
cx: &'a LateContext<'a, 'tcx>,
item: &'tcx Item<'tcx>,
self_ty: Ty<'tcx>,
}

impl<'a, 'tcx> Visitor<'tcx> for SemanticUseSelfVisitor<'a, 'tcx> {
impl<'a, 'tcx> ImplVisitor<'a, 'tcx> {
fn check_trait_method_impl_decl(
&mut self,
impl_item: &ImplItem<'tcx>,
impl_decl: &'tcx FnDecl<'tcx>,
impl_trait_ref: ty::TraitRef<'tcx>,
) {
let tcx = self.cx.tcx;
let trait_method = tcx
.associated_items(impl_trait_ref.def_id)
.find_by_name_and_kind(tcx, impl_item.ident, ty::AssocKind::Fn, impl_trait_ref.def_id)
.expect("impl method matches a trait method");

let trait_method_sig = tcx.fn_sig(trait_method.def_id);
let trait_method_sig = tcx.erase_late_bound_regions(&trait_method_sig);

let output_hir_ty = if let FnRetTy::Return(ty) = &impl_decl.output {
Some(&**ty)
} else {
None
};

// `impl_hir_ty` (of type `hir::Ty`) represents the type written in the signature.
// `trait_ty` (of type `ty::Ty`) is the semantic type for the signature in the trait.
// We use `impl_hir_ty` to see if the type was written as `Self`,
// `hir_ty_to_ty(...)` to check semantic types of paths, and
// `trait_ty` to determine which parts of the signature in the trait, mention
// the type being implemented verbatim (as opposed to `Self`).
for (impl_hir_ty, trait_ty) in impl_decl
.inputs
.iter()
.chain(output_hir_ty)
.zip(trait_method_sig.inputs_and_output)
{
// Check if the input/output type in the trait method specifies the implemented
// type verbatim, and only suggest `Self` if that isn't the case.
// This avoids suggestions to e.g. replace `Vec<u8>` with `Vec<Self>`,
// in an `impl Trait for u8`, when the trait always uses `Vec<u8>`.
// See also https://github.com/rust-lang/rust-clippy/issues/2894.
let self_ty = impl_trait_ref.self_ty();
if !trait_ty.walk().any(|inner| inner == self_ty.into()) {
self.visit_ty(&impl_hir_ty);
}
}
}
}

impl<'a, 'tcx> Visitor<'tcx> for ImplVisitor<'a, 'tcx> {
type Map = Map<'tcx>;

fn visit_ty(&mut self, hir_ty: &'tcx hir::Ty<'_>) {
if let TyKind::Path(QPath::Resolved(_, path)) = &hir_ty.kind {
fn nested_visit_map(&mut self) -> NestedVisitorMap<Self::Map> {
NestedVisitorMap::OnlyBodies(self.cx.tcx.hir())
}

fn visit_ty(&mut self, hir_ty: &'tcx hir::Ty<'tcx>) {
if let TyKind::Path(QPath::Resolved(_, path)) = hir_ty.kind {
match path.res {
def::Res::SelfTy(..) => {},
_ => {
if hir_ty_to_ty(self.cx.tcx, hir_ty) == self.self_ty {
span_use_self_lint(self.cx, path, None);
match self.cx.tcx.hir().find(self.cx.tcx.hir().get_parent_node(hir_ty.hir_id)) {
Some(Node::Expr(Expr {
kind: ExprKind::Path(QPath::TypeRelative(_, last_segment)),
..
})) =>
// FIXME: this span manipulation should not be necessary
// @flip1995 found an ast lowering issue in
// https://github.com/rust-lang/rust/blob/master/src/librustc_ast_lowering/path.rs#L142-L162
{
span_lint(self.cx, hir_ty.span.with_hi(last_segment.ident.span.lo() - BytePos(2)))
},
_ => span_lint(self.cx, hir_ty.span),
}
}
},
}
}

walk_ty(self, hir_ty)
}

fn nested_visit_map(&mut self) -> NestedVisitorMap<Self::Map> {
NestedVisitorMap::None
walk_ty(self, hir_ty);
}
}

fn check_trait_method_impl_decl<'a, 'tcx>(
cx: &'a LateContext<'a, 'tcx>,
impl_item: &ImplItem<'_>,
impl_decl: &'tcx FnDecl<'_>,
impl_trait_ref: ty::TraitRef<'tcx>,
) {
let trait_method = cx
.tcx
.associated_items(impl_trait_ref.def_id)
.find_by_name_and_kind(cx.tcx, impl_item.ident, ty::AssocKind::Fn, impl_trait_ref.def_id)
.expect("impl method matches a trait method");

let trait_method_sig = cx.tcx.fn_sig(trait_method.def_id);
let trait_method_sig = cx.tcx.erase_late_bound_regions(&trait_method_sig);

let output_hir_ty = if let FnRetTy::Return(ty) = &impl_decl.output {
Some(&**ty)
} else {
None
};
fn visit_expr(&mut self, expr: &'tcx Expr<'tcx>) {
fn expr_ty_matches<'a, 'tcx>(expr: &'tcx Expr<'tcx>, self_ty: Ty<'tcx>, cx: &LateContext<'a, 'tcx>) -> bool {
cx.tcx
.typeck_tables_of(expr.hir_id.owner.to_def_id())
.node_type(expr.hir_id)
== self_ty
}
match expr.kind {
ExprKind::Struct(QPath::Resolved(_, path), ..) => {
if expr_ty_matches(expr, self.self_ty, self.cx) {
match path.res {
def::Res::SelfTy(..) => (),
def::Res::Def(DefKind::Variant, _) => span_lint_ignore_last_segment(self.cx, path),
_ => {
span_lint(self.cx, path.span);
},
}
}
},
// type-relative fn calls (`Foo::new()`) and tuple-like instantiation (`Foo(arg)` or `Enum::Foo(arg)`)
ExprKind::Call(
fun
@ Expr {
kind: ExprKind::Path(ref qpath),
..
},
_,
) => {
if expr_ty_matches(expr, self.self_ty, self.cx) {
let res = self.cx.tables.qpath_res(qpath, fun.hir_id);

// `impl_hir_ty` (of type `hir::Ty`) represents the type written in the signature.
// `trait_ty` (of type `ty::Ty`) is the semantic type for the signature in the trait.
// We use `impl_hir_ty` to see if the type was written as `Self`,
// `hir_ty_to_ty(...)` to check semantic types of paths, and
// `trait_ty` to determine which parts of the signature in the trait, mention
// the type being implemented verbatim (as opposed to `Self`).
for (impl_hir_ty, trait_ty) in impl_decl
.inputs
.iter()
.chain(output_hir_ty)
.zip(trait_method_sig.inputs_and_output)
{
// Check if the input/output type in the trait method specifies the implemented
// type verbatim, and only suggest `Self` if that isn't the case.
// This avoids suggestions to e.g. replace `Vec<u8>` with `Vec<Self>`,
// in an `impl Trait for u8`, when the trait always uses `Vec<u8>`.
// See also https://github.com/rust-lang/rust-clippy/issues/2894.
let self_ty = impl_trait_ref.self_ty();
if !trait_ty.walk().any(|inner| inner == self_ty.into()) {
let mut visitor = SemanticUseSelfVisitor { cx, self_ty };
if let def::Res::Def(DefKind::Ctor(ctor_of, _), ..) = res {
match ctor_of {
def::CtorOf::Variant => span_lint_on_qpath_resolved(self.cx, qpath, true),
def::CtorOf::Struct => span_lint_on_qpath_resolved(self.cx, qpath, false),
}
} else if let def::Res::Def(DefKind::Variant, ..) = res {
span_lint_on_qpath_resolved(self.cx, qpath, true);
}
}
},
// unit enum variants (`Enum::A`)
ExprKind::Path(ref qpath) => {
if expr_ty_matches(expr, self.self_ty, self.cx) {
span_lint_on_qpath_resolved(self.cx, qpath, true);
}
},
_ => (),
}
walk_expr(self, expr);
}

visitor.visit_ty(&impl_hir_ty);
fn visit_impl_item(&mut self, ii: &'tcx ImplItem<'tcx>) {
let tcx = self.cx.tcx;
let impl_def_id = tcx.hir().local_def_id(self.item.hir_id);
let impl_trait_ref = tcx.impl_trait_ref(impl_def_id);
if_chain! {
if let Some(impl_trait_ref) = impl_trait_ref;
if let ImplItemKind::Fn(FnSig { decl: impl_decl, .. }, impl_body_id) = &ii.kind;
then {
self.check_trait_method_impl_decl(ii, impl_decl, impl_trait_ref);
let body = tcx.hir().body(*impl_body_id);
self.visit_body(body);
} else {
walk_impl_item(self, ii)
}
}
}
}
Expand All @@ -176,97 +268,21 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for UseSelf {
true
};

// TODO: don't short-circuit upon lifetime parameters
if should_check {
let visitor = &mut UseSelfVisitor {
item_path,
let self_ty= hir_ty_to_ty(cx.tcx, item_type);
let visitor = &mut ImplVisitor {
cx,
item,
self_ty,
};
let impl_def_id = cx.tcx.hir().local_def_id(item.hir_id);
let impl_trait_ref = cx.tcx.impl_trait_ref(impl_def_id);

if let Some(impl_trait_ref) = impl_trait_ref {
for impl_item_ref in refs {
let impl_item = cx.tcx.hir().impl_item(impl_item_ref.id);
if let ImplItemKind::Fn(FnSig{ decl: impl_decl, .. }, impl_body_id)
= &impl_item.kind {
check_trait_method_impl_decl(cx, impl_item, impl_decl, impl_trait_ref);

let body = cx.tcx.hir().body(*impl_body_id);
visitor.visit_body(body);
} else {
visitor.visit_impl_item(impl_item);
}
}
} else {
for impl_item_ref in refs {
let impl_item = cx.tcx.hir().impl_item(impl_item_ref.id);
visitor.visit_impl_item(impl_item);
}
for impl_item_ref in refs {
let impl_item = cx.tcx.hir().impl_item(impl_item_ref.id);
visitor.visit_impl_item(impl_item);
}
}
}
}
}
}

struct UseSelfVisitor<'a, 'tcx> {
item_path: &'a Path<'a>,
cx: &'a LateContext<'a, 'tcx>,
}

impl<'a, 'tcx> Visitor<'tcx> for UseSelfVisitor<'a, 'tcx> {
type Map = Map<'tcx>;

fn visit_path(&mut self, path: &'tcx Path<'_>, _id: HirId) {
if !path.segments.iter().any(|p| p.ident.span.is_dummy()) {
if path.segments.len() >= 2 {
let last_but_one = &path.segments[path.segments.len() - 2];
if last_but_one.ident.name != kw::SelfUpper {
let enum_def_id = match path.res {
Res::Def(DefKind::Variant, variant_def_id) => self.cx.tcx.parent(variant_def_id),
Res::Def(DefKind::Ctor(def::CtorOf::Variant, _), ctor_def_id) => {
let variant_def_id = self.cx.tcx.parent(ctor_def_id);
variant_def_id.and_then(|def_id| self.cx.tcx.parent(def_id))
},
_ => None,
};

if self.item_path.res.opt_def_id() == enum_def_id {
span_use_self_lint(self.cx, path, Some(last_but_one));
}
}
}

if path.segments.last().expect(SEGMENTS_MSG).ident.name != kw::SelfUpper {
if self.item_path.res == path.res {
span_use_self_lint(self.cx, path, None);
} else if let Res::Def(DefKind::Ctor(def::CtorOf::Struct, _), ctor_def_id) = path.res {
if self.item_path.res.opt_def_id() == self.cx.tcx.parent(ctor_def_id) {
span_use_self_lint(self.cx, path, None);
}
}
}
}

walk_path(self, path);
}

fn visit_item(&mut self, item: &'tcx Item<'_>) {
match item.kind {
ItemKind::Use(..)
| ItemKind::Static(..)
| ItemKind::Enum(..)
| ItemKind::Struct(..)
| ItemKind::Union(..)
| ItemKind::Impl { .. }
| ItemKind::Fn(..) => {
// Don't check statements that shadow `Self` or where `Self` can't be used
},
_ => walk_item(self, item),
}
}

fn nested_visit_map(&mut self) -> NestedVisitorMap<Self::Map> {
NestedVisitorMap::All(self.cx.tcx.hir())
}
}
Loading

0 comments on commit 13508cf

Please sign in to comment.