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

Document cargo-clippy feature #10229

Merged
merged 1 commit into from
Apr 3, 2023

Conversation

danielparks
Copy link
Contributor

@danielparks danielparks commented Jan 26, 2023

It is possible to use conditional compilation to prevent Clippy from evaluating certain code at all. Unfortunately, it was no longer documented anywhere. This adds a brief explanation of how to use the feature with conditional compilation, and mentions a few downsides.

Fixes #10220 — Ability to skip files or blocks entirely
changelog: none

@rustbot
Copy link
Collaborator

rustbot commented Jan 26, 2023

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @flip1995 (or someone else) soon.

Please see the contribution instructions for more information.

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Jan 26, 2023
@danielparks
Copy link
Contributor Author

I’m not entirely satisfied with this, but I wanted to get something out.

Note that the file seems to be wrapped at 120 characters instead of 80 like other book source files. I decided to match the file.

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 would argue that this isn't the right approach. If sections should be ignored by Clippy, you can just add #[allow(clippy::all)]. (You may have to add other lints or lint groups to that list to make it work. But for the average user clippy::all should be enough).

I think we only still support the cargo-clippy feature, because of backwards compat, which would also explain why it was removed from the documentation.

@danielparks
Copy link
Contributor Author

This is useful a few different situations that arise when working with generated code (particularly code generated by a tool you didn’t write):

  1. The code is slow to evaluate, or otherwise causes problems for Clippy. For example, in Ability to skip files or blocks entirely #10220 I generated a function that takes stable Clippy 20+ minutes to evaluate when all lints are allowed. Perhaps that will be fixed, but generated code in particular seems likely to trigger edge cases.
  2. You want to ensure that Clippy will never lint certain code, even if future groups beyond all, pedantic, etc. are added.
  3. You want to prevent Clippy from linting code that contains attributes with Clippy directives. (This seems unlikely to be an issue to me.)

Mostly I care about number 1, since that’s what I ran into.

Perhaps this is a rare enough need that it’s not worth documenting. It’s possible that #10220 is enough to help somebody who has a similar problem in the future — I spent quite some time searching for a way to do this before I filed that issue.

If you feel that this shouldn’t be documented, please go ahead and close. Otherwise, I’m happy to adjust the documentation, or move it somewhere else, or whatever you think is best.

@flip1995
Copy link
Member

flip1995 commented Feb 6, 2023

  1. 20+ minutes is definitely concerning. Is the code that causes this openly available. It might be worth doing a perf run with Clippy and try to find out what lint(s) cause this high runtime and maybe fix them. If there are only a few lints causing this, we can just add a check if the lint is allowed and skip the actual lint check for those expensive lints.
  2. That isn't a real concern. All warn-by-default lints will always be in the all group. It is really unlikely that a standalone allow-by-default group gets added. And even if that should happen, everyone first has to enable it for them to also having to disable it again.
  3. I don't really know what you mean by that. Why would you put Clippy attributes into code that you then exclude by a feature.

I think you convinced me that documenting this is worth it. However I think we shouldn't advertise it so prominently. I would put it at the end of the file you've put it in and add a note that this should only be needed in really special cases. You can add a note to a MD book with markdown quotes:

> Text of the note goes here

To properly address this, we should investigate what takes so long in Clippy and rather address the issue instead of the symptom.

@danielparks danielparks force-pushed the doc-feature-cargo-clippy branch from f8a74f1 to 33f3bc6 Compare February 11, 2023 05:34
@danielparks
Copy link
Contributor Author

danielparks commented Feb 11, 2023

Done. I shortened the copy a little more, but I thought it was probably best to keep the example. That said, I’m happy to change this however you like.


There’s more details about the underlying problem in #10220 (short version: I have a 48,000 line nested match expression), but I figured I wouldn’t bother pursuing it until the work on #10134 was complete, since I’m told the problem is in the same code.

It is possible to use conditional compilation to prevent Clippy from
evaluating certain code. This adds a brief explanation of how to use the
feature with conditional compilation, and mentions that generally it’s
preferable to use something like `#[allow(clippy::all)]`.

Fixes rust-lang#10220 — Ability to skip files or blocks entirely
@danielparks danielparks force-pushed the doc-feature-cargo-clippy branch from 33f3bc6 to 471de0c Compare February 11, 2023 15:52
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.

Thanks! I really like the wording now. This totally slipped through, I have way too many GH notifications...

@flip1995
Copy link
Member

flip1995 commented Apr 3, 2023

@bors r+

@bors
Copy link
Contributor

bors commented Apr 3, 2023

📌 Commit 471de0c has been approved by flip1995

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Apr 3, 2023

⌛ Testing commit 471de0c with merge e903af5...

@bors
Copy link
Contributor

bors commented Apr 3, 2023

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

@bors bors merged commit e903af5 into rust-lang:master Apr 3, 2023
@danielparks danielparks deleted the doc-feature-cargo-clippy branch April 3, 2023 17:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ability to skip files or blocks entirely
4 participants