Skip to content
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

Improve S3776: Exclude complexity of JSX short-circuits #377

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
9 changes: 0 additions & 9 deletions ruling/snapshots/cognitive-complexity
Expand Up @@ -472,7 +472,6 @@ src/reveal.js/plugin/zoom-js/zoom.js: 34
src/socket.io/lib/index.js: 69,143
src/spectrum/admin/src/helpers/utils.js: 52
src/spectrum/admin/src/views/communities/components/search/index.js: 86
src/spectrum/admin/src/views/dashboard/components/coreMetrics.js: 114
src/spectrum/admin/src/views/users/components/search/index.js: 92
src/spectrum/api/authentication.js: 274
src/spectrum/api/models/community.js: 168,330
Expand Down Expand Up @@ -503,16 +502,13 @@ src/spectrum/shared/graphql/queries/thread/getThreadMessageConnection.js: 78
src/spectrum/src/components/avatar/hoverProfile.js: 44
src/spectrum/src/components/chatInput/index.js: 152
src/spectrum/src/components/composer/index.js: 131
src/spectrum/src/components/draftjs-editor/index.js: 177
src/spectrum/src/components/globals/index.js: 334
src/spectrum/src/components/listItems/index.js: 129
src/spectrum/src/components/modals/ChangeChannelModal/channelSelector.js: 22
src/spectrum/src/components/modals/DeleteDoubleCheckModal/index.js: 74
src/spectrum/src/components/profile/channel.js: 95
src/spectrum/src/components/profile/community.js: 60
src/spectrum/src/components/profile/user.js: 68
src/spectrum/src/components/threadComposer/components/composer.js: 120,215,346
src/spectrum/src/components/threadFeed/index.js: 195
src/spectrum/src/helpers/utils.js: 96
src/spectrum/src/registerServiceWorker.js: 24
src/spectrum/src/views/channel/index.js: 159
Expand All @@ -524,20 +520,15 @@ src/spectrum/src/views/communityMembers/components/communityMembers.js: 141
src/spectrum/src/views/communityMembers/components/importSlack.js: 117
src/spectrum/src/views/dashboard/components/sidebarChannels.js: 50
src/spectrum/src/views/dashboard/components/threadFeed.js: 93,213
src/spectrum/src/views/dashboard/index.js: 98
src/spectrum/src/views/directMessages/containers/newThread.js: 201
src/spectrum/src/views/directMessages/index.js: 83
src/spectrum/src/views/explore/components/search.js: 115
src/spectrum/src/views/explore/view.js: 109
src/spectrum/src/views/navbar/components/notificationsTab.js: 51,234
src/spectrum/src/views/navbar/index.js: 75
src/spectrum/src/views/newCommunity/index.js: 173
src/spectrum/src/views/notifications/components/sortAndGroupNotificationMessages.js: 3
src/spectrum/src/views/thread/components/actionBar.js: 209
src/spectrum/src/views/thread/components/messages.js: 142
src/spectrum/src/views/thread/index.js: 223,286
src/spectrum/src/views/user/components/communityList.js: 23
src/spectrum/src/views/user/index.js: 97
src/three.js/editor/js/Loader.js: 12,520
src/three.js/editor/js/Menubar.Edit.js: 125
src/three.js/editor/js/Script.js: 72,136
Expand Down
15 changes: 11 additions & 4 deletions src/rules/cognitive-complexity.ts
Expand Up @@ -19,17 +19,18 @@
*/
// https://sonarsource.github.io/rspec/#/rspec/S3776

import type { TSESTree, TSESLint } from '@typescript-eslint/experimental-utils';
import type { TSESLint, TSESTree } from '@typescript-eslint/experimental-utils';
import { isArrowFunctionExpression, isIfStatement, isLogicalExpression } from '../utils/nodes';
import {
getMainFunctionTokenLocation,
getFirstToken,
getFirstTokenAfter,
report,
getMainFunctionTokenLocation,
IssueLocation,
issueLocation,
report,
} from '../utils/locations';
import docsUrl from '../utils/docs-url';
import { getJsxShortCircuitNodes } from '../utils/jsx';

const DEFAULT_THRESHOLD = 15;

Expand Down Expand Up @@ -339,13 +340,19 @@ const rule: TSESLint.RuleModule<string, (number | 'metric' | 'sonar-runtime')[]>
}

