TypeScript Convention: should we require/forbid override keyword when implementing abstract methods?

[samreid]

@marlitas and I discovered that when overriding an abstract method, we don't have any tsc or lint rules that require or forbid the override keyword.

abstract class M {
  public abstract sayHello(): void
}

class N extends M {
  public override sayHello(): void {
    console.log( 'hello' );
  }
}

So right now it is developer preference. Should we leave it that way, or require override, or forbid override?

[pixelzoom]

+1 for choosing a convention and enforcing it via a lint rule.

I prefer override when implementing an abstract method. While it's not replacing a definition, it does indicate that it's something that is specified in the superclass. And I think that's useful for readability.

[jbphet]

This is approved to be a convention. @zepumph and @AgustinVallejo have volunteered to implement a lint rule for this.

@pixelzoom EDIT: To clarify, the convention that was approved is to use override when implementing abstract methods.

[zepumph]

@AgustinVallejo and I cranked away at this today. We have made very little progress, but have learned a lot about typescript/type-checking lint rules.

Here are some resources we collected along the way:
https://astexplorer.net/ is a great way to explore the tree, you can use @typescript-eslint/parser as a parser to get exactly the tree we are using for type information.

We then realized that we will need to basically learn the language of Typescript parsing nodes and how the typescript/TypeChecker works. Here is a copy of the interface that I grabbed from node_modules/eslint/lib/typescript.d.ts:

import { AssignmentPattern, BaseType, CallLikeExpression, CancellationToken, ElementAccessExpression, EntityName, EnumMember, ExportSpecifier, Expression, Identifier, ImportTypeNode, IndexInfo, IndexKind, IndexSignatureDeclaration, InterfaceType, Node, NodeArray, NodeBuilderFlags, ParameterDeclaration, PropertyAccessExpression, QualifiedName, Signature, SignatureDeclaration, SignatureKind, Symbol, SymbolFlags, SymbolFormatFlags, SyntaxKind, Type, TypeFlags, TypeFormatFlags, TypeNode, TypeParameter, TypeParameterDeclaration, TypePredicate, TypeReference } from './ts.js';

