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

Add excessive_nesting lint #10672

Merged
merged 10 commits into from
Jun 9, 2023
Merged

Conversation

Centri3
Copy link
Member

@Centri3 Centri3 commented Apr 20, 2023

changelog: new lint [excessive_nesting]

@rustbot
Copy link
Collaborator

rustbot commented Apr 20, 2023

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

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 Apr 20, 2023
@Centri3
Copy link
Member Author

Centri3 commented Apr 20, 2023

ok, I see an issue with this. local bindings seem to be checked as well even if they're a block, which makes sense. this means, even if the actual code never reaches above column 100, it'll still be triggered if the length of the entire block is above 100. Not ideal... I'll check back on here later with hopefully the knowledge needed to eliminate that.

@Centri3 Centri3 changed the title WIP: excessive_width and excessive_indentation lints WIP: ~~excessive_width~~ and excessive_indentation lints Apr 20, 2023
@Centri3 Centri3 changed the title WIP: ~~excessive_width~~ and excessive_indentation lints WIP: ~~excessive_width~~ and excessive_indentation lints Apr 20, 2023
@Centri3 Centri3 changed the title WIP: ~~excessive_width~~ and excessive_indentation lints WIP: ~~excessive_width~~ and excessive_indentation lints Apr 20, 2023
@Centri3 Centri3 changed the title WIP: ~~excessive_width~~ and excessive_indentation lints WIP: excessive_width(?) and excessive_indentation lints Apr 20, 2023
@Alexendoo
Copy link
Member

Although it's unstable rustfmt does have an option to error on lines that go above the max_width - error_on_line_overflow. We want to avoid doing things that are/could be covered by rustfmt

For indentation that does seem like something more in our area, at least I wouldn't expect for rustfmt to add an option for that. Does that sound reasonable @flip1995?

@Centri3
Copy link
Member Author

Centri3 commented Apr 20, 2023

Although it's unstable rustfmt does have an option to error on lines that go above the max_width - error_on_line_overflow. We want to avoid doing things that are/could be covered by rustfmt

Yeah, I remembered that option and so excessive_width seems unnecessary. I'll remove it in a later commit

@flip1995
Copy link
Member

For general feedback, please see Zulip. Also please discuss the IF we should add this lint there.

Implementing a lint on number of whitespaces is not a good idea. Maybe some people use 2 whitespace indentation vs 4. Then this lint would behave differently. But if you try to determine indentation by analyzing statements, see Zulip and #3793 (comment)

FYI @felix91gr: This again is an attempt at doing something in the direction of cognitive/cyclomatic complexity. I like to page you in on those discussions, if you're still around and interested (either here, on Zulip or not at all) 😄

@Centri3
Copy link
Member Author

Centri3 commented Apr 21, 2023

Ok, I think this is ready! @Alexendoo, feel free to review this whenever you can and once consensus on whether this should be added has been reached 🙂 the current implementation is a bit verbose but I'm not sure if it's possible to get it better.

I'll also write tests and the description later, once I know whether this should be added.

@Centri3 Centri3 changed the title WIP: excessive_width(?) and excessive_indentation lints Add excessive_nesting lint Apr 21, 2023
@bors
Copy link
Contributor

bors commented Apr 23, 2023

☔ The latest upstream changes (presumably #10578) made this pull request unmergeable. Please resolve the merge conflicts.

@Centri3 Centri3 force-pushed the excessive-width-lints branch 2 times, most recently from 9d45979 to c584823 Compare April 25, 2023 10:18
Copy link
Member

@Alexendoo Alexendoo left a comment

Choose a reason for hiding this comment

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

👋

clippy_lints/src/excessive_nesting.rs Outdated Show resolved Hide resolved
clippy_lints/src/excessive_nesting.rs Outdated Show resolved Hide resolved
clippy_lints/src/excessive_nesting.rs Outdated Show resolved Hide resolved
clippy_lints/src/excessive_nesting.rs Outdated Show resolved Hide resolved
@Centri3
Copy link
Member Author

Centri3 commented Apr 25, 2023

I'll apply these changes tomorrow 🙂 thanks! Glad to see this could've been done better

clippy_lints/src/excessive_nesting.rs Outdated Show resolved Hide resolved
clippy_lints/src/excessive_nesting.rs Outdated Show resolved Hide resolved
clippy_lints/src/excessive_nesting.rs Show resolved Hide resolved
clippy_lints/src/excessive_nesting.rs Outdated Show resolved Hide resolved
@Centri3 Centri3 force-pushed the excessive-width-lints branch from a633034 to 7fb987b Compare June 7, 2023 23:26
@Centri3 Centri3 force-pushed the excessive-width-lints branch from 7fb987b to 5da3455 Compare June 7, 2023 23:34
@Alexendoo
Copy link
Member

Thanks! @bors r+

@bors
Copy link
Contributor

bors commented Jun 9, 2023

📌 Commit 6afb355 has been approved by Alexendoo

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Jun 9, 2023

⌛ Testing commit 6afb355 with merge 476efe9...

@bors
Copy link
Contributor

bors commented Jun 9, 2023

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: Alexendoo
Pushing 476efe9 to master...

@bors bors merged commit 476efe9 into rust-lang:master Jun 9, 2023
@Centri3 Centri3 deleted the excessive-width-lints branch June 9, 2023 21:07
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.

5 participants