Create rule S6959: "Array.reduce()" calls should include an initial value #4626

yassin-kammoun-sonarsource · 2024-03-26T10:54:00Z

zglicz

Please add an invalid test case before merging

zglicz · 2024-03-26T15:54:33Z

packages/jsts/src/rules/S6959/unit.test.ts

+      code: 'xs.reduce((acc, x) => acc + x);',
+    },
+  ],
+  invalid: [],


could you add an invalid case?

If you come back to the rule implementation, you might have noticed this weird check:

SonarJS/packages/jsts/src/rules/S6959/rule.ts

Lines 32 to 35 in 5f2dbfe

const services = context.parserServices;

if (!isRequiredParserServices(services)) {

return {};

}

While the function isRequiredParserServices isn't well-named, it actually tests that type-checking is available. If not, the rule turns into a noop. In order to test this behavior, this file uses ESLint rule tester for code coverage purposes. We currently don't have any other way to test this edge case (or at least, we haven't invested time on a better solution so far). That's why there is intentionally no invalid test case here.

I'm reading and re-reading and I don't understand.
In this test, is there type-checking available?
I assume no, and thanks to that this rule becomes a no-op, as the code sample that is provided is missing the initial value.

Why isn't there typechecking in this test then? What should be the parserOptions to include type-checking? Or am I completely off-base here?

When we parse JS/TS code, we rely on TypeScript ESLint parser which is able to parse both syntaxes. In addition, TypeScript ESLint wraps some utilities from TypeScript compiler, including type-checking. However, to benefit from type-checking, you need to configure the parsing options with a TSConfig somewhere.

By default, ESLint's rule tester uses Esprima (I think) to parse the code being tested. If you want to have type-checking available while using this rule tester, you first need to manually configure the parser to use as follows:

SonarJS/packages/jsts/src/rules/S117/unit.test.ts

Lines 23 to 26 in 911655e

const ruleTester = new RuleTester({

parser: require.resolve('@typescript-eslint/parser'),

parserOptions: { ecmaVersion: 2018 },

});

However, this is not enough as it only allows us to parse TypeScript syntax as well. To enable type-checking, you need to tweak ESLint's rule tester with a TSConfig. I am not going to show how because we want to avoid that and prefer using the comment-based testing framework which always makes type-checking available.

I'm reading and re-reading and I don't understand. In this test, is there type-checking available?

No, type-checking is not available because we use ESLint's rule tester without customization.

I assume no, and thanks to that this rule becomes a no-op, as the code sample that is provided is missing the initial value.

Exactly!

Why isn't there typechecking in this test then? What should be the parserOptions to include type-checking? Or am I completely off-base here?

Configure the parser options to use TypeScript ESLint parser

Configure ESLint's rule tester with a TSConfig

zglicz · 2024-03-27T08:00:19Z

packages/jsts/src/rules/S6959/cb.fixture.ts

+    return xs.reduce((acc, x) => acc + x, 0); // Compliant
+}
+
+function coverage(x: any) {


what is this for?

This file as a whole is not executed at all but rather denotes a test file with actual unit tests. The file uses a homemade testing framework that we call comment-based testing framework. A test file contains special comments indicating that a rule should raise an issue at specific locations. For instance, this is what is meant here:

SonarJS/packages/jsts/src/rules/S6959/cb.fixture.ts

Lines 1 to 3 in 5f2dbfe

function noncompliant(xs: number[]) {

return xs.reduce((acc, x) => acc + x); // Noncompliant {{Add an initial value to this "reduce()" call.}}

}

The comment // Noncompliant indicates that an issue should be raised at line 2 and whatever is inside {{...}} is the expected message of the reported issue. We could assert even more things like code fixes, starting and ending columns, and secondary locations.

Coming back to your question, I guess you should understand by now what's the purpose of the function coverage. No issue will be raised here, the rule will visit those method invocations for the sake of increasing the code coverage of the rule implementation, this line in other words:

SonarJS/packages/jsts/src/rules/S6959/rule.ts

Line 39 in 5f2dbfe

if (isCallingMethod(node, 1, 'reduce') && isArray(node.callee.object, services)) {

So this is just to increase code-coverage? That seems counter-productive and actually adds confusion where it shouldn't.

I don't necessarily agree that's confusing since the test file explicitly separates what's noncompliant from what's compliant and what's for coverage. Maybe I am missing something to make it less confusion.

Feel free to suggest anything that comes to your mind :)

zglicz · 2024-03-27T08:00:54Z

...n/javascript-checks/src/main/resources/org/sonar/l10n/javascript/rules/javascript/S6959.json

+  "status": "ready",
+  "remediation": {
+    "func": "Constant\/Issue",
+    "constantCost": "5min"


heh, that's generous if not intimidating

Good point! Could you actually comment on this PR?

Each rule has a description, and a rule description is stored in the RSPEC GitHub repository, a company-wide database for all Sonar rule descriptions. New descriptions or description updates are first introduced in this repository through pull requests and eventually merged. To bring back those changes in a given analyzer, we then use an internal tool called rule-api that fetches any changes for any rule of the analyzer or changes for a specific rule.

You will get familiar with this tool tomorrow when starting the release process of the analyzer.

zglicz

Thank you for the explanations. It's now making a bit more sense, but still I left a few questions.

zglicz · 2024-03-27T09:34:44Z

packages/jsts/src/rules/S6959/cb.fixture.ts

+    return xs.reduce((acc, x) => acc + x, 0); // Compliant
+}
+
+function coverage(x: any) {


So this is just to increase code-coverage? That seems counter-productive and actually adds confusion where it shouldn't.

zglicz · 2024-03-27T09:41:49Z

packages/jsts/src/rules/S6959/unit.test.ts

+      code: 'xs.reduce((acc, x) => acc + x);',
+    },
+  ],
+  invalid: [],