export interface TypeChecker {
  getTypeOfSymbolAtLocation(symbol: Symbol, node: Node): Type;
  getDeclaredTypeOfSymbol(symbol: Symbol): Type;
  getPropertiesOfType(type: Type): Symbol[];
  getPropertyOfType(type: Type, propertyName: string): Symbol | undefined;
  getPrivateIdentifierPropertyOfType(leftType: Type, name: string, location: Node): Symbol | undefined;
  getIndexInfoOfType(type: Type, kind: IndexKind): IndexInfo | undefined;
  getIndexInfosOfType(type: Type): readonly IndexInfo[];
  getSignaturesOfType(type: Type, kind: SignatureKind): readonly Signature[];
  getIndexTypeOfType(type: Type, kind: IndexKind): Type | undefined;
  getBaseTypes(type: InterfaceType): BaseType[];
  getBaseTypeOfLiteralType(type: Type): Type;
  getWidenedType(type: Type): Type;
  getReturnTypeOfSignature(signature: Signature): Type;
  getNullableType(type: Type, flags: TypeFlags): Type;
  getNonNullableType(type: Type): Type;
  getTypeArguments(type: TypeReference): readonly Type[];
  /** Note that the resulting nodes cannot be checked. */
  typeToTypeNode(type: Type, enclosingDeclaration: Node | undefined, flags: NodeBuilderFlags | undefined): TypeNode | undefined;
  /** Note that the resulting nodes cannot be checked. */
  signatureToSignatureDeclaration(signature: Signature, kind: SyntaxKind, enclosingDeclaration: Node | undefined, flags: NodeBuilderFlags | undefined): SignatureDeclaration & {
    typeArguments?: NodeArray<TypeNode>;
  } | undefined;
  /** Note that the resulting nodes cannot be checked. */
  indexInfoToIndexSignatureDeclaration(indexInfo: IndexInfo, enclosingDeclaration: Node | undefined, flags: NodeBuilderFlags | undefined): IndexSignatureDeclaration | undefined;
  /** Note that the resulting nodes cannot be checked. */
  symbolToEntityName(symbol: Symbol, meaning: SymbolFlags, enclosingDeclaration: Node | undefined, flags: NodeBuilderFlags | undefined): EntityName | undefined;
  /** Note that the resulting nodes cannot be checked. */
  symbolToExpression(symbol: Symbol, meaning: SymbolFlags, enclosingDeclaration: Node | undefined, flags: NodeBuilderFlags | undefined): Expression | undefined;
  /** Note that the resulting nodes cannot be checked. */
  symbolToTypeParameterDeclarations(symbol: Symbol, enclosingDeclaration: Node | undefined, flags: NodeBuilderFlags | undefined): NodeArray<TypeParameterDeclaration> | undefined;
  /** Note that the resulting nodes cannot be checked. */
  symbolToParameterDeclaration(symbol: Symbol, enclosingDeclaration: Node | undefined, flags: NodeBuilderFlags | undefined): ParameterDeclaration | undefined;
  /** Note that the resulting nodes cannot be checked. */
  typeParameterToDeclaration(parameter: TypeParameter, enclosingDeclaration: Node | undefined, flags: NodeBuilderFlags | undefined): TypeParameterDeclaration | undefined;
  getSymbolsInScope(location: Node, meaning: SymbolFlags): Symbol[];
  getSymbolAtLocation(node: Node): Symbol | undefined;
  getSymbolsOfParameterPropertyDeclaration(parameter: ParameterDeclaration, parameterName: string): Symbol[];
  /**
   * The function returns the value (local variable) symbol of an identifier in the short-hand property assignment.
   * This is necessary as an identifier in short-hand property assignment can contains two meaning: property name and property value.
   */
  getShorthandAssignmentValueSymbol(location: Node | undefined): Symbol | undefined;
  getExportSpecifierLocalTargetSymbol(location: ExportSpecifier | Identifier): Symbol | undefined;
  /**
   * If a symbol is a local symbol with an associated exported symbol, returns the exported symbol.
   * Otherwise returns its input.
   * For example, at `export type T = number;`:
   *     - `getSymbolAtLocation` at the location `T` will return the exported symbol for `T`.
   *     - But the result of `getSymbolsInScope` will contain the *local* symbol for `T`, not the exported symbol.
   *     - Calling `getExportSymbolOfSymbol` on that local symbol will return the exported symbol.
   */
  getExportSymbolOfSymbol(symbol: Symbol): Symbol;
  getPropertySymbolOfDestructuringAssignment(location: Identifier): Symbol | undefined;
  getTypeOfAssignmentPattern(pattern: AssignmentPattern): Type;
  getTypeAtLocation(node: Node): Type;
  getTypeFromTypeNode(node: TypeNode): Type;
  signatureToString(signature: Signature, enclosingDeclaration?: Node, flags?: TypeFormatFlags, kind?: SignatureKind): string;
  typeToString(type: Type, enclosingDeclaration?: Node, flags?: TypeFormatFlags): string;
  symbolToString(symbol: Symbol, enclosingDeclaration?: Node, meaning?: SymbolFlags, flags?: SymbolFormatFlags): string;
  typePredicateToString(predicate: TypePredicate, enclosingDeclaration?: Node, flags?: TypeFormatFlags): string;
  getFullyQualifiedName(symbol: Symbol): string;
  getAugmentedPropertiesOfType(type: Type): Symbol[];
  getRootSymbols(symbol: Symbol): readonly Symbol[];
  getSymbolOfExpando(node: Node, allowDeclaration: boolean): Symbol | undefined;
  getContextualType(node: Expression): Type | undefined;
  /**
   * returns unknownSignature in the case of an error.
   * returns undefined if the node is not valid.
   * @param argumentCount Apparent number of arguments, passed in case of a possibly incomplete call. This should come from an ArgumentListInfo. See `signatureHelp.ts`.
   */
  getResolvedSignature(node: CallLikeExpression, candidatesOutArray?: Signature[], argumentCount?: number): Signature | undefined;
  getSignatureFromDeclaration(declaration: SignatureDeclaration): Signature | undefined;
  isImplementationOfOverload(node: SignatureDeclaration): boolean | undefined;
  isUndefinedSymbol(symbol: Symbol): boolean;
  isArgumentsSymbol(symbol: Symbol): boolean;
  isUnknownSymbol(symbol: Symbol): boolean;
  getConstantValue(node: EnumMember | PropertyAccessExpression | ElementAccessExpression): string | number | undefined;
  isValidPropertyAccess(node: PropertyAccessExpression | QualifiedName | ImportTypeNode, propertyName: string): boolean;
  /** Follow all aliases to get the original symbol. */
  getAliasedSymbol(symbol: Symbol): Symbol;
  /** Follow a *single* alias to get the immediately aliased symbol. */
  getImmediateAliasedSymbol(symbol: Symbol): Symbol | undefined;
  getExportsOfModule(moduleSymbol: Symbol): Symbol[];
  getJsxIntrinsicTagNamesAt(location: Node): Symbol[];
  isOptionalParameter(node: ParameterDeclaration): boolean;
  getAmbientModules(): Symbol[];
  tryGetMemberInModuleExports(memberName: string, moduleSymbol: Symbol): Symbol | undefined;
  getApparentType(type: Type): Type;
  getBaseConstraintOfType(type: Type): Type | undefined;
  getDefaultFromTypeParameter(type: Type): Type | undefined;
  getTypePredicateOfSignature(signature: Signature): TypePredicate | undefined;
  /**
   * Depending on the operation performed, it may be appropriate to throw away the checker
   * if the cancellation token is triggered. Typically, if it is used for error checking
   * and the operation is cancelled, then it should be discarded, otherwise it is safe to keep.
   */
  runWithCancellationToken<T>(token: CancellationToken, cb: (checker: TypeChecker) => T): T;
}

