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

Add a lint to warn on `T: Drop` bounds #3776

Merged
merged 4 commits into from Feb 19, 2019

Conversation

Projects
None yet
6 participants
@notriddle
Copy link
Contributor

notriddle commented Feb 17, 2019

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.

Known problems: None.

Example:

fn foo<T: Drop>() {}

Fixes #3773

@badboy

badboy approved these changes Feb 17, 2019

Show resolved Hide resolved clippy_lints/src/drop_bounds.rs Outdated
@oli-obk

This comment has been minimized.

Copy link
Collaborator

oli-obk commented Feb 17, 2019

Idk how hard it is to create a structured suggestion for this lint, but I'd be delighted if you tried to create one. Maybe modify a copy of the AST in place and pretty print that?

@notriddle

This comment has been minimized.

Copy link
Contributor Author

notriddle commented Feb 17, 2019

I don't think suggestions would help very much. While "just remove the bound" would "fix" the code in the sense that it would stop the lint from firing and it wouldn't break anything that worked, the proper way to fix it is to switch to std::mem::needs_drop instead, and I am not going to try to generate that kind of massive refactoring.

@notriddle

This comment has been minimized.

Copy link
Contributor Author

notriddle commented Feb 17, 2019

I also just realized that I hadn't actually tested this with where clauses. I should probably fix that...

@notriddle notriddle force-pushed the notriddle:drop-bounds branch from 94fb1bb to 1d5710d Feb 17, 2019

notriddle added some commits Feb 17, 2019

Add a lint to warn on `T: Drop` bounds
**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.

**Known problems:** None.

**Example:**
```rust
fn foo<T: Drop>() {}
```

@notriddle notriddle force-pushed the notriddle:drop-bounds branch from 1d5710d to cb1c0b6 Feb 18, 2019

@oli-obk

This comment has been minimized.

Copy link
Collaborator

oli-obk commented Feb 18, 2019

r=me with docs nit and travis passing

@notriddle

This comment has been minimized.

Copy link
Contributor Author

notriddle commented Feb 19, 2019

Okay, @oli-obk. Fixed the formatting that Travis complained about and added the second paragraph to why-it's-bad.

@oli-obk

This comment has been minimized.

Copy link
Collaborator

oli-obk commented Feb 19, 2019

@bors r+

superb, thanks!

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Feb 19, 2019

📌 Commit bc4c3b6 has been approved by oli-obk

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Feb 19, 2019

⌛️ Testing commit bc4c3b6 with merge 075c212...

bors added a commit that referenced this pull request Feb 19, 2019

Auto merge of #3776 - notriddle:drop-bounds, r=oli-obk
Add a lint to warn on `T: Drop` bounds

**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.

**Known problems:** None.

**Example:**
```rust
fn foo<T: Drop>() {}
```

Fixes #3773
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Feb 19, 2019

☀️ Test successful - checks-travis, status-appveyor
Approved by: oli-obk
Pushing 075c212 to master...

@bors bors merged commit bc4c3b6 into rust-lang:master Feb 19, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
homu Test successful
Details

@notriddle notriddle deleted the notriddle:drop-bounds branch Feb 19, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.