function visitLogicalExpression(logicalExpression: TSESTree.LogicalExpression) {
const jsxShortCircuitNodes = getJsxShortCircuitNodes(logicalExpression);
if (jsxShortCircuitNodes != null) {
jsxShortCircuitNodes.forEach(node => consideredLogicalExpressions.add(node));
return;
}

if (!consideredLogicalExpressions.has(logicalExpression)) {
const flattenedLogicalExpressions = flattenLogicalExpression(logicalExpression);

let previous: TSESTree.LogicalExpression | undefined;
for (const current of flattenedLogicalExpressions) {
if (!previous || previous.operator !== current.operator) {
const operatorTokenLoc = getFirstTokenAfter(logicalExpression.left, context)!.loc;
ilia-kebets-sonarsource marked this conversation as resolved.
Show resolved Hide resolved
const operatorTokenLoc = getFirstTokenAfter(current.left, context)!.loc;
addComplexity(operatorTokenLoc);
}
previous = current;
Expand Down
50 changes: 50 additions & 0 deletions src/utils/jsx.ts
@@ -0,0 +1,50 @@
/*
* eslint-plugin-sonarjs
* Copyright (C) 2018-2021 SonarSource SA
* mailto:info AT sonarsource DOT com
*
* This program is free software; you can redistribute it and/or
* modify it under the terms of the GNU Lesser General Public
* License as published by the Free Software Foundation; either
* version 3 of the License, or (at your option) any later version.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
* Lesser General Public License for more details.
*
* You should have received a copy of the GNU Lesser General Public License
* along with this program; if not, write to the Free Software Foundation,
* Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
*/

import type { TSESTree } from '@typescript-eslint/experimental-utils';

export function getJsxShortCircuitNodes(logicalExpression: TSESTree.LogicalExpression) {
if (logicalExpression.parent?.type !== 'JSXExpressionContainer') {
return null;
} else {
return flattenJsxShortCircuitNodes(logicalExpression, logicalExpression);
}
}

function flattenJsxShortCircuitNodes(
root: TSESTree.LogicalExpression,
node: TSESTree.Node,
): TSESTree.LogicalExpression[] | null {
if (
node.type === 'ConditionalExpression' ||
(node.type === 'LogicalExpression' && node.operator !== root.operator)
) {
return null;
} else if (node.type !== 'LogicalExpression') {
return [];
} else {
const leftNodes = flattenJsxShortCircuitNodes(root, node.left);
const rightNodes = flattenJsxShortCircuitNodes(root, node.right);
if (leftNodes == null || rightNodes == null) {
return null;
}
return [...leftNodes, node, ...rightNodes];
}
}
218 changes: 174 additions & 44 deletions tests/rules/cognitive-complexity.test.ts
Expand Up @@ -19,10 +19,79 @@
*/
import { TSESLint } from '@typescript-eslint/experimental-utils';
import { ruleTester } from '../rule-tester';
import { IssueLocation } from '../../src/utils/locations';
import rule = require('../../src/rules/cognitive-complexity');

ruleTester.run('cognitive-complexity', rule, {
valid: [{ code: `function zero_complexity() {}`, options: [0] }],
valid: [
{ code: `function zero_complexity() {}`, options: [0] },
{
code: `
function Component(obj) {
return (
<span>{ obj.title?.text }</span>
);
}`,
parserOptions: { ecmaFeatures: { jsx: true } },
options: [0],
},
{
code: `
function Component(obj) {
return (
<>
{ obj.isFriendly && <strong>Welcome</strong> }
</>
);
}`,
parserOptions: { ecmaFeatures: { jsx: true } },
options: [0],
},
{
code: `
function Component(obj) {
return (
<>
{ obj.isFriendly && obj.isLoggedIn && <strong>Welcome</strong> }
</>
);
}`,
parserOptions: { ecmaFeatures: { jsx: true } },
options: [0],
},
{
code: `
function Component(obj) {
return (
<>
{ obj.x && obj.y && obj.z && <strong>Welcome</strong> }
</>
);
}`,
parserOptions: { ecmaFeatures: { jsx: true } },
options: [0],
},
{
code: `
function Component(obj) {
return (
<span title={ obj.title || obj.disclaimer }>Text</span>
);
}`,
parserOptions: { ecmaFeatures: { jsx: true } },
options: [0],
},
{
code: `
function Component(obj) {
return (
<button type="button" disabled={ obj.user?.isBot ?? obj.isDemo }>Logout</button>
);
}`,
parserOptions: { ecmaFeatures: { jsx: true } },
options: [0],
},
],
invalid: [
// if
{
Expand Down Expand Up @@ -196,8 +265,8 @@ ruleTester.run('cognitive-complexity', rule, {
options: [0],
errors: [message(2)],
},
{
code: `
testCaseWithSonarRuntime(
`
function check_secondaries() {
if (condition) { // +1 "if"
if (condition) {} else {} // +2 "if", +1 "else"
Expand All @@ -217,51 +286,46 @@ ruleTester.run('cognitive-complexity', rule, {

return foo(a && b) && c; // +1 "&&", +1 "&&"
}`,
options: [0, 'sonar-runtime'],
errors: [
[
{ line: 3, column: 8, endLine: 3, endColumn: 10, message: '+1' }, // if
{ line: 7, column: 10, endLine: 7, endColumn: 14, message: '+1' }, // else
{
messageId: 'sonarRuntime',
data: {
complexityAmount: 13,
threshold: 0,
sonarRuntimeData: JSON.stringify({
secondaryLocations: [
{ line: 3, column: 8, endLine: 3, endColumn: 10, message: '+1' }, // if
{ line: 7, column: 10, endLine: 7, endColumn: 14, message: '+1' }, // else
{
line: 4,
column: 10,
endLine: 4,
endColumn: 12,
message: '+2 (incl. 1 for nesting)',
}, // if
{ line: 4, column: 28, endLine: 4, endColumn: 32, message: '+1' }, // else
{
line: 6,
column: 10,
endLine: 6,
endColumn: 15,
message: '+2 (incl. 1 for nesting)',
}, // catch
{ line: 11, column: 8, endLine: 11, endColumn: 13, message: '+1' }, // while
{ line: 12, column: 10, endLine: 12, endColumn: 15, message: '+1' }, // break
{ line: 15, column: 10, endLine: 15, endColumn: 11, message: '+1' }, // ?
{ line: 17, column: 8, endLine: 17, endColumn: 14, message: '+1' }, // switch
{ line: 19, column: 27, endLine: 19, endColumn: 29, message: '+1' }, // &&
{ line: 19, column: 21, endLine: 19, endColumn: 23, message: '+1' }, // &&
],
message:
'Refactor this function to reduce its Cognitive Complexity from 13 to the 0 allowed.',
cost: 13,
}),
...message(13),
cost: 13,
},
},
line: 4,
column: 10,
endLine: 4,
endColumn: 12,
message: '+2 (incl. 1 for nesting)',
}, // if
{ line: 4, column: 28, endLine: 4, endColumn: 32, message: '+1' }, // else
{
line: 6,
column: 10,
endLine: 6,
endColumn: 15,
message: '+2 (incl. 1 for nesting)',
}, // catch
{ line: 11, column: 8, endLine: 11, endColumn: 13, message: '+1' }, // while
{ line: 12, column: 10, endLine: 12, endColumn: 15, message: '+1' }, // break
{ line: 15, column: 10, endLine: 15, endColumn: 11, message: '+1' }, // ?
{ line: 17, column: 8, endLine: 17, endColumn: 14, message: '+1' }, // switch
{ line: 19, column: 27, endLine: 19, endColumn: 29, message: '+1' }, // &&
{ line: 19, column: 21, endLine: 19, endColumn: 23, message: '+1' }, // &&
],
},
13,
),

// expressions
testCaseWithSonarRuntime(
`
function and_or_locations() {
foo(1 && 2 || 3 && 4);
}`,
[
{ line: 3, column: 14, endLine: 3, endColumn: 16, message: '+1' }, // &&
{ line: 3, column: 19, endLine: 3, endColumn: 21, message: '+1' }, // ||
{ line: 3, column: 24, endLine: 3, endColumn: 26, message: '+1' }, // &&
],
),
{
code: `
function and_or() {
Expand Down Expand Up @@ -516,6 +580,48 @@ ruleTester.run('cognitive-complexity', rule, {
options: [0],
errors: [message(1, { line: 2 }), message(1, { line: 3 })],
},
testCaseWithSonarRuntime(
`
function Component(obj) {
return (
<>
<span title={ obj.user?.name ?? (obj.isDemo ? 'demo' : 'none') }>Text</span>
</>
);
}`,
[
{ line: 5, column: 41, endLine: 5, endColumn: 43, message: '+1' }, // ??
{ line: 5, column: 56, endLine: 5, endColumn: 57, message: '+1' }, // ?:
],
),
testCaseWithSonarRuntime(
`
function Component(obj) {
return (
<>
{ obj.isUser && (obj.name || obj.surname) }
</>
);
}`,
[
{ line: 5, column: 25, endLine: 5, endColumn: 27, message: '+1' }, // &&
{ line: 5, column: 38, endLine: 5, endColumn: 40, message: '+1' }, // ||
],
),
testCaseWithSonarRuntime(
`
function Component(obj) {
return (
<>
{ obj.isUser && (obj.isDemo ? <strong>Demo</strong> : <em>None</em>) }
</>
);
}`,
[
{ line: 5, column: 25, endLine: 5, endColumn: 27, message: '+1' }, // &&
{ line: 5, column: 40, endLine: 5, endColumn: 41, message: '+1' }, // ||
],
),
],
});

Expand Down Expand Up @@ -655,6 +761,30 @@ class TopLevel {
],
});

function testCaseWithSonarRuntime(
code: string,
secondaryLocations: IssueLocation[],
complexity?: number,
): TSESLint.InvalidTestCase<string, (number | 'sonar-runtime')[]> {
const cost = complexity ?? secondaryLocations.length;
const message = `Refactor this function to reduce its Cognitive Complexity from ${cost} to the 0 allowed.`;
const sonarRuntimeData = JSON.stringify({ secondaryLocations, message, cost });
return {
code,
parserOptions: { ecmaFeatures: { jsx: true } },
options: [0, 'sonar-runtime'],
errors: [
{
messageId: 'sonarRuntime',
data: {
threshold: 0,
sonarRuntimeData,
},
},
],
};
}

function message(complexityAmount: number, other: Partial<TSESLint.TestCaseError<string>> = {}) {
return {
messageId: 'refactorFunction',
Expand Down