Merge pull request #2589 from esbena/js/ignore-duplicate-params-for-empty-functions

Approved by erik-krogh

Author: semmle-qlci

change-notes/1.24/analysis-javascript.md

@@ -26,6 +26,7 @@
 | **Query**                      | **Expected impact**          | **Change**                                                                |
 |--------------------------------|------------------------------|---------------------------------------------------------------------------|
 | Clear-text logging of sensitive information (`js/clear-text-logging`) | More results | More results involving `process.env` and indirect calls to logging methods are recognized. |
+| Duplicate parameter names (`js/duplicate-parameter-name`) | Fewer results | This query now recognizes additional parameters that reasonably can have duplicated names. |
 | Incomplete string escaping or encoding (`js/incomplete-sanitization`) | Fewer false positive results | This query now recognizes additional cases where a single replacement is likely to be intentional. |
 | Unbound event handler receiver (`js/unbound-event-handler-receiver`) | Fewer false positive results | This query now recognizes additional ways event handler receivers can be bound. | 
 | Expression has no effect (`js/useless-expression`) | Fewer false positive results | The query now recognizes block-level flow type annotations. |

javascript/ql/src/Declarations/UniqueParameterNames.ql

@@ -36,6 +36,9 @@ where
   i < j and
   j = max(int k | parmBinds(f, k, _, name) | k) and
   not isDummy(p) and
+  // ignore functions without bodies or empty bodies
+  f.hasBody() and
+  exists(f.getABodyStmt()) and
   // duplicate parameters in strict mode functions are flagged by the 'Syntax error' rule
   not f.isStrict()
 select p, "This parameter has the same name as $@ of the same function.", q, "another parameter"

javascript/ql/test/query-tests/Declarations/UniqueParameterNames/tst.js

@@ -2,7 +2,7 @@ function f(
 x,
 x, // NOT OK
 \u0078 // NOT OK
-) {}
+) { return; }
 
 this.addPropertyListener(prop.name, function(_, _, _, a) {
   proxy.delegate = a.dao;
@@ -12,3 +12,10 @@ this.addPropertyListener(prop.name, function(_, _, _, a) {
 function f(x, y, x) {
   'use strict';
 }
+
+function f(
+x,
+x // OK: empty function
+) { }
+
+(a, a) => a + a; // OK: for strict mode functions, duplicate parameter names are a syntax error

javascript/ql/test/query-tests/LanguageFeatures/SyntaxError/SyntaxError.expected

@@ -1 +1,2 @@
+| arrows.js:1:5:1:5 | Error: Argument name clash | Error: Argument name clash |
 | tst.js:2:12:2:12 | Error: Unterminated string constant | Error: Unterminated string constant |

javascript/ql/test/query-tests/LanguageFeatures/SyntaxError/arrows.js

@@ -0,0 +1 @@
+(a, a) => a + a;