From cd159fd7f9f8eec5792e6fd7f1b347ea3e028301 Mon Sep 17 00:00:00 2001 From: Michael Howell Date: Wed, 19 Aug 2020 03:05:44 -0700 Subject: [PATCH 1/2] Uplift drop-bounds lint from clippy --- compiler/rustc_lint/src/lib.rs | 3 + compiler/rustc_lint/src/traits.rs | 79 +++++++++++++++++++ library/core/src/mem/mod.rs | 1 + .../ui/drop-bounds/drop-bounds-impl-drop.rs | 14 ++++ src/test/ui/drop-bounds/drop-bounds.rs | 19 +++++ src/test/ui/drop-bounds/drop-bounds.stderr | 50 ++++++++++++ 6 files changed, 166 insertions(+) create mode 100644 compiler/rustc_lint/src/traits.rs create mode 100644 src/test/ui/drop-bounds/drop-bounds-impl-drop.rs create mode 100644 src/test/ui/drop-bounds/drop-bounds.rs create mode 100644 src/test/ui/drop-bounds/drop-bounds.stderr diff --git a/compiler/rustc_lint/src/lib.rs b/compiler/rustc_lint/src/lib.rs index 33caedfc198..49e80f9d8a5 100644 --- a/compiler/rustc_lint/src/lib.rs +++ b/compiler/rustc_lint/src/lib.rs @@ -53,6 +53,7 @@ mod non_ascii_idents; mod nonstandard_style; mod passes; mod redundant_semicolon; +mod traits; mod types; mod unused; @@ -75,6 +76,7 @@ use internal::*; use non_ascii_idents::*; use nonstandard_style::*; use redundant_semicolon::*; +use traits::*; use types::*; use unused::*; @@ -157,6 +159,7 @@ macro_rules! late_lint_passes { MissingDebugImplementations: MissingDebugImplementations::default(), ArrayIntoIter: ArrayIntoIter, ClashingExternDeclarations: ClashingExternDeclarations::new(), + DropTraitConstraints: DropTraitConstraints, ] ); }; diff --git a/compiler/rustc_lint/src/traits.rs b/compiler/rustc_lint/src/traits.rs new file mode 100644 index 00000000000..d4f79036e5a --- /dev/null +++ b/compiler/rustc_lint/src/traits.rs @@ -0,0 +1,79 @@ +use crate::LateContext; +use crate::LateLintPass; +use crate::LintContext; +use rustc_hir as hir; +use rustc_span::symbol::sym; + +declare_lint! { + /// The `drop_bounds` lint checks for generics with `std::ops::Drop` as + /// bounds. + /// + /// ### Example + /// + /// ```rust + /// fn foo() {} + /// ``` + /// + /// {{produces}} + /// + /// ### Explanation + /// + /// `Drop` bounds do not really accomplish anything. A type may have + /// compiler-generated drop glue without implementing the `Drop` trait + /// itself. The `Drop` trait also only has one method, `Drop::drop`, and + /// that function is by fiat not callable in user code. So there is really + /// no use case for using `Drop` in trait bounds. + /// + /// The most likely use case of a drop bound is to distinguish between + /// types that have destructors and types that don't. Combined with + /// specialization, a naive coder would write an implementation that + /// assumed a type could be trivially dropped, then write a specialization + /// for `T: Drop` that actually calls the destructor. Except that doing so + /// is not correct; String, for example, doesn't actually implement Drop, + /// but because String contains a Vec, assuming it can be trivially dropped + /// will leak memory. + pub DROP_BOUNDS, + Warn, + "bounds of the form `T: Drop` are useless" +} + +declare_lint_pass!( + /// Lint for bounds of the form `T: Drop`, which usually + /// indicate an attempt to emulate `std::mem::needs_drop`. + DropTraitConstraints => [DROP_BOUNDS] +); + +impl<'tcx> LateLintPass<'tcx> for DropTraitConstraints { + fn check_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx hir::Item<'tcx>) { + use rustc_middle::ty::PredicateAtom::*; + + let def_id = cx.tcx.hir().local_def_id(item.hir_id); + let predicates = cx.tcx.explicit_predicates_of(def_id); + for &(predicate, span) in predicates.predicates { + let trait_predicate = match predicate.skip_binders() { + Trait(trait_predicate, _constness) => trait_predicate, + _ => continue, + }; + let def_id = trait_predicate.trait_ref.def_id; + if cx.tcx.lang_items().drop_trait() == Some(def_id) { + // Explicitly allow `impl Drop`, a drop-guards-as-Voldemort-type pattern. + if trait_predicate.trait_ref.self_ty().is_impl_trait() { + continue; + } + cx.struct_span_lint(DROP_BOUNDS, span, |lint| { + let needs_drop = match cx.tcx.get_diagnostic_item(sym::needs_drop) { + Some(needs_drop) => needs_drop, + None => return, + }; + let msg = format!( + "bounds on `{}` are useless, consider instead \ + using `{}` to detect if a type has a destructor", + predicate, + cx.tcx.def_path_str(needs_drop) + ); + lint.build(&msg).emit() + }); + } + } + } +} diff --git a/library/core/src/mem/mod.rs b/library/core/src/mem/mod.rs index aa1b5529df2..a2c7da6e695 100644 --- a/library/core/src/mem/mod.rs +++ b/library/core/src/mem/mod.rs @@ -568,6 +568,7 @@ pub unsafe fn align_of_val_raw(val: *const T) -> usize { #[inline] #[stable(feature = "needs_drop", since = "1.21.0")] #[rustc_const_stable(feature = "const_needs_drop", since = "1.36.0")] +#[rustc_diagnostic_item = "needs_drop"] pub const fn needs_drop() -> bool { intrinsics::needs_drop::() } diff --git a/src/test/ui/drop-bounds/drop-bounds-impl-drop.rs b/src/test/ui/drop-bounds/drop-bounds-impl-drop.rs new file mode 100644 index 00000000000..063efc7b31a --- /dev/null +++ b/src/test/ui/drop-bounds/drop-bounds-impl-drop.rs @@ -0,0 +1,14 @@ +// run-pass +#![deny(drop_bounds)] +// As a special exemption, `impl Drop` in the return position raises no error. +// This allows a convenient way to return an unnamed drop guard. +fn voldemort_type() -> impl Drop { + struct Voldemort; + impl Drop for Voldemort { + fn drop(&mut self) {} + } + Voldemort +} +fn main() { + let _ = voldemort_type(); +} diff --git a/src/test/ui/drop-bounds/drop-bounds.rs b/src/test/ui/drop-bounds/drop-bounds.rs new file mode 100644 index 00000000000..c73538278d3 --- /dev/null +++ b/src/test/ui/drop-bounds/drop-bounds.rs @@ -0,0 +1,19 @@ +#![deny(drop_bounds)] +fn foo() {} //~ ERROR +fn bar() +where + U: Drop, //~ ERROR +{ +} +fn baz(_x: impl Drop) {} //~ ERROR +struct Foo { //~ ERROR + _x: T +} +struct Bar where U: Drop { //~ ERROR + _x: U +} +trait Baz: Drop { //~ ERROR +} +impl Baz for T { //~ ERROR +} +fn main() {} diff --git a/src/test/ui/drop-bounds/drop-bounds.stderr b/src/test/ui/drop-bounds/drop-bounds.stderr new file mode 100644 index 00000000000..15ba4c9a649 --- /dev/null +++ b/src/test/ui/drop-bounds/drop-bounds.stderr @@ -0,0 +1,50 @@ +error: bounds on `T: Drop` are useless, consider instead using `std::mem::needs_drop` to detect if a type has a destructor + --> $DIR/drop-bounds.rs:2:11 + | +LL | fn foo() {} + | ^^^^ + | +note: the lint level is defined here + --> $DIR/drop-bounds.rs:1:9 + | +LL | #![deny(drop_bounds)] + | ^^^^^^^^^^^ + +error: bounds on `U: Drop` are useless, consider instead using `std::mem::needs_drop` to detect if a type has a destructor + --> $DIR/drop-bounds.rs:5:8 + | +LL | U: Drop, + | ^^^^ + +error: bounds on `impl Drop: Drop` are useless, consider instead using `std::mem::needs_drop` to detect if a type has a destructor + --> $DIR/drop-bounds.rs:8:17 + | +LL | fn baz(_x: impl Drop) {} + | ^^^^ + +error: bounds on `T: Drop` are useless, consider instead using `std::mem::needs_drop` to detect if a type has a destructor + --> $DIR/drop-bounds.rs:9:15 + | +LL | struct Foo { + | ^^^^ + +error: bounds on `U: Drop` are useless, consider instead using `std::mem::needs_drop` to detect if a type has a destructor + --> $DIR/drop-bounds.rs:12:24 + | +LL | struct Bar where U: Drop { + | ^^^^ + +error: bounds on `Self: Drop` are useless, consider instead using `std::mem::needs_drop` to detect if a type has a destructor + --> $DIR/drop-bounds.rs:15:12 + | +LL | trait Baz: Drop { + | ^^^^ + +error: bounds on `T: Drop` are useless, consider instead using `std::mem::needs_drop` to detect if a type has a destructor + --> $DIR/drop-bounds.rs:17:9 + | +LL | impl Baz for T { + | ^^^^ + +error: aborting due to 7 previous errors + From dceb81af1ea4cbefd5e8bf49f738e03e9e3fac9c Mon Sep 17 00:00:00 2001 From: Michael Howell Date: Fri, 2 Oct 2020 11:34:14 -0700 Subject: [PATCH 2/2] Deprecate clippy lint --- .../clippy_lints/src/deprecated_lints.rs | 9 +++ .../clippy/clippy_lints/src/drop_bounds.rs | 73 ------------------- src/tools/clippy/clippy_lints/src/lib.rs | 9 +-- .../clippy/clippy_lints/src/utils/paths.rs | 1 - src/tools/clippy/src/lintlist/mod.rs | 7 -- src/tools/clippy/tests/ui/deprecated.rs | 1 + src/tools/clippy/tests/ui/deprecated.stderr | 8 +- src/tools/clippy/tests/ui/drop_bounds.rs | 8 -- src/tools/clippy/tests/ui/drop_bounds.stderr | 16 ---- 9 files changed, 21 insertions(+), 111 deletions(-) delete mode 100644 src/tools/clippy/clippy_lints/src/drop_bounds.rs delete mode 100644 src/tools/clippy/tests/ui/drop_bounds.rs delete mode 100644 src/tools/clippy/tests/ui/drop_bounds.stderr diff --git a/src/tools/clippy/clippy_lints/src/deprecated_lints.rs b/src/tools/clippy/clippy_lints/src/deprecated_lints.rs index c17a0e83330..c5884361dff 100644 --- a/src/tools/clippy/clippy_lints/src/deprecated_lints.rs +++ b/src/tools/clippy/clippy_lints/src/deprecated_lints.rs @@ -163,3 +163,12 @@ declare_deprecated_lint! { pub REGEX_MACRO, "the regex! macro has been removed from the regex crate in 2018" } + +declare_deprecated_lint! { + /// **What it does:** Nothing. This lint has been deprecated. + /// + /// **Deprecation reason:** This lint has been uplifted to rustc and is now called + /// `drop_bounds`. + pub DROP_BOUNDS, + "this lint has been uplifted to rustc and is now called `drop_bounds`" +} diff --git a/src/tools/clippy/clippy_lints/src/drop_bounds.rs b/src/tools/clippy/clippy_lints/src/drop_bounds.rs deleted file mode 100644 index ec3b6afa630..00000000000 --- a/src/tools/clippy/clippy_lints/src/drop_bounds.rs +++ /dev/null @@ -1,73 +0,0 @@ -use crate::utils::{match_def_path, paths, span_lint}; -use if_chain::if_chain; -use rustc_hir::{GenericBound, GenericParam, WhereBoundPredicate, WherePredicate}; -use rustc_lint::{LateContext, LateLintPass}; -use rustc_session::{declare_lint_pass, declare_tool_lint}; - -declare_clippy_lint! { - /// **What it does:** Checks for generics with `std::ops::Drop` as bounds. - /// - /// **Why is this bad?** `Drop` bounds do not really accomplish anything. - /// A type may have compiler-generated drop glue without implementing the - /// `Drop` trait itself. The `Drop` trait also only has one method, - /// `Drop::drop`, and that function is by fiat not callable in user code. - /// So there is really no use case for using `Drop` in trait bounds. - /// - /// The most likely use case of a drop bound is to distinguish between types - /// that have destructors and types that don't. Combined with specialization, - /// a naive coder would write an implementation that assumed a type could be - /// trivially dropped, then write a specialization for `T: Drop` that actually - /// calls the destructor. Except that doing so is not correct; String, for - /// example, doesn't actually implement Drop, but because String contains a - /// Vec, assuming it can be trivially dropped will leak memory. - /// - /// **Known problems:** None. - /// - /// **Example:** - /// ```rust - /// fn foo() {} - /// ``` - /// Could be written as: - /// ```rust - /// fn foo() {} - /// ``` - pub DROP_BOUNDS, - correctness, - "bounds of the form `T: Drop` are useless" -} - -const DROP_BOUNDS_SUMMARY: &str = "bounds of the form `T: Drop` are useless, \ - use `std::mem::needs_drop` to detect if a type has drop glue"; - -declare_lint_pass!(DropBounds => [DROP_BOUNDS]); - -impl<'tcx> LateLintPass<'tcx> for DropBounds { - fn check_generic_param(&mut self, cx: &LateContext<'tcx>, p: &'tcx GenericParam<'_>) { - for bound in p.bounds.iter() { - lint_bound(cx, bound); - } - } - fn check_where_predicate(&mut self, cx: &LateContext<'tcx>, p: &'tcx WherePredicate<'_>) { - if let WherePredicate::BoundPredicate(WhereBoundPredicate { bounds, .. }) = p { - for bound in *bounds { - lint_bound(cx, bound); - } - } - } -} - -fn lint_bound<'tcx>(cx: &LateContext<'tcx>, bound: &'tcx GenericBound<'_>) { - if_chain! { - if let GenericBound::Trait(t, _) = bound; - if let Some(def_id) = t.trait_ref.path.res.opt_def_id(); - if match_def_path(cx, def_id, &paths::DROP_TRAIT); - then { - span_lint( - cx, - DROP_BOUNDS, - t.span, - DROP_BOUNDS_SUMMARY - ); - } - } -} diff --git a/src/tools/clippy/clippy_lints/src/lib.rs b/src/tools/clippy/clippy_lints/src/lib.rs index c3ff34e6e1e..70efdaeb9c6 100644 --- a/src/tools/clippy/clippy_lints/src/lib.rs +++ b/src/tools/clippy/clippy_lints/src/lib.rs @@ -179,7 +179,6 @@ mod derive; mod doc; mod double_comparison; mod double_parens; -mod drop_bounds; mod drop_forget_ref; mod duration_subsec; mod else_if_without_else; @@ -478,6 +477,10 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: "clippy::regex_macro", "the regex! macro has been removed from the regex crate in 2018", ); + store.register_removed( + "clippy::drop_bounds", + "this lint has been uplifted to rustc and is now called `drop_bounds`", + ); // end deprecated lints, do not remove this comment, it’s used in `update_lints` // begin register lints, do not remove this comment, it’s used in `update_lints` @@ -532,7 +535,6 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: &doc::NEEDLESS_DOCTEST_MAIN, &double_comparison::DOUBLE_COMPARISONS, &double_parens::DOUBLE_PARENS, - &drop_bounds::DROP_BOUNDS, &drop_forget_ref::DROP_COPY, &drop_forget_ref::DROP_REF, &drop_forget_ref::FORGET_COPY, @@ -959,7 +961,6 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: store.register_late_pass(|| box strings::StringLitAsBytes); store.register_late_pass(|| box derive::Derive); store.register_late_pass(|| box types::CharLitAsU8); - store.register_late_pass(|| box drop_bounds::DropBounds); store.register_late_pass(|| box get_last_with_len::GetLastWithLen); store.register_late_pass(|| box drop_forget_ref::DropForgetRef); store.register_late_pass(|| box empty_enum::EmptyEnum); @@ -1282,7 +1283,6 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(&doc::NEEDLESS_DOCTEST_MAIN), LintId::of(&double_comparison::DOUBLE_COMPARISONS), LintId::of(&double_parens::DOUBLE_PARENS), - LintId::of(&drop_bounds::DROP_BOUNDS), LintId::of(&drop_forget_ref::DROP_COPY), LintId::of(&drop_forget_ref::DROP_REF), LintId::of(&drop_forget_ref::FORGET_COPY), @@ -1714,7 +1714,6 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(&copies::IF_SAME_THEN_ELSE), LintId::of(&derive::DERIVE_HASH_XOR_EQ), LintId::of(&derive::DERIVE_ORD_XOR_PARTIAL_ORD), - LintId::of(&drop_bounds::DROP_BOUNDS), LintId::of(&drop_forget_ref::DROP_COPY), LintId::of(&drop_forget_ref::DROP_REF), LintId::of(&drop_forget_ref::FORGET_COPY), diff --git a/src/tools/clippy/clippy_lints/src/utils/paths.rs b/src/tools/clippy/clippy_lints/src/utils/paths.rs index 1583afad208..be837a61dc0 100644 --- a/src/tools/clippy/clippy_lints/src/utils/paths.rs +++ b/src/tools/clippy/clippy_lints/src/utils/paths.rs @@ -31,7 +31,6 @@ pub const DISPLAY_FMT_METHOD: [&str; 4] = ["core", "fmt", "Display", "fmt"]; pub const DISPLAY_TRAIT: [&str; 3] = ["core", "fmt", "Display"]; pub const DOUBLE_ENDED_ITERATOR: [&str; 4] = ["core", "iter", "traits", "DoubleEndedIterator"]; pub const DROP: [&str; 3] = ["core", "mem", "drop"]; -pub const DROP_TRAIT: [&str; 4] = ["core", "ops", "drop", "Drop"]; pub const DURATION: [&str; 3] = ["core", "time", "Duration"]; pub const EARLY_CONTEXT: [&str; 4] = ["rustc", "lint", "context", "EarlyContext"]; pub const EXIT: [&str; 3] = ["std", "process", "exit"]; diff --git a/src/tools/clippy/src/lintlist/mod.rs b/src/tools/clippy/src/lintlist/mod.rs index 9603023ed06..f6d529de9a3 100644 --- a/src/tools/clippy/src/lintlist/mod.rs +++ b/src/tools/clippy/src/lintlist/mod.rs @@ -423,13 +423,6 @@ pub static ref ALL_LINTS: Vec = vec![ deprecation: None, module: "double_parens", }, - Lint { - name: "drop_bounds", - group: "correctness", - desc: "bounds of the form `T: Drop` are useless", - deprecation: None, - module: "drop_bounds", - }, Lint { name: "drop_copy", group: "correctness", diff --git a/src/tools/clippy/tests/ui/deprecated.rs b/src/tools/clippy/tests/ui/deprecated.rs index 3eefb232780..9e32fe36ece 100644 --- a/src/tools/clippy/tests/ui/deprecated.rs +++ b/src/tools/clippy/tests/ui/deprecated.rs @@ -8,5 +8,6 @@ #[warn(clippy::into_iter_on_array)] #[warn(clippy::unused_label)] #[warn(clippy::regex_macro)] +#[warn(clippy::drop_bounds)] fn main() {} diff --git a/src/tools/clippy/tests/ui/deprecated.stderr b/src/tools/clippy/tests/ui/deprecated.stderr index a80e2bf31fe..d3400a7be09 100644 --- a/src/tools/clippy/tests/ui/deprecated.stderr +++ b/src/tools/clippy/tests/ui/deprecated.stderr @@ -60,11 +60,17 @@ error: lint `clippy::regex_macro` has been removed: `the regex! macro has been r LL | #[warn(clippy::regex_macro)] | ^^^^^^^^^^^^^^^^^^^ +error: lint `clippy::drop_bounds` has been removed: `this lint has been uplifted to rustc and is now called `drop_bounds`` + --> $DIR/deprecated.rs:11:8 + | +LL | #[warn(clippy::drop_bounds)] + | ^^^^^^^^^^^^^^^^^^^ + error: lint `clippy::str_to_string` has been removed: `using `str::to_string` is common even today and specialization will likely happen soon` --> $DIR/deprecated.rs:1:8 | LL | #[warn(clippy::str_to_string)] | ^^^^^^^^^^^^^^^^^^^^^ -error: aborting due to 11 previous errors +error: aborting due to 12 previous errors diff --git a/src/tools/clippy/tests/ui/drop_bounds.rs b/src/tools/clippy/tests/ui/drop_bounds.rs deleted file mode 100644 index 6d6a9dc0783..00000000000 --- a/src/tools/clippy/tests/ui/drop_bounds.rs +++ /dev/null @@ -1,8 +0,0 @@ -#![allow(unused)] -fn foo() {} -fn bar() -where - T: Drop, -{ -} -fn main() {} diff --git a/src/tools/clippy/tests/ui/drop_bounds.stderr b/src/tools/clippy/tests/ui/drop_bounds.stderr deleted file mode 100644 index 8208c0ed7e3..00000000000 --- a/src/tools/clippy/tests/ui/drop_bounds.stderr +++ /dev/null @@ -1,16 +0,0 @@ -error: bounds of the form `T: Drop` are useless, use `std::mem::needs_drop` to detect if a type has drop glue - --> $DIR/drop_bounds.rs:2:11 - | -LL | fn foo() {} - | ^^^^ - | - = note: `#[deny(clippy::drop_bounds)]` on by default - -error: bounds of the form `T: Drop` are useless, use `std::mem::needs_drop` to detect if a type has drop glue - --> $DIR/drop_bounds.rs:5:8 - | -LL | T: Drop, - | ^^^^ - -error: aborting due to 2 previous errors -