From 5f465e5baf2fb480687d02798692f904cff29de7 Mon Sep 17 00:00:00 2001 From: yassin-kammoun-sonarsource Date: Thu, 19 Sep 2024 15:36:00 +0200 Subject: [PATCH] JS-255 Restore cognitive complexity calculation at file level --- packages/jsts/src/rules/S3776/rule.ts | 243 ++++++++++++++---- packages/jsts/src/rules/S3776/unit.test.ts | 2 +- .../javascript/rules/javascript/S3776.html | 7 +- 3 files changed, 205 insertions(+), 47 deletions(-) diff --git a/packages/jsts/src/rules/S3776/rule.ts b/packages/jsts/src/rules/S3776/rule.ts index abc319a2e40..df19ef77f6f 100644 --- a/packages/jsts/src/rules/S3776/rule.ts +++ b/packages/jsts/src/rules/S3776/rule.ts @@ -26,6 +26,7 @@ import { getFirstTokenAfter, getJsxShortCircuitNodes, getMainFunctionTokenLocation, + isArrowFunctionExpression, isIfStatement, isLogicalExpression, IssueLocation, @@ -47,12 +48,7 @@ type LoopStatement = | TSESTree.DoWhileStatement | TSESTree.WhileStatement; -interface ScopeComplexity { - node: TSESTree.Program | TSESTree.FunctionLike; - nestingLevel: number; - nestingNodes: Set; - complexityPoints: ComplexityPoint[]; -} +type OptionalLocation = TSESTree.SourceLocation | null | undefined; const message = 'Refactor this function to reduce its Cognitive Complexity from {{complexityAmount}} to the {{threshold}} allowed.'; @@ -76,11 +72,60 @@ export const rule: Rule.RuleModule = { /** Indicator if the file complexity should be reported */ const isFileComplexity = context.options.includes('metric'); + /** Complexity of the file */ + let fileComplexity = 0; + + /** Complexity of the current function if it is *not* considered nested to the first level function */ + let complexityIfNotNested: ComplexityPoint[] = []; + + /** Complexity of the current function if it is considered nested to the first level function */ + let complexityIfNested: ComplexityPoint[] = []; + + /** Current nesting level (number of enclosing control flow statements and functions) */ + let nesting = 0; + + /** Indicator if the current top level function has a structural (generated by control flow statements) complexity */ + let topLevelHasStructuralComplexity = false; + + /** Indicator if the current top level function is React functional component */ + const reactFunctionalComponent = { + nameStartsWithCapital: false, + returnsJsx: false, + + isConfirmed() { + return this.nameStartsWithCapital && this.returnsJsx; + }, + + init(node: TSESTree.FunctionLike) { + this.nameStartsWithCapital = nameStartsWithCapital(node); + this.returnsJsx = false; + }, + }; + + /** Own (not including nested functions) complexity of the current top function */ + let topLevelOwnComplexity: ComplexityPoint[] = []; + + /** Nodes that should increase nesting level */ + const nestingNodes: Set = new Set(); + /** Set of already considered (with already computed complexity) logical expressions */ const consideredLogicalExpressions: Set = new Set(); - /** Stack of scopes that are either functions or the program */ - const scopes: ScopeComplexity[] = []; + /** Stack of enclosing functions */ + const enclosingFunctions: TSESTree.FunctionLike[] = []; + + /** Stack of complexity points for each function without accumulated nested complexity */ + const functionOwnComplexity: ComplexityPoint[][] = []; + + const functionOwnControlFlowNesting: number[] = []; + + let secondLevelFunctions: Array<{ + node: TSESTree.FunctionLike; + parent: TSESTree.Node | undefined; + complexityIfThisSecondaryIsTopLevel: ComplexityPoint[]; + complexityIfNested: ComplexityPoint[]; + loc: OptionalLocation; + }> = []; return { ':function': (node: estree.Node) => { @@ -90,37 +135,32 @@ export const rule: Rule.RuleModule = { onLeaveFunction(node as TSESTree.FunctionLike); }, '*'(node: estree.Node) { - if (scopes[scopes.length - 1]?.nestingNodes.has(node as TSESTree.Node)) { - scopes[scopes.length - 1].nestingLevel++; + if (nestingNodes.has(node as TSESTree.Node)) { + nesting++; + if (functionOwnControlFlowNesting.length > 0) { + functionOwnControlFlowNesting[functionOwnControlFlowNesting.length - 1]++; + } } }, '*:exit'(node: estree.Node) { - if (scopes[scopes.length - 1]?.nestingNodes.has(node as TSESTree.Node)) { - scopes[scopes.length - 1].nestingLevel--; - scopes[scopes.length - 1].nestingNodes.delete(node as TSESTree.Node); + if (nestingNodes.has(node as TSESTree.Node)) { + nesting--; + nestingNodes.delete(node as TSESTree.Node); + if (functionOwnControlFlowNesting.length > 0) { + functionOwnControlFlowNesting[functionOwnControlFlowNesting.length - 1]--; + } } }, - Program(node: estree.Program) { - scopes.push({ - node: node as TSESTree.Program, - nestingLevel: 0, - nestingNodes: new Set(), - complexityPoints: [], - }); + Program() { + fileComplexity = 0; }, 'Program:exit'(node: estree.Node) { - const programComplexity = scopes.pop()!; if (isFileComplexity) { // value from the message will be saved in SonarQube as file complexity metric context.report({ node, messageId: 'fileComplexity', - data: { - complexityAmount: programComplexity.complexityPoints.reduce( - (acc, cur) => acc + cur.complexity, - 0, - ) as any, - }, + data: { complexityAmount: fileComplexity as any }, }); } }, @@ -160,16 +200,73 @@ export const rule: Rule.RuleModule = { ConditionalExpression(node: estree.Node) { visitConditionalExpression(node as TSESTree.ConditionalExpression); }, + ReturnStatement(node: estree.Node) { + visitReturnStatement(node as TSESTree.ReturnStatement); + }, }; function onEnterFunction(node: TSESTree.FunctionLike) { - scopes.push({ node, nestingLevel: 0, nestingNodes: new Set(), complexityPoints: [] }); + if (enclosingFunctions.length === 0) { + // top level function + topLevelHasStructuralComplexity = false; + reactFunctionalComponent.init(node); + topLevelOwnComplexity = []; + secondLevelFunctions = []; + } else if (enclosingFunctions.length === 1) { + // second level function + complexityIfNotNested = []; + complexityIfNested = []; + } else { + nesting++; + nestingNodes.add(node); + } + + enclosingFunctions.push(node); + functionOwnComplexity.push([]); + functionOwnControlFlowNesting.push(0); } function onLeaveFunction(node: TSESTree.FunctionLike) { - const functionComplexity = scopes.pop()!; + const functionComplexity = functionOwnComplexity.pop(); + functionOwnControlFlowNesting.pop(); + + enclosingFunctions.pop(); + if (enclosingFunctions.length === 0) { + // top level function + if (topLevelHasStructuralComplexity && !reactFunctionalComponent.isConfirmed()) { + let totalComplexity = topLevelOwnComplexity; + secondLevelFunctions.forEach(secondLevelFunction => { + totalComplexity = totalComplexity.concat(secondLevelFunction.complexityIfNested); + }); + + fileComplexity += totalComplexity.reduce((acc, cur) => acc + cur.complexity, 0); + } else { + fileComplexity += topLevelOwnComplexity.reduce((acc, cur) => acc + cur.complexity, 0); + + secondLevelFunctions.forEach(secondLevelFunction => { + fileComplexity += secondLevelFunction.complexityIfThisSecondaryIsTopLevel.reduce( + (acc, cur) => acc + cur.complexity, + 0, + ); + }); + } + } else if (enclosingFunctions.length === 1) { + // second level function + secondLevelFunctions.push({ + node, + parent: node.parent, + complexityIfNested, + complexityIfThisSecondaryIsTopLevel: complexityIfNotNested, + loc: getMainFunctionTokenLocation(node, node.parent, context as unknown as RuleContext), + }); + } + + if (isFileComplexity) { + return; + } + checkFunction( - functionComplexity.complexityPoints, + functionComplexity, getMainFunctionTokenLocation(node, node.parent, context as unknown as RuleContext), ); } @@ -185,13 +282,13 @@ export const rule: Rule.RuleModule = { } // always increase nesting level inside `then` statement - scopes[scopes.length - 1].nestingNodes.add(ifStatement.consequent); + nestingNodes.add(ifStatement.consequent); // if `else` branch is not `else if` then // - increase nesting level inside `else` statement // - add +1 complexity if (ifStatement.alternate && !isIfStatement(ifStatement.alternate)) { - scopes[scopes.length - 1].nestingNodes.add(ifStatement.alternate); + nestingNodes.add(ifStatement.alternate); const elseTokenLoc = getFirstTokenAfter( ifStatement.consequent, context as unknown as RuleContext, @@ -202,7 +299,7 @@ export const rule: Rule.RuleModule = { function visitLoop(loop: LoopStatement) { addStructuralComplexity(getFirstToken(loop, context as unknown as RuleContext).loc); - scopes[scopes.length - 1].nestingNodes.add(loop.body); + nestingNodes.add(loop.body); } function visitSwitchStatement(switchStatement: TSESTree.SwitchStatement) { @@ -210,7 +307,7 @@ export const rule: Rule.RuleModule = { getFirstToken(switchStatement, context as unknown as RuleContext).loc, ); for (const switchCase of switchStatement.cases) { - scopes[scopes.length - 1].nestingNodes.add(switchCase); + nestingNodes.add(switchCase); } } @@ -224,7 +321,7 @@ export const rule: Rule.RuleModule = { function visitCatchClause(catchClause: TSESTree.CatchClause) { addStructuralComplexity(getFirstToken(catchClause, context as unknown as RuleContext).loc); - scopes[scopes.length - 1].nestingNodes.add(catchClause.body); + nestingNodes.add(catchClause.body); } function visitConditionalExpression(conditionalExpression: TSESTree.ConditionalExpression) { @@ -233,8 +330,37 @@ export const rule: Rule.RuleModule = { context as unknown as RuleContext, )!.loc; addStructuralComplexity(questionTokenLoc); - scopes[scopes.length - 1].nestingNodes.add(conditionalExpression.consequent); - scopes[scopes.length - 1].nestingNodes.add(conditionalExpression.alternate); + nestingNodes.add(conditionalExpression.consequent); + nestingNodes.add(conditionalExpression.alternate); + } + + function visitReturnStatement({ argument }: TSESTree.ReturnStatement) { + // top level function + if ( + enclosingFunctions.length === 1 && + argument && + ['JSXElement', 'JSXFragment'].includes(argument.type as any) + ) { + reactFunctionalComponent.returnsJsx = true; + } + } + + function nameStartsWithCapital(node: TSESTree.FunctionLike) { + const checkFirstLetter = (name: string) => { + const firstLetter = name[0]; + return firstLetter === firstLetter.toUpperCase(); + }; + + if (!isArrowFunctionExpression(node) && node.id) { + return checkFirstLetter(node.id.name); + } + + const { parent } = node; + if (parent && parent.type === 'VariableDeclarator' && parent.id.type === 'Identifier') { + return checkFirstLetter(parent.id.name); + } + + return false; } function visitLogicalExpression(logicalExpression: TSESTree.LogicalExpression) { @@ -301,20 +427,51 @@ export const rule: Rule.RuleModule = { } function addStructuralComplexity(location: TSESTree.SourceLocation) { - const added = scopes[scopes.length - 1].nestingLevel + 1; + const added = nesting + 1; const complexityPoint = { complexity: added, location }; - scopes[scopes.length - 1].complexityPoints.push(complexityPoint); + if (enclosingFunctions.length === 0) { + // top level scope + fileComplexity += added; + } else if (enclosingFunctions.length === 1) { + // top level function + topLevelHasStructuralComplexity = true; + topLevelOwnComplexity.push(complexityPoint); + } else { + // second+ level function + complexityIfNested.push({ complexity: added + 1, location }); + complexityIfNotNested.push(complexityPoint); + } + + if (functionOwnComplexity.length > 0) { + const addedWithoutFunctionNesting = + functionOwnControlFlowNesting[functionOwnControlFlowNesting.length - 1] + 1; + functionOwnComplexity[functionOwnComplexity.length - 1].push({ + complexity: addedWithoutFunctionNesting, + location, + }); + } } function addComplexity(location: TSESTree.SourceLocation) { const complexityPoint = { complexity: 1, location }; - scopes[scopes.length - 1].complexityPoints.push(complexityPoint); + if (functionOwnComplexity.length > 0) { + functionOwnComplexity[functionOwnComplexity.length - 1].push(complexityPoint); + } + + if (enclosingFunctions.length === 0) { + // top level scope + fileComplexity += 1; + } else if (enclosingFunctions.length === 1) { + // top level function + topLevelOwnComplexity.push(complexityPoint); + } else { + // second+ level function + complexityIfNested.push(complexityPoint); + complexityIfNotNested.push(complexityPoint); + } } function checkFunction(complexity: ComplexityPoint[] = [], loc: TSESTree.SourceLocation) { - if (isFileComplexity) { - return; - } const complexityAmount = complexity.reduce((acc, cur) => acc + cur.complexity, 0); if (complexityAmount > threshold) { const secondaryLocations: IssueLocation[] = complexity.map(complexityPoint => { diff --git a/packages/jsts/src/rules/S3776/unit.test.ts b/packages/jsts/src/rules/S3776/unit.test.ts index ff72f7ee604..78b52ea28bf 100644 --- a/packages/jsts/src/rules/S3776/unit.test.ts +++ b/packages/jsts/src/rules/S3776/unit.test.ts @@ -606,7 +606,7 @@ class TopLevel { } `, options: [0, 'metric'], - errors: [{ messageId: 'fileComplexity', data: { complexityAmount: 5 } }], + errors: [{ messageId: 'fileComplexity', data: { complexityAmount: 25 } }], }, ], }); diff --git a/sonar-plugin/javascript-checks/src/main/resources/org/sonar/l10n/javascript/rules/javascript/S3776.html b/sonar-plugin/javascript-checks/src/main/resources/org/sonar/l10n/javascript/rules/javascript/S3776.html index c908eccb5d0..08fff87bf89 100644 --- a/sonar-plugin/javascript-checks/src/main/resources/org/sonar/l10n/javascript/rules/javascript/S3776.html +++ b/sonar-plugin/javascript-checks/src/main/resources/org/sonar/l10n/javascript/rules/javascript/S3776.html @@ -15,6 +15,10 @@

Which syntax in code does impact cognitive complexity score?

apply to recursive calls, those will increment cognitive score.

The method of computation is fully detailed in the pdf linked in the resources.

+

Note that the calculation of cognitive complexity at function level deviates from the documented process. Given the functional nature of +JavaScript, nesting functions is a prevalent practice, especially within frameworks like React.js. Consequently, the cognitive complexity of functions +remains independent from one another. This means that the complexity of a nesting function does not increase with the complexity of nested +functions.

What is the potential impact?

Developers spend more time reading and understanding code than writing it. High cognitive complexity slows down changes and increases the cost of maintenance.

@@ -39,9 +43,6 @@

How to fix it

  • Use null-safe operations (if available in the language).
    When available the .? or ?? operator replaces multiple tests and simplifies the flow.
    • -

    Note that the calculation of cognitive complexity deviates from the documented process when functions are nested. Given the functional nature of -JavaScript, nesting functions is a prevalent practice, especially within frameworks like React.js. Consequently, the cognitive complexity of functions -remains independent of each other.

    Code examples

    Extraction of a complex condition in a new function.

    Noncompliant code example