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

Erroneous drop_bounds lint #86653

Open
jonhoo opened this issue Jun 26, 2021 · 3 comments
Open

Erroneous drop_bounds lint #86653

jonhoo opened this issue Jun 26, 2021 · 3 comments
Labels
A-destructors Area: Destructors (`Drop`, …) A-lints Area: Lints (warnings about flaws in source code) such as unused_mut. C-bug Category: This is a bug. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@jonhoo
Copy link
Contributor

jonhoo commented Jun 26, 2021

I tried this code (playground):

trait Foo where Self: Sized + Drop + 'static {
    fn drop(me: *mut Self) {
        bar(me)
    }
}
pub fn bar(_: *mut dyn Drop) {}

I expected to see this happen: the code should compile without warning.

Instead, this happened: the compiler gave the following warning:

warning: bounds on `Self: Drop` are useless, consider instead using `std::mem::needs_drop` to detect if a type has a destructor
 --> src/lib.rs:1:31
  |
1 | trait Foo where Self: Sized + Drop + 'static {
  |                               ^^^^
  |
  = note: `#[warn(drop_bounds)]` on by default

But, the Drop bound is not useless here — removing it causes the code to no longer compile. It's not clear if removing the bound should make the code stop compiling, but that's a different matter (and perhaps a different bug?).

Meta

rustc --version --verbose:

rustc 1.53.0 (53cb7b09b 2021-06-17)
binary: rustc
commit-hash: 53cb7b09b00cbea8754ffb78e7e3cb521cb8af4b
commit-date: 2021-06-17
host: x86_64-unknown-linux-gnu
release: 1.53.0
LLVM version: 12.0.1

The warning also occurs on nightly.

@jonhoo jonhoo added the C-bug Category: This is a bug. label Jun 26, 2021
@jonas-schievink jonas-schievink added A-lints Area: Lints (warnings about flaws in source code) such as unused_mut. A-destructors Area: Destructors (`Drop`, …) labels Jun 26, 2021
@JohnTitor JohnTitor added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jun 27, 2021
@scottmcm
Copy link
Member

scottmcm commented Jul 3, 2021

Hmm, dyn Drop is pretty weird. Notably, drop_in_place doesn't have a bound on Drop.

I think a warning is still appropriate for this code, even if perhaps it's in a supoptimal place.

@jonhoo
Copy link
Contributor Author

jonhoo commented Jul 3, 2021

For context, this code is used in a library that does deferred memory reclamation, so I want to store a heterogeneous collection of elements "to be dropped". I don't know what other type to use in that context than dyn Drop — it seems like exactly the right thing to use. I could introduce an empty placeholder trait instead, but that only adds unnecessary complexity.

GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this issue Aug 22, 2021
…eGomez

Improve wording of the `drop_bounds` lint

This PR addresses rust-lang#86653. The issue is sort of a false positive of the `drop_bounds` lint, but I would argue that the best solution for rust-lang#86653 is simply a rewording of the warning message and lint description, because even if the lint is _technically_ wrong, it still forces the programmer to think about what they are doing, and they can always use `#[allow(drop_bounds)]` if they think that they really need the `Drop` bound.

There are two issues with the current warning message and lint description:
- First, it says that `Drop` bounds are "useless", which is technically incorrect because they actually do have the effect of allowing you e.g. to call methods that also have a `Drop` bound on their generic arguments for some reason. I have changed the wording to emphasize not that the bound is "useless", but that it is most likely not what was intended.
- Second, it claims that `std::mem::needs_drop` detects whether a type has a destructor. But I think this is also technically wrong: The `Drop` bound says whether the type has a destructor or not, whereas `std::mem::needs_drop` also takes nested types with destructors into account, even if the top-level type does not itself have one (although I'm not 100% sure about the exact terminology here, i.e. whether the "drop glue" of the top-level type counts as a destructor or not).

cc `@jonhoo,` does this solve the issue for you?

r? `@GuillaumeGomez`
@kpreid
Copy link
Contributor

kpreid commented Nov 13, 2021

I could introduce an empty placeholder trait instead, but that only adds unnecessary complexity.

Perhaps the standard library should provide an empty trait implemented by everything. (Pointee, if stabilized, will be implemented by everything, but its actual purpose might be distracting.) Or perhaps there should be a way to write a dyn which has a vtable for drop but no actual methods; the obvious syntax is dyn with no following traits (which is already parsed but rejected with error[E0224]: at least one trait is required for an object type).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-destructors Area: Destructors (`Drop`, …) A-lints Area: Lints (warnings about flaws in source code) such as unused_mut. C-bug Category: This is a bug. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

5 participants