After my call with @AgustinVallejo where basically we made no progress trying to use the non-typechecking ast tree to traverse to abstract methods with no "override" keyword, I hoped on a call with @jessegreenberg (our resident eslint rule-making expert) to see if I was just crazy, or if this really is so hard. Turns out it is just this hard.

We did find this doc about using the type checker though https://github.com/microsoft/TypeScript/wiki/Using-the-Compiler-API.

Please also note that there is a proposal for this exact item to be added as a compiler option in microsoft/TypeScript#47250. Perhaps add your feedback there!

Our general sentiments (from this issue and from @jessegreenberg's work in phetsims/chipper#1270) is that this could be potentially so powerful, but that the internal node API for typescript functionality is seriously bare, and it would take a lot to learn how to utilize it to its fullest. I fully plan to spend more time on this issue, but I don't really know if I will be able make any more progress.

Index: chipper/eslint/.eslintrc.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/chipper/eslint/.eslintrc.js b/chipper/eslint/.eslintrc.js
--- a/chipper/eslint/.eslintrc.js	(revision de0bea9104b4243f204944493155c0587a5dfb97)
+++ b/chipper/eslint/.eslintrc.js	(date 1659649997796)
@@ -511,7 +511,10 @@
         'no-simple-type-checking-assertions': 'error',
 
         // Custom return type rule that only requires for methods. The includes return type was too overarching.
-        'explicit-method-return-type': 'error'
+        'explicit-method-return-type': 'error',
+
+        // TODO
+        'override-abstract-implementation': 'error'
       }
     }
   ],
