Merge pull request #1988 from erik-krogh/unreacableOverloads

JS: Unreachable overloads

Author: Max Schaefer

change-notes/1.23/analysis-javascript.md

@@ -21,6 +21,7 @@
 | Suspicious method name (`js/suspicious-method-name-declaration`)          | 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. |
 | Use of returnless function (`js/use-of-returnless-function`)              | maintainability, correctness                                      | Highlights calls where the return value is used, but the callee never returns a value. Results are shown on LGTM by default. |
 | Useless regular expression character escape (`js/useless-regexp-character-escape`) | correctness, security, external/cwe/cwe-20 | Highlights regular expression strings with useless character escapes, indicating a possible violation of [CWE-20](https://cwe.mitre.org/data/definitions/20.html). Results are shown on LGTM by default. |
+| Unreachable method overloads (`js/unreachable-method-overloads`)          | correctness, typescript                                           | Highlights method overloads that are impossible to use from client code. Results are shown on LGTM by default. |
 
 ## Changes to existing queries
 

javascript/config/suites/javascript/maintainability-more

@@ -7,6 +7,7 @@
 + semmlecode-javascript-queries/Declarations/UnusedParameter.ql: /Maintainability/Declarations
 + semmlecode-javascript-queries/Declarations/UnusedProperty.ql: /Maintainability/Declarations
 + semmlecode-javascript-queries/Declarations/UnusedVariable.ql: /Maintainability/Declarations
++ semmlecode-javascript-queries/Declarations/UnreachableMethodOverloads.ql: /Maintainability/Declarations
 + semmlecode-javascript-queries/Expressions/UnneededDefensiveProgramming.ql: /Maintainability/Expressions
 + semmlecode-javascript-queries/LanguageFeatures/ArgumentsCallerCallee.ql: /Maintainability/Language Features
 + semmlecode-javascript-queries/LanguageFeatures/ConditionalComments.ql: /Maintainability/Language Features

javascript/ql/src/Declarations/UnreachableMethodOverloads.qhelp

@@ -0,0 +1,67 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+<overview>
+    <p>
+        The TypeScript compiler has to choose which specific overload is called
+        when a method with multiple overloads is called.
+        The compiler will always choose the textually first overload that does
+        not give rise to any type errors with the arguments provided at the 
+        function call.
+    </p>
+    <p>
+       This behavior can be unintuitive for programmers unfamiliar with the
+       type system in TypeScript, and can in some instances lead to situations
+       where a programmer writes an overloaded method where only the first 
+       overload can ever be used. 
+    </p>
+</overview>
+<recommendation>
+    <p>
+       Either reorder the method overloads if an overload with more type
+       parameters is placed before a similar overload with fewer parameters.  
+       Alternatively, collapse multiple overloads with identical parameter types by 
+       creating a single overload that returns a union of the return types 
+       from the multiple overloads. 
+    </p>
+</recommendation>
+<example>
+    <p>
+        In the example below, a programmer has tried to express that a method 
+        can return multiple possible values by creating multiple overloads 
+        with identical parameter types. However, only the first overload 
+        will ever be selected by the TypeScript compiler.  
+    </p>
+    <sample src="examples/UnreachableMethodOverloads.ts" />
+    <p>
+       The error can be fixed by merging the overloads into a single method 
+       signature that returns a union of the previous return types.
+    </p>
+    <sample src="examples/UnreachableMethodOverloadsGood.ts" />
+    
+    <p>
+        In the example below, an interface <code>Foo</code> declares a method
+        <code>create()</code> with two overloads. The only difference between
+        the two overloads is the type parameter <code>T</code> in the first 
+        overload. The TypeScript compiler will always use the first overload 
+        when <code>create()</code> is called, as a default type will be used 
+        for the type parameter <code>T</code> if none is provided. 
+        This default type is <code>unknown</code> in TypeScript 3.5+, and 
+        <code>{}</code> in earlier versions. 
+    </p>
+    <sample src="examples/UnreachableMethodOverloadsTypeParameters.ts" />
+    <p>
+        In this example, the error has been fixed by switching the order of the two
+        overloads. In this fixed version, if the <code>create()</code> method
+        is called with an explicit type argument the second overload will be
+        used, as the first overload would give rise to a type error.
+    </p>
+    <sample src="examples/UnreachableMethodOverloadsTypeParametersGood.ts" />
+</example>
+<references>
+
+<li>TypeScript specification: <a href="https://github.com/microsoft/TypeScript/blob/7be7cba050799bc11c9411babd31f44c9ec087f0/doc/spec.md#4.15.1">Overload Resolution</a></li>
+
+</references>
+</qhelp>

javascript/ql/src/Declarations/UnreachableMethodOverloads.ql

@@ -0,0 +1,140 @@
+/**
+ * @name Unreachable method overloads
+ * @description Having multiple overloads with the same parameter types in TypeScript
+ *              makes all overloads except the first one unreachable, as the compiler
+ *              always resolves calls to the textually first matching overload.
+ * @kind problem
+ * @problem.severity warning
+ * @id js/unreachable-method-overloads
+ * @precision high
+ * @tags correctness
+ *       typescript
+ */
+
+import javascript
+
+/**
+ * Gets the `i`th parameter from the method signature.
+ */
+SimpleParameter getParameter(MethodSignature sig, int i) { result = sig.getBody().getParameter(i) }
+
+/**
+ * Gets a string-representation of the type-annotation from the `i`th parameter in the method signature.
+ */
+string getParameterTypeAnnotation(MethodSignature sig, int i) {
+  result = getParameter(sig, i).getTypeAnnotation().toString()
+}
+
+/**
+ * Gets the other overloads for an overloaded method signature.
+ */
+MethodSignature getOtherMatchingSignatures(MethodSignature sig) {
+  signaturesMatch(result, sig) and
+  result != sig
+}
+
+/**
+ * Gets the kind of the member-declaration. Either "static" or "instance".
+ */
+string getKind(MemberDeclaration m) {
+  if m.isStatic() then result = "static" else result = "instance"
+}
+
+/**
+ * A call-signature that originates from a MethodSignature in the AST.
+ */
+private class MethodCallSig extends CallSignatureType {
+    string name;
+    
+    MethodCallSig() {
+        exists(MethodSignature sig | 
+            this = sig.getBody().getCallSignature() and
+            name = sig.getName()
+        )
+    }
+    
+    /**
+     * Gets the name of any member that has this signature.
+     */
+    string getName() {
+        result = name
+    }
+}
+
+/**
+ * Holds if the two call signatures could be overloads of each other and have the same parameter types.
+ */
+predicate matchingCallSignature(MethodCallSig method, MethodCallSig other) {
+  method.getName() = other.getName() and 
+  
+  method.getNumOptionalParameter() = other.getNumOptionalParameter() and
+  method.getNumParameter() = other.getNumParameter() and
+  method.getNumRequiredParameter() = other.getNumRequiredParameter() and
+  // purposely not looking at number of type arguments. 
+  
+  method.getKind() = other.getKind() and 
+  
+  
+  forall(int i | i in [0 .. -1 + method.getNumParameter()] | 
+    method.getParameter(i) = other.getParameter(i) // This is sometimes imprecise, so it is still a good idea to compare type annotations.
+  ) and 
+  
+  // shared type parameters are equal. 
+  forall(int i | i in [0 .. -1 + min(int num | num = method.getNumTypeParameter() or num = other.getNumTypeParameter())] |
+    method.getTypeParameterBound(i) = other.getTypeParameterBound(i)
+  )
+}
+
+/**
+ * Gets which overload index the MethodSignature has among the overloads of the same name.
+ */
+int getOverloadIndex(MethodSignature sig) {
+    sig.getDeclaringType().getMethodOverload(sig.getName(), result) = sig
+}
+
+/**
+ * Holds if the two method signatures are overloads of each other and have the same parameter types.
+ */
+predicate signaturesMatch(MethodSignature method, MethodSignature other) {
+  // declared in the same interface/class.
+  method.getDeclaringType() = other.getDeclaringType() and
+  // same static modifier.
+  getKind(method) = getKind(other) and
+
+  // same name.
+  method.getName() = other.getName() and
+
+  // same number of parameters.
+  method.getBody().getNumParameter() = other.getBody().getNumParameter() and
+  
+  // The types are compared in matchingCallSignature. This is sanity-check that the textual representation of the type-annotations are somewhat similar.
+  forall(int i | i in [0 .. -1 + method.getBody().getNumParameter()] |
+    getParameterTypeAnnotation(method, i) = getParameterTypeAnnotation(other, i)   
+  ) and
+  
+  matchingCallSignature(method.getBody().getCallSignature(), other.getBody().getCallSignature())
+}
+
+from ClassOrInterface decl, string name, MethodSignature previous, MethodSignature unreachable
+where
+  previous = decl.getMethod(name) and
+  unreachable = getOtherMatchingSignatures(previous) and
+  
+  // If the method is part of inheritance between classes/interfaces, then there can sometimes be reasons for having this pattern.
+  not exists(decl.getASuperTypeDeclaration().getMethod(name)) and 
+  not exists(ClassOrInterface sub | 
+    decl = sub.getASuperTypeDeclaration() and
+    exists(sub.getMethod(name))
+  ) and
+  
+  
+  // If a later method overload has more type parameters, then that overload can be selected by explicitly declaring the type arguments at the callsite.
+  // This comparison removes those cases.
+  unreachable.getBody().getNumTypeParameter() <= previous.getBody().getNumTypeParameter() and
+  
+  // We always select the first of the overloaded methods.
+  not exists(MethodSignature later | later = getOtherMatchingSignatures(previous) |
+    getOverloadIndex(later) < getOverloadIndex(previous)
+  )
+select unreachable,
+  "This overload of " + name + "() is unreachable, the $@ overload will always be selected.", previous, "previous"

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

@@ -0,0 +1,5 @@
+interface Foo {
+    getParsedThing(id: string): string[];
+    getParsedThing(id: string): number[];
+    getParsedThing(id: string): object[];
+}

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

@@ -0,0 +1,3 @@
+interface Foo {
+    getParsedThing(id: string): object[] | number[] | string[];
+}

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

@@ -0,0 +1,4 @@
+interface Foo {
+    create<T>(a: string): MyObject<T>;
+    create(a: string): MyObject<any>;
+}

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

@@ -0,0 +1,4 @@
+interface Foo {
+    create(a: string): Array<any>;
+    create<T>(a: string): Array<T>;
+}

javascript/ql/src/semmle/javascript/Classes.qll

@@ -123,6 +123,14 @@ class ClassOrInterface extends @classorinterface, TypeParameterized {
    * Anonymous classes and interfaces do not have a canonical name.
    */
   TypeName getTypeName() { result.getADefinition() = this }
+  
+  /**
+   * Gets the ClassOrInterface corresponding to either a super type or an implemented interface.
+   */
+  ClassOrInterface getASuperTypeDeclaration() {
+    this.getSuperClass().(VarAccess).getVariable().getADeclaration() = result.getIdentifier() or
+    this.getASuperInterface().(LocalTypeAccess).getLocalTypeName().getADeclaration() = result.getIdentifier()
+  }
 }
 
 /**

javascript/ql/test/query-tests/Declarations/UnreachableOverloads/UnreachableMethodOverloads.expected

@@ -0,0 +1,4 @@
+| tst.ts:3:3:3:30 | method( ... number; | This overload of method() is unreachable, the $@ overload will always be selected. | tst.ts:2:3:2:30 | method( ... string; | previous |
+| tst.ts:6:3:6:17 | types1(): any[] | This overload of types1() is unreachable, the $@ overload will always be selected. | tst.ts:5:3:5:18 | types1<T>(): T[] | previous |
+| tst.ts:15:3:15:74 | on(even ... nction; | This overload of on() is unreachable, the $@ overload will always be selected. | tst.ts:14:3:14:74 | on(even ... nction; | previous |
+| tst.ts:21:3:21:30 | method( ... number; | This overload of method() is unreachable, the $@ overload will always be selected. | tst.ts:20:3:20:30 | method( ... string; | previous |