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.

Sign up for GitHub

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

Jump to bottom

Lint enabling the whole restriction group #5750

Merged
merged 2 commits into from
Jun 30, 2020
Merged

Lint enabling the whole restriction group #5750

merged 2 commits into from
Jun 30, 2020

Conversation

ebroto
Copy link
Member

@ebroto ebroto commented Jun 26, 2020

I've added it to the correctness category, but I may be missing some valid use cases. In that case it could be changed to pedantic.

changelog: Add [blanket_clippy_restriction_lints] to check against enabling the whole restriction group.

@rust-highfive
Copy link

r? @flip1995

(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 Jun 26, 2020
flip1995
flip1995 approved these changes Jun 26, 2020
View reviewed changes
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.

cc @rust-lang/clippy should we warn on enabling the complete restriction group? I think this is a good idea, since restriction lints aren't supposed to be used without careful consideration.

clippy_lints/src/attrs.rs Outdated
/// #![deny(clippy::as_conversions)]
/// ```
pub BLANKET_CLIPPY_RESTRICTION_LINTS,
correctness,
Copy link
Member

Choose a reason for hiding this comment

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

correctness is the wrong group, since enabling those lints doesn't have any influence on code semantics and doesn't change behavior.

I think style is the best group here, since "idomatic Clippy style" is to not enable the complete restriction group.

Copy link

Choose a reason for hiding this comment

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

Correctness lints are for "code that is just outright wrong or very very useless" so this fits IMO.

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess there are different ways to understand this, probably all of them valid.

My take was that this is an incorrect usage of clippy, a footgun that can't be easily prevented due to restriction being a category like the others.

TBH I would be happy just with this being linted, as it's most of the time an error that is not hard to make (just requires not reading the README thoroughly).

Copy link
Member

@Manishearth Manishearth Jun 26, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I'm okay with correctness or style. No strong preference. This is an incorrect use of clippy, but erroring about it might be harsh.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I guess this comes down to everyone's interpretation of 'code that is outright wrong or very very useless' from the correctness lint group description in the README.

Going from that description, it would certainly fit correctness. Still, I would tend to agree with @flip1995 here. IMO correctness is meant to warn about actual bugs in the code and nothing further. I don't think an incorrect Clippy usage is a bug in that sense.

Copy link
Contributor

Choose a reason for hiding this comment

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

IMO correctness is meant to warn about actual bugs in the code and nothing further. I don't think an incorrect Clippy usage is a bug in that sense.

I agree with this

@flip1995 flip1995 added the S-needs-discussion Status: Needs further discussion before merging or work can be started label Jun 26, 2020
@Manishearth
Copy link
Member

I agree with adding this lint

@ebroto
Copy link
Member Author

ebroto commented Jun 27, 2020

I moved the lint to style as there seems to be a majority in favor of that. Thanks all!

@oli-obk
Copy link
Contributor

oli-obk commented Jun 28, 2020

@bors r=Manishearth,flip1995,phansch,oli-obk

@bors
Copy link
Contributor

bors commented Jun 28, 2020

📌 Commit 3b83040 has been approved by Manishearth,flip1995,phansch,oli-obk

@bors
Copy link
Contributor

bors commented Jun 28, 2020

⌛ Testing commit 3b83040 with merge 9c6060e...

bors added a commit that referenced this pull request Jun 28, 2020
@bors
Auto merge of #5750 - ebroto:blanket_clippy_restriction_lints, r=Mani…
9c6060e 
…shearth,flip1995,phansch,oli-obk

Lint enabling the whole restriction group

I've added it to the `correctness` category, but I may be missing some valid use cases. In that case it could be changed to `pedantic`.

changelog: Add [`blanket_clippy_restriction_lints`] to check against enabling the whole restriction group.
@bors
Copy link
Contributor

bors commented Jun 28, 2020

💔 Test failed - checks-action_test

@flip1995 flip1995 added S-waiting-on-bors Status: The marked PR was approved and is only waiting bors and removed S-needs-discussion Status: Needs further discussion before merging or work can be started S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Jun 28, 2020
@ebroto ebroto force-pushed the blanket_clippy_restriction_lints branch from 3b83040 to c5d8f53 Compare June 30, 2020 20:14
@flip1995
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Jun 30, 2020

📌 Commit c5d8f53 has been approved by flip1995

@bors
Copy link
Contributor

bors commented Jun 30, 2020

⌛ Testing commit c5d8f53 with merge 2aad06f...

bors added a commit that referenced this pull request Jun 30, 2020
@bors
Auto merge of #5750 - ebroto:blanket_clippy_restriction_lints, r=flip…
2aad06f 
…1995

Lint enabling the whole restriction group

I've added it to the `correctness` category, but I may be missing some valid use cases. In that case it could be changed to `pedantic`.

changelog: Add [`blanket_clippy_restriction_lints`] to check against enabling the whole restriction group.
@bors
Copy link
Contributor

bors commented Jun 30, 2020

💥 Test timed out

@flip1995
Copy link
Member

@bors r=Manishearth,flip1995,phansch,oli-obk

@bors
Copy link
Contributor

bors commented Jun 30, 2020

💡 This pull request was already approved, no need to approve it again.

  • This pull request previously failed. You should add more commits to fix the bug, or use retry to trigger a build again.

@bors
Copy link
Contributor

bors commented Jun 30, 2020

📌 Commit c5d8f53 has been approved by Manishearth,flip1995,phansch,oli-obk

@bors
Copy link
Contributor

bors commented Jun 30, 2020

⌛ Testing commit c5d8f53 with merge d05d6ab...

@bors
Copy link
Contributor

bors commented Jun 30, 2020

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: Manishearth,flip1995,phansch,oli-obk
Pushing d05d6ab to master...

@bors bors merged commit d05d6ab into rust-lang:master Jun 30, 2020
@ebroto ebroto deleted the blanket_clippy_restriction_lints branch June 30, 2020 22:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oli-obk oli-obk oli-obk left review comments

@Manishearth Manishearth Manishearth left review comments

@phansch phansch phansch left review comments

@flip1995 flip1995 flip1995 approved these changes

Assignees

@flip1995 flip1995

Labels
S-waiting-on-bors Status: The marked PR was approved and is only waiting bors
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@ebroto @rust-highfive @Manishearth @oli-obk @bors @flip1995 @phansch