@samitouri / QOS-React-1 / commits / f993f0c2a5

[pipeline] panicOnBailout -> panicThreshold; remove isDev logging

--- Changed `panicOnBailout: boolean` to `panicThreshold`, which has the following options. Note that `ALL_ERRORS` corresponds to `panicOnBailout = true` and `CRITICAL_ERRORS` corresponds to `panicOnBailout = false`. `NONE` is a new option. ```js export type PanicThresholdOptions = // Bail out of compilation on all errors by throwing an exception. | "ALL_ERRORS" // Bail out of compilation only on critical or unrecognized errors. // Instead, silently skip the erroring function. | "CRITICAL_ERRORS" // Never bail out of compilation. Instead, silently skip the erroring // function or file. | "NONE"; ``` Jest seems to run babel through a different pipeline than Metro and - (perhaps through its complex `require` interjection logic). When running jest tests, exceptions thrown by babel transforms will bubble up to the nearest exception boundary which is often the jest test itself. This may not be a useful signal to anyone running a jest test with Forget enabled, as the erroring code may be within Forget itself or a transitively required module. Another reason to immediately bailing out on critical errors is that we may want to record errors found in the rest of the file. --- I'm not convinced that this change makes sense. A counterargument is that any CriticalErrors *should* be reported as parse errors, regardless of the runtime mode. Anyhow, this would be useful long term for our static analysis scripts (e.g. collecting info on bailouts and compilation info e.g. # slots used for JSX) as we want to compile-as-much-as-possible in those.

