Skip to content

Commit 9c10d86

Browse files
committed
build(linter/plugins): fully remove debug assertions (#16163)
`debugAssert` and friends are meant to be completely removed in release build (like `debug_assert!` in Rust). Turns out they weren't. Minifier does remove all calls to these functions, but it can't always prove that the expressions *inside* the function calls don't have side effects, so was leaving them in place in some cases. Fix that by visiting the AST in TSDown plugin, and removing all calls to debug assert functions entirely. Source code: ```ts if (visitor === null) { debugAssertIsNonNull(ruleDetails.rule.create); visitor = ruleDetails.rule.create(ruleDetails.context); } ``` Release build, before this PR: ```ts if (visitor === null) ruleDetails.rule.create, visitor = ruleDetails.rule.create(ruleDetails.context); ``` After this PR: ```ts if (visitor === null) visitor = ruleDetails.rule.create(ruleDetails.context); ```
1 parent 75249e0 commit 9c10d86

File tree

3 files changed

+234
-16
lines changed

3 files changed

+234
-16
lines changed

apps/oxlint/package.json

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@
2929
"esquery": "^1.6.0",
3030
"execa": "^9.6.0",
3131
"jiti": "^2.6.0",
32+
"oxc-parser": "^0.99.0",
3233
"rolldown": "catalog:",
3334
"tsdown": "catalog:",
3435
"type-fest": "^5.2.0",

apps/oxlint/tsdown.config.ts

Lines changed: 70 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
import { join } from "node:path";
22
import { defineConfig } from "tsdown";
3+
import { parseSync, Visitor } from "oxc-parser";
34

45
import type { Plugin } from "rolldown";
56

@@ -58,26 +59,47 @@ export default defineConfig([
5859
]);
5960

6061
/**
61-
* Create a plugin to remove imports of `assert*` functions from `src-js/utils/asserts.ts`,
62-
* and replace those imports with empty function declarations.
62+
* Create a plugin to remove imports of `debugAssert*` / `typeAssert*` functions from `src-js/utils/asserts.ts`,
63+
* and all their call sites.
6364
*
6465
* ```ts
6566
* // Original code
66-
* import { assertIs, assertIsNonNull } from '../utils/asserts.ts';
67+
* import { debugAssertIsNonNull } from '../utils/asserts.ts';
68+
* const foo = getFoo();
69+
* debugAssertIsNonNull(foo.bar);
6770
*
6871
* // After transform
69-
* function assertIs() {}
70-
* function assertIsNonNull() {}
72+
* const foo = getFoo();
7173
* ```
7274
*
75+
* This solves 2 problems:
76+
*
77+
* # 1. Minifier works chunk-by-chunk
78+
*
7379
* Minifier can already remove all calls to these functions as dead code, but only if the functions are defined
7480
* in the same file as the call sites.
7581
*
7682
* Problem is that `asserts.ts` is imported by files which end up in all output chunks.
7783
* So without this transform, TSDown creates a shared chunk for `asserts.ts`. Minifier works chunk-by-chunk,
7884
* so can't see that these functions are no-ops, and doesn't remove the function calls.
7985
*
80-
* Inlining these functions in each file solves the problem, and minifier removes all trace of them.
86+
* # 2. Not entirely removed
87+
*
88+
* Even if minifier does remove all calls to these functions, it can't prove that expressions *inside* the calls
89+
* don't have side effects.
90+
*
91+
* In example above, it can't know if `foo` has a getter for `bar` property.
92+
* So it removes the call to `debugAssertIsNonNull`, but leaves behind the `foo.bar` expression.
93+
*
94+
* ```ts
95+
* const foo = getFoo();
96+
* foo.bar;
97+
* ```
98+
*
99+
* This plugin visits AST and removes all calls to `debugAssert*` / `typeAssert*` functions entirely,
100+
* *including* the expressions inside the calls.
101+
*
102+
* This makes these debug assertion functions act like `debug_assert!` in Rust.
81103
*
82104
* @returns Plugin
83105
*/
@@ -90,31 +112,63 @@ function createReplaceAssertsPlugin(): Plugin {
90112
// Only process TS files in `src-js` directory
91113
filter: { id: /\/src-js\/.+\.ts$/ },
92114

93-
async handler(code, id, meta) {
115+
async handler(code, path, meta) {
94116
const magicString = meta.magicString!;
95-
const program = this.parse(code, { lang: "ts" });
117+
const { program, errors } = parseSync(path, code);
118+
if (errors.length !== 0) throw new Error(`Failed to parse ${path}: ${errors[0].message}`);
96119

97-
stmts: for (const stmt of program.body) {
120+
// Gather names of assertion functions imported from `asserts.ts`.
121+
// Also gather all identifiers used in the `import` statements, so can avoid erroring on them in visitor below.
122+
const assertFnNames: Set<string> = new Set(),
123+
idents = new Set();
124+
for (const stmt of program.body) {
98125
if (stmt.type !== "ImportDeclaration") continue;
99126

100127
// Check if import is from `utils/asserts.ts`.
101128
// `endsWith` check is just a shortcut to avoid resolving the specifier to a full path for most imports.
102129
const source = stmt.source.value;
103130
if (!source.endsWith("/asserts.ts") && !source.endsWith("/asserts.js")) continue;
104131
// oxlint-disable-next-line no-await-in-loop
105-
const importedId = await this.resolve(source, id);
132+
const importedId = await this.resolve(source, path);
106133
if (importedId === null || importedId.id !== ASSERTS_PATH) continue;
107134

108-
// Replace `import` statement with empty function declarations
109-
let functionsCode = "";
135+
// Remove `import` statement
110136
for (const specifier of stmt.specifiers) {
111-
// Skip this `import` statement if it's a default or namespace import - can't handle those
112-
if (specifier.type !== "ImportSpecifier") continue stmts;
113-
functionsCode += `function ${specifier.local.name}() {}\n`;
137+
if (specifier.type !== "ImportSpecifier") {
138+
throw new Error(`Only use named imports when importing from \`asserts.ts\`: ${path}`);
139+
}
140+
idents.add(specifier.local);
141+
if (specifier.imported.type === "Identifier") idents.add(specifier.imported);
142+
assertFnNames.add(specifier.local.name);
114143
}
115-
magicString.overwrite(stmt.start, stmt.end, functionsCode);
144+
magicString.remove(stmt.start, stmt.end);
116145
}
117146

147+
if (assertFnNames.size === 0) return;
148+
149+
// Visit AST and remove all calls to assertion functions
150+
const visitor = new Visitor({
151+
// Replace `debugAssert(...)` calls with `null`. Minifier will remove the `null`.
152+
CallExpression(node) {
153+
const { callee } = node;
154+
if (callee.type !== "Identifier") return;
155+
if (assertFnNames.has(callee.name)) {
156+
idents.add(callee);
157+
magicString.overwrite(node.start, node.end, "null");
158+
}
159+
},
160+
// Error if assertion functions are used in any other way. We lack logic to deal with that.
161+
Identifier(node) {
162+
const { name } = node;
163+
if (assertFnNames.has(name) && !idents.has(node)) {
164+
throw new Error(
165+
`Do not use \`${name}\` imported from \`asserts.ts\` except in function calls: ${path}`,
166+
);
167+
}
168+
},
169+
});
170+
visitor.visit(program);
171+
118172
return { code: magicString };
119173
},
120174
},

pnpm-lock.yaml

Lines changed: 163 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

0 commit comments

Comments
 (0)