Merge pull request #19741 from Napalys/js/quality/suspicious_method_names

JS: Promote `js/suspicious-method-name-declaration` to the Code Quality suite.

Author: Napalys

javascript/ql/integration-tests/query-suite/javascript-code-quality.qls.expected

@@ -1,4 +1,5 @@
 ql/javascript/ql/src/Declarations/IneffectiveParameterType.ql
+ql/javascript/ql/src/Declarations/SuspiciousMethodNameDeclaration.ql
 ql/javascript/ql/src/Expressions/ExprHasNoEffect.ql
 ql/javascript/ql/src/Expressions/MissingAwait.ql
 ql/javascript/ql/src/LanguageFeatures/SpuriousArguments.ql

javascript/ql/src/Declarations/SuspiciousMethodNameDeclaration.qhelp

@@ -4,50 +4,73 @@
 <qhelp>
 <overview>
 <p>
-In TypeScript the keywords <code>constructor</code> and <code>new</code> for 
-member declarations are used to declare constructors in classes and interfaces 
-respectively. 
-However, a member declaration with the name <code>new</code> in an interface 
-or <code>constructor</code> in a class, will declare an ordinary method named 
-<code>new</code> or <code>constructor</code> rather than a constructor.
-Similarly, the keyword <code>function</code> is used to declare functions in 
-some contexts. However, using the name <code>function</code> for a class 
-or interface member declaration declares a method named <code>function</code>.    
+In TypeScript, certain keywords have special meanings for member declarations, and misusing them can create confusion:
+</p>
+
+<ul>
+<li>In classes, use <code>constructor</code> rather than <code>new</code> to declare constructors. Using <code>new</code> within a class creates a method named "new" and not a constructor signature.</li>
+<li>In interfaces, use <code>new</code> rather than <code>constructor</code> to declare constructor signatures. Using <code>constructor</code> within an interface creates a method named "constructor" and not a constructor signature.</li>
+<li>Similarly, the keyword <code>function</code> is used to declare functions in some contexts. However, using the name <code>function</code> for a class or interface member declaration declares a method named "function".</li>
+</ul>
+
+<p>
+When these keywords are misused, TypeScript will interpret them as regular method names rather than their intended special syntax, leading to code that may not work as expected.
 </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 a call signature in an 
-interface.
+Consider following these guidelines for clearer code:
 </p>
 
+<ul>
+<li>For classes, use <code>constructor</code> to declare constructors.</li>
+<li>For interfaces, use <code>new</code> to declare constructor signatures (call signatures that create new instances).</li>
+<li>Avoid accidentally creating methods named <code>function</code> by misusing the <code>function</code> keyword within class or interface declarations.</li>
+</ul>
+
 </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.
+The following examples show common mistakes when using these keywords:
 </p>
-<sample src="examples/SuspiciousMethodNameDeclaration.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.
+This interface mistakenly uses <code>constructor</code>, which creates a method named "constructor" instead of a constructor signature:
 </p>
+<sample src="examples/SuspiciousMethodNameDeclaration.ts" />
 
+<p>
+Use <code>new</code> for constructor signatures in interfaces:
+</p>
 <sample src="examples/SuspiciousMethodNameDeclarationFixed.ts" />
 
+<p>
+This class mistakenly uses <code>new</code>, which creates a method named "new" instead of a constructor:
+</p>
+<sample src="examples/SuspiciousMethodNameDeclarationClass.ts" />
+
+<p>
+Use <code>constructor</code> for constructors in classes:
+</p>
+<sample src="examples/SuspiciousMethodNameDeclarationClassFixed.ts" />
+
+<p>
+This interface uses <code>function</code> as a method name, which declares a method named "function" rather than declaring a function:
+</p>
+<sample src="examples/SuspiciousMethodNameDeclarationFunction.ts" />
+
+<p>
+Use a descriptive method name instead:
+</p>
+<sample src="examples/SuspiciousMethodNameDeclarationFunctionFixed.ts" />
+
 </example>
 <references>
 
+<li>TypeScript Handbook: <a href="https://www.typescriptlang.org/docs/handbook/2/classes.html#constructors">Classes - Constructors</a>.</li>
 <li>TypeScript specification: <a href="https://github.com/microsoft/TypeScript/blob/30cb20434a6b117e007a4959b2a7c16489f86069/doc/spec-ARCHIVED.md#3.8.9">Constructor Type Literals</a>.</li>
 <li>TypeScript specification: <a href="https://github.com/microsoft/TypeScript/blob/30cb20434a6b117e007a4959b2a7c16489f86069/doc/spec-ARCHIVED.md#8.3.1">Constructor Parameters</a>.</li>
 

javascript/ql/src/Declarations/SuspiciousMethodNameDeclaration.ql

@@ -6,7 +6,9 @@
  * @problem.severity warning
  * @id js/suspicious-method-name-declaration
  * @precision high
- * @tags correctness
+ * @tags quality
+ *       reliability
+ *       correctness
  *       typescript
  *       methods
  */

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

