Skip to content

excessive_nesting: Move excessive_nesting from complexity to pedantic - #17509

Open
hlcfan wants to merge 3 commits into
rust-lang:masterfrom
hlcfan:pedantic-excessive-nesting
Open

excessive_nesting: Move excessive_nesting from complexity to pedantic#17509
hlcfan wants to merge 3 commits into
rust-lang:masterfrom
hlcfan:pedantic-excessive-nesting

Conversation

@hlcfan

@hlcfan hlcfan commented Aug 5, 2026 •
edited by rustbot
Loading

Copy link
Copy Markdown

Fix #14923

  • Move excessive_nesting to pedantic
  • Set default value 6 and update line doc comment
  • Add UI test
changelog: [`excessive_nesting`]: Move it from the `complexity` group to `pedantic`

Caution

Concerns (1 active)

Managed by @rustbot—see help for details.

@rustbot rustbot added the S-waiting-on-community-reviews Status: This is awaiting for positive reviews from the community before a maintainer is assigned. label Aug 5, 2026
@rustbot

rustbot commented Aug 5, 2026

Copy link
Copy Markdown
Collaborator

Thanks for the pull request, and welcome!

You should hear from one of our reviewers after this PR gets at least 2 reviews from the community.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

  • @rustbot author: the review is finished, PR author should check the comments and take action accordingly
  • @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Aug 5, 2026
@hlcfan
Move excessive_nesting from complexity to pedantic
fd844a6 
- Move excessive_nesting to pedantic
- Set default value 6 and update line doc comment
- Add UI test

```
changelog: [`excessive_nesting`]: Move it from the `complexity` group to `pedantic`
```
@hlcfan
hlcfan force-pushed the pedantic-excessive-nesting branch from f50ddf9 to fd844a6 Compare August 5, 2026 15:26
@github-actions

github-actions Bot commented Aug 5, 2026 •
edited
Loading

Copy link
Copy Markdown

Lintcheck changes for 164a994

Lint Added Removed Changed
clippy::excessive_nesting 323 0 0

This comment will be updated if you push new changes

CommanderStorm
CommanderStorm approved these changes Aug 5, 2026
View reviewed changes

@CommanderStorm CommanderStorm left a comment •
edited by rustbot
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

community review: makes sense

View changes since this review

@CommanderStorm

CommanderStorm commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

but CI is failing, so please fix this..

@rustbot

rustbot commented Aug 6, 2026

Copy link
Copy Markdown
Collaborator

Some changes occurred in clippy_lints/src/doc

cc @notriddle

CommanderStorm
CommanderStorm reviewed Aug 6, 2026
View reviewed changes
Comment thread clippy_dev/src/parse.rs Outdated
@@ -1,3 +1,4 @@
#![allow(clippy::excessive_nesting)]

@CommanderStorm CommanderStorm Aug 6, 2026 •
edited by rustbot
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

can you use expect for the tests? This way they will get cleaned up after refactors of the tests 😉

View changes since the review

@hlcfan hlcfan Aug 6, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Updated, thanks.

@hlcfan
hlcfan force-pushed the pedantic-excessive-nesting branch from 981ee0f to 164a994 Compare August 6, 2026 00:43
CommanderStorm
CommanderStorm reviewed Aug 6, 2026
View reviewed changes
Comment thread tests/ui/excessive_nesting.rs
#![warn(clippy::pedantic)]
#![allow(clippy::let_and_return)]

fn main() {

@CommanderStorm CommanderStorm Aug 6, 2026 •
edited by rustbot
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Sorry to string you along like this. only noticed this now:

Actually, lets add a testcase for level 6 as well, so that we know that we don't lint there.

View changes since the review

@samueltardieu

samueltardieu commented Aug 6, 2026

Copy link
Copy Markdown
Member

@rustbot concern Too many hits

It looks like the proposed value of 6 will make the lint trigger in too many cases (323 hits in lintcheck). I'm not sure we can agree on a reasonable default value.

@rustbot rustbot added the S-waiting-on-concerns Status: This PR/issue has concerns that need to be addressed before moving forward with it label Aug 6, 2026
@hlcfan
hlcfan force-pushed the pedantic-excessive-nesting branch from 164a994 to bf40f22 Compare August 6, 2026 15:28
@hlcfan

hlcfan commented Aug 6, 2026

Copy link
Copy Markdown
Author

Had the same concern when making the changes with default excessive-nesting-threshold=6. I ran lintcheck with different thresholds, these are the results:

Threshold Hitcount
6 73
7 5
8 1
9 0

Shall we go with 8? It sits at the inflection point. Since the threshold is configurable via clippy.toml, the repo that has false positives can adjust it.

@hlcfan
hlcfan force-pushed the pedantic-excessive-nesting branch from bf40f22 to 42554ab Compare August 6, 2026 16:00
@samueltardieu

samueltardieu commented Aug 6, 2026

Copy link
Copy Markdown
Member

I'd say that the mere fact that you're proposing several thresholds without a good explanation for what the value should be shows that there exists no correct default value.

Many people use pedantic and are satisfied with what the default values for the lint configuration are. Here I can envision some friction where people will have to create a clippy.toml file just to change the value, or disable the lint.

@hlcfan

hlcfan commented Aug 8, 2026

Copy link
Copy Markdown
Author

lintcheck's crate sets are not exhaustive, but it's the best proxy we have.

Agreed that it breaks the developer experience for pedantic users if we roll out with a threshold. But since we move it to pedantic group, expecting it to be stricter and beneficial, setting a threshold makes sense.

@samueltardieu

samueltardieu commented Aug 8, 2026

Copy link
Copy Markdown
Member

But since we move it to pedantic group, expecting it to be stricter and beneficial, setting a threshold makes sense.

This will need to be agreed on. At this stage, I'm not convinced that this category change is a good idea.

@rustbot label needs-fcp

@rustbot rustbot added the needs-fcp PRs that add, remove, or rename lints and need an FCP label Aug 8, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer
@CommanderStorm CommanderStorm CommanderStorm approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

needs-fcp PRs that add, remove, or rename lints and need an FCP S-waiting-on-community-reviews Status: This is awaiting for positive reviews from the community before a maintainer is assigned. S-waiting-on-concerns Status: This PR/issue has concerns that need to be addressed before moving forward with it S-waiting-on-review Status: Awaiting review from the assignee but also interested parties

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Move excessive_nesting to pedantic

4 participants

@hlcfan @rustbot @CommanderStorm @samueltardieu