Add lint rule to check that Debug.assert calls do not eagerly interpolate strings
#17125
Conversation
ghost
requested a review
from 
rbuckton
left a comment
rbuckton
left a comment
There was a problem hiding this comment.
In most cases the interpolation is probably cheaper than the closure. For the common cases, we can create specific assertions on Debug.
src/compiler/core.ts
Outdated
|
|
||
| Debug.assert(start >= 0, "start must be non-negative, is " + start); | ||
| Debug.assert(length >= 0, "length must be non-negative, is " + length); | ||
| Debug.assert(start >= 0, "start must be non-negative", () => `is ${start}`); |
There was a problem hiding this comment.
Adding callbacks is arguably more expensive, as you have to create an environment record allocation each time.
It might make more sense to add something like Debug.assertNonNegative(start, "start") or similar.
| */ | ||
| function createInlineBreak(label: Label, location?: TextRange): ReturnStatement { | ||
| Debug.assert(label > 0, `Invalid label: ${label}`); | ||
| Debug.assert(label > 0, "Invalid label", () => label.toString()); |
There was a problem hiding this comment.
The callback is probably more expensive.
src/compiler/core.ts
Outdated
|
|
||
| Debug.assert(start >= 0, "start must be non-negative, is " + start); | ||
| Debug.assert(length >= 0, "length must be non-negative, is " + length); | ||
| Debug.assertLessThanOrEqual(0, start); |
There was a problem hiding this comment.
assertGreaterThanOrEqual?
| } | ||
|
|
||
| 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 { |
There was a problem hiding this comment.
This avoids needing a closure for the second argument (if it is not an interpolation).
src/server/project.ts
Outdated
| const scriptInfo = this.projectService.getScriptInfoForPath(sourceFile.path); | ||
| if (!scriptInfo) { | ||
| Debug.assert(false, `scriptInfo for a file '${sourceFile.fileName}' is missing.`); | ||
| Debug.assert(false, "scriptInfo for a file is missing:", sourceFile.fileName); |
There was a problem hiding this comment.
No need to change this, other than from an assert(false, to fail as the condition was already checked.
ghost
changed the title
Debug.assert calls do not eagerly interpolate stringsDebug.assert calls do not eagerly interpolate strings
2 participants
The second argument is usually never used (unless an assertion fails), so we shouldn't spend time eagerly computing its value.