Skip to content

Commit 181f556

Browse files
committed
feat(dev-infra):: add lint rule to enforce no-implicit-override abstract members
TypeScript introduced a new flag called `noImplicitOverride` as part of TypeScript v4.3. This flag introduces a new keyword called `override` that can be applied to members which override declarations from a base class. This helps with code health as TS will report an error if e.g. the base class changes the method name but the override would still have the old method name. Similarly, if the base class removes the method completely, TS would complain that the memeber with `override` no longer overrides any method. A similar concept applies to abstract methods, with the exception that TypeScript's builtin `noImplicitOverride` option does not flag members which are implemented as part of an abstract class. We want to enforce this as a best-practice in the repository as adding `override` to such implemented members will cause TS to complain if an abstract member is removed, but still implemented by derived classes. More details: microsoft/TypeScript#44457.
1 parent 3473061 commit 181f556

11 files changed

+207
-7
lines changed

dev-infra/BUILD.bazel

+1
Original file line numberDiff line numberDiff line change
@@ -78,6 +78,7 @@ pkg_npm(
7878
"//dev-infra/benchmark/driver-utilities",
7979
"//dev-infra/commit-message",
8080
"//dev-infra/ts-circular-dependencies",
81+
"//dev-infra/tslint-rules",
8182
],
8283
)
8384

dev-infra/tslint-rules/BUILD.bazel

+13
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
load("@npm//@bazel/typescript:index.bzl", "ts_library")
2+
3+
ts_library(
4+
name = "tslint-rules",
5+
srcs = glob(["*.ts"]),
6+
module_name = "@angular/dev-infra-private/tslint-rules",
7+
visibility = ["//dev-infra:__subpackages__"],
8+
deps = [
9+
"@npm//tslib",
10+
"@npm//tslint",
11+
"@npm//typescript",
12+
],
13+
)
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,141 @@
1+
/**
2+
* @license
3+
* Copyright Google LLC All Rights Reserved.
4+
*
5+
* Use of this source code is governed by an MIT-style license that can be
6+
* found in the LICENSE file at https://angular.io/license
7+
*/
8+
9+
import {Replacement, RuleFailure, WalkContext} from 'tslint/lib';
10+
import {TypedRule} from 'tslint/lib/rules';
11+
import * as ts from 'typescript';
12+
13+
const FAILURE_MESSAGE = 'Missing override modifier. Members implemented as part of ' +
14+
'abstract classes should explicitly set the "override" modifier. ' +
15+
'More details: https://github.com/microsoft/TypeScript/issues/44457#issuecomment-856202843.';
16+
17+
/**
18+
* Rule which enforces that class members implementing abstract members
19+
* from base classes explicitly specify the `override` modifier.
20+
*
21+
* This ensures we follow the best-practice of applying `override` for abstract-implemented
22+
* members so that TypeScript creates diagnostics in both scenarios where either the abstract
23+
* class member is removed, or renamed.
24+
*
25+
* More details can be found here: https://github.com/microsoft/TypeScript/issues/44457.
26+
*/
27+
export class Rule extends TypedRule {
28+
override applyWithProgram(sourceFile: ts.SourceFile, program: ts.Program): RuleFailure[] {
29+
return this.applyWithFunction(sourceFile, ctx => visitNode(sourceFile, ctx, program));
30+
}
31+
}
32+
33+
function visitNode(node: ts.Node, ctx: WalkContext, program: ts.Program) {
34+
// If a class element implements an abstract member but does not have the
35+
// `override` keyword, create a lint failure.
36+
if (ts.isClassElement(node) && !hasOverrideModifier(node) &&
37+
matchesParentAbstractElement(node, program)) {
38+
ctx.addFailureAtNode(
39+
node, FAILURE_MESSAGE, Replacement.appendText(node.getStart(), `override `));
40+
}
41+
42+
ts.forEachChild(node, node => visitNode(node, ctx, program));
43+
}
44+
45+
/**
46+
* Checks if the specified class element matches a parent abstract class element. i.e.
47+
* whether the specified member "implements" an abstract member from a base class.
48+
*/
49+
function matchesParentAbstractElement(node: ts.ClassElement, program: ts.Program): boolean {
50+
const containingClass = node.parent as ts.ClassDeclaration;
51+
52+
// If the property we check does not have a property name, we cannot look for similarly-named
53+
// members in parent classes and therefore return early.
54+
if (node.name === undefined) {
55+
return false;
56+
}
57+
58+
const propertyName = getPropertyNameText(node.name);
59+
const typeChecker = program.getTypeChecker();
60+
61+
// If the property we check does not have a statically-analyzable property name,
62+
// we cannot look for similarly-named members in parent classes and return early.
63+
if (propertyName === null) {
64+
return false;
65+
}
66+
67+
return checkClassForInheritedMatchingAbstractMember(containingClass, typeChecker, propertyName);
68+
}
69+
70+
/** Checks if the given class inherits an abstract member with the specified name. */
71+
function checkClassForInheritedMatchingAbstractMember(
72+
clazz: ts.ClassDeclaration, typeChecker: ts.TypeChecker, searchMemberName: string): boolean {
73+
const baseClass = getBaseClass(clazz, typeChecker);
74+
75+
// If the class is not `abstract`, then all parent abstract methods would need to
76+
// be implemented, and there is never an abstract member within the class.
77+
if (baseClass === null || !hasAbstractModifier(baseClass)) {
78+
return false;
79+
}
80+
81+
const matchingMember = baseClass.members.find(
82+
m => m.name !== undefined && getPropertyNameText(m.name) === searchMemberName);
83+
84+
if (matchingMember !== undefined) {
85+
return hasAbstractModifier(matchingMember);
86+
}
87+
88+
return checkClassForInheritedMatchingAbstractMember(baseClass, typeChecker, searchMemberName);
89+
}
90+
91+
/** Gets the base class for the given class declaration. */
92+
function getBaseClass(node: ts.ClassDeclaration, typeChecker: ts.TypeChecker): ts.ClassDeclaration|
93+
null {
94+
const baseTypes = getExtendsHeritageExpressions(node);
95+
96+
if (baseTypes.length > 1) {
97+
throw Error('Class unexpectedly extends from multiple types.');
98+
}
99+
100+
const baseClass = typeChecker.getTypeAtLocation(baseTypes[0]).getSymbol();
101+
const baseClassDecl = baseClass?.valueDeclaration ?? baseClass?.declarations?.[0];
102+
103+
if (baseClassDecl !== undefined && ts.isClassDeclaration(baseClassDecl)) {
104+
return baseClassDecl;
105+
}
106+
107+
return null;
108+
}
109+
110+
/** Gets the `extends` base type expressions of the specified class. */
111+
function getExtendsHeritageExpressions(classDecl: ts.ClassDeclaration):
112+
ts.ExpressionWithTypeArguments[] {
113+
if (classDecl.heritageClauses === undefined) {
114+
return [];
115+
}
116+
const result: ts.ExpressionWithTypeArguments[] = [];
117+
for (const clause of classDecl.heritageClauses) {
118+
if (clause.token === ts.SyntaxKind.ExtendsKeyword) {
119+
result.push(...clause.types);
120+
}
121+
}
122+
return result;
123+
}
124+
125+
/** Gets whether the specified node has the `abstract` modifier applied. */
126+
function hasAbstractModifier(node: ts.Node): boolean {
127+
return !!node.modifiers?.some(s => s.kind === ts.SyntaxKind.AbstractKeyword);
128+
}
129+
130+
/** Gets whether the specified node has the `override` modifier applied. */
131+
function hasOverrideModifier(node: ts.Node): boolean {
132+
return !!node.modifiers?.some(s => s.kind === ts.SyntaxKind.OverrideKeyword);
133+
}
134+
135+
/** Gets the property name text of the specified property name. */
136+
function getPropertyNameText(name: ts.PropertyName): string|null {
137+
if (ts.isComputedPropertyName(name)) {
138+
return null;
139+
}
140+
return name.text;
141+
}

