diff --git a/change-notes/1.19/analysis-javascript.md b/change-notes/1.19/analysis-javascript.md index 12c71391ce98..37c20fae2fcf 100644 --- a/change-notes/1.19/analysis-javascript.md +++ b/change-notes/1.19/analysis-javascript.md @@ -40,6 +40,7 @@ | User-controlled bypass of security check | Fewer results | This rule no longer flags conditions that guard early returns. The precision of this rule has been revised to "medium". Results are no longer shown on LGTM by default. | | Whitespace contradicts operator precedence | Fewer false-positive results | This rule no longer flags operators with asymmetric whitespace. | | Unused import | Fewer false-positive results | This rule no longer flags imports used by the `transform-react-jsx` Babel plugin. | +| Self assignment | Fewer false-positive results | This rule now ignores self-assignments preceded by a JSDoc comment with a `@type` tag. | ## Changes to QL libraries diff --git a/javascript/ql/src/Expressions/Clones.qll b/javascript/ql/src/Expressions/Clones.qll index c82e360d53cd..54a996717717 100644 --- a/javascript/ql/src/Expressions/Clones.qll +++ b/javascript/ql/src/Expressions/Clones.qll @@ -152,7 +152,14 @@ class SelfAssignment extends StructurallyCompared { } override Expr candidate() { - result = getParent().(AssignExpr).getRhs() + result = getAssignment().getRhs() + } + + /** + * Gets the enclosing assignment. + */ + AssignExpr getAssignment() { + result.getLhs() = this } } diff --git a/javascript/ql/src/Expressions/SelfAssignment.qhelp b/javascript/ql/src/Expressions/SelfAssignment.qhelp index 40ffccc1b16e..4b31615434ec 100644 --- a/javascript/ql/src/Expressions/SelfAssignment.qhelp +++ b/javascript/ql/src/Expressions/SelfAssignment.qhelp @@ -15,6 +15,11 @@ Assigning a variable to itself typically indicates a mistake such as a missing Carefully inspect the assignment to check for misspellings or missing qualifiers.

+

+If the self-assignment is intentional and is needed for documentation or optimization purposes, +add a JSDoc comment with a @type tag. This will indicate the self-assignment is intentional. +

+ diff --git a/javascript/ql/src/Expressions/SelfAssignment.ql b/javascript/ql/src/Expressions/SelfAssignment.ql index 095a5b8bc66a..d07f568a6a60 100644 --- a/javascript/ql/src/Expressions/SelfAssignment.ql +++ b/javascript/ql/src/Expressions/SelfAssignment.ql @@ -43,5 +43,7 @@ where e.same(_) and propName = any(AccessorMethodDeclaration amd).getName() ) and // exclude DOM properties - not isDOMProperty(e.(PropAccess).getPropertyName()) -select e.getParent(), "This expression assigns " + dsc + " to itself." \ No newline at end of file + not isDOMProperty(e.(PropAccess).getPropertyName()) and + // exclude self-assignments that have been inserted to satisfy the TypeScript JS-checker + not e.getAssignment().getParent().(ExprStmt).getDocumentation().getATag().getTitle() = "type" +select e.getParent(), "This expression assigns " + dsc + " to itself." diff --git a/javascript/ql/test/query-tests/Expressions/SelfAssignment/SelfAssignment.expected b/javascript/ql/test/query-tests/Expressions/SelfAssignment/SelfAssignment.expected index 9b31d0607579..e97b3e8fe783 100644 --- a/javascript/ql/test/query-tests/Expressions/SelfAssignment/SelfAssignment.expected +++ b/javascript/ql/test/query-tests/Expressions/SelfAssignment/SelfAssignment.expected @@ -1,3 +1,5 @@ +| jsdoc.js:9:5:9:19 | this.y = this.y | This expression assigns property y to itself. | +| jsdoc.js:11:5:11:23 | this.arg = this.arg | This expression assigns property arg to itself. | | tst.js:5:2:5:14 | width = width | This expression assigns variable width to itself. | | tst.js:24:1:24:19 | array[1] = array[1] | This expression assigns element 1 to itself. | | tst.js:27:1:27:9 | o.x = o.x | This expression assigns property x to itself. | diff --git a/javascript/ql/test/query-tests/Expressions/SelfAssignment/jsdoc.js b/javascript/ql/test/query-tests/Expressions/SelfAssignment/jsdoc.js new file mode 100644 index 000000000000..0329cdf038d1 --- /dev/null +++ b/javascript/ql/test/query-tests/Expressions/SelfAssignment/jsdoc.js @@ -0,0 +1,15 @@ +class C extends Q { + constructor(arg) { + /** + * Something. + * @type {string | undefined} + */ + this.x = this.x; // OK - documentation + + this.y = this.y; // NOT OK + + this.arg = this.arg; // NOT OK + } +} + +// semmle-extractor-options: --experimental