Skip to content

Correct the Cognitive Complexity lint's documentation #14915

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+14 −7
Open

Correct the Cognitive Complexity lint's documentation #14915

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 14 additions & 7 deletions clippy_lints/src/cognitive_complexity.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,18 +14,25 @@ use rustc_span::def_id::LocalDefId;

declare_clippy_lint! {
/// ### What it does
/// Checks for methods with high cognitive complexity.
/// We used to think it measured how hard a method is to understand.
///
/// ### Why is this bad?
/// Methods of high cognitive complexity tend to be hard to
/// both read and maintain. Also LLVM will tend to optimize small methods better.
/// Ideally, we would like to be able to measure how hard a function is
/// to understand given its context (what we call its Cognitive Complexity).
/// But that's not what this lint does. See "Known problems"
///
/// ### Known problems
/// Sometimes it's hard to find a way to reduce the
/// complexity.
/// The true Cognitive Complexity of a method is not something we can
/// calculate using modern technology. This lint has been left in the
/// `nursery` so as to not mislead users into using this lint as a
/// measurement tool.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// measurement tool.
/// measurement tool.
/// In case you still want to use the lint, don't. The main use cases this lint still has when considering its flawed reasoning are to lint against heavy nesting or exceedingly long functions, both of which have dedicated lints.

Ignore if we deprecate this

Copy link
Member

Choose a reason for hiding this comment

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

To be clear, I mean adding this alongside the new section

///
/// ### Example
/// You'll see it when you get the warning.
/// For more detailed information, see [rust-clippy#3793](https://github.com/rust-lang/rust-clippy/issues/3793)
///
/// ### Lints to consider instead of this
///
/// * [`excessive_nesting`](https://rust-lang.github.io/rust-clippy/master/index.html#excessive_nesting)
/// * [`too_many_lines`](https://rust-lang.github.io/rust-clippy/master/index.html#too_many_lines)
#[clippy::version = "1.35.0"]
pub COGNITIVE_COMPLEXITY,
nursery,
Copy link
Member

@Centri3 Centri3 May 29, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I think if it's clear we'll never fix this, this should be moved to restriction or deprecated. I think it's clearly not within the scope of restriction but nursery implies we'll fix it (the former suggestion could likely be challenged by others).

I think this whole doc is written more like it would be for a deprecated lint (and written in a way I prefer, might I add), and there is already an internal lint that could probably get tripped up on this in the future. That's kinda my reasoning, though there are definitely still users of this lint (this wasn't always allow-by-default, most warn attributes are likely to be them enabling this intentionally.)

Suggesting the usage of excessive_nesting and too_many_lines is probably enough for them, as those are kinda the main uses of this anyways when the user correctly takes into account it's flawed.

Copy link
Member

Choose a reason for hiding this comment

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

Though, if anybody does ever work on it, in that coming decade, then this would be a bit of a problem, but maybe enough of a special case.

Copy link
Member

Choose a reason for hiding this comment

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

I don't see much maintenance burden here anyways.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think if it's clear we'll never fix this, this should be moved to restriction or deprecated. I think it's clearly not within the scope of restriction but nursery implies we'll fix it (the former suggestion could likely be challenged by others).

@Centri3 I agree. I would rather see it deprecated (among other things, because even the concept of the lint itself is flawed). If I understood @flip1995 correctly, they'd rather it be kept in nursery; I'm tagging them so that they can give their two cents :)

I think this whole doc is written more like it would be for a deprecated lint (and written in a way I prefer, might I add), and there is already an internal lint that could probably get tripped up on this in the future.

❤️

though there are definitely still users of this lint (this wasn't always allow-by-default, most warn attributes are likely to be them enabling this intentionally.)

Indeed. Phillip said so as well.

Suggesting the usage of excessive_nesting and too_many_lines is probably enough for them, as those are kinda the main uses of this anyways when the user correctly takes into account it's flawed.

That's a very good point, I hadn't thought of that and it makes a lot of sense!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've now added both suggestions. What's left is to hear Phillip's thoughts on deprecation :3

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see a problem with deprecating the lint:

  • It brings no value.
  • We have git, if someone wants to dig up the old code and resuscitate the lint later, they can always to it.
  • Release notes are the perfect place to explain why we have deprecated it, and what lints can be used instead.
  • It is one less lint to maintain.

Copy link
Member

Choose a reason for hiding this comment

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

+1 on deprecating

Expand Down