Skip to content

Commit ceae613

Browse files
author
Andy
authored
Add lint rule to check that Debug.assert calls do not eagerly interpolate strings (#17125)
* And lint rule to check that `Debug.assert` calls do not eagerly interpolate strings * Use more specific 'assert' functions to avoid callbacks * Respond to PR feedback
1 parent a9a30d7 commit ceae613

File tree

10 files changed

+95
-17
lines changed

10 files changed

+95
-17
lines changed

‎scripts/tslint/booleanTriviaRule.ts‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,7 @@ function walk(ctx: Lint.WalkContext<void>): void {
3434
switch (methodName) {
3535
case "apply":
3636
case "assert":
37+
case "assertEqual":
3738
case "call":
3839
case "equal":
3940
case "fail":
@@ -69,7 +70,7 @@ function walk(ctx: Lint.WalkContext<void>): void {
6970

7071
const ranges = ts.getTrailingCommentRanges(sourceFile.text, arg.pos) || ts.getLeadingCommentRanges(sourceFile.text, arg.pos);
7172
if (ranges === undefined || ranges.length !== 1 || ranges[0].kind !== ts.SyntaxKind.MultiLineCommentTrivia) {
72-
ctx.addFailureAtNode(arg, "Tag boolean argument with parameter name");
73+
ctx.addFailureAtNode(arg, "Tag argument with parameter name");
7374
return;
7475
}
7576

‎scripts/tslint/debugAssertRule.ts‎

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,45 @@
1+
import * as Lint from "tslint/lib";
2+
import * as ts from "typescript";
3+
4+
export class Rule extends Lint.Rules.AbstractRule {
5+
public apply(sourceFile: ts.SourceFile): Lint.RuleFailure[] {
6+
return this.applyWithFunction(sourceFile, ctx => walk(ctx));
7+
}
8+
}
9+
10+
function walk(ctx: Lint.WalkContext<void>): void {
11+
ts.forEachChild(ctx.sourceFile, function recur(node) {
12+
if (ts.isCallExpression(node)) {
13+
checkCall(node);
14+
}
15+
ts.forEachChild(node, recur);
16+
});
17+
18+
function checkCall(node: ts.CallExpression) {
19+
if (!isDebugAssert(node.expression) || node.arguments.length < 2) {
20+
return;
21+
}
22+
23+
const message = node.arguments[1];
24+
if (!ts.isStringLiteral(message)) {
25+
ctx.addFailureAtNode(message, "Second argument to 'Debug.assert' should be a string literal.");
26+
}
27+
28+
if (node.arguments.length < 3) {
29+
return;
30+
}
31+
32+
const message2 = node.arguments[2];
33+
if (!ts.isStringLiteral(message2) && !ts.isArrowFunction(message2)) {
34+
ctx.addFailureAtNode(message, "Third argument to 'Debug.assert' should be a string literal or arrow function.");
35+
}
36+
}
37+
38+
function isDebugAssert(expr: ts.Node): boolean {
39+
return ts.isPropertyAccessExpression(expr) && isName(expr.expression, "Debug") && isName(expr.name, "assert");
40+
}
41+
42+
function isName(expr: ts.Node, text: string): boolean {
43+
return ts.isIdentifier(expr) && expr.text === text;
44+
}
45+
}

‎src/compiler/core.ts‎

Lines changed: 31 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1290,12 +1290,12 @@ namespace ts {
12901290
export function createFileDiagnostic(file: SourceFile, start: number, length: number, message: DiagnosticMessage): Diagnostic {
12911291
const end = start + length;
12921292

1293-
Debug.assert(start >= 0, "start must be non-negative, is " + start);
1294-
Debug.assert(length >= 0, "length must be non-negative, is " + length);
1293+
Debug.assertGreaterThanOrEqual(start, 0);
1294+
Debug.assertGreaterThanOrEqual(length, 0);
12951295

12961296
if (file) {
1297-
Debug.assert(start <= file.text.length, `start must be within the bounds of the file. ${start} > ${file.text.length}`);
1298-
Debug.assert(end <= file.text.length, `end must be the bounds of the file. ${end} > ${file.text.length}`);
1297+
Debug.assertLessThanOrEqual(start, file.text.length);
1298+
Debug.assertLessThanOrEqual(end, file.text.length);
12991299
}
13001300

13011301
let text = getLocaleSpecificMessage(message);
@@ -2389,15 +2389,40 @@ namespace ts {
23892389
return currentAssertionLevel >= level;
23902390
}
23912391

2392-
export function assert(expression: boolean, message?: string, verboseDebugInfo?: () => string, stackCrawlMark?: Function): void {
2392+
export function assert(expression: boolean, message?: string, verboseDebugInfo?: string | (() => string), stackCrawlMark?: Function): void {
23932393
if (!expression) {
23942394
if (verboseDebugInfo) {
2395-
message += "\r\nVerbose Debug Information: " + verboseDebugInfo();
2395+
message += "\r\nVerbose Debug Information: " + (typeof verboseDebugInfo === "string" ? verboseDebugInfo : verboseDebugInfo());
23962396
}
23972397
fail(message ? "False expression: " + message : "False expression.", stackCrawlMark || assert);
23982398
}
23992399
}
24002400

2401+
export function assertEqual<T>(a: T, b: T, msg?: string, msg2?: string): void {
2402+
if (a !== b) {
2403+
const message = msg ? msg2 ? `${msg} ${msg2}` : msg : "";
2404+
fail(`Expected ${a} === ${b}. ${message}`);
2405+
}
2406+
}
2407+
2408+
export function assertLessThan(a: number, b: number, msg?: string): void {
2409+
if (a >= b) {
2410+
fail(`Expected ${a} < ${b}. ${msg || ""}`);
2411+
}
2412+
}
2413+
2414+
export function assertLessThanOrEqual(a: number, b: number): void {
2415+
if (a > b) {
2416+
fail(`Expected ${a} <= ${b}`);
2417+
}
2418+
}
2419+
2420+
export function assertGreaterThanOrEqual(a: number, b: number): void {
2421+
if (a < b) {
2422+
fail(`Expected ${a} >= ${b}`);
2423+
}
2424+
}
2425+
24012426
export function fail(message?: string, stackCrawlMark?: Function): void {
24022427
debugger;
24032428
const e = new Error(message ? `Debug Failure. ${message}` : "Debug Failure.");

‎src/compiler/transformers/generators.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2448,7 +2448,7 @@ namespace ts {
24482448
* @param location An optional source map location for the statement.
24492449
*/
24502450
function createInlineBreak(label: Label, location?: TextRange): ReturnStatement {
2451-
Debug.assert(label > 0, `Invalid label: ${label}`);
2451+
Debug.assertLessThan(0, label, "Invalid label");
24522452
return setTextRange(
24532453
createReturn(
24542454
createArrayLiteral([

‎src/server/project.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -363,7 +363,7 @@ namespace ts.server {
363363
return map(this.program.getSourceFiles(), sourceFile => {
364364
const scriptInfo = this.projectService.getScriptInfoForPath(sourceFile.path);
365365
if (!scriptInfo) {
366-
Debug.assert(false, `scriptInfo for a file '${sourceFile.fileName}' is missing.`);
366+
Debug.fail(`scriptInfo for a file '${sourceFile.fileName}' is missing.`);
367367
}
368368
return scriptInfo;
369369
});

‎src/services/classifier.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -260,11 +260,11 @@ namespace ts {
260260
templateStack.pop();
261261
}
262262
else {
263-
Debug.assert(token === SyntaxKind.TemplateMiddle, "Should have been a template middle. Was " + token);
263+
Debug.assertEqual(token, SyntaxKind.TemplateMiddle, "Should have been a template middle.");
264264
}
265265
}
266266
else {
267-
Debug.assert(lastTemplateStackToken === SyntaxKind.OpenBraceToken, "Should have been an open brace. Was: " + token);
267+
Debug.assertEqual(lastTemplateStackToken, SyntaxKind.OpenBraceToken, "Should have been an open brace");
268268
templateStack.pop();
269269
}
270270
}

‎src/services/services.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1258,7 +1258,7 @@ namespace ts {
12581258
// We do not support the scenario where a host can modify a registered
12591259
// file's script kind, i.e. in one project some file is treated as ".ts"
12601260
// and in another as ".js"
1261-
Debug.assert(hostFileInformation.scriptKind === oldSourceFile.scriptKind, "Registered script kind (" + oldSourceFile.scriptKind + ") should match new script kind (" + hostFileInformation.scriptKind + ") for file: " + path);
1261+
Debug.assertEqual(hostFileInformation.scriptKind, oldSourceFile.scriptKind, "Registered script kind should match new script kind.", path);
12621262

12631263
return documentRegistry.updateDocumentWithKey(fileName, path, newSettings, documentRegistryBucketKey, hostFileInformation.scriptSnapshot, hostFileInformation.version, hostFileInformation.scriptKind);
12641264
}

‎src/services/signatureHelp.ts‎

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -136,7 +136,9 @@ namespace ts.SignatureHelp {
136136

137137
const kind = invocation.typeArguments && invocation.typeArguments.pos === list.pos ? ArgumentListKind.TypeArguments : ArgumentListKind.CallArguments;
138138
const argumentCount = getArgumentCount(list);
139-
Debug.assert(argumentIndex === 0 || argumentIndex < argumentCount, `argumentCount < argumentIndex, ${argumentCount} < ${argumentIndex}`);
139+
if (argumentIndex !== 0) {
140+
Debug.assertLessThan(argumentIndex, argumentCount);
141+
}
140142
const argumentsSpan = getApplicableSpanForArguments(list, sourceFile);
141143
return { kind, invocation, argumentsSpan, argumentIndex, argumentCount };
142144
}
@@ -270,7 +272,9 @@ namespace ts.SignatureHelp {
270272
? 1
271273
: (<TemplateExpression>tagExpression.template).templateSpans.length + 1;
272274

273-
Debug.assert(argumentIndex === 0 || argumentIndex < argumentCount, `argumentCount < argumentIndex, ${argumentCount} < ${argumentIndex}`);
275+
if (argumentIndex !== 0) {
276+
Debug.assertLessThan(argumentIndex, argumentCount);
277+
}
274278
return {
275279
kind: ArgumentListKind.TaggedTemplateArguments,
276280
invocation: tagExpression,
@@ -402,7 +406,9 @@ namespace ts.SignatureHelp {
402406
};
403407
});
404408

405-
Debug.assert(argumentIndex === 0 || argumentIndex < argumentCount, `argumentCount < argumentIndex, ${argumentCount} < ${argumentIndex}`);
409+
if (argumentIndex !== 0) {
410+
Debug.assertLessThan(argumentIndex, argumentCount);
411+
}
406412

407413
const selectedItemIndex = candidates.indexOf(resolvedSignature);
408414
Debug.assert(selectedItemIndex !== -1); // If candidates is non-empty it should always include bestSignature. We check for an empty candidates before calling this function.

‎src/services/transpile.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -78,11 +78,11 @@ namespace ts {
7878
getSourceFile: (fileName) => fileName === normalizePath(inputFileName) ? sourceFile : undefined,
7979
writeFile: (name, text) => {
8080
if (fileExtensionIs(name, ".map")) {
81-
Debug.assert(sourceMapText === undefined, `Unexpected multiple source map outputs for the file '${name}'`);
81+
Debug.assertEqual(sourceMapText, undefined, "Unexpected multiple source map outputs, file:", name);
8282
sourceMapText = text;
8383
}
8484
else {
85-
Debug.assert(outputText === undefined, `Unexpected multiple outputs for the file: '${name}'`);
85+
Debug.assertEqual(outputText, undefined, "Unexpected multiple outputs, file:", name);
8686
outputText = text;
8787
}
8888
},

‎tslint.json‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77
"check-space"
88
],
99
"curly":[true, "ignore-same-line"],
10+
"debug-assert": true,
1011
"indent": [true,
1112
"spaces"
1213
],

0 commit comments

Comments
 (0)