@@ -1,6 +1,6 @@
-declare class Point {
+// BAD: Using 'constructor' in an interface creates a method, not a constructor signature
+interface Point {
    x: number;
    y: number;
-   constructor(x : number, y: number);
+   constructor(x: number, y: number); // This is just a method named "constructor"
 }
-

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

@@ -0,0 +1,6 @@
+// BAD: Using 'new' in a class creates a method, not a constructor
+class Point {
+   x: number;
+   y: number;
+   new(x: number, y: number) {}; // This is just a method named "new"
+}

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

@@ -0,0 +1,9 @@
+// GOOD: Using 'constructor' for constructors in classes
+class Point {
+   x: number;
+   y: number;
+   constructor(x: number, y: number) { // This is a proper constructor
+      this.x = x;
+      this.y = y;
+   }
+}

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

@@ -1,4 +1,6 @@
+// GOOD: Using 'new' for constructor signatures in interfaces
 interface Point {
    x: number;
    y: number;
+   new(x: number, y: number): Point; // This is a proper constructor signature
 }

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

@@ -0,0 +1,4 @@
+// BAD: Using 'function' as a method name is confusing
+interface Calculator {
+   function(a: number, b: number): number; // This is just a method named "function"
+}

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

@@ -0,0 +1,4 @@
+// GOOD: Using descriptive method names instead of 'function'
+interface Calculator {
+   calculate(a: number, b: number): number; // Clear, descriptive method name
+}

javascript/ql/src/change-notes/2025-06-12-suspicious-method-name.md

@@ -0,0 +1,4 @@
+---
+category: queryMetadata
+---
+* Added `reliability` tag to the `js/suspicious-method-name-declaration` query.

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

@@ -2,3 +2,14 @@
 | tst.ts:16:3:16:21 | function(): number; | The member name 'function' does not declare a function, it declares a method named 'function'. |
 | tst.ts:37:3:37:21 | function(): number; | The member name 'function' does not declare a function, it declares a method named 'function'. |
 | tst.ts:48:3:48:13 | new(): Quz; | The member name 'new' does not declare a constructor, but 'constructor' does in class declarations. |
+| tst.ts:60:3:60:21 | function(): number; | The member name 'function' does not declare a function, it declares a method named 'function'. |
+| tst.ts:64:3:64:24 | constru ... number; | The member name 'constructor' does not declare a constructor in interfaces, but it does in classes. |
+| tst.ts:74:3:74:30 | functio ... string; | The member name 'function' does not declare a function, it declares a method named 'function'. |
+| tst.ts:75:3:75:30 | functio ... number; | The member name 'function' does not declare a function, it declares a method named 'function'. |
+| tst.ts:76:3:76:24 | functio ... ): any; | The member name 'function' does not declare a function, it declares a method named 'function'. |
+| tst.ts:80:3:80:23 | abstrac ... : void; | The member name 'new' does not declare a constructor, but 'constructor' does in class declarations. |
+| tst.ts:84:3:84:30 | abstrac ... number; | The member name 'function' does not declare a function, it declares a method named 'function'. |
+| tst.ts:93:5:93:21 | function(): void; | The member name 'function' does not declare a function, it declares a method named 'function'. |
+| tst.ts:98:3:98:21 | function(): number; | The member name 'function' does not declare a function, it declares a method named 'function'. |
+| tst.ts:110:3:110:24 | constru ... number; | The member name 'constructor' does not declare a constructor in interfaces, but it does in classes. |
+| tst.ts:116:3:116:24 | constru ... number; | The member name 'constructor' does not declare a constructor in interfaces, but it does in classes. |

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

@@ -50,3 +50,70 @@ declare class Quz {
 
 var bla = new Foo();
 var blab = new Baz();
+
+
+interface X {
+  constructor: () => string; // Just a property, not a method.
+}
+
+type A = {
+  function(): number; // $ Alert
+};
+
+type B = {
+  constructor(): number; // $ Alert
+  new(): number;
+};
+
+class StaticMethods {
+  static function(): void {}
+  static new(): void {}
+}
+
+interface Overloaded {
+  function(x: string): string; // $Alert
+  function(x: number): number; // $Alert
+  function(x: any): any; // $Alert
+}
+
+abstract class AbstractFoo {
+  abstract new(): void; // $Alert
+}
+
+abstract class AbstractFooFunction {
+  abstract function(): number; // $Alert
+}
+
+abstract class AbstractFooConstructor {
+  constructor(){}
+}
+
+declare module "some-module" {
+  interface ModuleInterface {
+    function(): void;  // $Alert
+  }
+}
+
+type Intersection = {
+  function(): number; // $Alert
+} & {
+  other(): string;
+};
+
+type Union = {
+  new(): number;
+} | {
+  valid(): string;
+};
+
+type Union2 = {
+  constructor(): number; // $Alert
+} | {
+  valid(): string;
+};
+
+type Intersection2 = {
+  constructor(): number; // $Alert
+} & {
+  other(): string;
+};