From 4bac9a9710b534afa694f9a9258b53f22fdf803b Mon Sep 17 00:00:00 2001 From: quentin-jaquier-sonarsource <43733433+quentin-jaquier-sonarsource@users.noreply.github.com> Date: Sat, 26 Oct 2019 15:31:09 +0200 Subject: [PATCH] Add rule S4158 ('no-empty-collection') for TS/JS (new rule for JS) (#1697) --- eslint-bridge/src/rules/main.ts | 2 + .../src/rules/no-empty-collection.ts | 201 +++++++++++++ .../src/rules/no-unused-collection.ts | 19 +- eslint-bridge/src/rules/utils.ts | 28 +- eslint-bridge/src/utils/collections.ts | 39 +++ .../tests/rules/no-empty-collection.test.ts | 276 ++++++++++++++++++ .../tests/rules/no-unused-collection.test.ts | 7 + its/plugin/projects/ts-rule-project/S4158.ts | 185 ++++++++++++ .../resources/ts-rules-project-profile.xml | 5 + .../sonar/javascript/checks/CheckList.java | 1 + .../checks/NoEmptyCollectionCheck.java | 36 +++ .../javascript/rules/javascript/S4158.html | 14 + .../javascript/rules/javascript/S4158.json | 20 ++ .../rules/javascript/Sonar_way_profile.json | 3 +- .../Sonar_way_recommended_profile.json | 1 + 15 files changed, 804 insertions(+), 33 deletions(-) create mode 100644 eslint-bridge/src/rules/no-empty-collection.ts create mode 100644 eslint-bridge/src/utils/collections.ts create mode 100644 eslint-bridge/tests/rules/no-empty-collection.test.ts create mode 100644 its/plugin/projects/ts-rule-project/S4158.ts create mode 100644 javascript-checks/src/main/java/org/sonar/javascript/checks/NoEmptyCollectionCheck.java create mode 100644 javascript-checks/src/main/resources/org/sonar/l10n/javascript/rules/javascript/S4158.html create mode 100644 javascript-checks/src/main/resources/org/sonar/l10n/javascript/rules/javascript/S4158.json diff --git a/eslint-bridge/src/rules/main.ts b/eslint-bridge/src/rules/main.ts index e5468ffc9ad..676b07a83c1 100644 --- a/eslint-bridge/src/rules/main.ts +++ b/eslint-bridge/src/rules/main.ts @@ -40,6 +40,7 @@ import { rule as noRedundantJump } from "./no-redundant-jump"; import { rule as cyclomaticComplexity } from "./cyclomatic-complexity"; import { rule as noAlphabeticalSort } from "./no-alphabetical-sort"; import { rule as noTab } from "./no-tab"; +import { rule as noEmptyCollection } from "./no-empty-collection"; import { rule as nonExistentOperator } from "./non-existent-operator"; import { rule as noInMisuse } from "./no-in-misuse"; import { rule as noInvariantReturns } from "./no-invariant-returns"; @@ -93,6 +94,7 @@ ruleModules["no-redundant-jump"] = noRedundantJump; ruleModules["cyclomatic-complexity"] = cyclomaticComplexity; ruleModules["no-alphabetical-sort"] = noAlphabeticalSort; ruleModules["no-tab"] = noTab; +ruleModules["no-empty-collection"] = noEmptyCollection; ruleModules["non-existent-operator"] = nonExistentOperator; ruleModules["no-in-misuse"] = noInMisuse; ruleModules["no-invariant-returns"] = noInvariantReturns; diff --git a/eslint-bridge/src/rules/no-empty-collection.ts b/eslint-bridge/src/rules/no-empty-collection.ts new file mode 100644 index 00000000000..66559c6690d --- /dev/null +++ b/eslint-bridge/src/rules/no-empty-collection.ts @@ -0,0 +1,201 @@ +/* + * SonarQube JavaScript Plugin + * Copyright (C) 2011-2019 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. + */ +// https://jira.sonarsource.com/browse/RSPEC-4158 + +import { Rule, Scope } from "eslint"; +import { isIdentifier, findFirstMatchingAncestor, isReferenceTo, ancestorsChain } from "./utils"; +import { TSESTree } from "@typescript-eslint/experimental-utils"; +import * as estree from "estree"; +import { collectionConstructor } from "../utils/collections"; + +// Methods that mutate the collection but can't add elements +const nonAdditiveMutatorMethods = [ + // array methods + "copyWithin", + "pop", + "reverse", + "shift", + "sort", + // map, set methods + "clear", + "delete", +]; +const accessorMethods = [ + // array methods + "concat", + "flat", + "flatMap", + "includes", + "indexOf", + "join", + "lastIndexOf", + "slice", + "toSource", + "toString", + "toLocaleString", + // map, set methods + "get", + "has", +]; +const iterationMethods = [ + "entries", + "every", + "filter", + "find", + "findIndex", + "forEach", + "keys", + "map", + "reduce", + "reduceRight", + "some", + "values", +]; + +const strictlyReadingMethods = new Set([ + ...nonAdditiveMutatorMethods, + ...accessorMethods, + ...iterationMethods, +]); + +export const rule: Rule.RuleModule = { + create(context: Rule.RuleContext) { + return { + "Program:exit": function() { + reportEmptyCollectionsUsage(context.getScope(), context); + }, + }; + }, +}; + +export function reportEmptyCollectionsUsage(scope: Scope.Scope, context: Rule.RuleContext) { + if (scope.type !== "global") { + scope.variables.forEach(v => { + reportEmptyCollectionUsage(v, context); + }); + } + + scope.childScopes.forEach(childScope => { + reportEmptyCollectionsUsage(childScope, context); + }); +} + +function reportEmptyCollectionUsage(variable: Scope.Variable, context: Rule.RuleContext) { + if (variable.references.length <= 1) { + return; + } + + const usedReferences = []; + let isEmptyCollection = false; + + for (const ref of variable.references) { + if (ref.isWriteOnly()) { + isEmptyCollection = isReferenceAssigningEmptyCollection(ref); + } else { + if (isReadCollectionPattern(ref)) { + if (isEmptyCollection) { + usedReferences.push(ref); + } + } else { + // One references is a write + return; + } + } + } + + usedReferences.forEach(ref => { + context.report({ + message: `Review this usage of "${ref.identifier.name}" as it can only be empty here.`, + node: ref.identifier, + }); + }); +} + +function isReferenceAssigningEmptyCollection(ref: Scope.Reference) { + const declOrExprStmt = findFirstMatchingAncestor( + ref.identifier as TSESTree.Node, + n => n.type === "VariableDeclarator" || n.type === "ExpressionStatement", + ) as estree.Node; + if (declOrExprStmt) { + if (declOrExprStmt.type === "VariableDeclarator" && declOrExprStmt.init) { + return isEmptyCollectionType(declOrExprStmt.init); + } + + if (declOrExprStmt.type === "ExpressionStatement") { + const expression = declOrExprStmt.expression; + return ( + expression.type === "AssignmentExpression" && + isReferenceTo(ref, expression.left) && + isEmptyCollectionType(expression.right) + ); + } + } + return false; +} + +function isEmptyCollectionType(node: estree.Node) { + if (node && node.type === "ArrayExpression") { + return node.elements.length === 0; + } else if (node && (node.type === "CallExpression" || node.type === "NewExpression")) { + return isIdentifier(node.callee, ...collectionConstructor) && node.arguments.length === 0; + } + return false; +} + +function isReadCollectionPattern(ref: Scope.Reference) { + return isStrictlyReadingMethodCall(ref) || isForIterationPattern(ref) || isElementRead(ref); +} + +function isStrictlyReadingMethodCall(usage: Scope.Reference) { + const parent = (usage.identifier as TSESTree.Node).parent; + if (parent && parent.type === "MemberExpression") { + const memberExpressionParent = parent.parent; + if (memberExpressionParent && memberExpressionParent.type === "CallExpression") { + return isIdentifier(parent.property as estree.Node, ...strictlyReadingMethods); + } + } + return false; +} + +function isForIterationPattern(ref: Scope.Reference) { + const forInOrOfStatement = findFirstMatchingAncestor( + ref.identifier as TSESTree.Node, + n => n.type === "ForOfStatement" || n.type === "ForInStatement", + ) as TSESTree.ForOfStatement | TSESTree.ForInStatement; + + return forInOrOfStatement && forInOrOfStatement.right === ref.identifier; +} + +function isElementRead(ref: Scope.Reference) { + const parent = (ref.identifier as TSESTree.Node).parent; + + return parent && parent.type === "MemberExpression" && parent.computed && !isElementWrite(parent); +} + +function isElementWrite(memberExpression: TSESTree.MemberExpression) { + const ancestors = ancestorsChain(memberExpression, new Set()); + const assignment = ancestors.find( + n => n.type === "AssignmentExpression", + ) as TSESTree.AssignmentExpression; + if (assignment && assignment.operator === "=") { + return [memberExpression, ...ancestors].includes(assignment.left); + } + return false; +} diff --git a/eslint-bridge/src/rules/no-unused-collection.ts b/eslint-bridge/src/rules/no-unused-collection.ts index 890cdd67a6b..61527c377fe 100644 --- a/eslint-bridge/src/rules/no-unused-collection.ts +++ b/eslint-bridge/src/rules/no-unused-collection.ts @@ -23,27 +23,10 @@ import { Rule, Scope } from "eslint"; import * as estree from "estree"; import { TSESTree } from "@typescript-eslint/experimental-utils"; import { findFirstMatchingAncestor, isElementWrite, isIdentifier, isReferenceTo } from "./utils"; +import { collectionConstructor, writingMethods } from "../utils/collections"; const message = "Either use this collection's contents or remove the collection."; -const writingMethods = [ - "copyWithin", - "fill", - "pop", - "push", - "reverse", - "shift", - "sort", - "splice", - "unshift", - "clear", - "delete", - "set", - "add", -]; - -const collectionConstructor = ["Array", "Set", "Map", "WeakSet", "WeakMap"]; - export const rule: Rule.RuleModule = { create(context: Rule.RuleContext) { return { diff --git a/eslint-bridge/src/rules/utils.ts b/eslint-bridge/src/rules/utils.ts index 9b1b576c8cb..f6ba2287a7e 100644 --- a/eslint-bridge/src/rules/utils.ts +++ b/eslint-bridge/src/rules/utils.ts @@ -202,36 +202,36 @@ function toSecondaryLocation( }; } -export function findFirstMatchingAncestor( +export function findFirstMatchingLocalAncestor( node: TSESTree.Node, predicate: (node: TSESTree.Node) => boolean, ) { - return findFirstMatchingAncestorWithBoundaries(node, predicate, new Set()); + return localAncestorsChain(node).find(predicate); } -export function findFirstMatchingLocalAncestor( +export function findFirstMatchingAncestor( node: TSESTree.Node, predicate: (node: TSESTree.Node) => boolean, ) { - return findFirstMatchingAncestorWithBoundaries(node, predicate, functionLike); + return ancestorsChain(node, new Set()).find(predicate); } -export function findFirstMatchingAncestorWithBoundaries( - node: TSESTree.Node, - predicate: (node: TSESTree.Node) => boolean, - boundaryTypes: Set, -) { +export function localAncestorsChain(node: TSESTree.Node) { + return ancestorsChain(node, functionLike); +} + +export function ancestorsChain(node: TSESTree.Node, boundaryTypes: Set) { + const chain: TSESTree.Node[] = []; + let currentNode = node.parent; while (currentNode) { - if (predicate(currentNode)) { - return currentNode; - } + chain.push(currentNode); if (boundaryTypes.has(currentNode.type)) { - return undefined; + break; } currentNode = currentNode.parent; } - return undefined; + return chain; } /** diff --git a/eslint-bridge/src/utils/collections.ts b/eslint-bridge/src/utils/collections.ts new file mode 100644 index 00000000000..0e5c6822851 --- /dev/null +++ b/eslint-bridge/src/utils/collections.ts @@ -0,0 +1,39 @@ +/* + * SonarQube JavaScript Plugin + * Copyright (C) 2011-2019 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. + */ + +export const collectionConstructor = ["Array", "Map", "Set", "WeakSet", "WeakMap"]; + +export const writingMethods = [ + // array methods + "copyWithin", + "fill", + "pop", + "push", + "reverse", + "set", + "shift", + "sort", + "splice", + "unshift", + // map, set methods + "add", + "clear", + "delete", +]; diff --git a/eslint-bridge/tests/rules/no-empty-collection.test.ts b/eslint-bridge/tests/rules/no-empty-collection.test.ts new file mode 100644 index 00000000000..78b18001e19 --- /dev/null +++ b/eslint-bridge/tests/rules/no-empty-collection.test.ts @@ -0,0 +1,276 @@ +/* + * SonarQube JavaScript Plugin + * Copyright (C) 2011-2019 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 { RuleTester } from "eslint"; + +const ruleTester = new RuleTester({ + parser: require.resolve("@typescript-eslint/parser"), + parserOptions: { ecmaVersion: 2018, sourceType: "module" }, +}); + +import { rule } from "../../src/rules/no-empty-collection"; + +ruleTester.run("Empty collections should not be accessed or iterated", rule, { + valid: [ + { + code: `function okForNotEmptyInit() { + const nonEmptyArray = [1, 2, 3]; + foo(nonEmptyArray[2]); // OK + nonEmptyArray.forEach(item => console.log()); // OK + for (const _ of nonEmptyArray) { console.log(); } // OK + } + + function okLatelyWritten() { + const okLatelyWritten: number[] = []; + okLatelyWritten.push(1); + okLatelyWritten.forEach(item => console.log()); // OK + } + + function testCollectionContructors(){ + const notEmptyarrayConstructor = new Array(1, 2, 3); + notEmptyarrayConstructor.forEach(item => console.log()); // Ok + }`, + }, + { + code: `function parametersAreIgnore(parameterArray: number[]) { + foo(parameterArray[1]); + }`, + }, + { + code: `class MyClass { + myArray: string [] = []; + propertiesAreIgnored() { + foo(this.myArray[1]); // OK + } + }`, + }, + { + code: `function arrayUsedAsArgument() { + const array: number[] = []; + foo(array); + const copy = new Array(...array); + copy.push(42); + foo(array[1]); // OK + + return copy; + }`, + }, + { + code: `function reassignment() { + let overwrittenArray = []; + const otherArray = [1,2,3,4]; + overwrittenArray = otherArray; + foo(overwrittenArray[1]); // OK + + const arrayWrittenInsideArrow: number[] = []; + foo((n: number) => arrayWrittenInsideArrow.push(n)); + foo(arrayWrittenInsideArrow[1]); // OK + + let arrayWrittenInsideArrow2: number[] = []; + foo((n: number) => arrayWrittenInsideArrow2 = otherArray); + foo(arrayWrittenInsideArrow2[1]); // OK + }`, + }, + { + code: `// Interface Declaration + interface Array { + equals(array: Array): boolean // OK, symbol Array is an interface declaration + } + + // Type Alias Declaration + type MyArrayTypeAlias = T[]; + // OK, symbol MyArrayTypeAlias is a TypeAliasDeclaration + function isMyArrayTypeAlias(value: MyArrayTypeAlias | number): value is MyArrayTypeAlias { + return !!(value as any).length; + }`, + }, + { + code: `function arrayUsedInPropertyDeclaration() { + const emptyArray: number[] = []; + return { + a: emptyArray // OK, emptyArray is used in a property declaration + }; + } + + function arrayUsedInReturnStatement() { + const emptyArray: number[] = []; + return emptyArray; // OK, emptyArray is used in a return statement + }`, + }, + { + code: `function writeOnAliasVariable() { + const reassignedArray: number[] = []; + const aliasArray = reassignedArray; + aliasArray.push(1); + + foo(aliasArray[0]); // OK + foo(reassignedArray[0]); // OK + }`, + }, + { + code: `function assignmentEmptyArray() { + const assignmentEmptyArray: number[] = []; + assignmentEmptyArray[1] = 42; // ok + }`, + }, + { + code: `function arrayNotInitialized() { + let notInitializedArray!: number[]; + foo(notInitializedArray[0]); // Not reported + }`, + }, + { + code: `function arrayInitializedByFunctionCall(init: () => number[]) { + const externalInitializedArray: number[] = init(); + foo(externalInitializedArray[0]); // OK + }`, + }, + { + code: `function arrayUsedInORExpression(otherArray: number[]) { + const emptyArray: number[] = []; + console.log(otherArray || emptyArray); // OK used in OR expression + }`, + }, + { + code: `function writeWithTernaryOperator(flag: boolean) { + const potentiallyNonEmptyArray1 : number [] = []; + const potentiallyNonEmptyArray2: number[] = []; + (flag ? potentiallyNonEmptyArray1 : potentiallyNonEmptyArray2).push(1); + + foo(potentiallyNonEmptyArray1[0]); // OK + foo(potentiallyNonEmptyArray2[0]); // OK + }`, + }, + { + code: `function destructuringAssignmentEmptyArray() { + const destructuringAssignmentEmptyArray: number[] = []; + [ , destructuringAssignmentEmptyArray[1]] = [42, 42]; // ok + foo(destructuringAssignmentEmptyArray[1]); + }`, + }, + { + code: `import { IMPORTED_ARRAY } from "./dep"; + foo(IMPORTED_ARRAY[1]); // OK`, + }, + ], + invalid: [ + { + code: `const array : number[] = []; + export function testElementAccessRead() { + console.log(array[2]); + }`, + errors: [ + { + message: 'Review this usage of "array" as it can only be empty here.', + line: 3, + endLine: 3, + column: 29, + endColumn: 34, + }, + ], + }, + { + code: `function testAccessorMethodsRead(otherArray: number[]) { + const initialArray: number[] = []; + return initialArray.concat(otherArray); // Noncompliant + }`, + errors: [ + { + message: 'Review this usage of "initialArray" as it can only be empty here.', + }, + ], + }, + { + code: `const array : number[] = []; + export function testElementAccessRead() { + console.log(array[2]); + console.log(array[2]); + console.log(array[2]); + console.log(array[2]); + }`, + errors: 4, + }, + { + code: `function parametersAreIgnoreUnlessReAsigned(parameterArray: number[]) { + foo(parameterArray[1]); + parameterArray = []; + foo(parameterArray[1]); + }`, + errors: 1, + }, + { + code: `const array : number[] = []; + function testLoopRead() { + for (const _ of array) { // Noncompliant + } + for (const _ in array) { // Noncompliant + } + } + function testIterationMethodsRead() { + array.forEach(item => console.log()); // Noncompliant + array[Symbol.iterator](); // Noncompliant + } + `, + errors: 4, + }, + { + code: `function testCollectionContructors(){ + const arrayConstructor = new Array(); + arrayConstructor.forEach(item => console.log()); // Noncompliant + + const arrayWithoutNew = Array(); + arrayWithoutNew.map(item => console.log()); // Noncompliant + + const myMap = new Map(); + myMap.get(1); // Noncompliant + + const mySet = new Set(); + mySet.has(1); // Noncompliant + }`, + errors: 4, + }, + { + code: `function compoundAssignmentEmptyArray() { + const compoundAssignmentEmptyArray: number[] = []; + compoundAssignmentEmptyArray[1] += 42; // Noncompliant + }`, + errors: 1, + }, + { + code: `function elementAccessWithoutAssignment() { + const elementAccessWithoutAssignment: number[] = []; + foo(elementAccessWithoutAssignment[1]); // Noncompliant + }`, + errors: 1, + }, + { + code: `function okLatelyInitialized() { + let arrayLatelyInitialized: number[]; + arrayLatelyInitialized = []; + arrayLatelyInitialized.forEach(item => console.log()); // Noncompliant + }`, + errors: 1, + }, + { + code: `export let exportedArray: number[] = []; + foo(exportedArray[1]); // Can be a FP, but it's a corner case`, + errors: 1, + }, + ], +}); diff --git a/eslint-bridge/tests/rules/no-unused-collection.test.ts b/eslint-bridge/tests/rules/no-unused-collection.test.ts index 1db0f3dcdf0..70df1934be0 100644 --- a/eslint-bridge/tests/rules/no-unused-collection.test.ts +++ b/eslint-bridge/tests/rules/no-unused-collection.test.ts @@ -161,6 +161,13 @@ ruleTester.run("Primitive return types should be used.", rule, { return subgroups; `, }, + { + code: ` + let array = []; + for (let i in array) { // used here + console.log(i); + }`, + }, ], invalid: [ { diff --git a/its/plugin/projects/ts-rule-project/S4158.ts b/its/plugin/projects/ts-rule-project/S4158.ts new file mode 100644 index 00000000000..657cf3e8954 --- /dev/null +++ b/its/plugin/projects/ts-rule-project/S4158.ts @@ -0,0 +1,185 @@ +export function toCreateModule() {} + +const array : number[] = []; + +export function testElementAccessRead() { + console.log(array[2]); // Noncompliant +} + +function testLoopRead() { + for (const _ of array) { // Noncompliant + } + + for (const _ in array) { // Noncompliant + } +} + +function testIterationMethodsRead() { + array.forEach(item => console.log()); // Noncompliant + array[Symbol.iterator](); // Noncompliant +} + +function testAccessorMethodsRead(otherArray: number[]) { + const initialArray: number[] = []; + return initialArray.concat(otherArray); // Noncompliant +} + +function okForNotEmptyInit() { + const nonEmptyArray = [1, 2, 3]; + foo(nonEmptyArray[2]); // OK + nonEmptyArray.forEach(item => console.log()); // OK + for (const _ of nonEmptyArray) { console.log(); } // OK +} + +function okLatelyWritten() { + const okLatelyWritten: number[] = []; + okLatelyWritten.push(1); + okLatelyWritten.forEach(item => console.log()); // OK +} + + +function okLatelyInitialized() { + let arrayLatelyInitialized: number[]; + arrayLatelyInitialized = []; + arrayLatelyInitialized.forEach(item => console.log()); // Noncompliant +} + +function testCollectionContructors(){ + const arrayConstructor = new Array(); + arrayConstructor.forEach(item => console.log()); // Noncompliant + + const notEmptyarrayConstructor = new Array(1, 2, 3); + notEmptyarrayConstructor.forEach(item => console.log()); // Ok + + const arrayWithoutNew = Array(); + arrayWithoutNew.forEach(item => console.log()); // Noncompliant + + const myMap = new Map(); + myMap.get(1); // Noncompliant + + const mySet = new Set(); + mySet.has(1); // Noncompliant +} + +export let exportedArray: number[] = []; +foo(exportedArray[1]); // Noncompliant + +import { IMPORTED_ARRAY } from "./dep"; +foo(IMPORTED_ARRAY[1]); // OK + +function parametersAreIgnore(parameterArray: number[]) { + foo(parameterArray[1]); + parameterArray = []; + foo(parameterArray[1]); // Noncompliant +} + +class MyClass { + myArray: string [] = []; + propertiesAreIgnored() { + foo(this.myArray[1]); // OK + } +} + +function arrayUsedAsArgument() { + const array: number[] = []; + foo(array); + const copy = new Array(...array); + copy.push(42); + foo(array[1]); // OK + + return copy; +} + +function reassignment() { + let overwrittenArray = []; + const otherArray = [1,2,3,4]; + overwrittenArray = otherArray; + foo(overwrittenArray[1]); // OK + + const arrayWrittenInsideArrow: number[] = []; + foo((n: number) => arrayWrittenInsideArrow.push(n)); + foo(arrayWrittenInsideArrow[1]); // OK + + let arrayWrittenInsideArrow2: number[] = []; + foo((n: number) => arrayWrittenInsideArrow2 = otherArray); + foo(arrayWrittenInsideArrow2[1]); // OK +} + +// Interface Declaration +interface Array { + equals(array: Array): boolean // OK, symbol Array is an interface declaration +} + +// Type Alias Declaration +type MyArrayTypeAlias = T[]; +// OK, symbol MyArrayTypeAlias is a TypeAliasDeclaration +function isMyArrayTypeAlias(value: MyArrayTypeAlias | number): value is MyArrayTypeAlias { + return !!(value as any).length; +} + +function arrayUsedInORExpression(otherArray: number[]) { + const emptyArray: number[] = []; + console.log(otherArray || emptyArray); // OK used in OR expression +} + +function arrayUsedInPropertyDeclaration() { + const emptyArray: number[] = []; + return { + a: emptyArray // OK, emptyArray is used in a property declaration + }; +} + +function arrayUsedInReturnStatement() { + const emptyArray: number[] = []; + return emptyArray; // OK, emptyArray is used in a return statement +} + + +function writeWithTernaryOperator(flag: boolean) { + const potentiallyNonEmptyArray1 : number [] = []; + const potentiallyNonEmptyArray2: number[] = []; + (flag ? potentiallyNonEmptyArray1 : potentiallyNonEmptyArray2).push(1); + + foo(potentiallyNonEmptyArray1[0]); // OK + foo(potentiallyNonEmptyArray2[0]); // OK +} + +function writeOnAliasVariable() { + const reassignedArray: number[] = []; + const aliasArray = reassignedArray; + aliasArray.push(1); + + foo(aliasArray[0]); // OK + foo(reassignedArray[0]); // OK +} + +function arrayInitializedByFunctionCall(init: () => number[]) { + const externalInitializedArray: number[] = init(); + foo(externalInitializedArray[0]); // OK +} + +function arrayNotInitialized() { + let notInitializedArray!: number[]; + foo(notInitializedArray[0]); // Noncompliant +} + +function compoundAssignmentEmptyArray() { + const compoundAssignmentEmptyArray: number[] = []; + compoundAssignmentEmptyArray[1] += 42; // Noncompliant +} + +function assignmentEmptyArray() { + const assignmentEmptyArray: number[] = []; + assignmentEmptyArray[1] = 42; // ok +} + +function destructuringAssignmentEmptyArray() { + const destructuringAssignmentEmptyArray: number[] = []; + [ , destructuringAssignmentEmptyArray[1]] = [42, 42]; // ok + foo(destructuringAssignmentEmptyArray[1]); +} + +function elementAccessWithoutAssignment() { + const elementAccessWithoutAssignment: number[] = []; + foo(elementAccessWithoutAssignment[1]); // Noncompliant +} diff --git a/its/plugin/tests/src/test/resources/ts-rules-project-profile.xml b/its/plugin/tests/src/test/resources/ts-rules-project-profile.xml index 2b51e73a64e..6ae9f7f3c82 100644 --- a/its/plugin/tests/src/test/resources/ts-rules-project-profile.xml +++ b/its/plugin/tests/src/test/resources/ts-rules-project-profile.xml @@ -53,6 +53,11 @@ S4335 INFO + + typescript + S4158 + INFO + typescript S4524 diff --git a/javascript-checks/src/main/java/org/sonar/javascript/checks/CheckList.java b/javascript-checks/src/main/java/org/sonar/javascript/checks/CheckList.java index 03a80184053..46a2db845c8 100644 --- a/javascript-checks/src/main/java/org/sonar/javascript/checks/CheckList.java +++ b/javascript-checks/src/main/java/org/sonar/javascript/checks/CheckList.java @@ -185,6 +185,7 @@ public static List getAllChecks() { NoDuplicateInCompositeCheck.class, NoDuplicateStringCheck.class, NoElementOverwriteCheck.class, + NoEmptyCollectionCheck.class, NoEmptyInterfaceCheck.class, NoForInArrayCheck.class, NoInferrableTypesCheck.class, diff --git a/javascript-checks/src/main/java/org/sonar/javascript/checks/NoEmptyCollectionCheck.java b/javascript-checks/src/main/java/org/sonar/javascript/checks/NoEmptyCollectionCheck.java new file mode 100644 index 00000000000..8ebc7174b72 --- /dev/null +++ b/javascript-checks/src/main/java/org/sonar/javascript/checks/NoEmptyCollectionCheck.java @@ -0,0 +1,36 @@ +/* + * SonarQube JavaScript Plugin + * Copyright (C) 2011-2019 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. + */ +package org.sonar.javascript.checks; + +import org.sonar.check.Rule; +import org.sonar.javascript.checks.annotations.JavaScriptRule; +import org.sonar.javascript.checks.annotations.TypeScriptRule; + +@JavaScriptRule +@TypeScriptRule +@Rule(key = "S4158") +public class NoEmptyCollectionCheck extends EslintBasedCheck { + + @Override + public String eslintKey() { + return "no-empty-collection"; + } + +} diff --git a/javascript-checks/src/main/resources/org/sonar/l10n/javascript/rules/javascript/S4158.html b/javascript-checks/src/main/resources/org/sonar/l10n/javascript/rules/javascript/S4158.html new file mode 100644 index 00000000000..996322e575c --- /dev/null +++ b/javascript-checks/src/main/resources/org/sonar/l10n/javascript/rules/javascript/S4158.html @@ -0,0 +1,14 @@ +

When a collection is empty it makes no sense to access or iterate it. Doing so anyway is surely an error; either population was accidentally +omitted or the developer doesn't understand the situation.

+

Noncompliant Code Example

+
+let strings = [];
+
+if (strings.includes("foo")) {}  // Noncompliant
+
+for (str of strings) {}  // Noncompliant
+
+strings.forEach(str => doSomething(str)); // Noncompliant
+
+
+ diff --git a/javascript-checks/src/main/resources/org/sonar/l10n/javascript/rules/javascript/S4158.json b/javascript-checks/src/main/resources/org/sonar/l10n/javascript/rules/javascript/S4158.json new file mode 100644 index 00000000000..eef0ccdf959 --- /dev/null +++ b/javascript-checks/src/main/resources/org/sonar/l10n/javascript/rules/javascript/S4158.json @@ -0,0 +1,20 @@ +{ + "title": "Empty collections should not be accessed or iterated", + "type": "BUG", + "compatibleLanguages": [ + "JAVASCRIPT", + "TYPESCRIPT" + ], + "status": "ready", + "remediation": { + "func": "Constant\/Issue", + "constantCost": "15min" + }, + "tags": [ + + ], + "defaultSeverity": "Minor", + "ruleSpecification": "RSPEC-4158", + "sqKey": "S4158", + "scope": "All" +} diff --git a/javascript-checks/src/main/resources/org/sonar/l10n/javascript/rules/javascript/Sonar_way_profile.json b/javascript-checks/src/main/resources/org/sonar/l10n/javascript/rules/javascript/Sonar_way_profile.json index e21b6d543ee..17e5a3df8f5 100644 --- a/javascript-checks/src/main/resources/org/sonar/l10n/javascript/rules/javascript/Sonar_way_profile.json +++ b/javascript-checks/src/main/resources/org/sonar/l10n/javascript/rules/javascript/Sonar_way_profile.json @@ -97,6 +97,7 @@ "S4140", "S4143", "S4144", + "S4158", "S4165", "S4325", "S4335", @@ -117,4 +118,4 @@ "UnusedVariable", "WithStatement" ] -} +} diff --git a/javascript-checks/src/main/resources/org/sonar/l10n/javascript/rules/javascript/Sonar_way_recommended_profile.json b/javascript-checks/src/main/resources/org/sonar/l10n/javascript/rules/javascript/Sonar_way_recommended_profile.json index 2426b8fae3a..4442d13376e 100644 --- a/javascript-checks/src/main/resources/org/sonar/l10n/javascript/rules/javascript/Sonar_way_recommended_profile.json +++ b/javascript-checks/src/main/resources/org/sonar/l10n/javascript/rules/javascript/Sonar_way_recommended_profile.json @@ -151,6 +151,7 @@ "S4143", "S4144", "S4156", + "S4158", "S4165", "S4322", "S4323",