diff --git a/dev-infra/BUILD.bazel b/dev-infra/BUILD.bazel index bb88f64182..f2ca982a5c 100644 --- a/dev-infra/BUILD.bazel +++ b/dev-infra/BUILD.bazel @@ -78,6 +78,7 @@ pkg_npm( "//dev-infra/benchmark/driver-utilities", "//dev-infra/commit-message", "//dev-infra/ts-circular-dependencies", + "//dev-infra/tslint-rules", ], ) diff --git a/dev-infra/tslint-rules/BUILD.bazel b/dev-infra/tslint-rules/BUILD.bazel new file mode 100644 index 0000000000..7d7baf8d33 --- /dev/null +++ b/dev-infra/tslint-rules/BUILD.bazel @@ -0,0 +1,13 @@ +load("@npm//@bazel/typescript:index.bzl", "ts_library") + +ts_library( + name = "tslint-rules", + srcs = glob(["*.ts"]), + module_name = "@angular/dev-infra-private/tslint-rules", + visibility = ["//dev-infra:__subpackages__"], + deps = [ + "@npm//tslib", + "@npm//tslint", + "@npm//typescript", + ], +) diff --git a/dev-infra/tslint-rules/noImplicitOverrideAbstractRule.ts b/dev-infra/tslint-rules/noImplicitOverrideAbstractRule.ts new file mode 100644 index 0000000000..73ad2f9fc6 --- /dev/null +++ b/dev-infra/tslint-rules/noImplicitOverrideAbstractRule.ts @@ -0,0 +1,145 @@ +/** + * @license + * Copyright Google LLC All Rights Reserved. + * + * Use of this source code is governed by an MIT-style license that can be + * found in the LICENSE file at https://angular.io/license + */ + +import {Replacement, RuleFailure, WalkContext} from 'tslint/lib'; +import {TypedRule} from 'tslint/lib/rules'; +import * as ts from 'typescript'; + +const FAILURE_MESSAGE = 'Missing override modifier. Members implemented as part of ' + + 'abstract classes must explicitly set the "override" modifier. ' + + 'More details: https://github.com/microsoft/TypeScript/issues/44457#issuecomment-856202843.'; + +/** + * Rule which enforces that class members implementing abstract members + * from base classes explicitly specify the `override` modifier. + * + * This ensures we follow the best-practice of applying `override` for abstract-implemented + * members so that TypeScript creates diagnostics in both scenarios where either the abstract + * class member is removed, or renamed. + * + * More details can be found here: https://github.com/microsoft/TypeScript/issues/44457. + */ +export class Rule extends TypedRule { + override applyWithProgram(sourceFile: ts.SourceFile, program: ts.Program): RuleFailure[] { + return this.applyWithFunction(sourceFile, ctx => visitNode(sourceFile, ctx, program)); + } +} + +/** + * For a TypeScript AST node and each of its child nodes, check whether the node is a class + * element which implements an abstract member but does not have the `override` keyword. + */ +function visitNode(node: ts.Node, ctx: WalkContext, program: ts.Program) { + // If a class element implements an abstract member but does not have the + // `override` keyword, create a lint failure. + if (ts.isClassElement(node) && !hasOverrideModifier(node) && + matchesParentAbstractElement(node, program)) { + ctx.addFailureAtNode( + node, FAILURE_MESSAGE, Replacement.appendText(node.getStart(), `override `)); + } + + ts.forEachChild(node, node => visitNode(node, ctx, program)); +} + +/** + * Checks if the specified class element matches a parent abstract class element. i.e. + * whether the specified member "implements" an abstract member from a base class. + */ +function matchesParentAbstractElement(node: ts.ClassElement, program: ts.Program): boolean { + const containingClass = node.parent as ts.ClassDeclaration; + + // If the property we check does not have a property name, we cannot look for similarly-named + // members in parent classes and therefore return early. + if (node.name === undefined) { + return false; + } + + const propertyName = getPropertyNameText(node.name); + const typeChecker = program.getTypeChecker(); + + // If the property we check does not have a statically-analyzable property name, + // we cannot look for similarly-named members in parent classes and return early. + if (propertyName === null) { + return false; + } + + return checkClassForInheritedMatchingAbstractMember(containingClass, typeChecker, propertyName); +} + +/** Checks if the given class inherits an abstract member with the specified name. */ +function checkClassForInheritedMatchingAbstractMember( + clazz: ts.ClassDeclaration, typeChecker: ts.TypeChecker, searchMemberName: string): boolean { + const baseClass = getBaseClass(clazz, typeChecker); + + // If the class is not `abstract`, then all parent abstract methods would need to + // be implemented, and there is never an abstract member within the class. + if (baseClass === null || !hasAbstractModifier(baseClass)) { + return false; + } + + const matchingMember = baseClass.members.find( + m => m.name !== undefined && getPropertyNameText(m.name) === searchMemberName); + + if (matchingMember !== undefined) { + return hasAbstractModifier(matchingMember); + } + + return checkClassForInheritedMatchingAbstractMember(baseClass, typeChecker, searchMemberName); +} + +/** Gets the base class for the given class declaration. */ +function getBaseClass(node: ts.ClassDeclaration, typeChecker: ts.TypeChecker): ts.ClassDeclaration| + null { + const baseTypes = getExtendsHeritageExpressions(node); + + if (baseTypes.length > 1) { + throw Error('Class unexpectedly extends from multiple types.'); + } + + const baseClass = typeChecker.getTypeAtLocation(baseTypes[0]).getSymbol(); + const baseClassDecl = baseClass?.valueDeclaration ?? baseClass?.declarations?.[0]; + + if (baseClassDecl !== undefined && ts.isClassDeclaration(baseClassDecl)) { + return baseClassDecl; + } + + return null; +} + +/** Gets the `extends` base type expressions of the specified class. */ +function getExtendsHeritageExpressions(classDecl: ts.ClassDeclaration): + ts.ExpressionWithTypeArguments[] { + if (classDecl.heritageClauses === undefined) { + return []; + } + const result: ts.ExpressionWithTypeArguments[] = []; + for (const clause of classDecl.heritageClauses) { + if (clause.token === ts.SyntaxKind.ExtendsKeyword) { + result.push(...clause.types); + } + } + return result; +} + +/** Gets whether the specified node has the `abstract` modifier applied. */ +function hasAbstractModifier(node: ts.Node): boolean { + return !!node.modifiers?.some(s => s.kind === ts.SyntaxKind.AbstractKeyword); +} + +/** Gets whether the specified node has the `override` modifier applied. */ +function hasOverrideModifier(node: ts.Node): boolean { + return !!node.modifiers?.some(s => s.kind === ts.SyntaxKind.OverrideKeyword); +} + +/** Gets the property name text of the specified property name. */ +function getPropertyNameText(name: ts.PropertyName): string|null { + if (ts.isComputedPropertyName(name)) { + return null; + } + return name.text; +} diff --git a/package.json b/package.json index 130abd1ec9..dc9627a4ad 100644 --- a/package.json +++ b/package.json @@ -29,7 +29,7 @@ "test-fixme-ivy-aot": "bazelisk test --config=ivy --build_tag_filters=-no-ivy-aot --test_tag_filters=-no-ivy-aot", "list-fixme-ivy-targets": "bazelisk query --output=label 'attr(\"tags\", \"\\[.*fixme-ivy.*\\]\", //...) except kind(\"sh_binary\", //...) except kind(\"devmode_js_sources\", //...)' | sort", "lint": "yarn -s tslint && yarn -s ng-dev format changed --check", - "tslint": "tsc -p tools/tsconfig.json && tslint -c tslint.json \"+(dev-infra|packages|modules|scripts|tools)/**/*.+(js|ts)\"", + "tslint": "tslint -c tslint.json --project tsconfig-tslint.json", "public-api:check": "node goldens/public-api/manage.js test", "public-api:update": "node goldens/public-api/manage.js accept", "symbol-extractor:check": "node tools/symbol-extractor/run_all_symbols_extractor_tests.js test", diff --git a/tools/tslint/tsNodeLoaderRule.js b/tools/tslint/tsNodeLoaderRule.js new file mode 100644 index 0000000000..091cc3dda8 --- /dev/null +++ b/tools/tslint/tsNodeLoaderRule.js @@ -0,0 +1,19 @@ +/** + * @license + * Copyright Google LLC All Rights Reserved. + * + * Use of this source code is governed by an MIT-style license that can be + * found in the LICENSE file at https://angular.io/license + */ + +const path = require('path'); +const Lint = require('tslint'); + +// Custom rule that registers all of the custom rules, written in TypeScript, with ts-node. +// This is necessary, because `tslint` and IDEs won't execute any rules that aren't in a .js file. +require('ts-node').register(); + +// Add a noop rule so tslint doesn't complain. +exports.Rule = class Rule extends Lint.Rules.AbstractRule { + apply() {} +}; diff --git a/tsconfig-tslint.json b/tsconfig-tslint.json new file mode 100644 index 0000000000..e5a235c7c9 --- /dev/null +++ b/tsconfig-tslint.json @@ -0,0 +1,12 @@ +{ + "compilerOptions": { + "allowJs": true + }, + "include": [ + "dev-infra/**/*", + "packages/**/*", + "modules/**/*", + "tools/**/*", + "scripts/**/*" + ] +} diff --git a/tslint.json b/tslint.json index 9c0ccf7fc3..a08ecf0e5d 100644 --- a/tslint.json +++ b/tslint.json @@ -1,11 +1,19 @@ { "rulesDirectory": [ - "dist/tools/tslint", + "tools/tslint", + "dev-infra/tslint-rules", "node_modules/vrsource-tslint-rules/rules", "node_modules/tslint-eslint-rules/dist/rules", "node_modules/tslint-no-toplevel-property-access/rules" ], "rules": { + // The first rule needs to be `ts-node-loader` which sets up `ts-node` within TSLint so + // that rules written in TypeScript can be loaded without needing to be transpiled. + "ts-node-loader": true, + // Custom rules written in TypeScript. + "require-internal-with-underscore": true, + "no-implicit-override-abstract": true, + "eofline": true, "file-header": [ true, @@ -26,7 +34,6 @@ true, "object" ], - "require-internal-with-underscore": true, "no-toplevel-property-access": [ true, "packages/animations/src/", @@ -57,6 +64,12 @@ ] }, "jsRules": { + // The first rule needs to be `ts-node-loader` which sets up `ts-node` within TSLint so + // that rules written in TypeScript can be loaded without needing to be transpiled. + "ts-node-loader": true, + // Custom rules written in TypeScript. + "require-internal-with-underscore": true, + "eofline": true, "file-header": [ true, @@ -71,7 +84,6 @@ ], "no-duplicate-imports": true, "no-duplicate-variable": true, - "require-internal-with-underscore": true, "semicolon": [ true ],