nordic-dev.net/rust
nordic-dev.net/rust
2
0
Fork 0
mirror of https://github.com/rust-lang/rust.git synced 2024-11-22 23:04:33 +00:00
Code Issues Packages Projects Releases Wiki Activity

Auto merge of #5774 - ThibsG:FixNewRetNoSelf, r=ebroto

Browse Source 
Fix FP in `new_ret_no_self`: trigger in trait def instead of impl block

Lint in trait def instead of impl block.

Fixes: #5435

changelog: none
This commit is contained in:
bors 2020-08-28 11:54:07 +00:00
commit 3d0b0e66af
4 changed files with 230 additions and 30 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

85
clippy_lints/src/methods/mod.rs
View File

@ -15,11 +15,11 @@ use rustc_ast::ast;
use rustc_errors::Applicability;
use rustc_hir as hir;
use rustc_hir::intravisit::{self, Visitor};
use rustc_hir::{TraitItem, TraitItemKind};
use rustc_lint::{LateContext, LateLintPass, Lint, LintContext};
use rustc_middle::hir::map::Map;
use rustc_middle::lint::in_external_macro;
use rustc_middle::ty::subst::GenericArgKind;
use rustc_middle::ty::{self, Ty, TyS};
use rustc_middle::ty::{self, TraitRef, Ty, TyS};
use rustc_session::{declare_lint_pass, declare_tool_lint};
use rustc_span::source_map::Span;
use rustc_span::symbol::{sym, SymbolStr};
@ -27,12 +27,12 @@ use rustc_span::symbol::{sym, SymbolStr};
use crate::consts::{constant, Constant};
use crate::utils::usage::mutated_variables;
use crate::utils::{
get_arg_name, get_parent_expr, get_trait_def_id, has_iter_method, higher, implements_trait, in_macro, is_copy,
is_ctor_or_promotable_const_function, is_expn_of, is_type_diagnostic_item, iter_input_pats, last_path_segment,
match_def_path, match_qpath, match_trait_method, match_type, match_var, method_calls, method_chain_args, paths,
remove_blocks, return_ty, single_segment_path, snippet, snippet_with_applicability, snippet_with_macro_callsite,
span_lint, span_lint_and_help, span_lint_and_note, span_lint_and_sugg, span_lint_and_then, sugg, walk_ptrs_ty,
walk_ptrs_ty_depth, SpanlessEq,
contains_ty, get_arg_name, get_parent_expr, get_trait_def_id, has_iter_method, higher, implements_trait, in_macro,
is_copy, is_ctor_or_promotable_const_function, is_expn_of, is_type_diagnostic_item, iter_input_pats,
last_path_segment, match_def_path, match_qpath, match_trait_method, match_type, match_var, method_calls,
method_chain_args, paths, remove_blocks, return_ty, single_segment_path, snippet, snippet_with_applicability,
snippet_with_macro_callsite, span_lint, span_lint_and_help, span_lint_and_note, span_lint_and_sugg,
span_lint_and_then, sugg, walk_ptrs_ty, walk_ptrs_ty_depth, SpanlessEq,
};
declare_clippy_lint! {
@ -724,6 +724,7 @@ declare_clippy_lint! {
/// **Known problems:** None.
///
/// **Example:**
/// In an impl block:
/// ```rust
/// # struct Foo;
/// # struct NotAFoo;
@ -736,25 +737,40 @@ declare_clippy_lint! {
///
/// ```rust
/// # struct Foo;
/// # struct FooError;
/// struct Bar(Foo);
/// impl Foo {
/// // Good. Return type contains `Self`
/// fn new() -> Result<Foo, FooError> {
/// # Ok(Foo)
/// // Bad. The type name must contain `Self`
/// fn new() -> Bar {
/// # Bar(Foo)
/// }
/// }
/// ```
///
/// ```rust
/// # struct Foo;
/// struct Bar(Foo);
/// # struct FooError;
/// impl Foo {
/// // Bad. The type name must contain `Self`.
/// fn new() -> Bar {
/// # Bar(Foo)
/// // Good. Return type contains `Self`
/// fn new() -> Result<Foo, FooError> {
/// # Ok(Foo)
/// }
/// }
/// ```
///
/// Or in a trait definition:
/// ```rust
/// pub trait Trait {
/// // Bad. The type name must contain `Self`
/// fn new();
/// }
/// ```
///
/// ```rust
/// pub trait Trait {
/// // Good. Return type contains `Self`
/// fn new() -> Self;
/// }
/// ```
pub NEW_RET_NO_SELF,
style,
"not returning type containing `Self` in a `new` method"
@ -1631,19 +1647,16 @@ impl<'tcx> LateLintPass<'tcx> for Methods {
}
}
// if this impl block implements a trait, lint in trait definition instead
if let hir::ItemKind::Impl { of_trait: Some(_), .. } = item.kind {
return;
}
if let hir::ImplItemKind::Fn(_, _) = impl_item.kind {
let ret_ty = return_ty(cx, impl_item.hir_id);
let contains_self_ty = |ty: Ty<'tcx>| {
ty.walk().any(|inner| match inner.unpack() {
GenericArgKind::Type(inner_ty) => TyS::same_type(self_ty, inner_ty),
GenericArgKind::Lifetime(_) | GenericArgKind::Const(_) => false,
})
};
// walk the return type and check for Self (this does not check associated types)
if contains_self_ty(ret_ty) {
if contains_ty(ret_ty, self_ty) {
return;
}
@ -1653,7 +1666,7 @@ impl<'tcx> LateLintPass<'tcx> for Methods {
for &(predicate, _span) in cx.tcx.predicates_of(def_id).predicates {
if let ty::PredicateAtom::Projection(projection_predicate) = predicate.skip_binders() {
// walk the associated type and check for Self
if contains_self_ty(projection_predicate.ty) {
if contains_ty(projection_predicate.ty, self_ty) {
return;
}
}
@ -1670,6 +1683,26 @@ impl<'tcx> LateLintPass<'tcx> for Methods {
}
}
}
fn check_trait_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx TraitItem<'_>) {
if_chain! {
if !in_external_macro(cx.tcx.sess, item.span);
if item.ident.name == sym!(new);
if let TraitItemKind::Fn(_, _) = item.kind;
let ret_ty = return_ty(cx, item.hir_id);
let self_ty = TraitRef::identity(cx.tcx, item.hir_id.owner.to_def_id()).self_ty();
if !contains_ty(ret_ty, self_ty);
then {
span_lint(
cx,
NEW_RET_NO_SELF,
item.span,
"methods called `new` usually return `Self`",
);
}
}
}
}
/// Checks for the `OR_FUN_CALL` lint.

11
clippy_lints/src/utils/mod.rs
View File

@ -42,7 +42,8 @@ use rustc_hir::{
use rustc_infer::infer::TyCtxtInferExt;
use rustc_lint::{LateContext, Level, Lint, LintContext};
use rustc_middle::hir::map::Map;
use rustc_middle::ty::{self, layout::IntegerExt, subst::GenericArg, Ty, TyCtxt, TypeFoldable};
use rustc_middle::ty::subst::{GenericArg, GenericArgKind};
use rustc_middle::ty::{self, layout::IntegerExt, Ty, TyCtxt, TypeFoldable};
use rustc_mir::const_eval;
use rustc_span::hygiene::{ExpnKind, MacroKind};
use rustc_span::source_map::original_sp;
@ -866,6 +867,14 @@ pub fn return_ty<'tcx>(cx: &LateContext<'tcx>, fn_item: hir::HirId) -> Ty<'tcx>
cx.tcx.erase_late_bound_regions(&ret_ty)
}
/// Walks into `ty` and returns `true` if any inner type is the same as `other_ty`
pub fn contains_ty(ty: Ty<'_>, other_ty: Ty<'_>) -> bool {
ty.walk().any(|inner| match inner.unpack() {
GenericArgKind::Type(inner_ty) => ty::TyS::same_type(other_ty, inner_ty),
GenericArgKind::Lifetime(_) | GenericArgKind::Const(_) => false,
})
}
/// Returns `true` if the given type is an `unsafe` function.
pub fn type_is_unsafe_function<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> bool {
match ty.kind {

134
tests/ui/new_ret_no_self.rs
View File

@ -137,9 +137,9 @@ impl MutPointerReturnerOk {
}
}
struct MutPointerReturnerOk2;
struct ConstPointerReturnerOk2;
impl MutPointerReturnerOk2 {
impl ConstPointerReturnerOk2 {
// should not trigger lint
pub fn new() -> *const Self {
unimplemented!();
@ -210,3 +210,133 @@ impl<'a> WithLifetime<'a> {
unimplemented!();
}
}
mod issue5435 {
struct V;
pub trait TraitRetSelf {
// should not trigger lint
fn new() -> Self;
}
pub trait TraitRet {
// should trigger lint as we are in trait definition
fn new() -> String;
}
pub struct StructRet;
impl TraitRet for StructRet {
// should not trigger lint as we are in the impl block
fn new() -> String {
unimplemented!();
}
}
pub trait TraitRet2 {
// should trigger lint
fn new(_: String) -> String;
}
trait TupleReturnerOk {
// should not trigger lint
fn new() -> (Self, u32)
where
Self: Sized,
{
unimplemented!();
}
}
trait TupleReturnerOk2 {
// should not trigger lint (it doesn't matter which element in the tuple is Self)
fn new() -> (u32, Self)
where
Self: Sized,
{
unimplemented!();
}
}
trait TupleReturnerOk3 {
// should not trigger lint (tuple can contain multiple Self)
fn new() -> (Self, Self)
where
Self: Sized,
{
unimplemented!();
}
}
trait TupleReturnerBad {
// should trigger lint
fn new() -> (u32, u32) {
unimplemented!();
}
}
trait MutPointerReturnerOk {
// should not trigger lint
fn new() -> *mut Self
where
Self: Sized,
{
unimplemented!();
}
}
trait ConstPointerReturnerOk2 {
// should not trigger lint
fn new() -> *const Self
where
Self: Sized,
{
unimplemented!();
}
}
trait MutPointerReturnerBad {
// should trigger lint
fn new() -> *mut V {
unimplemented!();
}
}
trait GenericReturnerOk {
// should not trigger lint
fn new() -> Option<Self>
where
Self: Sized,
{
unimplemented!();
}
}
trait NestedReturnerOk {
// should not trigger lint
fn new() -> (Option<Self>, u32)
where
Self: Sized,
{
unimplemented!();
}
}
trait NestedReturnerOk2 {
// should not trigger lint
fn new() -> ((Self, u32), u32)
where
Self: Sized,
{
unimplemented!();
}
}
trait NestedReturnerOk3 {
// should not trigger lint
fn new() -> Option<(Self, u32)>
where
Self: Sized,
{
unimplemented!();
}
}
}

30
tests/ui/new_ret_no_self.stderr
View File

@ -48,5 +48,33 @@ LL | | unimplemented!();
LL | | }
| |_____^
error: aborting due to 6 previous errors
error: methods called `new` usually return `Self`
--> $DIR/new_ret_no_self.rs:224:9
|
LL | fn new() -> String;
| ^^^^^^^^^^^^^^^^^^^
error: methods called `new` usually return `Self`
--> $DIR/new_ret_no_self.rs:236:9
|
LL | fn new(_: String) -> String;
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
error: methods called `new` usually return `Self`
--> $DIR/new_ret_no_self.rs:271:9
|
LL | / fn new() -> (u32, u32) {
LL | | unimplemented!();
LL | | }
| |_________^
error: methods called `new` usually return `Self`
--> $DIR/new_ret_no_self.rs:298:9
|
LL | / fn new() -> *mut V {
LL | | unimplemented!();
LL | | }
| |_________^
error: aborting due to 10 previous errors