PR feedback (low hanging fruit first)

crates/rome_analyze/CONTRIBUTING.md

@@ -463,7 +463,7 @@ impl Visitor for MissingYieldVisitor {
                 // entry of the stack and the `has_yield` flag is `false`, emit a query match
                 if let Some(exit_node) = AnyFunctionLike::cast_ref(node) {
                     if let Some((enter_node, has_yield)) = self.stack.pop() {
-                        assert_eq!(enter_node, exit_node);
+                        debug_assert_eq!(enter_node, exit_node);
                         if !has_yield {
                             ctx.match_query(MissingYield(enter_node));
                         }

crates/rome_js_analyze/src/analyzers/correctness/use_yield.rs

@@ -4,11 +4,8 @@ use rome_analyze::{
     Visitor, VisitorContext,
 };
 use rome_console::markup;
-use rome_js_syntax::{
-    AnyJsFunction, JsLanguage, JsMethodClassMember, JsMethodObjectMember, JsStatementList,
-    JsYieldExpression, TextRange, WalkEvent,
-};
-use rome_rowan::{declare_node_union, AstNode, AstNodeList, Language, SyntaxNode};
+use rome_js_syntax::{AnyFunctionLike, JsLanguage, JsYieldExpression, TextRange, WalkEvent};
+use rome_rowan::{AstNode, AstNodeList, Language, SyntaxNode};
 
 declare_rule! {
     /// Require generator functions to contain `yield`.
@@ -48,40 +45,6 @@ declare_rule! {
     }
 }
 
-declare_node_union! {
-    pub(crate) AnyFunctionLike = AnyJsFunction | JsMethodObjectMember | JsMethodClassMember
-}
-
-impl AnyFunctionLike {
-    fn is_generator(&self) -> bool {
-        match self {
-            AnyFunctionLike::AnyJsFunction(any_js_function) => any_js_function.is_generator(),
-            AnyFunctionLike::JsMethodClassMember(method_class_member) => {
-                method_class_member.star_token().is_some()
-            }
-            AnyFunctionLike::JsMethodObjectMember(method_obj_member) => {
-                method_obj_member.star_token().is_some()
-            }
-        }
-    }
-
-    fn statements(&self) -> Option<JsStatementList> {
-        Some(match self {
-            AnyFunctionLike::AnyJsFunction(any_js_function) => any_js_function
-                .body()
-                .ok()?
-                .as_js_function_body()?
-                .statements(),
-            AnyFunctionLike::JsMethodClassMember(method_class_member) => {
-                method_class_member.body().ok()?.statements()
-            }
-            AnyFunctionLike::JsMethodObjectMember(method_obj_member) => {
-                method_obj_member.body().ok()?.statements()
-            }
-        })
-    }
-}
-
 #[derive(Default)]
 struct MissingYieldVisitor {
     /// Vector to hold a function node and a boolean indicating whether the function

crates/rome_js_analyze/src/analyzers/nursery/no_excessive_complexity.rs

@@ -1,4 +1,3 @@
-use crate::analyzers::correctness::use_yield::AnyFunctionLike;
 use bpaf::Bpaf;
 use rome_analyze::{
     context::RuleContext, declare_rule, AddVisitor, Phases, QueryMatch, Queryable, Rule,
@@ -10,34 +9,33 @@ use rome_deserialize::{
     DeserializationDiagnostic, VisitNode,
 };
 use rome_js_syntax::{
-    AnyJsFunctionBody, JsBreakStatement, JsCatchClause, JsConditionalExpression,
-    JsContinueStatement, JsDoWhileStatement, JsElseClause, JsFinallyClause, JsForInStatement,
-    JsForOfStatement, JsForStatement, JsIfStatement, JsLanguage, JsLogicalExpression,
-    JsLogicalOperator, JsSwitchStatement, JsWhileStatement, JsWithStatement,
+    AnyFunctionLike, JsBreakStatement, JsContinueStatement, JsElseClause, JsLanguage,
+    JsLogicalExpression, JsLogicalOperator,
 };
 use rome_json_syntax::{JsonLanguage, JsonSyntaxNode};
-use rome_rowan::{AstNode, Language, SyntaxNode, SyntaxResult, TextRange, WalkEvent};
+use rome_rowan::{AstNode, Language, SyntaxNode, TextRange, WalkEvent};
 use serde::{Deserialize, Serialize};
 use std::str::FromStr;
 
 #[cfg(feature = "schemars")]
 use schemars::JsonSchema;
 
 declare_rule! {
+    /// Disallow functions that exceed a given complexity score.
+    ///
     /// The more complexity a function contains, the harder it is to understand
     /// later on.
     ///
     /// Reducing complexity helps to make code more maintenable, both by making
     /// it easier to understand as well as by reducing chances of accidental
     /// side-effects when making changes.
     ///
-    /// This rule calculates a complexity score for every function and signals
+    /// This rule calculates a complexity score for every function and disallows
     /// those that exceed a configured complexity threshold (default: 10).
     ///
-    /// Sources:
+    /// Source:
     ///
     /// * https://github.com/SonarSource/eslint-plugin-sonarjs/blob/HEAD/docs/rules/cognitive-complexity.md
-    /// * https://eslint.org/docs/latest/rules/complexity (note this rule uses "cyclomatic complexity" instead)
     ///
     /// ## Examples
     ///
@@ -88,16 +86,17 @@ impl Rule for NoExcessiveComplexity {
         Some(
             RuleDiagnostic::new(
                 rule_category!(),
-                match function_like.id_range() {
+                match function_like.name_range() {
                     Some(id_range) => id_range,
                     // This `unwrap()` is safe because we know there is a body,
                     // otherwise the visitor wouldn't have matched anything.
-                    _ => function_like.body().unwrap().range()
+                    _ => function_like.body().unwrap().range(),
                 },
                 markup!("Excessive complexity detected."),
             )
             .note(markup! {
-                "Please refactor this function to reduce its complexity score from "{calculated_score}" to "{max_allowed_complexity}"."
+                "Please refactor this function to reduce its complexity score from "
+                {calculated_score}" to the max allowed complexity "{max_allowed_complexity}"."
             }),
         )
     }
@@ -137,6 +136,11 @@ struct CognitiveComplexityFunctionState {
     function_like: AnyFunctionLike,
     score: usize,
     nesting_level: usize,
+
+    /// Cognitive complexity does not increase for every logical operator,
+    /// but for every *sequence* of identical logical operators. Therefore, we
+    /// track which operator was last seen and incur a penalty when a different
+    /// operator is encountered.
     last_seen_operator: Option<JsLogicalOperator>,
 }
 
@@ -202,7 +206,7 @@ impl Visitor for CognitiveComplexityVisitor {
             WalkEvent::Leave(node) => {
                 if let Some(exit_node) = AnyFunctionLike::cast_ref(node) {
                     if let Some(function_state) = self.stack.pop() {
-                        assert_eq!(function_state.function_like, exit_node);
+                        debug_assert_eq!(function_state.function_like, exit_node);
                         ctx.match_query(CognitiveComplexity {
                             function_like: exit_node,
                             score: ComplexityScore {
@@ -229,45 +233,70 @@ impl Visitor for CognitiveComplexityVisitor {
     }
 }
 
+/// Returns whether the node is considered to increase the nesting level inside
+/// the function.
+///
+/// Note: These are mostly nodes that increase the complexity of the function's
+/// control flow.
 fn increases_nesting(node: &SyntaxNode<JsLanguage>) -> bool {
-    let kind = node.kind();
+    use rome_js_syntax::JsSyntaxKind::*;
     is_loop_node(node)
-        || JsCatchClause::can_cast(kind)
-        || JsConditionalExpression::can_cast(kind)
-        || JsIfStatement::can_cast(kind)
-        || JsSwitchStatement::can_cast(kind)
+        || matches!(
+            node.kind(),
+            JS_CATCH_CLAUSE | JS_CONDITIONAL_EXPRESSION | JS_IF_STATEMENT | JS_SWITCH_STATEMENT
+        )
 }
 
 fn is_loop_node(node: &SyntaxNode<JsLanguage>) -> bool {
-    let kind = node.kind();
-    JsDoWhileStatement::can_cast(kind)
-        || JsForInStatement::can_cast(kind)
-        || JsForOfStatement::can_cast(kind)
-        || JsForStatement::can_cast(kind)
-        || JsWhileStatement::can_cast(kind)
+    use rome_js_syntax::JsSyntaxKind::*;
+    matches!(
+        node.kind(),
+        JS_DO_WHILE_STATEMENT
+            | JS_FOR_OF_STATEMENT
+            | JS_FOR_IN_STATEMENT
+            | JS_FOR_STATEMENT
+            | JS_WHILE_STATEMENT
+    )
 }
 
+/// Returns whether use of the given node results in a penalty for increasing
+/// the complexity of the structure of the function.
+///
+/// The structure of a function is mostly defined by its control flow, although
+/// there are some node types that we consider as increasing its structural
+/// complexity even though they do not affect its control flow.
+///
+/// A prime example of this is the `with` statement, which does not affect
+/// control flow, but which is considered to increase structural complexity
+/// since developers will need to spend additional effort tracing the scope of
+/// variables.
+///
+/// Do note that the SonarSource paper makes no mention of the `with` statement
+/// specifically (probably because it's highly specific to JavaScript), so its
+/// inclusion here is a personal judgement call.
 fn receives_structural_penalty(node: &SyntaxNode<JsLanguage>) -> bool {
-    let kind = node.kind();
+    use rome_js_syntax::JsSyntaxKind::*;
     receives_nesting_penalty(node)
+        || matches!(node.kind(), JS_FINALLY_CLAUSE | JS_WITH_STATEMENT)
         || JsBreakStatement::cast_ref(node)
             .and_then(|js_break| js_break.label_token())
             .is_some()
         || JsContinueStatement::cast_ref(node)
             .and_then(|js_continue| js_continue.label_token())
             .is_some()
-        || JsFinallyClause::can_cast(kind)
-        || JsWithStatement::can_cast(kind)
 }
 
-// Note: This is a strict subset of nodes that receive a structural penalty.
+/// Returns whether use of the given node receives an additional penalty based
+/// on the level of nesting in which it occurs.
+///
+/// Note: This is a strict subset of the nodes that receive a structural penalty.
 fn receives_nesting_penalty(node: &SyntaxNode<JsLanguage>) -> bool {
-    let kind = node.kind();
+    use rome_js_syntax::JsSyntaxKind::*;
     is_loop_node(node)
-        || JsCatchClause::can_cast(kind)
-        || JsConditionalExpression::can_cast(kind)
-        || JsIfStatement::can_cast(kind)
-        || JsSwitchStatement::can_cast(kind)
+        || matches!(
+            node.kind(),
+            JS_CATCH_CLAUSE | JS_CONDITIONAL_EXPRESSION | JS_IF_STATEMENT | JS_SWITCH_STATEMENT
+        )
 }
 
 #[derive(Clone, Default)]
@@ -335,31 +364,3 @@ impl VisitNode<JsonLanguage> for ComplexityOptions {
         Some(())
     }
 }
-
-impl AnyFunctionLike {
-    fn body(&self) -> SyntaxResult<AnyJsFunctionBody> {
-        match self {
-            AnyFunctionLike::AnyJsFunction(js_function) => js_function.body(),
-            AnyFunctionLike::JsMethodObjectMember(js_object_method) => js_object_method
-                .body()
-                .map(AnyJsFunctionBody::JsFunctionBody),
-            AnyFunctionLike::JsMethodClassMember(js_class_method) => js_class_method
-                .body()
-                .map(AnyJsFunctionBody::JsFunctionBody),
-        }
-    }
-
-    fn id_range(&self) -> Option<TextRange> {
-        match self {
-            AnyFunctionLike::AnyJsFunction(js_function) => {
-                js_function.id().ok().flatten().map(|id| id.range())
-            }
-            AnyFunctionLike::JsMethodObjectMember(js_object_method) => {
-                js_object_method.name().ok().map(|name| name.range())
-            }
-            AnyFunctionLike::JsMethodClassMember(js_class_method) => {
-                js_class_method.name().ok().map(|name| name.range())
-            }
-        }
-    }
-}

crates/rome_js_syntax/src/function_ext.rs

@@ -0,0 +1,64 @@
+use crate::{
+    AnyJsFunction, AnyJsFunctionBody, JsMethodClassMember, JsMethodObjectMember, JsStatementList,
+};
+use rome_rowan::{declare_node_union, AstNode, SyntaxResult, TextRange};
+
+declare_node_union! {
+    pub AnyFunctionLike = AnyJsFunction | JsMethodObjectMember | JsMethodClassMember
+}
+
+impl AnyFunctionLike {
+    pub fn body(&self) -> SyntaxResult<AnyJsFunctionBody> {
+        match self {
+            AnyFunctionLike::AnyJsFunction(js_function) => js_function.body(),
+            AnyFunctionLike::JsMethodObjectMember(js_object_method) => js_object_method
+                .body()
+                .map(AnyJsFunctionBody::JsFunctionBody),
+            AnyFunctionLike::JsMethodClassMember(js_class_method) => js_class_method
+                .body()
+                .map(AnyJsFunctionBody::JsFunctionBody),
+        }
+    }
+
+    pub fn is_generator(&self) -> bool {
+        match self {
+            AnyFunctionLike::AnyJsFunction(any_js_function) => any_js_function.is_generator(),
+            AnyFunctionLike::JsMethodClassMember(method_class_member) => {
+                method_class_member.star_token().is_some()
+            }
+            AnyFunctionLike::JsMethodObjectMember(method_obj_member) => {
+                method_obj_member.star_token().is_some()
+            }
+        }
+    }
+
+    pub fn name_range(&self) -> Option<TextRange> {
+        match self {
+            AnyFunctionLike::AnyJsFunction(js_function) => {
+                js_function.id().ok().flatten().map(|id| id.range())
+            }
+            AnyFunctionLike::JsMethodObjectMember(js_object_method) => {
+                js_object_method.name().ok().map(|name| name.range())
+            }
+            AnyFunctionLike::JsMethodClassMember(js_class_method) => {
+                js_class_method.name().ok().map(|name| name.range())
+            }
+        }
+    }
+
+    pub fn statements(&self) -> Option<JsStatementList> {
+        Some(match self {
+            AnyFunctionLike::AnyJsFunction(any_js_function) => any_js_function
+                .body()
+                .ok()?
+                .as_js_function_body()?
+                .statements(),
+            AnyFunctionLike::JsMethodClassMember(method_class_member) => {
+                method_class_member.body().ok()?.statements()
+            }
+            AnyFunctionLike::JsMethodObjectMember(method_obj_member) => {
+                method_obj_member.body().ok()?.statements()
+            }
+        })
+    }
+}

crates/rome_js_syntax/src/lib.rs

@@ -8,6 +8,7 @@ pub mod binding_ext;
 pub mod directive_ext;
 pub mod expr_ext;
 pub mod file_source;
+pub mod function_ext;
 pub mod identifier_ext;
 pub mod import_ext;
 pub mod jsx_ext;
@@ -24,6 +25,7 @@ mod union_ext;
 pub use self::generated::*;
 pub use expr_ext::*;
 pub use file_source::*;
+pub use function_ext::*;
 pub use identifier_ext::*;
 pub use modifier_ext::*;
 pub use rome_rowan::{