[Babel] Fix up eslint suppression logic
Babel doesn't attach Comment nodes to anything, so they dangle off of the Program node while only specifying a range. This meant that previously we first had to traverse all of the Program's comments to find an eslint suppression of the rules of React, then during traversal of the individual functions, we would check if there were any *global* eslint suppressions, then bailout all components. This PR updates our logic to determine if individual functions are affected by an eslint suppression range: - If an eslint suppression range falls within its body; or - If an eslint suppression wraps the function
Lauren Tan committed
Nov 17, 2023 at 10:22 UTC
6d101435d401d612c3a797fe88258b7c59fe7f49
10 files changed
+306
-81
compiler/packages/babel-plugin-react-forget/src/Entrypoint/EslintSuppression.ts
new
+162
@@ -0,0 +1,162 @@
1
+/*
2
+ * Copyright (c) Meta Platforms, Inc. and affiliates.
3
+ *
4
+ * This source code is licensed under the MIT license found in the
5
+ * LICENSE file in the root directory of this source tree.
6
+ */
7
+
8
+import { NodePath } from "@babel/core";
9
+import * as t from "@babel/types";
10
+import {
11
+ CompilerError,
12
+ CompilerErrorDetail,
13
+ CompilerSuggestionOperation,
14
+ ErrorSeverity,
15
+} from "../CompilerError";
16
+
17
+/**
18
+ * Captures the start and end range of a pair of eslint-disable ... eslint-enable comments. In the
19
+ * case of a CommentLine, both the disable and enable point to the same comment.
20
+ *
21
+ * The enable comment can be missing in the case where only a disable block is present, ie the rest
22
+ * of the file has potential React violations.
23
+ */
24
+export type EslintSuppressionRange = {
25
+ disableComment: t.Comment;
26
+ enableComment: t.Comment | null;
27
+};
28
+
29
+/**
30
+ * An eslint suppression affects a function if:
31
+ * 1. The suppression is within the function's body; or
32
+ * 2. The suppression wraps the function
33
+ */
34
+export function filterEslintSuppressionsThatAffectFunction(
35
+ suppressionRanges: Array<EslintSuppressionRange>,
36
+ fn: NodePath<t.Function>
37
+): Array<EslintSuppressionRange> {
38
+ const suppressionsInScope: Array<EslintSuppressionRange> = [];
39
+ const fnNode = fn.node;
40
+ for (const suppressionRange of suppressionRanges) {
41
+ if (
42
+ suppressionRange.disableComment.start == null ||
43
+ fnNode.start == null ||
44
+ fnNode.end == null
45
+ ) {
46
+ continue;
47
+ }
48
+ // The suppression is within the function
49
+ if (
50
+ suppressionRange.disableComment.start > fnNode.start &&
51
+ // If there is no matching enable, the rest of the file has potential violations
52
+ (suppressionRange.enableComment === null ||
53
+ (suppressionRange.enableComment.end != null &&
54
+ suppressionRange.enableComment.end < fnNode.end))
55
+ ) {
56
+ suppressionsInScope.push(suppressionRange);
57
+ }
58
+
59
+ // The suppression wraps the function
60
+ if (
61
+ suppressionRange.disableComment.start < fnNode.start &&
62
+ // If there is no matching enable, the rest of the file has potential violations
63
+ (suppressionRange.enableComment === null ||
64
+ (suppressionRange.enableComment.end != null &&
65
+ suppressionRange.enableComment.end > fnNode.end))
66
+ ) {
67
+ suppressionsInScope.push(suppressionRange);
68
+ }
69
+ }
70
+ return suppressionsInScope;
71
+}
72
+
73
+export function findProgramEslintSuppressions(
74
+ programComments: Array<t.Comment>
75
+): Array<EslintSuppressionRange> {
76
+ const suppressionRanges: Array<EslintSuppressionRange> = [];
77
+ let disableComment: t.Comment | null = null;
78
+ let enableComment: t.Comment | null = null;
79
+ for (const comment of programComments) {
80
+ if (comment.start == null || comment.end == null) {
81
+ continue;
82
+ }
83
+
84
+ if (
85
+ /*
86
+ * If we're already within a CommentBlock, we should not restart the range prematurely for a
87
+ * CommentLine within the block.
88
+ */
89
+ disableComment == null &&
90
+ /eslint-disable-next-line react-hooks\/(exhaustive-deps|rules-of-hooks)/.test(
91
+ comment.value
92
+ )
93
+ ) {
94
+ disableComment = comment;
95
+ enableComment = comment;
96
+ }
97
+
98
+ if (
99
+ /eslint-disable react-hooks\/(exhaustive-deps|rules-of-hooks)/.test(
100
+ comment.value
101
+ )
102
+ ) {
103
+ disableComment = comment;
104
+ }
105
+
106
+ if (
107
+ /eslint-enable react-hooks\/(exhaustive-deps|rules-of-hooks)/.test(
108
+ comment.value
109
+ )
110
+ ) {
111
+ enableComment = comment;
112
+ }
113
+
114
+ if (disableComment != null) {
115
+ suppressionRanges.push({
116
+ disableComment: disableComment,
117
+ enableComment: enableComment,
118
+ });
119
+ disableComment = null;
120
+ enableComment = null;
121
+ }
122
+ }
123
+ return suppressionRanges;
124
+}
125
+
126
+export function suppressionsToCompilerError(
127
+ suppressionRanges: Array<EslintSuppressionRange>
128
+): CompilerError | null {
129
+ if (suppressionRanges.length === 0) {
130
+ return null;
131
+ }
132
+ const reason =
133
+ "React Forget has bailed out of optimizing this component as one or more React eslint rules were disabled. React Forget only works when your components follow all the rules of React, disabling them may result in undefined behavior";
134
+ const error = new CompilerError();
135
+ for (const suppressionRange of suppressionRanges) {
136
+ if (
137
+ suppressionRange.disableComment.start == null ||
138
+ suppressionRange.disableComment.end == null
139
+ ) {
140
+ continue;
141
+ }
142
+ error.pushErrorDetail(
143
+ new CompilerErrorDetail({
144
+ reason,
145
+ description: suppressionRange.disableComment.value.trim(),
146
+ severity: ErrorSeverity.InvalidReact,
147
+ loc: suppressionRange.disableComment.loc ?? null,
148
+ suggestions: [
149
+ {
150
+ description: "Remove the eslint disable",
151
+ range: [
152
+ suppressionRange.disableComment.start,
153
+ suppressionRange.disableComment.end,
154
+ ],
155
+ op: CompilerSuggestionOperation.Remove,
156
+ },
157
+ ],
158
+ })
159
+ );
160
+ }
161
+ return error;
162
+}
compiler/packages/babel-plugin-react-forget/src/Entrypoint/Program.ts
+16
-50
@@ -10,7 +10,6 @@ import * as t from "@babel/types";
10
import {
11
CompilerError,
12
CompilerErrorDetail,
13
- CompilerSuggestionOperation,
13
ErrorSeverity,
14
} from "../CompilerError";
15
import {
@@ -21,6 +20,11 @@ import {
20
import { CodegenFunction } from "../ReactiveScopes";
21
import { isComponentDeclaration } from "../Utils/ComponentDeclaration";
22
import { assertExhaustive } from "../Utils/utils";
23
+import {
24
+ filterEslintSuppressionsThatAffectFunction,
25
+ findProgramEslintSuppressions,
26
+ suppressionsToCompilerError,
27
+} from "./EslintSuppression";
28
import { insertGatedFunctionDeclaration } from "./Gating";
29
import { addImportsToProgram, updateUseMemoCacheImport } from "./Imports";
30
import { addInstrumentForget } from "./Instrumentation";
@@ -163,50 +167,6 @@ function createNewFunctionNode(
167
return transformedFn;
168
}
169
166
-function findEslintSuppressions(
167
- fileComments: Array<t.CommentBlock | t.CommentLine>
168
-): CompilerError | null {
169
- const violations: Array<t.CommentBlock | t.CommentLine> = [];
170
-
171
- if (Array.isArray(fileComments)) {
172
- for (const comment of fileComments) {
173
- if (
174
- /eslint-disable(-next-line)? react-hooks\/(exhaustive-deps|rules-of-hooks)/.test(
175
- comment.value
176
- )
177
- ) {
178
- violations.push(comment);
179
- }
180
- }
181
- }
182
-
183
- if (violations.length > 0) {
184
- const reason =
185
- "React Forget has bailed out of optimizing this component as one or more React eslint rules were disabled. React Forget only works when your components follow all the rules of React, disabling them may result in undefined behavior";
186
- const error = new CompilerError();
187
- for (const violation of violations) {
188
- error.pushErrorDetail(
189
- new CompilerErrorDetail({
190
- reason,
191
- description: violation.value.trim(),
192
- severity: ErrorSeverity.InvalidReact,
193
- loc: violation.loc ?? null,
194
- suggestions: [
195
- {
196
- description: "Remove the eslint disable",
197
- range: [violation.start!, violation.end!],
198
- op: CompilerSuggestionOperation.Remove,
199
- },
200
- ],
201
- })
202
- );
203
- }
204
- return error;
205
- } else {
206
- return null;
207
- }
208
-}
209
-
170
/*
171
* This is a hack to work around what seems to be a Babel bug. Babel doesn't
172
* consistently respect the `skip()` function to avoid revisiting a node within
@@ -224,7 +184,8 @@ export function compileProgram(
184
* we may still need to run Forget's analysis on every function (even if we
185
* have already encountered errors) for reporting.
186
*/
227
- const lintError = findEslintSuppressions(pass.comments);
187
+ const eslintSuppressions = findProgramEslintSuppressions(pass.comments);
188
+ const lintError = suppressionsToCompilerError(eslintSuppressions);
189
let hasCriticalError = lintError != null;
190
const compiledFns: CompileResult[] = [];
191
@@ -242,11 +203,16 @@ export function compileProgram(
203
fn.skip();
204
205
if (lintError != null) {
245
- /*
246
- * Report lint suppressions as InvalidReact if we find forget-able
247
- * functions within the file
206
+ /**
207
+ * Note that Babel does not attach comment nodes to nodes; they are dangling off of the
208
+ * Program node itself. We need to figure out whether an eslint suppression range
209
+ * applies to this function first.
210
*/
249
- handleError(lintError, pass, fn.node.loc ?? null);
211
+ const eslintSuppressionsInFunction =
212
+ filterEslintSuppressionsThatAffectFunction(eslintSuppressions, fn);
213
+ if (eslintSuppressionsInFunction.length > 0) {
214
+ handleError(lintError, pass, fn.node.loc ?? null);
215
+ }
216
}
217
218
let compiledFn: CodegenFunction;
compiler/packages/babel-plugin-react-forget/src/Entrypoint/index.ts
+1
@@ -5,6 +5,7 @@
5
* LICENSE file in the root directory of this source tree.
6
*/
7
8
+export * from "./EslintSuppression";
9
export * from "./Gating";
10
export * from "./Imports";
11
export * from "./Instrumentation";
compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.bug-use-no-forget-multiple-with-eslint-suppression.expect.md
deleted
-30
@@ -1,30 +0,0 @@
1
-
2
-## Input
3
-
4
-```javascript
5
-const useControllableState = (options) => {};
6
-function NoopComponent() {}
7
-
8
-function Component() {
9
- "use no forget";
10
- const ref = useRef(null);
11
- // eslint-disable-next-line react-hooks/rules-of-hooks
12
- ref.current = "bad";
13
- return <MyButton ref={ref} />;
14
-}
15
-
16
-export const FIXTURE_ENTRYPOINT = {
17
- fn: Component,
18
- params: [],
19
-};
20
-
21
-```
22
-
23
-
24
-## Error
25
-
26
-```
27
-[ReactForget] InvalidReact: React Forget has bailed out of optimizing this component as one or more React eslint rules were disabled. React Forget only works when your components follow all the rules of React, disabling them may result in undefined behavior. eslint-disable-next-line react-hooks/rules-of-hooks (7:7)
28
-```
29
-
30
-
\ No newline at end of file
compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.invalid-sketchy-code-use-forget.expect.md
+3
@@ -6,6 +6,7 @@
6
function lowercasecomponent() {
7
"use forget";
8
const x = [];
9
+ // eslint-disable-next-line react-hooks/rules-of-hooks
10
return <div>{x}</div>;
11
}
12
/* eslint-enable react-hooks/rules-of-hooks */
@@ -17,6 +18,8 @@ function lowercasecomponent() {
18
19
```
20
[ReactForget] InvalidReact: React Forget has bailed out of optimizing this component as one or more React eslint rules were disabled. React Forget only works when your components follow all the rules of React, disabling them may result in undefined behavior. eslint-disable react-hooks/rules-of-hooks (1:1)
21
+
22
+[ReactForget] InvalidReact: React Forget has bailed out of optimizing this component as one or more React eslint rules were disabled. React Forget only works when your components follow all the rules of React, disabling them may result in undefined behavior. eslint-disable-next-line react-hooks/rules-of-hooks (5:5)
23
```
24
25
\ No newline at end of file
compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.invalid-sketchy-code-use-forget.js
+1
@@ -2,6 +2,7 @@
2
function lowercasecomponent() {
3
"use forget";
4
const x = [];
5
+ // eslint-disable-next-line react-hooks/rules-of-hooks
6
return <div>{x}</div>;
7
}
8
/* eslint-enable react-hooks/rules-of-hooks */
compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.invalid-unclosed-eslint-suppression.expect.md
new
+44
@@ -0,0 +1,44 @@
1
+
2
+## Input
3
+
4
+```javascript
5
+// Note: Everything below this is sketchy
6
+/* eslint-disable react-hooks/rules-of-hooks */
7
+function lowercasecomponent() {
8
+ "use forget";
9
+ const x = [];
10
+ return <div>{x}</div>;
11
+}
12
+
13
+function Haunted() {
14
+ return <div>This entire file is haunted oOoOo</div>;
15
+}
16
+
17
+function CrimesAgainstReact() {
18
+ let x = React.useMemo(async () => {
19
+ await a;
20
+ }, []);
21
+
22
+ class MyAmazingInnerComponent {
23
+ render() {
24
+ return <div>Why would you do this</div>;
25
+ }
26
+ }
27
+
28
+ // Note: This shouldn't reset the eslint suppression to just this line
29
+ // eslint-disable-next-line react-hooks/rules-of-hooks
30
+ return <MyAmazingInnerComponent />;
31
+}
32
+
33
+```
34
+
35
+
36
+## Error
37
+
38
+```
39
+[ReactForget] InvalidReact: React Forget has bailed out of optimizing this component as one or more React eslint rules were disabled. React Forget only works when your components follow all the rules of React, disabling them may result in undefined behavior. eslint-disable react-hooks/rules-of-hooks (2:2)
40
+
41
+[ReactForget] InvalidReact: React Forget has bailed out of optimizing this component as one or more React eslint rules were disabled. React Forget only works when your components follow all the rules of React, disabling them may result in undefined behavior. eslint-disable-next-line react-hooks/rules-of-hooks (25:25)
42
+```
43
+
44
+
\ No newline at end of file
compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.invalid-unclosed-eslint-suppression.js
new
+27
@@ -0,0 +1,27 @@
1
+// Note: Everything below this is sketchy
2
+/* eslint-disable react-hooks/rules-of-hooks */
3
+function lowercasecomponent() {
4
+ "use forget";
5
+ const x = [];
6
+ return <div>{x}</div>;
7
+}
8
+
9
+function Haunted() {
10
+ return <div>This entire file is haunted oOoOo</div>;
11
+}
12
+
13
+function CrimesAgainstReact() {
14
+ let x = React.useMemo(async () => {
15
+ await a;
16
+ }, []);
17
+
18
+ class MyAmazingInnerComponent {
19
+ render() {
20
+ return <div>Why would you do this</div>;
21
+ }
22
+ }
23
+
24
+ // Note: This shouldn't reset the eslint suppression to just this line
25
+ // eslint-disable-next-line react-hooks/rules-of-hooks
26
+ return <MyAmazingInnerComponent />;
27
+}
compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/use-no-forget-multiple-with-eslint-suppression.expect.md
new
+49
@@ -0,0 +1,49 @@
1
+
2
+## Input
3
+
4
+```javascript
5
+import { useRef } from "react";
6
+
7
+const useControllableState = (options) => {};
8
+function NoopComponent() {}
9
+
10
+function Component() {
11
+ "use no forget";
12
+ const ref = useRef(null);
13
+ // eslint-disable-next-line react-hooks/rules-of-hooks
14
+ ref.current = "bad";
15
+ return <button ref={ref} />;
16
+}
17
+
18
+export const FIXTURE_ENTRYPOINT = {
19
+ fn: Component,
20
+ params: [],
21
+};
22
+
23
+```
24
+
25
+## Code
26
+
27
+```javascript
28
+import { useRef } from "react";
29
+
30
+const useControllableState = (options) => {};
31
+function NoopComponent() {}
32
+
33
+function Component() {
34
+ "use no forget";
35
+ const ref = useRef(null);
36
+ // eslint-disable-next-line react-hooks/rules-of-hooks
37
+ ref.current = "bad";
38
+ return <button ref={ref} />;
39
+}
40
+
41
+export const FIXTURE_ENTRYPOINT = {
42
+ fn: Component,
43
+ params: [],
44
+};
45
+
46
+```
47
+
48
+### Eval output
49
+(kind: ok) <button></button>
\ No newline at end of file
compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/use-no-forget-multiple-with-eslint-suppression.js
renamed
+3
-1
@@ -1,3 +1,5 @@
1
+import { useRef } from "react";
2
+
3
const useControllableState = (options) => {};
4
function NoopComponent() {}
5
@@ -6,7 +8,7 @@ function Component() {
8
const ref = useRef(null);
9
// eslint-disable-next-line react-hooks/rules-of-hooks
10
ref.current = "bad";
9
- return <MyButton ref={ref} />;
11
+ return <button ref={ref} />;
12
}
13
14
export const FIXTURE_ENTRYPOINT = {