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

Replace #[allow] with #[expect] in Clippy #8797

Merged
merged 1 commit into from
May 9, 2022

Conversation

xFrednet
Copy link
Member

@xFrednet xFrednet commented May 7, 2022

Hey @rust-lang/clippy, @Alexendoo, @dswij, I'm currently working on the expect attribute as defined in Rust RFC 2383. With that, an #[allow] attribute can be replaced with a #[expect] attribute that suppresses the lint, but also emits a warning, if the lint isn't emitted in the expected scope.

With this PR I would like to test the attribute on a project scale and Clippy obviously came to mind. This PR replaces (almost) all #[allow] attributes in clippy_utils and clippy_lints with the #[expect] attribute. I was also able to remove some allows since, the related FPs have been fixed 🎉.

My question is now, are there any concerns regarding this? It's still okay to add normal #[allow] attributes, I see the need to nit-pick about that in new PRs, unless it's actually a FP. Also, I would not recommend using #[expect] in tests, as changes to a lint could the trigger the expect attribute in other files.

Additionally, I've noticed that Clippy has a bunch of #[allow(clippy::too_many_lines)] attributes. Should we maybe allow the lint all together or increase the threshold setting? To me, it seems like we mostly just ignore it in our code. 😅 🙃


changelog: none

r? @flip1995 (I've requested you for now, since you're also helping with reviewing the expect implementation. You are welcome to delegate this PR, even if it should be a simple review 🙃 )

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label May 7, 2022
@xFrednet
Copy link
Member Author

xFrednet commented May 7, 2022

Also, just to make sure

@bors try

bors added a commit that referenced this pull request May 7, 2022
Replace `#[allow]` with `#[expect]` in Clippy

Hey `@rust-lang/clippy,` `@Alexendoo,` `@dswij,` I'm currently working on the expect attribute as defined in [Rust RFC 2383](https://rust-lang.github.io/rfcs/2383-lint-reasons.html). With that, an `#[allow]` attribute can be replaced with a `#[expect]` attribute that suppresses the lint, but also emits a warning, if the lint isn't emitted in the expected scope.

With this PR I would like to test the attribute on a project scale and Clippy obviously came to mind. This PR replaces (almost) all `#[allow]` attributes in `clippy_utils` and `clippy_lints` with the `#[expect]` attribute. I was also able to remove some allows since, the related FPs have been fixed 🎉.

My question is now, are there any concerns regarding this? It's still okay to add normal `#[allow]` attributes, I see the need to nit-pick about that in new PRs, unless it's actually a FP. Also, I would not recommend using `#[expect]` in tests, as changes to a lint could the trigger the expect attribute in other files.

Additionally, I've noticed that Clippy has a bunch of `#[allow(clippy::too_many_lines)]` attributes. Should we maybe allow the lint all together or increase the threshold setting? To me, it seems like we mostly just ignore it in our code. 😅 🙃

---

changelog: none

r? `@flip1995` (I've requested you for now, since you're also helping with reviewing the expect implementation. You are welcome to delegate this PR, even if it should be a simple review 🙃 )
@bors
Copy link
Contributor

bors commented May 7, 2022

⌛ Trying commit 66b5c71 with merge 88af5f2...

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.

I think this is a good move. This will also give us tests for FPs and we won't miss fixes to them anymore as in the write.rs file.

Let's wait if other people have concerns. But I don't really see what would speak against this.

@flip1995
Copy link
Member

flip1995 commented May 7, 2022

Regarding the too_many_lines lint: I would keep it enabled by default with the same threshold. I think most places we allow this lint are in functions that just contain one huge match statement.

If this is added in a new PR, we might want to see what we can do to simplify/split up the function.

@bors
Copy link
Contributor

bors commented May 7, 2022

☀️ Try build successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Build commit: 88af5f2 (88af5f29dbc892f9fe0637ff7ea3a1c74bef5f70)

@xFrednet xFrednet added S-needs-discussion Status: Needs further discussion before merging or work can be started and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels May 7, 2022
@xFrednet
Copy link
Member Author

xFrednet commented May 7, 2022

I forgot to add the expect changes in clippy_utils, now they should be added 🙃

@Manishearth
Copy link
Member

I think we should do this 👍

@Jarcho
Copy link
Contributor

Jarcho commented May 7, 2022

Unrelated to the PR itself, but relevant for clippy. What happens to is_lint_allowed when using #[expect(...)]

@xFrednet
Copy link
Member Author

xFrednet commented May 8, 2022

Interesting question, the expect implementation requires the lint to be emitted like it would be for a warning. Therefore, is_lint_allowed will return false. This behavior is also documented in the new lint level documentation 🙃

@xFrednet
Copy link
Member Author

xFrednet commented May 9, 2022

It looks there are no concerns so far. @flip1995, I've pushed an update with the expects for clippy_utils since your review. Would you mind reviewing it and r+ if everything looks good? 🙃

@flip1995
Copy link
Member

flip1995 commented May 9, 2022

@bors r+

Thanks!

@bors
Copy link
Contributor

bors commented May 9, 2022

📌 Commit 03960eb has been approved by flip1995

@bors
Copy link
Contributor

bors commented May 9, 2022

⌛ Testing commit 03960eb with merge aa03344...

@bors
Copy link
Contributor

bors commented May 9, 2022

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: flip1995
Pushing aa03344 to master...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-needs-discussion Status: Needs further discussion before merging or work can be started
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants