add query for detecting suspisous method names in TypeScript

Author: erik-krogh

change-notes/1.23/analysis-javascript.md

@@ -16,6 +16,7 @@
 |---------------------------------------------------------------------------|-------------------------------------------------------------------|----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
 | Unused index variable (`js/unused-index-variable`)                        | correctness                                                       | Highlights loops that iterate over an array, but do not use the index variable to access array elements, indicating a possible typo or logic error. Results are shown on LGTM by default. |
 | Loop bound injection (`js/loop-bound-injection`)                          | security, external/cwe/cwe-834                                      | Highlights loops where a user-controlled object with an arbitrary .length value can trick the server to loop indefinitely. Results are not shown on LGTM by default. |
+| Suspicious method name (`js/suspicious-method-name`)                      | correctness, typescript, methods                                  | Highlights suspiciously named methods where the developer likely meant to write a constructor or function. Results are shown on LGTM by default. |
 
 ## Changes to existing queries
 

javascript/ql/src/Declarations/SuspiciousMethodName.qhelp

@@ -0,0 +1,50 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+<overview>
+<p>
+In TypeScript the keywords <code>constructor</code> and <code>new</code> are
+used to declare constructors in classes and interfaces respectively. 
+However, by using the wrong keyword a programmer can accidentally declare e.g. 
+a method called <code>constructor</code> inside an interface.
+Similarly the keyword <code>function</code> is used to declare functions in 
+some contexts, however, using the keyword <code>function</code> inside a class 
+or interface results in declaring a method called <code>function</code>.  
+</p>
+
+</overview>
+<recommendation>
+
+<p>
+Declare classes as classes and not as interfaces.
+Use the keyword <code>constructor</code> to declare constructors in a class,
+use the keyword <code>new</code> to declare constructors inside interfaces, 
+and don't use <code>function</code> when declaring an interface that is a 
+function.
+</p>
+
+</recommendation>
+<example>
+
+<p>
+The below example declares an interface <code>Point</code> with 2 fields 
+and a method called <code>constructor</code>. The interface does not declare
+a class <code>Point</code> with a constructor, which was likely what the 
+developer meant to create.
+</p>
+<sample src="examples/SuspiciousMethodName.ts" />
+
+<p>
+The below example is a fixed version of the above, where the interface is 
+instead declared as a class, thereby describing the type the developer meant 
+in the first place.
+</p>
+
+<sample src="examples/SuspiciousMethodNameFixed.ts" />
+
+</example>
+<references>
+
+</references>
+</qhelp>

javascript/ql/src/Declarations/SuspiciousMethodName.ql

@@ -0,0 +1,80 @@
+/**
+ * @name Suspicious method name
+ * @description A method having the name "function", "new", or "constructor"
+ *              is usually caused by a programmer being confused about the TypeScript syntax. 
+ * @kind problem
+ * @problem.severity warning
+ * @id js/suspicious-method-name
+ * @precision high
+ * @tags correctness
+ *       typescript
+ *       methods
+ */
+
+import javascript
+
+/**
+ * Holds if the method name on the given container is likely to be a mistake.
+ */
+predicate isSuspisousMethodName(string name, ClassOrInterface container) {
+  name = "function"
+  or
+  // "constructor" is only suspicious outside a class.  
+  name = "constructor" and not container instanceof ClassDefinition
+  or
+  // "new" is only suspicious inside a class.
+  name = "new" and container instanceof ClassDefinition
+}
+
+/**
+ * Holds if the beginning of the location is before the end.
+ */
+predicate isRealLocation(Location l) {
+  l.getEndLine() > l.getStartLine() or
+  (l.getStartLine() = l.getEndLine() and l.getEndColumn() > l.getStartColumn())
+}
+
+from MethodDeclaration member, ClassOrInterface container, string suffixMsg
+where
+  container.getLocation().getFile().getFileType().isTypeScript() and
+  container.getAMember() = member and
+  isSuspisousMethodName(member.getName(), container) and
+  
+  // Assume that a "new" method is intentional if the class has an explicit constructor.
+  not (
+    member.getName() = "new" and
+    container instanceof ClassDefinition and
+    exists(MemberDeclaration constructor |
+      container.getAMember() = constructor and
+      constructor.getName() = "constructor" and    
+      // Test that it is not an implicitly declared constructor.
+      isRealLocation(constructor.getLocation())
+    )
+  ) and
+  
+  // Explicitly declared static methods are fine.
+  not (
+    container instanceof ClassDefinition and
+    member.isStatic()
+  ) and
+  
+  // Only looking for declared methods. Methods with a body are OK. 
+  not exists(member.getBody().getBody()) and
+  
+  // The developer was not confused about "function" when there are other methods in the interface.
+  not (
+    member.getName() = "function" and 
+    exists(MethodDeclaration other | other = container.getMethod(_) |
+      other.getName() != "function" and
+      isRealLocation(other.getLocation())
+    )
+  ) and
+  
+  (
+    member.getName() = "constructor" and suffixMsg = "Did you mean to write a class instead of an interface?" 
+    or
+    member.getName() = "new" and suffixMsg = "Did you mean \"constructor\"?"
+    or
+    member.getName() = "function" and suffixMsg = "Did you mean to omit \"function\"?"
+  )
+select member, "Declares a suspiciously named method \"" + member.getName() + "\". " + suffixMsg