Index: chipper/eslint/.eslintignore
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/chipper/eslint/.eslintignore b/chipper/eslint/.eslintignore
--- a/chipper/eslint/.eslintignore	(revision de0bea9104b4243f204944493155c0587a5dfb97)
+++ b/chipper/eslint/.eslintignore	(date 1659651146117)
@@ -14,4 +14,5 @@
 ../phet-io-website/root/assets/js/QueryStringMachine_1.0.js
 ../phet-io-website/root/assets/js/QueryStringMachine_2.0.js
 ../phet-io-website/root/assets/js/phet-io-ga.js
-../chipper/dist
\ No newline at end of file
+../chipper/dist
+../joist/js
\ No newline at end of file
Index: chipper/eslint/rules/override-abstract-implementation.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/chipper/eslint/rules/override-abstract-implementation.js b/chipper/eslint/rules/override-abstract-implementation.js
new file mode 100644
--- /dev/null	(date 1659653774104)
+++ b/chipper/eslint/rules/override-abstract-implementation.js	(date 1659653774104)
@@ -0,0 +1,56 @@
+// Copyright 2022, University of Colorado Boulder
+/**
+ * @fileoverview Rule that checks for missing return types on method definitions for TypeScript files.
+ *
+ * This file was required, and we couldn't use the rule from the typescript-eslint plugin because we only want this
+ * rule checked when defining a method, and not on anonymous functions or other function declarations.
+ *
+ * @author Jesse Greenberg (PhET Interactive Simulations)
+ * @copyright 2022 University of Colorado Boulder
+ */
+
+const { ESLintUtils } = require( '@typescript-eslint/utils/' ); // eslint-disable-line
+
+// these are still MethodDefinition nodes, but don't require an annotation
+const exemptMethods = [ 'get', 'set', 'constructor' ];
+
+module.exports = {
+  meta: {
+    type: 'problem',
+    fixable: 'code'
+  },
+  create: context => {
+    return {
+      MethodDefinition: node => {
+
+
+        //   // Get the type checker.
+        const parserServices = ESLintUtils.getParserServices( context );
+        const tsNode = parserServices.esTreeNodeToTSNodeMap.get( node );
+        const checker = parserServices.program.getTypeChecker();
+        debugger;
+        checker.getMemberOverrideModifierStatus( tsNode, tsNode.modifiers [ 0 ] );
+
+        if ( tsNode.modifiers ) {
+
+          for ( let i = 0; i < tsNode.modifiers.length; i++ ) {
+            const modifier = tsNode.modifiers[ i ];
+            if ( modifier.kind === 159 ) { // override keyword (supposedly)
+              console.log( tsNode );
+              console.log( node );
+            }
+          }
+        }
+        console.log();
+
+        if ( !exemptMethods.includes( node.kind ) && node.value && !node.value.returnType ) {
+          //
+          // context.report( {
+          //   message: 'Missing return type.',
+          //   node: node
+          // } );
+        }
+      }
+    };
+  }
+};
\ No newline at end of file
Index: joist/Temp.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/joist/Temp.ts b/joist/Temp.ts
new file mode 100644
--- /dev/null	(date 1659655396911)
+++ b/joist/Temp.ts	(date 1659655396911)
@@ -0,0 +1,32 @@
+// Copyright 2021-2022, University of Colorado Boulder
+
+/**
+ * The panel for the PreferencesDialog containing preferences related to audio.
+ *
+ * @author Jesse Greenberg (PhET Interactive Simulations)
+ */
+
+
+abstract class M {
+  public abstract sayHello(): void;
+}
+
+abstract class X extends M {
+  public abstract another(): void;
+}
+
+export default class N extends M {
+  public override sayHello(): void {
+    console.log( 'hello' );
+  }
+}
+
+export class Z extends X {
+  public sayHello(): void {
+    console.log( 'hello' );
+  }
+
+  public another(): void {
+    //fdsaf
+  }
+}
\ No newline at end of file

[zepumph]

I am not going to work on this anytime soon. I don't think it is worth the time to ramp up on the typescript api for lint rules.

[AgustinVallejo]

I'm not working on this anytime soon either!