Mofei Zhang committed Sep 14, 2023 at 20:00 UTC f993f0c2a55e3fb106844c4d68362cef876d1156
4 files changed +112 -136
compiler/packages/babel-plugin-react-forget/src/Entrypoint/Options.ts
+15 -5
@@ -25,6 +25,19 @@ export type InstrumentForgetOptions = {
25 instrumentFn: ExternalFunction;
26 };
27
28 +export type PanicThresholdOptions =
29 + // Any errors will panic the compiler by throwing an exception, which will
30 + // bubble up to the nearest exception handler above the Forget transform.
31 + // If Forget is invoked through `ReactForgetBabelPlugin`, this will at the least
32 + // skip Forget compilation for the rest of current file.
33 + | "ALL_ERRORS"
34 + // Panic by throwing an exception only on critical or unrecognized errors.
35 + // For all other errors, skip the erroring function without inserting
36 + // a Forget-compiled version (i.e. same behavior as noEmit).
37 + | "CRITICAL_ERRORS"
38 + // Never panic by throwing an exception.
39 + | "NONE";
40 +
41 export type PluginOptions = {
42 environment: EnvironmentConfig | null;
43
@@ -73,9 +86,7 @@ export type PluginOptions = {
86 */
87 instrumentForget: ExternalFunction | null;
88
76 - panicOnBailout: boolean;
77 -
78 - isDev: boolean;
89 + panicThreshold: PanicThresholdOptions;
90
91 /**
92 * When enabled, Forget will continue statically analyzing and linting code, but skip over codegen
@@ -139,11 +150,10 @@ export type Logger = {
150
151 export const defaultOptions: PluginOptions = {
152 compilationMode: "infer",
142 - panicOnBailout: true,
153 + panicThreshold: "CRITICAL_ERRORS",
154 environment: null,
155 logger: null,
156 gating: null,
146 - isDev: false,
157 instrumentForget: null,
158 noEmit: false,
159 } as const;
compiler/packages/babel-plugin-react-forget/src/Entrypoint/Program.ts
+95 -124
@@ -50,6 +50,11 @@ function hasAnyUseNoForgetDirectives(directives: t.Directive[]): boolean {
50 }
51 return false;
52 }
53 +
54 +function isCriticalError(err: unknown): boolean {
55 + return !(err instanceof CompilerError) || err.isCritical();
56 +}
57 +
58 function handleError(
59 pass: CompilerPass,
60 fnLoc: t.SourceLocation | null,
@@ -65,10 +70,17 @@ function handleError(
70 });
71 }
72 } else {
73 + let stringifiedError;
74 + if (err instanceof Error) {
75 + stringifiedError = err.stack ?? err.message;
76 + } else {
77 + stringifiedError = err?.toString() ?? "[ null ]";
78 + }
79 +
80 pass.opts.logger.logEvent(pass.filename, {
81 kind: "PipelineError",
82 fnLoc,
71 - data: err,
83 + data: stringifiedError,
84 });
85 }
86 }
@@ -77,135 +89,89 @@ function handleError(
89 * {@link CompilerError.isCritical} for mappings.
90 * */
91 if (
80 - pass.opts.panicOnBailout ||
81 - !(err instanceof CompilerError) ||
82 - (err instanceof CompilerError && err.isCritical())
92 + pass.opts.panicThreshold === "ALL_ERRORS" ||
93 + (pass.opts.panicThreshold === "CRITICAL_ERRORS" && isCriticalError(err))
94 ) {
95 throw err;
85 - } else {
86 - if (pass.opts.isDev) {
87 - log(err, pass.filename ?? null);
88 - }
96 }
97 }
98
99 /**
93 - * Runs the Compiler pipeline and mutates the source AST to include the newly compiled function.
94 - * Returns a boolean denoting if the AST was mutated or not.
100 + * Mutates the source AST to include a newly Forget-compiled function.
101 */
96 -function compileAndInsertNewFunctionDeclaration(
97 - fnPath: NodePath<
102 +function insertNewFunctionDeclaration(
103 + originalFn: NodePath<
104 t.FunctionDeclaration | t.ArrowFunctionExpression | t.FunctionExpression
105 >,
106 + compiledFn: CodegenFunction,
107 pass: CompilerPass
101 -): boolean {
102 - if (ALREADY_COMPILED.has(fnPath.node)) {
103 - return false;
104 - }
105 -
106 - let compiledFn: CodegenFunction;
107 - try {
108 - compiledFn = compileFn(fnPath, pass.opts.environment);
109 - pass.opts.logger?.logEvent(pass.filename, {
110 - kind: "CompileSuccess",
111 - fnLoc: fnPath.node.loc ?? null,
112 - fnName: compiledFn.id?.name ?? null,
113 - memoSlots: compiledFn.memoSlotsUsed,
114 - });
115 - } catch (err) {
116 - handleError(pass, fnPath.node.loc ?? null, err);
117 - return false;
118 - }
119 -
120 - // Successfully compiled
121 - if (pass.opts.noEmit === true) {
122 - return false;
123 - }
124 -
125 - // We are generating a new FunctionDeclaration node, so we must skip over it or this
126 - // traversal will loop infinitely.
127 - fnPath.skip();
128 -
129 - let transformedFunction:
108 +): void {
109 + let transformedFn:
110 | t.FunctionDeclaration
111 | t.ArrowFunctionExpression
112 | t.FunctionExpression;
133 - switch (fnPath.node.type) {
113 + switch (originalFn.node.type) {
114 case "FunctionDeclaration": {
115 const fn: t.FunctionDeclaration = {
116 type: "FunctionDeclaration",
117 id: compiledFn.id,
138 - loc: fnPath.node.loc ?? null,
118 + loc: originalFn.node.loc ?? null,
119 async: compiledFn.async,
120 generator: compiledFn.generator,
121 params: compiledFn.params,
122 body: compiledFn.body,
123 };
144 - transformedFunction = fn;
124 + transformedFn = fn;
125 break;
126 }
127 case "ArrowFunctionExpression": {
128 const fn: t.ArrowFunctionExpression = {
129 type: "ArrowFunctionExpression",
150 - loc: fnPath.node.loc ?? null,
130 + loc: originalFn.node.loc ?? null,
131 async: compiledFn.async,
132 generator: compiledFn.generator,
133 params: compiledFn.params,
154 - expression: fnPath.node.expression,
134 + expression: originalFn.node.expression,
135 body: compiledFn.body,
136 };
157 - transformedFunction = fn;
137 + transformedFn = fn;
138 break;
139 }
140 case "FunctionExpression": {
141 const fn: t.FunctionExpression = {
142 type: "FunctionExpression",
143 id: compiledFn.id,
164 - loc: fnPath.node.loc ?? null,
144 + loc: originalFn.node.loc ?? null,
145 async: compiledFn.async,
146 generator: compiledFn.generator,
147 params: compiledFn.params,
148 body: compiledFn.body,
149 };
170 - transformedFunction = fn;
150 + transformedFn = fn;
151 break;
152 }
153 }
154
175 - // Ensure we avoid visiting the original function again (since we move it
176 - // within the AST in gating mode)
177 - ALREADY_COMPILED.add(fnPath);
178 - // And avoid visiting the new version as well
179 - ALREADY_COMPILED.add(transformedFunction);
180 -
181 - insertNewFunctionDeclaration(fnPath, transformedFunction, pass);
182 -
183 - return true;
184 -}
155 + // Avoid visiting the new transformed version
156 + ALREADY_COMPILED.add(transformedFn);
157
186 -function insertNewFunctionDeclaration(
187 - fnPath: NodePath<
188 - t.FunctionDeclaration | t.ArrowFunctionExpression | t.FunctionExpression
189 - >,
190 - compiledFn:
191 - | t.FunctionDeclaration
192 - | t.ArrowFunctionExpression
193 - | t.FunctionExpression,
194 - pass: CompilerPass
195 -): void {
158 if (pass.opts.instrumentForget != null) {
159 const instrumentFnName = pass.opts.instrumentForget.importSpecifierName;
198 - addInstrumentForget(compiledFn, instrumentFnName);
160 + addInstrumentForget(transformedFn, instrumentFnName);
161 }
162 if (pass.opts)
163 if (pass.opts.gating != null) {
164 if (pass.opts.instrumentForget != null) {
165 const instrumentFnName = pass.opts.instrumentForget.importSpecifierName;
204 - addInstrumentForget(fnPath.node, instrumentFnName);
166 + addInstrumentForget(originalFn.node, instrumentFnName);
167 }
206 - insertGatedFunctionDeclaration(fnPath, compiledFn, pass.opts.gating);
168 + insertGatedFunctionDeclaration(
169 + originalFn,
170 + transformedFn,
171 + pass.opts.gating
172 + );
173 } else {
208 - fnPath.replaceWith(compiledFn);
174 + originalFn.replaceWith(transformedFn);
175 }
176 }
177
@@ -263,9 +229,62 @@ export function compileProgram(
229 pass: CompilerPass
230 ): void {
231 const options = parsePluginOptions(pass.opts);
232 + // Record lint errors and critical errors as depending on Forget's config,
233 + // we may still need to run Forget's analysis on every function (even if we
234 + // have already encountered errors) for reporting.
235 const lintError = findEslintSuppressions(pass.comments);
236 + let hasCriticalError = lintError != null;
237 let hasForgetMutatedOriginalSource: boolean = false;
238
239 + const traverseFunction = (
240 + fn:
241 + | NodePath<t.FunctionDeclaration>
242 + | NodePath<t.FunctionExpression>
243 + | NodePath<t.ArrowFunctionExpression>,
244 + pass: CompilerPass
245 + ): void => {
246 + if (!shouldVisitNode(fn, pass) || ALREADY_COMPILED.has(fn.node)) {
247 + return;
248 + }
249 +
250 + // We may be generating a new FunctionDeclaration node, so we must skip over it or this
251 + // traversal will loop infinitely.
252 + // Ensure we avoid visiting the original function again.
253 + ALREADY_COMPILED.add(fn.node);
254 + fn.skip();
255 +
256 + if (lintError != null) {
257 + // Report lint suppressions as InvalidReact if we find forget-able
258 + // functions within the file
259 + handleError(pass, fn.node.loc ?? null, lintError);
260 + }
261 +
262 + let compiledFn: CodegenFunction;
263 + try {
264 + compiledFn = compileFn(fn, pass.opts.environment);
265 + pass.opts.logger?.logEvent(pass.filename, {
266 + kind: "CompileSuccess",
267 + fnLoc: fn.node.loc ?? null,
268 + fnName: compiledFn.id?.name ?? null,
269 + memoSlots: compiledFn.memoSlotsUsed,
270 + });
271 + } catch (err) {
272 + handleError(pass, fn.node.loc ?? null, err);
273 +
274 + hasCriticalError ||= isCriticalError(err);
275 + return;
276 + }
277 +
278 + if (pass.opts.noEmit) {
279 + return;
280 + } else if (!hasCriticalError) {
281 + // Only insert Forget-ified functions if we have not encountered a critical
282 + // error elsewhere in the file, regardless of bailout mode.
283 + insertNewFunctionDeclaration(fn, compiledFn, pass);
284 + hasForgetMutatedOriginalSource = true;
285 + }
286 + };
287 +
288 // Main traversal to compile with Forget
289 program.traverse(
290 {
@@ -283,47 +302,11 @@ export function compileProgram(
302 return;
303 },
304
286 - FunctionDeclaration(
287 - fn: NodePath<t.FunctionDeclaration>,
288 - pass: CompilerPass
289 - ): void {
290 - if (!shouldVisitNode(fn, pass)) {
291 - return;
292 - } else if (lintError != null) {
293 - handleError(pass, fn.node.loc ?? null, lintError);
294 - } else {
295 - const hasMutated = compileAndInsertNewFunctionDeclaration(fn, pass);
296 - hasForgetMutatedOriginalSource ||= hasMutated;
297 - }
298 - },
305 + FunctionDeclaration: traverseFunction,
306
300 - FunctionExpression(
301 - fn: NodePath<t.FunctionExpression>,
302 - pass: CompilerPass
303 - ): void {
304 - if (!shouldVisitNode(fn, pass)) {
305 - return;
306 - } else if (lintError != null) {
307 - handleError(pass, fn.node.loc ?? null, lintError);
308 - } else {
309 - const hasMutated = compileAndInsertNewFunctionDeclaration(fn, pass);
310 - hasForgetMutatedOriginalSource ||= hasMutated;
311 - }
312 - },
307 + FunctionExpression: traverseFunction,
308
314 - ArrowFunctionExpression(
315 - fn: NodePath<t.ArrowFunctionExpression>,
316 - pass: CompilerPass
317 - ): void {
318 - if (!shouldVisitNode(fn, pass)) {
319 - return;
320 - } else if (lintError != null) {
321 - handleError(pass, fn.node.loc ?? null, lintError);
322 - } else {
323 - const hasMutated = compileAndInsertNewFunctionDeclaration(fn, pass);
324 - hasForgetMutatedOriginalSource ||= hasMutated;
325 - }
326 - },
309 + ArrowFunctionExpression: traverseFunction,
310 },
311 {
312 ...pass,
@@ -409,18 +392,6 @@ function shouldVisitNode(
392 }
393 }
394
412 -function log(error: CompilerError, filename: string | null): void {
413 - const filenameStr = filename ? `in ${filename}` : "";
414 - console.log(
415 - error.details
416 - .map(
417 - (e) =>
418 - `[ReactForget] Skipping compilation of component ${filenameStr}: ${e.printErrorMessage()}`
419 - )
420 - .join("\n")
421 - );
422 -}
423 -
395 function isHookName(s: string): boolean {
396 return /^use[A-Z0-9]/.test(s);
397 }
compiler/packages/eslint-plugin-react-forget/src/rules/ReactForgetDiagnostics.ts
+1 -1
@@ -58,7 +58,7 @@ function isReportableDiagnostic(
58 const COMPILER_OPTIONS: Partial<PluginOptions> = {
59 noEmit: true,
60 compilationMode: "annotation",
61 - panicOnBailout: false,
61 + panicThreshold: "CRITICAL_ERRORS",
62 environment: {
63 validateHooksUsage: true,
64 validateFrozenLambdas: false,
compiler/packages/fixture-test-utils/src/compiler-utils.ts
+1 -6
@@ -19,7 +19,6 @@ export function transformFixtureInput(
19 let language = parseLanguage(firstLine);
20 let gating = null;
21 let instrumentForget = null;
22 - let panicOnBailout = true;
22 let memoizeJsxElements = true;
23 let enableAssumeHooksFollowRulesOfReact = false;
24 let enableTreatHooksAsFunctions = true;
@@ -57,9 +56,6 @@ export function transformFixtureInput(
56 importSpecifierName: "useRenderCounter",
57 };
58 }
60 - if (firstLine.includes("@panicOnBailout false")) {
61 - panicOnBailout = false;
62 - }
59 if (firstLine.includes("@memoizeJsxElements false")) {
60 memoizeJsxElements = false;
61 }
@@ -121,8 +117,7 @@ export function transformFixtureInput(
117 logger: null,
118 gating,
119 instrumentForget,
124 - panicOnBailout,
125 - isDev: true,
120 + panicThreshold: 'ALL_ERRORS',
121 noEmit: false,
122 },
123 includeAst