javascript/ql/src/Declarations/examples/SuspiciousMethodName.ts

@@ -0,0 +1,6 @@
+declare class Point {
+   x: number;
+   y: number;
+   constructor(x : number, y: number);
+}
+

javascript/ql/src/Declarations/examples/SuspiciousMethodNameFixed.ts

@@ -0,0 +1,4 @@
+interface Point {
+   x: number;
+   y: number;
+}

javascript/ql/test/query-tests/Declarations/SuspiciousMethodName/SuspiciousMethodName.expected

@@ -0,0 +1,4 @@
+| tst.ts:7:3:7:24 | constru ... string; | Declares a suspiciously named method "constructor". Did you mean "new"? |
+| tst.ts:16:3:16:21 | function(): number; | Declares a suspiciously named method "function". Did you mean to omit "function"? |
+| tst.ts:37:3:37:21 | function(): number; | Declares a suspiciously named method "function". Did you mean to omit "function"? |
+| tst.ts:48:3:48:13 | new(): Quz; | Declares a suspiciously named method "new". Did you mean "constructor"? |

javascript/ql/test/query-tests/Declarations/SuspiciousMethodName/SuspiciousMethodName.qlref

@@ -0,0 +1 @@
+Declarations/SuspiciousMethodName.ql

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

@@ -0,0 +1,19 @@
+ // OK: don't report anything in .js files.
+function getStuff(number) {
+    return {
+        "new": function() {
+            
+        },
+        "constructor": 123,
+        "function": "this is a string!"
+    }
+}
+
+class Foobar {
+    new() {
+        return 123;
+    }
+    function() {
+        return "string";
+    }
+}

javascript/ql/test/query-tests/Declarations/SuspiciousMethodName/tst.ts

@@ -0,0 +1,52 @@
+var foo: MyInterface = 123 as any;
+
+interface MyInterface {
+  function (): number; // OK. Highly unlikely that it is an accident when there are other named methods in the interface.
+  (): number; // OK: What was probaly meant above. 
+  new:() => void; // OK! This is a property, not a method, we ignore those.
+  constructor(): string; // NOT OK! This a called "constructor"
+  new(): Date; // OK! This a constructor signature.
+
+  myNumber: 123; 
+}
+
+var a : MyFunction = null as any;
+
+interface MyFunction {
+  function(): number; // NOT OK!
+}
+
+
+class Foo {
+  new(): number { // OK! Highly unlikely that a developer confuses "constructor" and "new" when both are present. 
+    return 123;
+  }
+  constructor() { // OK! This is a constructor.
+
+  }
+  myString = "foobar"
+  
+  myMethod(): boolean {
+    return Math.random() > 0.5;
+  }
+}
+
+var b : FunctionClass = new FunctionClass();
+
+declare class FunctionClass {
+  function(): number; // NOT OK:
+}
+
+class Baz {
+    new(): Baz { // OK! When there is a method body I assume the developer knows what they are doing.
+        return null as any;
+    }
+}
+
+
+declare class Quz {
+  new(): Quz; // NOT OK! The developer likely meant to write constructor.
+}
+
+var bla = new Foo();
+var blab = new Baz();