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

Uplift drop-bounds lint from clippy #75699

Merged
merged 2 commits into from
Oct 4, 2020
Merged

Conversation

notriddle
Copy link
Contributor

Bounds on T: Drop do nothing, so they should warn.

@rust-highfive
Copy link
Collaborator

r? @petrochenkov

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 19, 2020
@jonas-schievink jonas-schievink added the T-lang Relevant to the language team, which will review and decide on the PR/issue. label Aug 19, 2020
src/librustc_lint/traits.rs Outdated Show resolved Hide resolved
library/core/src/ops/drop.rs Outdated Show resolved Hide resolved
src/test/ui/drop-bounds/drop-bounds.rs Outdated Show resolved Hide resolved
src/librustc_lint/traits.rs Outdated Show resolved Hide resolved
@petrochenkov
Copy link
Contributor

This looks reasonable to me.
Could you open an MCP for this? (Example: rust-lang/compiler-team#346)

@petrochenkov petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 19, 2020
@notriddle
Copy link
Contributor Author

rust-lang/compiler-team#347 Is the MCP

src/librustc_lint/traits.rs Outdated Show resolved Hide resolved
@bors

This comment has been minimized.

@notriddle
Copy link
Contributor Author

@rustbot modify labels: +S-waiting-on-review and -S-waiting-on-author

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 11, 2020
@petrochenkov
Copy link
Contributor

petrochenkov commented Sep 11, 2020

r=me on the implementation after squashing commits.
Looks like this is still waiting on team though (rust-lang/compiler-team#347).

@petrochenkov petrochenkov added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-lang Relevant to the language team, which will review and decide on the PR/issue. labels Sep 11, 2020
@flip1995
Copy link
Member

flip1995 commented Sep 30, 2020

Should we directly deprecate the lint from Clippy in this PR or do it in Clippy after this PR is merged and sync it back to rustc?

I can guide through the process of deprecating Clippy lints.

@spastorino
Copy link
Member

This is no longer waiting on team, MCP has been accepted.

@spastorino spastorino added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels Sep 30, 2020
@bors
Copy link
Contributor

bors commented Oct 1, 2020

📌 Commit cd159fd has been approved by petrochenkov

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 1, 2020
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Oct 1, 2020
…chenkov

Uplift drop-bounds lint from clippy

Bounds on `T: Drop` do nothing, so they should warn.
@bors
Copy link
Contributor

bors commented Oct 2, 2020

⌛ Testing commit cd159fd with merge 866ac1183a3d7ac82f37bd74092d140977ac13ab...

@bors
Copy link
Contributor

bors commented Oct 2, 2020

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Oct 2, 2020
@petrochenkov
Copy link
Contributor

Some clippy tests need to be updated.

@petrochenkov petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 2, 2020
@flip1995
Copy link
Member

flip1995 commented Oct 2, 2020

Either we update the Clippy tests here, then sync these changes to Clippy, then deprecate the lint in Clippy, and then sync it back to rustc

or

We deprecate the lint here, don't have to go through the whole sync process twice and make Clippy test-pass at the same time.


Instructions how to deprecate a Clippy lint:

  1. Remove this file:
    use crate::utils::{match_def_path, paths, span_lint};
  2. Add this lint to this file
    macro_rules! declare_deprecated_lint {
  3. Remove the test file
  4. a. temporarily add src/tools/clippy/clippy_dev to workspace.members in the Cargo.toml
    b. cd into src/tools/clippy and run cargo dev update_lints
  5. Clean up Clippy until ./x.py check src/tools/clippy passes
  6. Add a deprecation test and update the deprecated.stderr reference file.

Or just apply this patch: https://gist.github.com/flip1995/bbde96b04ca768f4ed7713dd53f23641

Copy link
Member

@flip1995 flip1995 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK Clippy change 👍

@petrochenkov
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Oct 3, 2020

📌 Commit dceb81a has been approved by petrochenkov

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 3, 2020
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Oct 3, 2020
…chenkov

Uplift drop-bounds lint from clippy

Bounds on `T: Drop` do nothing, so they should warn.
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 4, 2020
Rollup of 11 pull requests

Successful merges:

 - rust-lang#75143 (Use `tracing` spans to trace the entire MIR interp stack)
 - rust-lang#75699 (Uplift drop-bounds lint from clippy)
 - rust-lang#76768 (Test and reject out-of-bounds shuffle vectors)
 - rust-lang#77190 (updated p! macro to accept literals)
 - rust-lang#77388 (Add some regression tests)
 - rust-lang#77419 (Create E0777 error code for invalid argument in derive)
 - rust-lang#77447 (BTreeMap: document DrainFilterInner better)
 - rust-lang#77468 (Fix test name)
 - rust-lang#77469 (Improve rustdoc error for failed intra-doc link resolution)
 - rust-lang#77473 (Make --all-targets in x.py check opt-in)
 - rust-lang#77508 (Fix capitalization in blog post name)

Failed merges:

r? `@ghost`
@bors bors merged commit b654555 into rust-lang:master Oct 4, 2020
@rustbot rustbot added this to the 1.49.0 milestone Oct 4, 2020
@notriddle notriddle deleted the drop-bounds-lint branch October 4, 2020 06:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants