-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Suggestion for new lint: Cognitive Complexity #3793
Comments
Our cyclomatic complexity lint already adresses a few of the issues mentioned in the cognitive complexity paper. E.g. a I wonder if it would make sense to simply rename our cyclomatic complexity lint to cognitive complexity and start integrating a few more of the suggestions from the linked paper |
Yeah I think that would be better. |
Aside: let's call Cyclomatic Complexity as "CyC" and Cognitive Complexity "CoC"? Since both of them can be called CC, but are not the same thing x3
Here I assume you meant CyC. If so... does the CyC lint in Clippy give all
(Quote from Wikipedia's first paragraph on CyC.)
In general, for introduction purposes, I think it makes sense! You'd have the same lints as always, but the current CyC lint becomes more powerful. Users just get an upgrade of their already great tool. But in the long term I wonder if it might be better to keep them separate. CyC is better as a metric of how hard a piece of code is to test, while CoC is better as a metric of how hard a piece of code is to understand. (Code Climate's article comparing both is very good to illustrate this point) Also I think we'd prefer (please correct me if I'm wrong!) to keep the lints smaller, because it would allow a user to use Clippy to tackle smaller, more approachable problems. |
Shouldn't it depend on the number of branches of the match,
Right, @oli-obk's point is that our CyC lint doesn't actually measure CyC,
it measures something in between CyC and CoC, which is what folks found to
be more useful. Moving it all the way towards CoC is fine by me. As a
linter engine "hard to test" is far less useful and actionable advice to
give.
…On Thu, Feb 21, 2019, 11:21 PM Félix Fischer ***@***.***> wrote:
*Aside:* let's call Cyclomatic Complexity as "CyC" and Cognitive
Complexity "CoC"? Since both of them can be called CC, but are not the same
thing x3
E.g. a match has a CC of 2 regardless of the number of arms.
Here I assume you meant CyC. If so... does the CyC lint in Clippy give all
match's a score of 2? Shouldn't it depend on the number of branches of
the match, since CyC measures...
"(...) the number of linearly independent paths through a program's source
code."
(Quote from Wikipedia
<https://en.wikipedia.org/wiki/Cyclomatic_complexity>'s first paragraph
on CyC.)
I wonder if it would make sense to simply rename our cyclomatic complexity
lint to cognitive complexity and start integrating a few more of the
suggestions from the linked paper.
In general, for introduction purposes, I think it makes sense! You'd have
the same lints as always, but the current CyC lint becomes more powerful.
Users just get an upgrade of their already great tool.
But in the long term I wonder if it might be better to keep them separate.
CyC is better as a metric of how hard a piece of code is to test, while CoC
is better as a metric of how hard a piece of code is to understand. (Code
Climate's article <https://docs.codeclimate.com/docs/cognitive-complexity>
comparing both is very good to illustrate this point)
Also I think we'd prefer (please correct me if I'm wrong!) to keep the
lints smaller, because it would allow a user to use Clippy to tackle
smaller, more approachable problems.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#3793 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABivSIv5yFX2t1wDbBeZh-n0tFCFsOasks5vPtypgaJpZM4bGkvY>
.
|
Ohhhhhhhhhhhhh, this makes much more sense to me now.
If there's consensus on this, I'm willing to help :) |
Yea, pure CyC has been found to be pretty useless in Rust as you often use matches not to run different code but more as a lookup table for values. Yea, I think we can just go ahead with this. As a first step I would nuke the cfg function call and the math resulting from it and just keep the numbers from the visitor. Then, change the visitor to match the CoC rules. If I understood the paper correctly, it's all syntactical, so we should probably move to a pre-expansion-pass. IMO macro calls reduce complexity, even if macro definitions increase it |
Oh, btw, feel free to bug me about this here, on discord or on irc. I'm very interested in any musings you have on this. |
With Oliver (@oli-obk) we're now working on this. If anyone wants to chip in, reach us ^^ we plan on making an MVP first and leave the space for later expansions on more fine-grained or complex scenarios, as the use-cases start to appear. |
Draft of CoC implementationInputA Rust function. OutputA Cognitive Complexity score that loosely fits the Specification laid out in the paper. How it worksFrom now on, (I can change it later if people prefer I copy some of the paper here!) this text assumes the reader is familiar with the paper. Nesting StructuresThe following structures increment the nesting level:
(Nesting-Dependent) IncrementsFor each of the following constructs, there is an increment in the CoC score of (1 +
(Nesting-Independent) IncrementsFor each of the following constructs, there is a constant increment of 1 in the CoC score.
Exempt of IncrementThe following constructs are explicitly exempt of increment, since they replace and simplify other constructs.
Pending PointsHere are things that we're explicitly postponing until we have the tools or the know-how necessary to implement them.
I need help with Rust specific features I might be missing. I'm still relatively new to the language, so any help is appreciated! 🙏 Changelog
|
doesn't exist in Rust ;)
Are these a problem? How is it less complex to have the function outside?
we have
both short circuiting and non-short-circuiting are relevant here imo Also, if I read the paper correctly, only changes of the binary operator in a sequence cause an increment. So
Since everything is an expression, I think having any nesting expression inside e.g. a binary operation (
|
Haha 😆 my bad, I'm definitely still learning.
In the paper, nested functions are considered part of the body of the function itself. They add no increment, but they are like a new if-block in that they add nesting. I think that's because they are being treated as closures, capturing the surrounding context. Nested functions in Rust can't do that
Oki doke!
Same.
Almost.
Good point! We do have to consider this from the perspective of expressions. I think your examples are right. Those expressions introduce abstraction, and that should be accounted for :)
That's a fair point. Hmm. Since there can be many |
Cognitive Complexity (step 1 out of 3+): name changes Following up on #3793 **Overall checklist:** 1. **Name changes** 2. MVP of functionality 3. Tests After this PR, we will start working on the implementation itself.
I think As for nested functions, I think they add to rightward drift, and that is often a key factor in making things harder to understand. Rightward drift is certainly something I try to fight as part of language design. Another thing to consider is |
I'm implementing this atm. Do you think that the score of Nested Functions should be added to that of its mother function? Or should they only be scored separately? I find NFs very useful. However, scoring them completely apart from their mother function seems not quite a great solution, since the complexity of a mother and one of its nested functions is much greater than two comparable functions that are 100% separated. This is because having a mother and a NF means that their design is intertwined, and that you need to understand both in order to understand the whole (the are, at the end of the day, one function). At the same time, the amount of complexity in a function tends to be reduced in most cases after adding a NF, because there's code repetition that the NF is consolidating in one unit. Scoring the NF once (and giving it a starting nesting bonus to its score) should reflect that in most cases, unless the NF itself is very complex. Thoughts? (all) |
More fodder for thoughts. Anyone who's interested in this topic, please feel free to reply :)
|
I would treat NFs as if they secretly were closures in general. While breaking things into separate functions is great, NFs are not individually testable and so a free helper function can be preferable (depends... maybe the helper makes no sense as a separate logical unit...).
I think there's a multiplicative cost here. So instead of giving maybe |
The current draft implementation does exactly that ^^
Nice. I was thinking the same thing :D I'm glad I'm not the only one. But it makes a lot of sense: every expression in an |
This probably applies to type parameters as well but less so. Yet, I would try to avoid penalizing generic code over less generic code since that's often nice for code reuse. Maybe start adding increments at 3? I'd give lifetimes higher increments than type parameters. |
Seems fair :) |
It's not just statements inside an unsafe block that increase the difficulty in understanding the code, but also statements ouside an unsafe block. In fact an unsafe block can essentially require you to understand the entire module. |
I would like this lint to be highly configurable. Since there are much more things to consider ( For nested functions, for example, you should be able to write:
and then the lint will increase the cognitive complexity only for nested functions with a nesting level greater or equal 2. I don't really know how to implement this, with our current configuration system. We probably need an own configuration (sub-)parser for the cognitive complexity lint config. After a configuration like this is possible, we can set the threshold of every configuration pretty low and wait for complains to figure out sensible default values for every configuration. |
Hmm. That is true. How would you suggest scoring them? Just a high constant (e.g. 20) if there is at least one @flip1995 you raise a fair point. We were thinking about having an MVP running and seeing where we could go from there. Maybe a finer-grained configuration for CoC can be added then after we understand it better in the field :D |
I think for now you can just pessimize |
I'm working on making the scores more fine-grained. Lemme show you what I mean. The paper works on a number of syntactic language constructs. However, now that I've implemented all of them, I still have left around half of the
These are the answers that I've got:
I would leave these three outside of the scope of this work for now.
I would score the as complex items in similar ways as the ones that appear in the draft, remembering to classify them as "Nesting-Dependent" or "Nesting-Independent". The less complex ones, I'd give a much lower score (a fifth or so) than the baseline, since they increase the complexity by smaller steps. (Points 3. and 4. continue in next comment) |
I agree very much. I want to document all of this, even if it takes me 2+ weeks to do it after the implementation is done :) |
Hi all, sorry for the lack of activity. I have news :D I've finally started my internship! It's at a local foundation here in Chile, they do super cool things and the people are awesome <3 I really want to see this lint through, and for the moment my intent is to finish it. |
No problem, thanks so much for all your work so far! Congrats on your internship! |
First off, congrats on your internship! 🎉 If you want help with specific tasks of this lint, I'd be happy to help, if you "mentor" me. Just delegate the tasks to me that are still open for implementation and you can't find the time for. |
Hi you guys, here's an update. Thank you for not pinging me in the meantime ^^ I had a couple of really rough weeks irl at the beginning of May, and had to ask for a break at work to work things out. Now I'm doing better, have come back to work and I think I have a steady pace there. I wouldn't call the issue I had a done thing yet, but I think the prospects are good. If all goes well, I want to try coming back to this. I'll write below what's left before the next step, and @oli-obk will correct me if I forgot something :) And @flip1995: I'd be glad to 🌈 tell me if there's something you're interested in from the list :3 Remaining tasks
After this step (what's not in scope for now, but is considered the last step before publishing the MVP)
|
I have just come here just to tell how amazing this is. Please, finish the work on it, I can't wait any longer, it is itchy to try it asap. |
The complexity level in the clippy can be set to zero, so even a main with a hello world string printed introduces a warning. Can we check for the value of zero? Or does it make any sense or a use-case? |
Yeah that will be possible. You can configure the threshold as low or as high as you want. But a threshold of 0 would mean: "Every code is to complex, so please don't write any code", which wouldn't be that useful, would it? 😄 There's still a WIP PR (#3963) where you can contribute, if you want that feature rather sooner than later 😉
Since I wrote that, I'm busy with my master thesis and will be for a few months. So I don't have the time to contribute to this project either :/ |
Hi everyone! It's been a while. I've been researching this topic more and more on my free time. I also contacted an expert on static analysis to bug them about it, and they were very kind about it. Anyway, I digress. This research has led me to conclude that this lint, in two different ways, is a mistake. I want to write about it more comprehensively than this, but if you'll believe my words, I really think it is. First: what do I mean by two different ways? There are two paths that I would like to follow. The first one is retracting my own proposal. The second one is blocking this entire lint family until we get much better analysis tools and understanding of code analysis than what we have today in both Clippy and the static analysis community. Let's start with why I would like to retract this proposal. Second: why do I want to reject my own proposal? Let's take a look at the "paper". Here's what I found while digging into it:
All in all, it's pretty clear to me that the lint that I was planning to emulate is bullshit. But there's more to this story. Why would I want to not only reject my proposal, but also mark the idea of Cognitive Complexity linting a non-goal in itself? Third: why go beyond and reject the idea? You'll have to read more to get me here, I'm sorry. First go here. The first problem with any Cognitive Complexity lint is a lack of technology: in order to really assess how complex code will be to read, we would need to emulate how a person processes code, or hack around it at least. And how hard could that be? Well, as mentioned in the article above, the information of a program’s design is largely not present in its code. There are isomorphic programs that are completely different in cognitive complexity, just because of their context. What does Can we do that today with Clippy? No, we cannot. And we should not promise that, either. Clippy is awesome, probably the best static analyzer that I've seen. But there are limits to how good of an analysis Clippy itself can do. We should we humble and accept the reality of the technological hardness of Cognitive Complexity. I want to thank @Centril and @flip1995 for being so kind to me <3, As a last point? Maybe? I want to draw you to this paper. This is a real paper, and it's about static analysis and at the end, about a more real attempt at measuring cognitive complexity. I would like to understand it more, because it seems to me that the shortcomings in Clippy could be reduced with those kinds of techniques. |
Personally, I did find some real rationale behind this paper draft. I know it hasn't been published and does not refer to any research, but I agree with most of the points there. Anyway, there's been the cyclomatic complexity for a long time, and this draft paper referred to it as well. And this cyclomatic complexity does not imply readability and maintanability, so can't be used as "cyclomatic complexity" any way. So this draft paper and the "cognitive complexity" principle is still something we have now, there isn't anything better now, and I doubt will ever be, if someone from some university, or a scientist(?) decides to really work hard on this. Given all of that said, by you and me, I still think we should let the lint live as it is. Once anything better occurs - we could implement it as well, or when the cognitive complexity becomes oficially recognized, we could change this lint or add new one like with Also, nothing prevents us from discussing here all the ways to improve it even so. So that we would implement something even better, than was suggested by the paper draft. |
I believe there will be. Give humanity 20 years and we will have automatic, human-like reasoning about code. There is much stronger tech than what Clippy has in the research circles already. Things can and I think, will, go beyond the current state of affairs.
Yeah. Cyclomatic Complexity is a really bad metric. It shouldn't be used for linting, except for setting up a test suite. It really has no meaning with regards to the understandability of code.
I think you're right, but I don't think any of what's implemented here or proposed in the Draft comes close to improving it. We aren't really measuring code complexity at all. If we were to enhance it and be honest to ourselves about it, we really need to start using something better than a structural pass over the code.
That is awesome. If you do find that your project is better off from this lint, great. But even still, I think we should remain humble. This lint's name doesn't say what it is. It can't measure Cognitive Complexity. We should at least change it to "Structural Complexity" or something similar. That would help a lot, in my opinion. That and/or changing it to Pedantic. |
I agree with all you've said, and
Also sounds reasonable. I can't think of any other name though. Also, I thought, this lint has already been marked as "pedantic". |
Thank you for listening @vityafx 😊 Also: I checked it just now on the lint list, and CoC is listed under "Complexity" (as opposed to "Pedantic"): https://rust-lang.github.io/rust-clippy/master/#cognitive_complexity |
Maybe we should create a working group for defining the cognitive complexity ourselves? Why not? Should we be scientists for this? We are experienced people, and we have all the cards to play this game. |
I mean you can. Why not, right? I will not accompany you though. Because:
I don't think we have all the cards. We need a better, much better understanding of the human mind in order to do this. If you wanna dive into it, by all means do! But I have retired myself from this endeavor at the moment. |
Hi all, I'd like to propose (and implement, if someone can mentor me a bit!) a lint for Cognitive Complexity. It's a way to measure the cognitive load that a particular function puts on the reader.
I think that having an upper bound on the allowed cognitive complexity of functions would help ensure that complex functions are split into simpler, more maintainable ones. It would also help to complement the role that the Cyclomatic Complexity lint currently has.
The text was updated successfully, but these errors were encountered: