Correct the Cognitive Complexity lint's documentation #14915
Conversation
rustbot
added
the
S-waiting-on-review
|
It has been observed in the wild (see below) that the current description (which I myself wrote many years ago) isn't descriptive enough for users who still want to activate this lint. This change addresses that. It should hopefully make future users aware of the deep limitations of this retired lint, as well as of the limitations of the concept behind it. Open issues motivating this change:
Closed issues motivating this change: |
|
r? @flip1995 |
|
With this PR, we might be able to finally close #3793 with peace of mind. |
| /// | ||
| /// ### Example | ||
| /// You'll see it when you get the warning. | ||
| /// - | ||
| #[clippy::version = "1.35.0"] | ||
| pub COGNITIVE_COMPLEXITY, | ||
| nursery, |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I don't see much maintenance burden here anyways.
There was a problem hiding this comment.
I think if it's clear we'll never fix this, this should be moved to
restrictionor deprecated. I think it's clearly not within the scope ofrestrictionbutnurseryimplies 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, mostwarnattributes are likely to be them enabling this intentionally.)
Indeed. Phillip said so as well.
Suggesting the usage of
excessive_nestingandtoo_many_linesis 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!
There was a problem hiding this comment.
I've now added both suggestions. What's left is to hear Phillip's thoughts on deprecation :3
There was a problem hiding this comment.
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.
| /// 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. |
There was a problem hiding this comment.
| /// 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
There was a problem hiding this comment.
To be clear, I mean adding this alongside the new section
changelog: corrected the Cognitive Complexity lint's documentation.