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

scripts/tslint/booleanTriviaRule.ts

@@ -34,6 +34,7 @@ function walk(ctx: Lint.WalkContext<void>): void {
             switch (methodName) {
                 case "apply":
                 case "assert":
+                case "assertEqual":
                 case "call":
                 case "equal":
                 case "fail":
@@ -69,7 +70,7 @@ function walk(ctx: Lint.WalkContext<void>): void {
 
         const ranges = ts.getTrailingCommentRanges(sourceFile.text, arg.pos) || ts.getLeadingCommentRanges(sourceFile.text, arg.pos);
         if (ranges === undefined || ranges.length !== 1 || ranges[0].kind !== ts.SyntaxKind.MultiLineCommentTrivia) {
-            ctx.addFailureAtNode(arg, "Tag boolean argument with parameter name");
+            ctx.addFailureAtNode(arg, "Tag argument with parameter name");
             return;
         }
 

scripts/tslint/debugAssertRule.ts

@@ -0,0 +1,45 @@
+import * as Lint from "tslint/lib";
+import * as ts from "typescript";
+
+export class Rule extends Lint.Rules.AbstractRule {
+    public apply(sourceFile: ts.SourceFile): Lint.RuleFailure[] {
+        return this.applyWithFunction(sourceFile, ctx => walk(ctx));
+    }
+}
+
+function walk(ctx: Lint.WalkContext<void>): void {
+    ts.forEachChild(ctx.sourceFile, function recur(node) {
+        if (ts.isCallExpression(node)) {
+            checkCall(node);
+        }
+        ts.forEachChild(node, recur);
+    });
+
+    function checkCall(node: ts.CallExpression) {
+        if (!isDebugAssert(node.expression) || node.arguments.length < 2) {
+            return;
+        }
+
+        const message = node.arguments[1];
+        if (!ts.isStringLiteral(message)) {
+            ctx.addFailureAtNode(message, "Second argument to 'Debug.assert' should be a string literal.");
+        }
+
+        if (node.arguments.length < 3) {
+            return;
+        }
+
+        const message2 = node.arguments[2];
+        if (!ts.isStringLiteral(message2) && !ts.isArrowFunction(message2)) {
+            ctx.addFailureAtNode(message, "Third argument to 'Debug.assert' should be a string literal or arrow function.");
+        }
+    }
+
+    function isDebugAssert(expr: ts.Node): boolean {
+        return ts.isPropertyAccessExpression(expr) && isName(expr.expression, "Debug") && isName(expr.name, "assert");
+    }
+
+    function isName(expr: ts.Node, text: string): boolean {
+        return ts.isIdentifier(expr) && expr.text === text;
+    }
+}

src/compiler/core.ts

@@ -1290,12 +1290,12 @@ namespace ts {
     export function createFileDiagnostic(file: SourceFile, start: number, length: number, message: DiagnosticMessage): Diagnostic {
         const end = start + length;
 
-        Debug.assert(start >= 0, "start must be non-negative, is " + start);
-        Debug.assert(length >= 0, "length must be non-negative, is " + length);
+        Debug.assertGreaterThanOrEqual(start, 0);
+        Debug.assertGreaterThanOrEqual(length, 0);
 
         if (file) {
-            Debug.assert(start <= file.text.length, `start must be within the bounds of the file. ${start} > ${file.text.length}`);
-            Debug.assert(end <= file.text.length, `end must be the bounds of the file. ${end} > ${file.text.length}`);
+            Debug.assertLessThanOrEqual(start, file.text.length);
+            Debug.assertLessThanOrEqual(end, file.text.length);
         }
 
         let text = getLocaleSpecificMessage(message);
@@ -2389,15 +2389,40 @@ namespace ts {
             return currentAssertionLevel >= level;
         }
 
-        export function assert(expression: boolean, message?: string, verboseDebugInfo?: () => string, stackCrawlMark?: Function): void {
+        export function assert(expression: boolean, message?: string, verboseDebugInfo?: string | (() => string), stackCrawlMark?: Function): void {
             if (!expression) {
                 if (verboseDebugInfo) {
-                    message += "\r\nVerbose Debug Information: " + verboseDebugInfo();
+                    message += "\r\nVerbose Debug Information: " + (typeof verboseDebugInfo === "string" ? verboseDebugInfo : verboseDebugInfo());
                 }
                 fail(message ? "False expression: " + message : "False expression.", stackCrawlMark || assert);
             }
         }
 
+        export function assertEqual<T>(a: T, b: T, msg?: string, msg2?: string): void {
+            if (a !== b) {
+                const message = msg ? msg2 ? `${msg} ${msg2}` : msg : "";
+                fail(`Expected ${a} === ${b}. ${message}`);
+            }
+        }
+
+        export function assertLessThan(a: number, b: number, msg?: string): void {
+            if (a >= b) {
+                fail(`Expected ${a} < ${b}. ${msg || ""}`);
+            }
+        }
+
+        export function assertLessThanOrEqual(a: number, b: number): void {
+            if (a > b) {
+                fail(`Expected ${a} <= ${b}`);
+            }
+        }
+
+        export function assertGreaterThanOrEqual(a: number, b: number): void {
+            if (a < b) {
+                fail(`Expected ${a} >= ${b}`);
+            }
+        }
+
         export function fail(message?: string, stackCrawlMark?: Function): void {
             debugger;
             const e = new Error(message ? `Debug Failure. ${message}` : "Debug Failure.");

src/compiler/transformers/generators.ts

@@ -2448,7 +2448,7 @@ namespace ts {
          * @param location An optional source map location for the statement.
          */
         function createInlineBreak(label: Label, location?: TextRange): ReturnStatement {
-            Debug.assert(label > 0, `Invalid label: ${label}`);
+            Debug.assertLessThan(0, label, "Invalid label");
             return setTextRange(
                 createReturn(
                     createArrayLiteral([

src/server/project.ts

@@ -363,7 +363,7 @@ namespace ts.server {
             return map(this.program.getSourceFiles(), sourceFile => {
                 const scriptInfo = this.projectService.getScriptInfoForPath(sourceFile.path);
                 if (!scriptInfo) {
-                    Debug.assert(false, `scriptInfo for a file '${sourceFile.fileName}' is missing.`);
+                    Debug.fail(`scriptInfo for a file '${sourceFile.fileName}' is missing.`);
                 }
                 return scriptInfo;
             });

src/services/classifier.ts

@@ -260,11 +260,11 @@ namespace ts {
                                     templateStack.pop();
                                 }
                                 else {
-                                    Debug.assert(token === SyntaxKind.TemplateMiddle, "Should have been a template middle. Was " + token);
+                                    Debug.assertEqual(token, SyntaxKind.TemplateMiddle, "Should have been a template middle.");
                                 }
                             }
                             else {
-                                Debug.assert(lastTemplateStackToken === SyntaxKind.OpenBraceToken, "Should have been an open brace. Was: " + token);
+                                Debug.assertEqual(lastTemplateStackToken, SyntaxKind.OpenBraceToken, "Should have been an open brace");
                                 templateStack.pop();
                             }
                         }

src/services/services.ts

@@ -1258,7 +1258,7 @@ namespace ts {
                         // We do not support the scenario where a host can modify a registered
                         // file's script kind, i.e. in one project some file is treated as ".ts"
                         // and in another as ".js"
-                        Debug.assert(hostFileInformation.scriptKind === oldSourceFile.scriptKind, "Registered script kind (" + oldSourceFile.scriptKind + ") should match new script kind (" + hostFileInformation.scriptKind + ") for file: " + path);
+                        Debug.assertEqual(hostFileInformation.scriptKind, oldSourceFile.scriptKind, "Registered script kind should match new script kind.", path);
 
                         return documentRegistry.updateDocumentWithKey(fileName, path, newSettings, documentRegistryBucketKey, hostFileInformation.scriptSnapshot, hostFileInformation.version, hostFileInformation.scriptKind);
                     }

src/services/signatureHelp.ts

@@ -136,7 +136,9 @@ namespace ts.SignatureHelp {
 
             const kind = invocation.typeArguments && invocation.typeArguments.pos === list.pos ? ArgumentListKind.TypeArguments : ArgumentListKind.CallArguments;
             const argumentCount = getArgumentCount(list);
-            Debug.assert(argumentIndex === 0 || argumentIndex < argumentCount, `argumentCount < argumentIndex, ${argumentCount} < ${argumentIndex}`);
+            if (argumentIndex !== 0) {
+                Debug.assertLessThan(argumentIndex, argumentCount);
+            }
             const argumentsSpan = getApplicableSpanForArguments(list, sourceFile);
             return { kind, invocation, argumentsSpan, argumentIndex, argumentCount };
         }
@@ -270,7 +272,9 @@ namespace ts.SignatureHelp {
             ? 1
             : (<TemplateExpression>tagExpression.template).templateSpans.length + 1;
 
-        Debug.assert(argumentIndex === 0 || argumentIndex < argumentCount, `argumentCount < argumentIndex, ${argumentCount} < ${argumentIndex}`);
+        if (argumentIndex !== 0) {
+            Debug.assertLessThan(argumentIndex, argumentCount);
+        }
         return {
             kind: ArgumentListKind.TaggedTemplateArguments,
             invocation: tagExpression,
@@ -402,7 +406,9 @@ namespace ts.SignatureHelp {
             };
         });
 
-        Debug.assert(argumentIndex === 0 || argumentIndex < argumentCount, `argumentCount < argumentIndex, ${argumentCount} < ${argumentIndex}`);
+        if (argumentIndex !== 0) {
+            Debug.assertLessThan(argumentIndex, argumentCount);
+        }
 
         const selectedItemIndex = candidates.indexOf(resolvedSignature);
         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

@@ -78,11 +78,11 @@ namespace ts {
             getSourceFile: (fileName) => fileName === normalizePath(inputFileName) ? sourceFile : undefined,
             writeFile: (name, text) => {
                 if (fileExtensionIs(name, ".map")) {
-                    Debug.assert(sourceMapText === undefined, `Unexpected multiple source map outputs for the file '${name}'`);
+                    Debug.assertEqual(sourceMapText, undefined, "Unexpected multiple source map outputs, file:", name);
                     sourceMapText = text;
                 }
                 else {
-                    Debug.assert(outputText === undefined, `Unexpected multiple outputs for the file: '${name}'`);
+                    Debug.assertEqual(outputText, undefined, "Unexpected multiple outputs, file:", name);
                     outputText = text;
                 }
             },

tslint.json

@@ -7,6 +7,7 @@
             "check-space"
         ],
         "curly":[true, "ignore-same-line"],
+        "debug-assert": true,
         "indent": [true,
             "spaces"
         ],