From 4f4ad2b942bd53e12858a4c2125dad6f9a102fb4 Mon Sep 17 00:00:00 2001 From: Asger F Date: Thu, 6 Sep 2018 16:31:10 +0100 Subject: [PATCH 1/5] JavaScript: ignore self-assignments with a JSDoc comment --- javascript/ql/src/Expressions/Clones.qll | 9 ++++++++- javascript/ql/src/Expressions/SelfAssignment.ql | 6 ++++-- .../SelfAssignment/SelfAssignment.expected | 2 ++ .../Expressions/SelfAssignment/jsdoc.js | 15 +++++++++++++++ 4 files changed, 29 insertions(+), 3 deletions(-) create mode 100644 javascript/ql/test/query-tests/Expressions/SelfAssignment/jsdoc.js 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.ql b/javascript/ql/src/Expressions/SelfAssignment.ql index 095a5b8bc66a..c7f594077cdd 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 with a JSDoc comment + not exists(e.getAssignment().getParent().(ExprStmt).getDocumentation().getATag()) +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 From e39b0c7a75d8cefdbf1712ead2c32a35f6cb5df5 Mon Sep 17 00:00:00 2001 From: Asger F Date: Fri, 7 Sep 2018 10:34:00 +0100 Subject: [PATCH 2/5] JavaScript: address comments --- javascript/ql/src/Expressions/SelfAssignment.ql | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/javascript/ql/src/Expressions/SelfAssignment.ql b/javascript/ql/src/Expressions/SelfAssignment.ql index c7f594077cdd..d07f568a6a60 100644 --- a/javascript/ql/src/Expressions/SelfAssignment.ql +++ b/javascript/ql/src/Expressions/SelfAssignment.ql @@ -44,6 +44,6 @@ where e.same(_) and ) and // exclude DOM properties not isDOMProperty(e.(PropAccess).getPropertyName()) and - // exclude self-assignments with a JSDoc comment - not exists(e.getAssignment().getParent().(ExprStmt).getDocumentation().getATag()) + // 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." From ad7ecc1df080757954c10dac6675c89facb8d384 Mon Sep 17 00:00:00 2001 From: Asger F Date: Mon, 10 Sep 2018 14:59:55 +0100 Subject: [PATCH 3/5] JavaScript: added change note --- change-notes/1.19/analysis-javascript.md | 1 + 1 file changed, 1 insertion(+) 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 From e6709198071435c43e7a53c106f9ab28432cbff3 Mon Sep 17 00:00:00 2001 From: Asger F Date: Wed, 31 Oct 2018 10:45:20 +0000 Subject: [PATCH 4/5] JS: mention @type tag in qhelp --- javascript/ql/src/Expressions/SelfAssignment.qhelp | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/javascript/ql/src/Expressions/SelfAssignment.qhelp b/javascript/ql/src/Expressions/SelfAssignment.qhelp index 40ffccc1b16e..8851ef75eea7 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. +

+ From 1252cde7f3ab11a0bee540157e8fc17229dfae84 Mon Sep 17 00:00:00 2001 From: Asger F Date: Tue, 6 Nov 2018 12:24:34 +0000 Subject: [PATCH 5/5] JS: remove a comma --- javascript/ql/src/Expressions/SelfAssignment.qhelp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/javascript/ql/src/Expressions/SelfAssignment.qhelp b/javascript/ql/src/Expressions/SelfAssignment.qhelp index 8851ef75eea7..4b31615434ec 100644 --- a/javascript/ql/src/Expressions/SelfAssignment.qhelp +++ b/javascript/ql/src/Expressions/SelfAssignment.qhelp @@ -16,7 +16,7 @@ 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, +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.