Cognitive Complexity (step 2 (and maybe 3) out of 3+): implementation! #3963
Conversation
felix91gr
force-pushed
the
cognitive_complexity_redo
branch
from
d0d5a03
to
fe75af7
Compare
|
Tagging @oli-obk and everyone who showed interest in this: @Manishearth, @Centril, @flip1995 ^^ |
tests/ui/cognitive_complexity.stderr
Outdated
| --> $DIR/cognitive_complexity.rs:376:1 | ||
| | | ||
| LL | / fn osscilating_logical_chain_3() -> bool { | ||
| LL | | (true && false) || (true && false) |
There was a problem hiding this comment.
Not something blocking or anything, but this stderr file is getting quite long and it would be nice to split it into multiple files by some criteria. cc #2038
There was a problem hiding this comment.
Good idea. Will do that, possibly tomorrow ^^
There are many different topics by which you can guide the testing of this lint, so a split actually fits quite well.
There was a problem hiding this comment.
Maybe also create a subdir for testing this lint. I can imagine, that we will need many tests for this.
There was a problem hiding this comment.
I've been doing this for a while now. It's not done yet, but it certainly made things much cleaner! :)
I've been splitting them up by type:
- Nesting-Dependent Structures
- Nesting-Independent Structures
- No-Ops (non-scoring cases)
- Too Complex By Default (functions that trigger the default threshold)
- Other Cases (miscellaneous patterns)
There was a problem hiding this comment.
That said, the tests for the Binary Logical Operators Chain are just a ton 😅 . I might end up splitting the NI-Structures test and make one test just for testing that part of the lint.
flip1995
added
the
S-waiting-on-review
I looked at those functions and didn't want to read them at all, because they look too complex. I think that is a good indication that these should be refactored. 😄 |
felix91gr
force-pushed
the
cognitive_complexity_redo
branch
2 times, most recently
from
8a432b9
to
43ba821
Compare
|
Ohh, it seems the current CI errors are caused by changes in upstream that clash with my changes. I will upload a patch for this, hopefully today. |
felix91gr
force-pushed
the
cognitive_complexity_redo
branch
2 times, most recently
from
ccad40d
to
84a4778
Compare
It's also a good indication that the PR is working well :D |
| /// **Example:** No. You'll see it when you get the warning. | ||
| /// **Example:** Sorry. Examples are too big and varied to put in here. For a | ||
| /// complete explanation of the analysis being made though, you can read this paper: | ||
| /// https://www.sonarsource.com/docs/CognitiveComplexity.pdf |
There was a problem hiding this comment.
You may want to link to a README somewhere that has a fuller list of examples; the .pdf will not be so Rust specific and it seems there will be plenty of Rust specific aspects to this lint.
There was a problem hiding this comment.
Ohh, that's true! I guess a README is in order :)
|
This morning I annotated the failing tests so that Clippy allows them to have any score of CoC. However, now I've changed my mind: I think I should be reviewing them and contacting the code owners so that we can assess a possible splitting up of the flagged functions. I will add that to the to-do list. PS: now the PR has a to-do list of remaining tasks. |
| self.match_arms += arms_n - 2; | ||
| } | ||
| impl CoCHelper { | ||
| fn push_nesting(&mut self) { |
There was a problem hiding this comment.
Fold these two methods into one method that takes a closure and does the increment/decrement logic around calling that closure.
There was a problem hiding this comment.
I'm sort of doing that in the ExprKind::Block branch. I like it because it doesn't need closures to work and is pretty simple. I fear that taking a closure as parameter would make this code much harder to "get" for a reader in the future who wishes to modify it. What do you think?
There was a problem hiding this comment.
Maybe I've just seen too many such impls in rustc. I was imagining something like
ExprKind::Block(..) => self.nest(|| {
walk_expr(self, ex);
}),And writing that down has shown me that it's not actionable :D
Are all nestings going to be of this kind? How do you feel about a walk_nested method like
fn walk_nested(&mut self, ex: &Expr) {
self.current_nesting += 1;
walk_expr(self, ex);
self.current_nesting -= 1;
}so all nesting sites become
ExprKind::Block(..) => self.walk_nested(ex),There was a problem hiding this comment.
(However, I might be underestimating how clear that code can get. I'll have a look at it nevertheless)
There was a problem hiding this comment.
Are all nestings going to be of this kind?
I think they are. I'll think of it again, but atm of writing the implementation and checking the draft, it seemed to me that all nestings coincided with blocks (pairs of {}). That does not make the concept of nesting only possible within blocks, though. Refactoring the idea into a separate walking function that's independent of ExprKind::Block is definitely how it should look if we expand into it :D
There was a problem hiding this comment.
I can think of two main ways to nest something outside (figuratively) of a Block:
-
One is when you have scoring expressions inside of a macro or otherwise a function call:
println!("The truth value is: {}", a && b || c); -
The other is when you have parenthesis:
a + (...)
The other ones would be these:
-
After a
box, however, this is unstable:box Expr -
Inside of a tuple:
(a, b, if c { 5 } else { - 6 }) -
While indexing something:
foo[...] -
In a range:
a..(b + 5) -
In an array definition:
[a, b, [c, d && e]]
felix91gr
force-pushed
the
cognitive_complexity_redo
branch
2 times, most recently
from
be82fe7
to
6d97f4a
Compare
|
I was mistaken! Simply because |
Ah, yes, the AST-visitor visits inner items, too. Only the HIR-visitor requires extra fiddling to recurse into inner items. |
felix91gr
force-pushed
the
cognitive_complexity_redo
branch
2 times, most recently
from
dd8a022
to
a74d060
Compare
felix91gr
force-pushed
the
cognitive_complexity_redo
branch
from
a74d060
to
032bfc2
Compare
felix91gr
force-pushed
the
cognitive_complexity_redo
branch
from
032bfc2
to
b22341f
Compare
|
☔ The latest upstream changes (presumably #3968) made this pull request unmergeable. Please resolve the merge conflicts. |
|
For the ui tests that were being flagged, I've marked I did this, because functions in ui tests aren't used in the traditional way: most of them are just a compendium of snippets to be linted. Therefore, a Cognitive Complexity analysis doesn't really make sense in their context :) |
tests/ui/cognitive_complexity.rs
Outdated
| } | ||
| } | ||
| lucas_number(n) | ||
| } |
There was a problem hiding this comment.
It would be good to have a test that this doens't trigger in external macros. To do that you'd add a new auxiliary file that contains a proc macro which contains code triggering the lint. Similar to tests/ui/auxiliary/proc_macro_derive.rs but probably better as a separate file.
There was a problem hiding this comment.
Not only that! Also a test with a local macro :D since this lint is supposed to be run before macro expansion ^^
There was a problem hiding this comment.
I've made a test for local macros, that expands to this function and a call to it. It's in ui/cognitive_complexity/no_ops.rs
I will make another one for non-local macros as soon as I know how!
There was a problem hiding this comment.
Thing is, while writing that response I realized that you were talking about procedural macros. Where should I learn how to make one? I'm currently learning my way through the macros by example :D
* CoC is now a pre-expansion Early pass. * The full draft of issue 3793 has been implemented. * `compile-test.rs` now allows `clippy::cognitive_complexity` in ui tests by default. This is because most ui tests don't use functions in a standard manner, and tend to have rather large ones. * Tests for CoC have been updated.
felix91gr
force-pushed
the
cognitive_complexity_redo
branch
from
508115e
to
8625c3f
Compare
|
Scoring is now ready to be more fine-grained as I've mentioned at #3793 (comment). All base scoring has been multiplied by 10, with which we can now (a) have low-score structures, and (b) have multiplying factors (like x1.2 por example) without using floating point numbers ^^ |
|
Just wondering, what is the status on this PR? It doesn't seem like much is needed to merge it, and it would be nice to have an implementation of cognitive complexity that is accurate (I assume based on the Campbell paper). The current cyclomatic/cognitive hybrid is a bit confusing, so if this PR is expected to be delayed longer for one reason or another, I think it would be a good idea to mention in the docs that the current measure is non-standard, and perhaps give more details on how it differs from cyclomatic complexity. |
|
I'm sorry for the delay, @rzumer. I've been very busy dealing with irl things and then my internship (which also takes a toll on me and my hands <~ I can only code for a certain amount a day). All in all, just haven't been able to come back to this project. It is, indeed, based on the Campbell paper. At least the idea is to have an MVP based off of it, tune it before release, and then start exploring beyond it after that. Here is the topic thread for you to take a look: #3793 In the last entry, you can see what's missing and currently planned. If you have experience with this topic, opinions, or anything you'd like to add, please do! I'd love to up the discussion there ^^ Bear in mind that I'm currently in hiatus, so I might not be able to be super helpful. But the discussion can still go on of course :) |
|
Excuse me please, but could anyone explain to me why this PR is so big? 213 files were changed, but I expected only a few, related to the |
|
Oh no. Must be because I haven't updated the PR files... and since Clippy uses unstable APIs from Lemme see what I can do. I will try to come back to you today. |
|
Looks like a failed rebase TBH. |
|
Yeah, also that. >.< |
|
I might end up just re-gitting this PR on a different branch just to save @vitjafx the hassle. I think I did mess up at least 1 rebase, although I dunno how to fix it. @mati865 have you got any tips? |
|
@felix91gr it's possible to rebase badly rebased changes 😀. Though depending on how bad was the rebase it requires some skills with Another possibility is to find last commit before failed rebase with |
|
Hmm. @mati865 I don't think I have the git skills necessary. I'm okay at it but no pro. I know it's a big ask, but can I ask you for guidance? Just having you on Discord or something for questions should help me a lot. |
|
@felix91gr I haven't dealt with issue like that for years, I'll see if I can do something with it but I'm not the pro you are looking for 😄. |
|
The problem is that you have pushed up a large commit with lots of extra changes rolled into it. We can't separate them without knowing what files were supposed to change, there is no git magic we can do to fix this from our end. Can you find an older commit from your reflog without these changes and push that? |
|
e.g. do you still have commit 81525c7 locally? |
|
I think it's worth trying to rewrite the patch using this patch as a template. Even if we do fix the git issues, the rebase will be super ugly. commit 8625c3f69839bd4ef928f7b254d155612a36eb67
Author: Félix Fischer <[email protected]>
Date: Thu Apr 11 18:22:19 2019 -0400
Changed Cognitive Complexity as per issue 3793
* CoC is now a pre-expansion Early pass.
* The full draft of issue 3793 has been implemented.
* `compile-test.rs` now allows `clippy::cognitive_complexity` in ui tests by default.
This is because most ui tests don't use functions in a standard manner, and tend to have rather large ones.
* Tests for CoC have been updated.
diff --git a/clippy_lints/src/cognitive_complexity.rs b/clippy_lints/src/cognitive_complexity.rs
index d4478484..41c62a66 100644
--- a/clippy_lints/src/cognitive_complexity.rs
+++ b/clippy_lints/src/cognitive_complexity.rs
@@ -1,15 +1,13 @@
//! calculate cognitive complexity and warn about overly complex functions
-use rustc::cfg::CFG;
-use rustc::hir::intravisit::{walk_expr, NestedVisitorMap, Visitor};
-use rustc::hir::*;
-use rustc::lint::{LateContext, LateLintPass, LintArray, LintContext, LintPass};
-use rustc::ty;
-use rustc::{declare_tool_lint, lint_array};
+use rustc::lint::{EarlyContext, EarlyLintPass};
+use rustc::lint::{LintArray, LintContext, LintPass};
+use rustc::{declare_tool_lint, impl_lint_pass};
use syntax::ast::Attribute;
-use syntax::source_map::Span;
+use syntax::ast::*;
+use syntax::visit::{walk_expr, Visitor};
-use crate::utils::{in_macro, is_allowed, match_type, paths, span_help_and_lint, LimitStack};
+use crate::utils::{span_help_and_lint, LimitStack};
declare_clippy_lint! {
/// **What it does:** Checks for methods with high cognitive complexity.
@@ -20,7 +18,9 @@ declare_clippy_lint! {
/// **Known problems:** Sometimes it's hard to find a way to reduce the
/// complexity.
///
- /// **Example:** No. You'll see it when you get the warning.
+ /// **Example:** Sorry. Examples are too big and varied to put in here. For a
+ /// complete explanation of the analysis being made though, you can read this paper:
+ /// https://www.sonarsource.com/docs/CognitiveComplexity.pdf
pub COGNITIVE_COMPLEXITY,
complexity,
"functions that should be split up into multiple functions"
@@ -28,210 +28,475 @@ declare_clippy_lint! {
pub struct CognitiveComplexity {
limit: LimitStack,
+ current_enclosing_function: Option<NodeId>,
}
impl CognitiveComplexity {
pub fn new(limit: u64) -> Self {
Self {
limit: LimitStack::new(limit),
+ current_enclosing_function: None,
}
}
}
-impl LintPass for CognitiveComplexity {
- fn get_lints(&self) -> LintArray {
- lint_array!(COGNITIVE_COMPLEXITY)
+impl_lint_pass!(CognitiveComplexity => [COGNITIVE_COMPLEXITY]);
+
+impl EarlyLintPass for CognitiveComplexity {
+ fn check_item_post(&mut self, _: &EarlyContext<'_>, item: &Item) {
+ // After processing the inner AST of a function, we unrecord its
+ // id, so that other functions can now be recognized and processed.
+ if let Some(fn_id) = self.current_enclosing_function {
+ if item.id == fn_id {
+ self.current_enclosing_function = None;
+ }
+ }
+ }
+
+ fn check_item(&mut self, cx: &EarlyContext<'_>, item: &Item) {
+ if let ItemKind::Fn(_, _, _, fn_block) = &item.node {
+ // Before scoring a function, we check that it's not
+ // an inner function. If it was, then we'd be scoring it
+ // twice: once for its parent and once for itself.
+ if let None = self.current_enclosing_function {
+ // Now that we've entered a function, we record it
+ // as the current enclosing one. No functions inside it
+ // will ever be scored now.
+ self.current_enclosing_function = Some(item.id);
+
+ // If the function being explored is marked as "test",
+ // then we skip it.
+ if item.attrs.iter().any(|a| a.check_name("test")) {
+ return;
+ }
+
+ let mut helper = CoCHelper::new();
+
+ helper.visit_block(&fn_block);
+
+ let fn_score = helper.score;
+ let score_limit = self.limit.limit();
+
+ if fn_score > score_limit {
+ span_help_and_lint(
+ cx,
+ COGNITIVE_COMPLEXITY,
+ item.span,
+ &format!("the function has a cognitive complexity of {}", fn_score),
+ "you could split it up into multiple smaller functions",
+ );
+ }
+ }
+ }
}
- fn name(&self) -> &'static str {
- "CognitiveComplexity"
+ fn enter_lint_attrs(&mut self, cx: &EarlyContext<'_>, attrs: &[Attribute]) {
+ self.limit.push_attrs(cx.sess(), attrs, "cognitive_complexity");
+ }
+ fn exit_lint_attrs(&mut self, cx: &EarlyContext<'_>, attrs: &[Attribute]) {
+ self.limit.pop_attrs(cx.sess(), attrs, "cognitive_complexity");
}
}
-impl CognitiveComplexity {
- fn check<'a, 'tcx: 'a>(&mut self, cx: &'a LateContext<'a, 'tcx>, body: &'tcx Body, span: Span) {
- if in_macro(span) {
- return;
- }
+/// Helps keep track of the Cognitive Complexity Score
+/// of a function being analyzed.
+struct CoCHelper {
+ /// Current Nesting value
+ current_nesting: u64,
+ /// Current Cognitive Complexity score
+ score: u64,
+ /// Current Nesting of Binary Logical Operations
+ /// (used for proper score calculation)
+ logical_binop_nesting: u64,
+}
- let cfg = CFG::new(cx.tcx, body);
- let expr = &body.value;
- let n = cfg.graph.len_nodes() as u64;
- let e = cfg.graph.len_edges() as u64;
- if e + 2 < n {
- // the function has unreachable code, other lints should catch this
- return;
- }
- let cc = e + 2 - n;
- let mut helper = CCHelper {
- match_arms: 0,
- divergence: 0,
- short_circuits: 0,
- returns: 0,
- cx,
- };
- helper.visit_expr(expr);
- let CCHelper {
- match_arms,
- divergence,
- short_circuits,
- returns,
- ..
- } = helper;
- let ret_ty = cx.tables.node_type(expr.hir_id);
- let ret_adjust = if match_type(cx, ret_ty, &paths::RESULT) {
- returns
- } else {
- returns / 2
- };
-
- if cc + divergence < match_arms + short_circuits {
- report_cc_bug(
- cx,
- cc,
- match_arms,
- divergence,
- short_circuits,
- ret_adjust,
- span,
- body.id().hir_id,
- );
- } else {
- let mut rust_cc = cc + divergence - match_arms - short_circuits;
- // prevent degenerate cases where unreachable code contains `return` statements
- if rust_cc >= ret_adjust {
- rust_cc -= ret_adjust;
- }
- if rust_cc > self.limit.limit() {
- span_help_and_lint(
- cx,
- COGNITIVE_COMPLEXITY,
- span,
- &format!("the function has a cognitive complexity of {}", rust_cc),
- "you could split it up into multiple smaller functions",
- );
- }
+enum ComplexityLevel {
+ /// Almost no individual score (Paren, Assign, AssignOp)
+ Low,
+ /// Most common score (If, Return, Yield)
+ Normal,
+ // FIXME: delete or populate this case.
+ /// High score (no cases yet?)
+ High,
+ /// Custom score (a catch-all for other cases)
+ Custom(u64),
+}
+
+impl ComplexityLevel {
+ fn get_score(&self) -> u64 {
+ match self {
+ ComplexityLevel::Low => 2,
+ ComplexityLevel::Normal => 10,
+ ComplexityLevel::High => 50,
+ ComplexityLevel::Custom(score) => *score,
}
}
}
-impl<'a, 'tcx> LateLintPass<'a, 'tcx> for CognitiveComplexity {
- fn check_fn(
- &mut self,
- cx: &LateContext<'a, 'tcx>,
- _: intravisit::FnKind<'tcx>,
- _: &'tcx FnDecl,
- body: &'tcx Body,
- span: Span,
- hir_id: HirId,
- ) {
- let def_id = cx.tcx.hir().local_def_id_from_hir_id(hir_id);
- if !cx.tcx.has_attr(def_id, "test") {
- self.check(cx, body, span);
+impl CoCHelper {
+ /// Create a clean CoCHelper
+ fn new() -> CoCHelper {
+ CoCHelper {
+ current_nesting: 0,
+ score: 0,
+ logical_binop_nesting: 0,
}
}
- fn enter_lint_attrs(&mut self, cx: &LateContext<'a, 'tcx>, attrs: &'tcx [Attribute]) {
- self.limit.push_attrs(cx.sess(), attrs, "cognitive_complexity");
+ /// Increment the nesting level by one
+ fn push_nesting(&mut self) {
+ self.current_nesting += 1;
}
- fn exit_lint_attrs(&mut self, cx: &LateContext<'a, 'tcx>, attrs: &'tcx [Attribute]) {
- self.limit.pop_attrs(cx.sess(), attrs, "cognitive_complexity");
+
+ /// Decrement the nesting level by one
+ fn pop_nesting(&mut self) {
+ assert!(self.current_nesting > 0);
+ self.current_nesting -= 1;
+ }
+
+ /// Mark a determined amount of score
+ fn add_to_score(&mut self, amount: u64) {
+ self.score += amount;
+ }
+
+ /// Mark score for a Nesting-Dependent Structure
+ fn score_nd_structure(&mut self, level: ComplexityLevel) {
+ self.add_to_score(self.current_nesting * level.get_score());
+ self.score_ni_structure(level);
}
-}
-struct CCHelper<'a, 'tcx: 'a> {
- match_arms: u64,
- divergence: u64,
- returns: u64,
- short_circuits: u64, // && and ||
- cx: &'a LateContext<'a, 'tcx>,
+ /// Mark score for a Nesting-Independent Structure
+ fn score_ni_structure(&mut self, level: ComplexityLevel) {
+ self.add_to_score(level.get_score());
+ }
+
+ /// Let the helper know that we've entered a Binary Logical Operation
+ fn enter_logical_binop(&mut self) {
+ // We score once every time we enter a *new*
+ // binary logical operation.
+ // That way, the score for chains is
+ // `1 + (number of operator changes in the chain)`
+ if self.logical_binop_nesting == 0 {
+ self.score_ni_structure(ComplexityLevel::Normal);
+ }
+ self.logical_binop_nesting += 1;
+ }
+
+ /// Let the helper know that we've exited a Binary Logical Operation
+ fn exit_logical_binop(&mut self) {
+ self.logical_binop_nesting -= 1;
+ }
}
-impl<'a, 'tcx> Visitor<'tcx> for CCHelper<'a, 'tcx> {
- fn visit_expr(&mut self, e: &'tcx Expr) {
- match e.node {
- ExprKind::Match(_, ref arms, _) => {
- walk_expr(self, e);
- let arms_n: u64 = arms.iter().map(|arm| arm.pats.len() as u64).sum();
- if arms_n > 1 {
- self.match_arms += arms_n - 2;
- }
+impl<'ast> Visitor<'ast> for CoCHelper {
+ /*
+ # Implemented here:
+
+ ## Nesting Structures
+ IMPORTANT: ExprKind::Block(..)
+ already covers all cases.
+ ## (Nesting-Dependent) Increments
+ if
+ match
+ for, while, loop
+ ## (Nesting-Independent) Increments
+ break, continue
+ Sequences of binary logical operators
+ Function calls
+ Macro calls
+ */
+
+ fn visit_expr(&mut self, ex: &'ast Expr) {
+ match ex.node {
+ // Nesting-Increasing (the one and only)
+ ExprKind::Block(..) => {
+ self.push_nesting();
+ walk_expr(self, ex);
+ self.pop_nesting();
+ },
+
+ ExprKind::Closure(.., _)
+ | ExprKind::IfLet(..)
+ | ExprKind::Lit(..)
+ | ExprKind::Try(..) => {
+ // "If Let" and "Try" are free of own increment.
+ // This is because they are language constructs
+ // specifically designed to save on complexity.
+ walk_expr(self, ex);
+ },
+
+ // FIXME (FAR FUTURE): make a separate, documented case, for recursive calls,
+ // such that it's treated differently from function or method calls.
+ ExprKind::Call(..) | ExprKind::MethodCall(..) => {
+ self.score_ni_structure(ComplexityLevel::Normal);
+ walk_expr(self, ex);
+ },
+
+ // Nesting-Dependent
+ ExprKind::If(..)
+ | ExprKind::Match(..)
+ | ExprKind::ForLoop(..)
+ | ExprKind::While(..)
+ | ExprKind::Loop(..)
+ | ExprKind::WhileLet(..) => {
+ // (For the IF-Case)
+ // Important: this pays for one "if" and one "else".
+ // Every "if" in an "else if" comes here again to pay
+ // for itself and its subsequent else.
+ self.score_nd_structure(ComplexityLevel::Normal);
+ walk_expr(self, ex);
+ },
+
+ // Nesting-Independent
+ ExprKind::Mac(ref mac) => {
+ self.visit_mac(mac);
+ },
+
+ // Nesting-Independent
+ ExprKind::Continue(_) => {
+ self.score_ni_structure(ComplexityLevel::Normal);
},
- ExprKind::Call(ref callee, _) => {
- walk_expr(self, e);
- let ty = self.cx.tables.node_type(callee.hir_id);
- match ty.sty {
- ty::FnDef(..) | ty::FnPtr(_) => {
- let sig = ty.fn_sig(self.cx.tcx);
- if sig.skip_binder().output().sty == ty::Never {
- self.divergence += 1;
- }
- },
- _ => (),
+
+ // Nesting-Independent,
+ // Sometimes Nesting
+ ExprKind::Break(_, ref maybe_inner_ex) => {
+ self.score_ni_structure(ComplexityLevel::Normal);
+ if let Some(ref inner_ex) = maybe_inner_ex {
+ walk_expr(self, inner_ex);
}
},
- ExprKind::Closure(.., _) => (),
- ExprKind::Binary(op, _, _) => {
- walk_expr(self, e);
- match op.node {
- BinOpKind::And | BinOpKind::Or => self.short_circuits += 1,
- _ => (),
+
+ // (Nesting-Independent) When boolean operators change, we add 1 to the score.
+ ExprKind::Binary(binop, ref l_ex, ref r_ex) => {
+ // Here, we're either looking for the leftmost logical operator on the right side,
+ // or the rightmost logical operator on the left side. It looks like this:
+ //
+ // Let's say our Expr is `(a && b) || ((c ^ d) & e)`, and its AST:
+ //
+ // Or
+ // / \
+ // / \
+ // / \
+ // Paren Paren
+ // | |
+ // And BitAnd
+ // / \ / \
+ // a b / \
+ // Paren e
+ // |
+ // Xor
+ // / \
+ // c d
+ //
+ // Then, when we call `log_op_at(right_branch, At:LeftMostSide)`,
+ // We're looking for that Xor at the leftmost side of the right branch:
+ // Or
+ // / \
+ // / \
+ // / \
+ // Paren Paren
+ // | |
+ // And BitAnd
+ // / \ / \
+ // a b / \
+ // Paren e
+ // |
+ // Xor <~ THIS ONE :D
+ // / \
+ // c d
+ //
+ // Doing this, we can effectively mark a score whenever there is a change
+ // in the current chain of logical operators.
+ //
+ // So say for example, that we're scoring `a && b && c || d && e`.
+ // There are 2 changes in the operator chain in this expression,
+ // once at `c || d` (it changes from `&&` to `||`) and once at
+ // `d && e` (it changes from `||` to `&&`).
+ //
+ // In order for us to score this change, regardless of the shape of
+ // the AST, we need to be able to know which operator sits right
+ // next to the current one. If it's then a different operator,
+ // we know there is a change in the chain, and we can score it.
+ //
+ // But what about scoring it twice? Will we see the same change
+ // more than once?
+ // The answer is no: since ASTs are recursive, child operators
+ // can't see their parent operators. Given we're only scoring
+ // a change whenever the operators right next to the current one
+ // are different to it, AND in our subsequent calls the current
+ // operator will not be visible, it's effectively impossible
+ // to score this change in the chain more than once.
+
+ /// A location in the AST.
+ enum At {
+ LeftMostSide,
+ RightMostSide,
+ }
+
+ /// A logical operator
+ #[derive(PartialEq)]
+ enum LogOp {
+ LogAnd, // &&
+ LogOr, // ||
+ BitAnd, // &
+ BitOr, // |
+ BitXor, // ^
+ None, // Other
+ }
+
+ /// Translate from a binary operator to a logical operator
+ fn log_op_from_bin_op(bop_kind: BinOpKind) -> LogOp {
+ match bop_kind {
+ BinOpKind::And => LogOp::LogAnd,
+ BinOpKind::Or => LogOp::LogOr,
+ BinOpKind::BitAnd => LogOp::BitAnd,
+ BinOpKind::BitOr => LogOp::BitOr,
+ BinOpKind::BitXor => LogOp::BitXor,
+ _ => LogOp::None,
+ }
+ }
+
+ /// Find the rightmost or leftmost logical operator inside of the given `Expr`
+ fn log_op_at(expr: &Expr, at: At) -> LogOp {
+ match &expr.node {
+ ExprKind::Binary(binop, ref left_side, ref right_side) => {
+ let current_operator = log_op_from_bin_op(binop.node);
+
+ let next_operator = match at {
+ At::LeftMostSide => log_op_at(left_side, at),
+ At::RightMostSide => log_op_at(right_side, at),
+ };
+
+ match next_operator {
+ LogOp::None => current_operator,
+ _ => next_operator,
+ }
+ },
+ ExprKind::Paren(expr) | ExprKind::Unary(_, expr) => log_op_at(&expr, at),
+ _ => LogOp::None,
+ }
+ }
+
+ let current_log_op = log_op_from_bin_op(binop.node);
+
+ let is_log_op = current_log_op != LogOp::None;
+
+ if is_log_op {
+ // Here we separate the left and right branches, and go looking
+ // for the rightmost and leftmost logical operator in them, respectively
+ let op_at_left_side = log_op_at(l_ex, At::RightMostSide);
+ let op_at_right_side = log_op_at(r_ex, At::LeftMostSide);
+
+ if op_at_left_side != LogOp::None && current_log_op != op_at_left_side {
+ self.score_ni_structure(ComplexityLevel::Normal);
+ }
+
+ if op_at_right_side != LogOp::None && current_log_op != op_at_right_side {
+ self.score_ni_structure(ComplexityLevel::Normal);
+ }
+
+ self.enter_logical_binop();
}
+
+ walk_expr(self, ex);
+
+ if is_log_op {
+ self.exit_logical_binop();
+ }
+ },
+
+ /*
+ Low complexity cases
+ */
+
+ // (...)
+ // ExprKind::Paren(..) => {},
+
+ // # a += bar()
+ // ExprKind::AssignOp(..) => {},
+
+ // # a = foo()
+ // ExprKind::Assign(..) => {},
+
+ // # foo[2]
+ // ExprKind::Index(..) => {},
+
+ // # a.count, or b.0
+ // ExprKind::Field(..) => {},
+
+ // # &a or &mut a
+ // ExprKind::AddrOf(..) => {},
+
+ // !a, *b
+ // ExprKind::Unary(..) => {},
+
+ /*
+ Medium complexity cases
+ */
+
+ // Return and Yield have the same cog. complexity
+ // ExprKind::Ret(..) => {},
+ // ExprKind::Yield(..) => {},
+
+ // # foo as f32
+ // ExprKind::Cast(..) => {},
+
+ // # Struct literal: Foo { (things) }
+ // ExprKind::Struct(..) => {},
+
+ // # (a, b, c)
+ // ExprKind::Tup(..) => {},
+
+ // # [a, b, c, d]
+ // ExprKind::Array(..) => {},
+
+ // # m..n
+ // ExprKind::Range(..) => {},
+
+ /*
+ ### Pending ones (FIXME) ###
+ */
+
+ // # [m; n]
+ // ExprKind::Repeat(..) => {},
+
+ // Haven't used these. Investigate further.
+ // ExprKind::TryBlock(..) => {},
+
+ // # Variable reference??
+ // ExprKind::Path(..) => {},
+
+ // # (unstable) `box a` syntax
+ // ExprKind::Box(..) => {},
+
+ // # FIXME: what is this?
+ // ExprKind::ObsoleteInPlace(..) => {},
+
+ // What is Type Ascription??
+ // ExprKind::Type(..) => {},
+
+ // Unstable, leave it for after the MVP.
+ // ExprKind::Async(..) => {},
+
+ // # asm!(), basically, inline assembly
+ // ExprKind::InlineAsm(..) => {},
+
+ // Ill formed expressions.
+ ExprKind::Err => {
+ panic!("Found an ExprKind::Err. Is this a compiler bug??");
+ },
+
+ _ => {
+ walk_expr(self, ex);
},
- ExprKind::Ret(_) => self.returns += 1,
- _ => walk_expr(self, e),
}
}
- fn nested_visit_map<'this>(&'this mut self) -> NestedVisitorMap<'this, 'tcx> {
- NestedVisitorMap::None
- }
-}
-#[cfg(feature = "debugging")]
-#[allow(clippy::too_many_arguments)]
-fn report_cc_bug(
- _: &LateContext<'_, '_>,
- cc: u64,
- narms: u64,
- div: u64,
- shorts: u64,
- returns: u64,
- span: Span,
- _: HirId,
-) {
- span_bug!(
- span,
- "Clippy encountered a bug calculating cognitive complexity: cc = {}, arms = {}, \
- div = {}, shorts = {}, returns = {}. Please file a bug report.",
- cc,
- narms,
- div,
- shorts,
- returns
- );
-}
-#[cfg(not(feature = "debugging"))]
-#[allow(clippy::too_many_arguments)]
-fn report_cc_bug(
- cx: &LateContext<'_, '_>,
- cc: u64,
- narms: u64,
- div: u64,
- shorts: u64,
- returns: u64,
- span: Span,
- id: HirId,
-) {
- if !is_allowed(cx, COGNITIVE_COMPLEXITY, id) {
- cx.sess().span_note_without_error(
- span,
- &format!(
- "Clippy encountered a bug calculating cognitive complexity \
- (hide this message with `#[allow(cognitive_complexity)]`): \
- cc = {}, arms = {}, div = {}, shorts = {}, returns = {}. \
- Please file a bug report.",
- cc, narms, div, shorts, returns
- ),
- );
+ fn visit_mac(&mut self, _mac: &Mac) {
+ // We override this so that the compiler
+ // doesn't panic. See the original implementation
+ // of `visit_mac` at rustc's src/libsyntax/visit.rs
+ // to know what normally happens.
+ self.score_ni_structure(ComplexityLevel::Normal);
}
} |
|
I think i got something building |
|
https://github.com/Manishearth/rust-clippy/tree/cognitive-complexity2 I moved the lint to the nursery for now, we can update all tests with an SOme of the new tests fail, and there's an ICE. also, since the patch was written |
|
Thank you @Manishearth for coming to the rescue ❤️ Okay, in order:
Dangit! Ahh. I know I shouldn't complain right now about git, but I honestly can't wait until pijul ships and I get to use the concept of "patches" directly. It makes a lot more sense in my head than whatever git does 🤔 Well, maybe... this is also related to the idea of "rebasing"? Maybe if we could rebase on merge only, things would be easier to do while the PRs are still open.
I think I can! I still have a copy of my old directory here. What should I do?
I'm completely cool with that. I get the feeling that the older the PRs get, the more unwieldy they become. It probably is because I'm not as good with git as I'd like, but that's how it feels anyway.
I might've forgotten something. What is the compile test hack?
Uh oh. How can I help here?
Ah, that's okay. Thanks for pointing it out, I would've missed the trigger for a week if you didn't ^^ |
|
Git actually handles this pretty well: normally messing this up would still lead to distinct patches. The problem was the merge got rolled into the commit -- git rebase has some UI issues that probably caused it. You don't need to bother finding your old reflog or whatever. I cleaned up the patch and pushed it to https://github.com/Manishearth/rust-clippy/tree/cognitive-complexity2 The ICE is just a panic from the cognitive complexity lint, see Manishearth@75173c9#diff-80830efdb26c234483d2d7d14c1a84b5R7 . |
Ah, I see. I'm sorry I messed it up, but I also agree with the UI issues. I have read papers about the matter as well, it seems that git has some serious UX problems. (edit: here are some references for those interested)
Thank you Manish, seriously. I saw it, haven't built it yet but it seems perfectly fine from here. Will use it asap :) The ICE stems from this line I wrote: Manishearth@c3116c1#diff-c09c9aa9cafeea7716a67ba096f4a306R483 iirc I understood that I should panic when finding an (edit: |
|
Yeah, so |
|
I see. Okay, I will fix that as well :) Thank you for putting it into nursery as well. You helping out and myself being in a better state overall make me want to try and push this for completion. After I reconfigure the PR (which will be as soon as I can so that others can depend on it), I want to try to dedicate at least one afternoon to this a week. Thank you Manish. Talk to you later. PS: about this,
What is the |
oh, just that you had modified compile-test.rs to add |
|
Ahh, perfect. I remember now. Thank you! That makes a lot of sense 😄 |
|
Closing this PR, it will be succeeded by #4516 |
Following up on #3803
Overall checklist:
Notes of this PR:
cognitive_complexity_thresholdhas been raised to 50.allowmarker on top of them, because they are either a compendium (likelib.rs:register_plugins) or a very repetitive pattern put into a utility function (likeutils/inspector.rs:print_expr). There's two for which I can't say, though. Please take a look at them:rust-clippy/clippy_lints/src/types.rs
Line 246 in fbb3a47
rust-clippy/clippy_lints/src/loops.rs
Line 1071 in fbb3a47
Details left to be worked out:
allow(clippy::cognitive_complexity)), because those tests might actually need to be split up into more understandable pieces.