I'm reading and re-reading and I don't understand.
In this test, is there type-checking available?
I assume no, and thanks to that this rule becomes a no-op, as the code sample that is provided is missing the initial value.

Why isn't there typechecking in this test then? What should be the parserOptions to include type-checking? Or am I completely off-base here?

yassin-kammoun-sonarsource · 2024-03-27T10:49:32Z

packages/jsts/src/rules/S6959/rule.ts

+    function isArray(node: estree.Node) {
+      if (isRequiredParserServices(services)) {
+        return isArrayType(node, services);
+      } else {
+        return isArrayExpression(getUniqueWriteUsageOrNode(context, node));
+      }
+    }


@zglicz I updated the array detection so that it tries to determine syntactically if a node denotes an array in case type-checking is missing for whatever reason.

…alue

sonarqube-next · 2024-03-27T16:59:54Z

Quality Gate passed

Issues
0 New issues
0 Fixed issues
0 Accepted issues

Measures
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube

yassin-kammoun-sonarsource force-pushed the i4591 branch from 70a144f to ecefde7 Compare March 26, 2024 11:00

yassin-kammoun-sonarsource requested a review from zglicz March 26, 2024 11:29

yassin-kammoun-sonarsource force-pushed the i4591 branch from ecefde7 to 5f2dbfe Compare March 26, 2024 12:55

zglicz approved these changes Mar 27, 2024

View reviewed changes

zglicz reviewed Mar 27, 2024

View reviewed changes

yassin-kammoun-sonarsource commented Mar 27, 2024

View reviewed changes

yassin-kammoun-sonarsource requested a review from zglicz March 27, 2024 10:49

yassin-kammoun-sonarsource force-pushed the i4591 branch from 56e5486 to 7e77e0d Compare March 27, 2024 10:56

yassin-kammoun-sonarsource added 5 commits March 27, 2024 17:39

Create rule S6959: "Array.reduce()" calls should include an initial v…

91bd0d5

…alue

Assert exact issue location

631ca84

Detect array callees without type-checking

5dacd6f

Update ruling expectations

67e5ca4

Update rule metadata

c6a23d3

yassin-kammoun-sonarsource force-pushed the i4591 branch from 7e77e0d to c6a23d3 Compare March 27, 2024 16:40

yassin-kammoun-sonarsource merged commit 06615d0 into master Mar 27, 2024
14 checks passed

yassin-kammoun-sonarsource deleted the i4591 branch March 27, 2024 17:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create rule S6959: "Array.reduce()" calls should include an initial value #4626

Create rule S6959: "Array.reduce()" calls should include an initial value #4626

yassin-kammoun-sonarsource commented Mar 26, 2024

zglicz left a comment

zglicz Mar 26, 2024

yassin-kammoun-sonarsource Mar 27, 2024

zglicz Mar 27, 2024

yassin-kammoun-sonarsource Mar 27, 2024

zglicz Mar 27, 2024

yassin-kammoun-sonarsource Mar 27, 2024

zglicz Mar 27, 2024

yassin-kammoun-sonarsource Mar 27, 2024

zglicz Mar 27, 2024

yassin-kammoun-sonarsource Mar 27, 2024

zglicz left a comment

zglicz Mar 27, 2024

zglicz Mar 27, 2024

yassin-kammoun-sonarsource Mar 27, 2024

sonarqube-next bot commented Mar 27, 2024

	const services = context.parserServices;
	if (!isRequiredParserServices(services)) {
	return {};
	}

	const ruleTester = new RuleTester({
	parser: require.resolve('@typescript-eslint/parser'),
	parserOptions: { ecmaVersion: 2018 },
	});

	function noncompliant(xs: number[]) {
	return xs.reduce((acc, x) => acc + x); // Noncompliant {{Add an initial value to this "reduce()" call.}}
	}

Create rule S6959: "Array.reduce()" calls should include an initial value #4626

Create rule S6959: "Array.reduce()" calls should include an initial value #4626

Conversation

yassin-kammoun-sonarsource commented Mar 26, 2024

zglicz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zglicz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarqube-next bot commented Mar 27, 2024

Quality Gate passed