package.json

+1-1
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,7 @@
2929
"test-fixme-ivy-aot": "bazelisk test --config=ivy --build_tag_filters=-no-ivy-aot --test_tag_filters=-no-ivy-aot",
3030
"list-fixme-ivy-targets": "bazelisk query --output=label 'attr(\"tags\", \"\\[.*fixme-ivy.*\\]\", //...) except kind(\"sh_binary\", //...) except kind(\"devmode_js_sources\", //...)' | sort",
3131
"lint": "yarn -s tslint && yarn -s ng-dev format changed --check",
32-
"tslint": "tsc -p tools/tsconfig.json && tslint -c tslint.json \"+(dev-infra|packages|modules|scripts|tools)/**/*.+(js|ts)\"",
32+
"tslint": "tslint -c tslint.json --project tsconfig-tslint.json",
3333
"public-api:check": "node goldens/public-api/manage.js test",
3434
"public-api:update": "node goldens/public-api/manage.js accept",
3535
"symbol-extractor:check": "node tools/symbol-extractor/run_all_symbols_extractor_tests.js test",

tools/contributing-stats/get-data.ts

+2-1
Original file line numberDiff line numberDiff line change
@@ -237,7 +237,8 @@ async function run(date: string) {
237237
console.info(['Username', ...buildQueryAndParams('', date).labels].join(','));
238238

239239
for (const username of allOrgMembers) {
240-
const results = await graphql(buildQueryAndParams(username, date).query.toString());
240+
const results: [{issueCount: number}] =
241+
await graphql(buildQueryAndParams(username, date).query.toString());
241242
const values = Object.values(results).map(result => `${result.issueCount}`);
242243
console.info([username, ...values].join(','));
243244
}

tools/size-tracking/size_tracker.ts

+1-1
Original file line numberDiff line numberDiff line change
@@ -94,7 +94,7 @@ export class SizeTracker {
9494
filePath = filePath.replace(/\\/g, '/');
9595

9696
// Workaround for https://github.com/angular/angular/issues/30060
97-
if (process.env['BAZEL_TARGET'].includes('test/bundling/core_all:size_test')) {
97+
if (process.env['BAZEL_TARGET']!.includes('test/bundling/core_all:size_test')) {
9898
return filePath.replace(/^(\.\.\/)+external/, 'external')
9999
.replace(/^(\.\.\/)+packages\/core\//, '@angular/core/')
100100
.replace(/^(\.\.\/){3}/, '@angular/core/');

tools/tsconfig.json

+1
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66
"experimentalDecorators": true,
77
"module": "commonjs",
88
"moduleResolution": "node",
9+
"strict": true,
910
"outDir": "../dist/tools/",
1011
"noImplicitAny": true,
1112
"noFallthroughCasesInSwitch": true,

tools/tslint/requireInternalWithUnderscoreRule.ts

+1-1
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,6 @@ class TypedefWalker extends RuleWalker {
4747
}
4848
this.addFailure(this.createFailure(
4949
node.getStart(), node.getWidth(),
50-
`module-private member ${node.name.getText()} must be annotated @internal`));
50+
`module-private member ${node.name?.getText()} must be annotated @internal`));
5151
}
5252
}

tools/tslint/tsNodeLoaderRule.js

+19
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,19 @@
1+
/**
2+
* @license
3+
* Copyright Google LLC All Rights Reserved.
4+
*
5+
* Use of this source code is governed by an MIT-style license that can be
6+
* found in the LICENSE file at https://angular.io/license
7+
*/
8+
9+
const path = require('path');
10+
const Lint = require('tslint');
11+
12+
// Custom rule that registers all of the custom rules, written in TypeScript, with ts-node.
13+
// This is necessary, because `tslint` and IDEs won't execute any rules that aren't in a .js file.
14+
require('ts-node').register();
15+
16+
// Add a noop rule so tslint doesn't complain.
17+
exports.Rule = class Rule extends Lint.Rules.AbstractRule {
18+
apply() {}
19+
};

tsconfig-tslint.json

+12
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
{
2+
"compilerOptions": {
3+
"allowJs": true
4+
},
5+
"include": [
6+
"dev-infra/**/*",
7+
"packages/**/*",
8+
"modules/**/*",
9+
"tools/**/*",
10+
"scripts/**/*"
11+
]
12+
}

tslint.json

+15-3
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,19 @@
11
{
22
"rulesDirectory": [
3-
"dist/tools/tslint",
3+
"tools/tslint",
4+
"dev-infra/tslint-rules",
45
"node_modules/vrsource-tslint-rules/rules",
56
"node_modules/tslint-eslint-rules/dist/rules",
67
"node_modules/tslint-no-toplevel-property-access/rules"
78
],
89
"rules": {
10+
// The first rule needs to be `ts-node-loader` which sets up `ts-node` within TSLint so
11+
// that rules written in TypeScript can be loaded without needing to be transpiled.
12+
"ts-node-loader": true,
13+
// Custom rules written in TypeScript.
14+
"require-internal-with-underscore": true,
15+
"no-implicit-override-abstract": true,
16+
917
"eofline": true,
1018
"file-header": [
1119
true,
@@ -26,7 +34,6 @@
2634
true,
2735
"object"
2836
],
29-
"require-internal-with-underscore": true,
3037
"no-toplevel-property-access": [
3138
true,
3239
"packages/animations/src/",
@@ -57,6 +64,12 @@
5764
]
5865
},
5966
"jsRules": {
67+
// The first rule needs to be `ts-node-loader` which sets up `ts-node` within TSLint so
68+
// that rules written in TypeScript can be loaded without needing to be transpiled.
69+
"ts-node-loader": true,
70+
// Custom rules written in TypeScript.
71+
"require-internal-with-underscore": true,
72+
6073
"eofline": true,
6174
"file-header": [
6275
true,
@@ -71,7 +84,6 @@
7184
],
7285
"no-duplicate-imports": true,
7386
"no-duplicate-variable": true,
74-
"require-internal-with-underscore": true,
7587
"semicolon": [
7688
true
7789
],

0 commit comments

Comments
 (0)