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

Improve wording of the drop_bounds lint #86747

Merged
merged 2 commits into from
Aug 22, 2021
Merged
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
36 changes: 20 additions & 16 deletions compiler/rustc_lint/src/traits.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,23 +18,27 @@ declare_lint! {
///
/// ### 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.
/// A generic trait bound of the form `T: Drop` is most likely misleading
/// and not what the programmer intended (they probably should have used
/// `std::mem::needs_drop` instead).
///
/// 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.
/// `Drop` bounds do not actually indicate whether a type can be trivially
/// dropped or not, because a composite type containing `Drop` types does
/// not necessarily implement `Drop` itself. Naïvely, one might be tempted
/// to write an implementation that assumes that a type can be trivially
/// dropped while also supplying a specialization for `T: Drop` that
/// actually calls the destructor. However, this breaks down e.g. when `T`
/// is `String`, which does not implement `Drop` itself but contains a
/// `Vec`, which does implement `Drop`, so assuming `T` can be trivially
/// dropped would lead to a memory leak here.
///
/// Furthermore, the `Drop` trait only contains one method, `Drop::drop`,
/// which may not be called explicitly in user code (`E0040`), so there is
/// really no use case for using `Drop` in trait bounds, save perhaps for
/// some obscure corner cases, which can use `#[allow(drop_bounds)]`.
pub DROP_BOUNDS,
Warn,
"bounds of the form `T: Drop` are useless"
"bounds of the form `T: Drop` are most likely incorrect"
}

declare_lint_pass!(
Expand Down Expand Up @@ -65,8 +69,8 @@ impl<'tcx> LateLintPass<'tcx> for DropTraitConstraints {
None => return,
};
let msg = format!(
"bounds on `{}` are useless, consider instead \
using `{}` to detect if a type has a destructor",
"bounds on `{}` are most likely incorrect, consider instead \
using `{}` to detect whether a type can be trivially dropped",
predicate,
cx.tcx.def_path_str(needs_drop)
);
Expand Down
14 changes: 7 additions & 7 deletions src/test/ui/drop-bounds/drop-bounds.stderr
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
error: bounds on `T: Drop` are useless, consider instead using `std::mem::needs_drop` to detect if a type has a destructor
error: bounds on `T: Drop` are most likely incorrect, consider instead using `std::mem::needs_drop` to detect whether a type can be trivially dropped
--> $DIR/drop-bounds.rs:2:11
|
LL | fn foo<T: Drop>() {}
Expand All @@ -10,37 +10,37 @@ note: the lint level is defined here
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
error: bounds on `U: Drop` are most likely incorrect, consider instead using `std::mem::needs_drop` to detect whether a type can be trivially dropped
--> $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
error: bounds on `impl Drop: Drop` are most likely incorrect, consider instead using `std::mem::needs_drop` to detect whether a type can be trivially dropped
--> $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
error: bounds on `T: Drop` are most likely incorrect, consider instead using `std::mem::needs_drop` to detect whether a type can be trivially dropped
--> $DIR/drop-bounds.rs:9:15
|
LL | struct Foo<T: Drop> {
| ^^^^

error: bounds on `U: Drop` are useless, consider instead using `std::mem::needs_drop` to detect if a type has a destructor
error: bounds on `U: Drop` are most likely incorrect, consider instead using `std::mem::needs_drop` to detect whether a type can be trivially dropped
--> $DIR/drop-bounds.rs:12:24
|
LL | struct Bar<U> 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
error: bounds on `Self: Drop` are most likely incorrect, consider instead using `std::mem::needs_drop` to detect whether a type can be trivially dropped
--> $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
error: bounds on `T: Drop` are most likely incorrect, consider instead using `std::mem::needs_drop` to detect whether a type can be trivially dropped
--> $DIR/drop-bounds.rs:17:9
|
LL | impl<T: Drop> Baz for T {
Expand Down