From c65bc5cc90555e8c31353986a120f7325eeb85c8 Mon Sep 17 00:00:00 2001 From: Esben Sparre Andreasen Date: Thu, 11 Oct 2018 13:44:38 +0200 Subject: [PATCH 1/2] JS: add Util::pluralize, also add tests for Util::capitalize --- javascript/ql/src/semmle/javascript/Util.qll | 13 +++++++++++++ .../ql/test/library-tests/Util/capitalize.expected | 1 + javascript/ql/test/library-tests/Util/capitalize.ql | 3 +++ .../ql/test/library-tests/Util/pluralize.expected | 1 + javascript/ql/test/library-tests/Util/pluralize.ql | 3 +++ javascript/ql/test/library-tests/Util/tst.js | 1 + 6 files changed, 22 insertions(+) create mode 100644 javascript/ql/test/library-tests/Util/capitalize.expected create mode 100644 javascript/ql/test/library-tests/Util/capitalize.ql create mode 100644 javascript/ql/test/library-tests/Util/pluralize.expected create mode 100644 javascript/ql/test/library-tests/Util/pluralize.ql create mode 100644 javascript/ql/test/library-tests/Util/tst.js diff --git a/javascript/ql/src/semmle/javascript/Util.qll b/javascript/ql/src/semmle/javascript/Util.qll index caa1105191ee..d669643be58f 100644 --- a/javascript/ql/src/semmle/javascript/Util.qll +++ b/javascript/ql/src/semmle/javascript/Util.qll @@ -11,3 +11,16 @@ bindingset[s] string capitalize(string s) { result = s.charAt(0).toUpperCase() + s.suffix(1) } + + /** + * Gets the pluralization for `n` occurrences of `noun`. + * + * For example, the pluralization of `"function"` for `n = 2` is `"functions"`. + */ +bindingset[noun, n] +string pluralize(string noun, int n) { + if n = 1 then + result = noun + else + result = noun + "s" +} \ No newline at end of file diff --git a/javascript/ql/test/library-tests/Util/capitalize.expected b/javascript/ql/test/library-tests/Util/capitalize.expected new file mode 100644 index 000000000000..a92936e701a4 --- /dev/null +++ b/javascript/ql/test/library-tests/Util/capitalize.expected @@ -0,0 +1 @@ +| X | X | Xx | XX | Xx | XX | diff --git a/javascript/ql/test/library-tests/Util/capitalize.ql b/javascript/ql/test/library-tests/Util/capitalize.ql new file mode 100644 index 000000000000..0e90efece9b7 --- /dev/null +++ b/javascript/ql/test/library-tests/Util/capitalize.ql @@ -0,0 +1,3 @@ +import semmle.javascript.Util + +select capitalize("x"), capitalize("X"), capitalize("xx"), capitalize("XX"), capitalize("Xx"), capitalize("xX") diff --git a/javascript/ql/test/library-tests/Util/pluralize.expected b/javascript/ql/test/library-tests/Util/pluralize.expected new file mode 100644 index 000000000000..09cfc2d49bfd --- /dev/null +++ b/javascript/ql/test/library-tests/Util/pluralize.expected @@ -0,0 +1 @@ +| xs | x | xs | xs | diff --git a/javascript/ql/test/library-tests/Util/pluralize.ql b/javascript/ql/test/library-tests/Util/pluralize.ql new file mode 100644 index 000000000000..7b7f0ed053df --- /dev/null +++ b/javascript/ql/test/library-tests/Util/pluralize.ql @@ -0,0 +1,3 @@ +import semmle.javascript.Util + +select pluralize("x", 0), pluralize("x", 1), pluralize("x", 2), pluralize("x", -1) diff --git a/javascript/ql/test/library-tests/Util/tst.js b/javascript/ql/test/library-tests/Util/tst.js new file mode 100644 index 000000000000..a9eba82d5340 --- /dev/null +++ b/javascript/ql/test/library-tests/Util/tst.js @@ -0,0 +1 @@ +// used by qltest to identify the language From 9c2ca9a7faae533761aa5df7f5724c6c3cda8f2c Mon Sep 17 00:00:00 2001 From: Esben Sparre Andreasen Date: Thu, 11 Oct 2018 09:43:24 +0200 Subject: [PATCH 2/2] JS: make js/unused-local-variable flag import statements --- change-notes/1.19/analysis-javascript.md | 1 + .../ql/src/Declarations/UnusedVariable.ql | 107 ++++++++++++++---- .../UnusedVariable/UnusedVariable.expected | 8 +- .../UnusedVariable/multi-imports.js | 4 + 4 files changed, 93 insertions(+), 27 deletions(-) create mode 100644 javascript/ql/test/query-tests/Declarations/UnusedVariable/multi-imports.js diff --git a/change-notes/1.19/analysis-javascript.md b/change-notes/1.19/analysis-javascript.md index beedd61d6da4..9e2c73dd3d46 100644 --- a/change-notes/1.19/analysis-javascript.md +++ b/change-notes/1.19/analysis-javascript.md @@ -35,6 +35,7 @@ | Remote property injection | Fewer results | The precision of this rule has been revised to "medium". Results are no longer shown on LGTM by default. | | Missing CSRF middleware | Fewer false-positive results | This rule now recognizes additional CSRF protection middlewares. | | Server-side URL redirect | More results | This rule now recognizes redirection calls in more cases. | +| Unused variable, import, function or class | Fewer results | This rule now flags import statements with multiple unused imports once. | | 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. | diff --git a/javascript/ql/src/Declarations/UnusedVariable.ql b/javascript/ql/src/Declarations/UnusedVariable.ql index 093de8b4bf7f..c1d31e1b9d6b 100644 --- a/javascript/ql/src/Declarations/UnusedVariable.ql +++ b/javascript/ql/src/Declarations/UnusedVariable.ql @@ -94,36 +94,95 @@ predicate isEnumMember(VarDecl decl) { } /** - * Gets a description of the declaration `vd`, which is either of the form "function f" if - * it is a function name, or "variable v" if it is not. + * Gets a description of the declaration `vd`, which is either of the form + * "function f", "variable v" or "class c". */ -string describe(VarDecl vd) { +string describeVarDecl(VarDecl vd) { if vd = any(Function f).getId() then result = "function " + vd.getName() else if vd = any(ClassDefinition c).getIdentifier() then result = "class " + vd.getName() - else if (vd = any(ImportSpecifier im).getLocal() or vd = any(ImportEqualsDeclaration im).getId()) then - result = "import " + vd.getName() else result = "variable " + vd.getName() } -from VarDecl vd, UnusedLocal v -where v = vd.getVariable() and - // exclude variables mentioned in JSDoc comments in externs - not mentionedInJSDocComment(v) and - // exclude variables used to filter out unwanted properties - not isPropertyFilter(v) and - // exclude imports of React that are implicitly referenced by JSX - not isReactImportForJSX(v) and - // exclude names that are used as types - not isUsedAsType(vd) and - // exclude names that are used as namespaces from inside a type - not isUsedAsNamespace(vd) and - // exclude decorated functions and classes - not isDecorated(vd) and - // exclude names of enum members; they also define property names - not isEnumMember(vd) and - // ignore ambient declarations - too noisy - not vd.isAmbient() -select vd, "Unused " + describe(vd) + "." +/** + * An import statement that provides variable declarations. + */ +class ImportVarDeclProvider extends Stmt { + + ImportVarDeclProvider() { + this instanceof ImportDeclaration or + this instanceof ImportEqualsDeclaration + } + + /** + * Gets a variable declaration of this import. + */ + VarDecl getAVarDecl() { + result = this.(ImportDeclaration).getASpecifier().getLocal() or + result = this.(ImportEqualsDeclaration).getId() + } + + /** + * Gets an unacceptable unused variable declared by this import. + */ + UnusedLocal getAnUnacceptableUnusedLocal() { + result = getAVarDecl().getVariable() and + not whitelisted(result) + } + +} + +/** + * Holds if it is acceptable that `v` is unused. + */ +predicate whitelisted(UnusedLocal v) { + // exclude variables mentioned in JSDoc comments in externs + mentionedInJSDocComment(v) or + // exclude variables used to filter out unwanted properties + isPropertyFilter(v) or + // exclude imports of React that are implicitly referenced by JSX + isReactImportForJSX(v) or + // exclude names that are used as types + exists (VarDecl vd | + v = vd.getVariable() | + isUsedAsType(vd) or + // exclude names that are used as namespaces from inside a type + isUsedAsNamespace(vd)or + // exclude decorated functions and classes + isDecorated(vd) or + // exclude names of enum members; they also define property names + isEnumMember(vd) or + // ignore ambient declarations - too noisy + vd.isAmbient() + ) +} + +/** + * Holds if `vd` declares an unused variable that does not come from an import statement, as explained by `msg`. + */ +predicate unusedNonImports(VarDecl vd, string msg) { + exists (UnusedLocal v | + v = vd.getVariable() and + msg = "Unused " + describeVarDecl(vd) + "." and + not vd = any(ImportVarDeclProvider p).getAVarDecl() and + not whitelisted(v) + ) +} + +/** + * Holds if `provider` declares one or more unused variables, as explained by `msg`. + */ +predicate unusedImports(ImportVarDeclProvider provider, string msg) { + exists (string imports, string names | + imports = pluralize("import", count(provider.getAnUnacceptableUnusedLocal())) and + names = strictconcat(provider.getAnUnacceptableUnusedLocal().getName(), ", ") and + msg = "Unused " + imports + " " + names + "." + ) +} + +from ASTNode sel, string msg +where unusedNonImports(sel, msg) or + unusedImports(sel, msg) +select sel, msg diff --git a/javascript/ql/test/query-tests/Declarations/UnusedVariable/UnusedVariable.expected b/javascript/ql/test/query-tests/Declarations/UnusedVariable/UnusedVariable.expected index fc187550b366..7e1ae154cfe5 100644 --- a/javascript/ql/test/query-tests/Declarations/UnusedVariable/UnusedVariable.expected +++ b/javascript/ql/test/query-tests/Declarations/UnusedVariable/UnusedVariable.expected @@ -1,6 +1,8 @@ -| decorated.ts:1:9:1:21 | actionHandler | Unused import actionHandler. | +| decorated.ts:1:1:1:126 | import ... where'; | Unused import actionHandler. | | decorated.ts:4:10:4:12 | fun | Unused function fun. | | externs.js:6:5:6:13 | iAmUnused | Unused variable iAmUnused. | +| multi-imports.js:1:1:1:29 | import ... om 'x'; | Unused imports a, b, d. | +| multi-imports.js:2:1:2:42 | import ... om 'x'; | Unused imports alphabetically, ordered. | | typeoftype.ts:9:7:9:7 | y | Unused variable y. | -| unusedShadowed.ts:1:8:1:8 | T | Unused import T. | -| unusedShadowed.ts:2:8:2:13 | object | Unused import object. | +| unusedShadowed.ts:1:1:1:26 | import ... where'; | Unused import T. | +| unusedShadowed.ts:2:1:2:31 | import ... where'; | Unused import object. | diff --git a/javascript/ql/test/query-tests/Declarations/UnusedVariable/multi-imports.js b/javascript/ql/test/query-tests/Declarations/UnusedVariable/multi-imports.js new file mode 100644 index 000000000000..b32fa341a832 --- /dev/null +++ b/javascript/ql/test/query-tests/Declarations/UnusedVariable/multi-imports.js @@ -0,0 +1,4 @@ +import {a, b, c, d} from 'x'; +import {ordered, alphabetically} from 'x'; + +c();