Prefer reporting conditional hook violations over "hook as value" violations
After running the latest hook validation internally, I found some cases where there was a violation but the error message was not ideal. For example on this code: ```javascript usePossiblyNullHook?.(); ``` We reported a "hooks can't be used as normal values" violation, when we'd ideally report a "hooks can't be called conditionally" violation. The solution in this PR is to track errors by source location, and upgrade the former violation to the latter, more serious violation. See fixtures for examples.
Joe Savona committed
Dec 12, 2023 at 16:42 UTC
a6e65e83e8b0fb8407509e994a85422a9d3f933d
15 files changed
+206
-22
compiler/packages/babel-plugin-react-forget/src/Validation/ValidateHooksUsage.ts
+42
-22
@@ -17,6 +17,7 @@ import {
17
HIRFunction,
18
IdentifierId,
19
Place,
20
+ SourceLocation,
21
getHookKind,
22
} from "../HIR/HIR";
23
import {
@@ -109,32 +110,47 @@ export function validateHooksUsage(fn: HIRFunction): void {
110
current = dominators.get(current);
111
}
112
112
- const errors = new CompilerError();
113
+ const errorsByPlace = new Map<SourceLocation, CompilerErrorDetail>();
114
function recordConditionalHookError(place: Place): void {
115
+ // Once a particular hook has a conditional call error, don't report any further issues for this hook
116
setKind(place, Kind.Error);
115
- errors.pushErrorDetail(
116
- new CompilerErrorDetail({
117
- description: null,
118
- reason:
119
- "Hooks must always be called in a consistent order, and may not be called conditionally. See the Rules of Hooks (https://react.dev/warnings/invalid-hook-call-warning)",
120
- loc: place.loc,
121
- severity: ErrorSeverity.InvalidReact,
122
- suggestions: null,
123
- })
124
- );
117
+
118
+ const reason =
119
+ "Hooks must always be called in a consistent order, and may not be called conditionally. See the Rules of Hooks (https://react.dev/warnings/invalid-hook-call-warning)";
120
+ const previousError = errorsByPlace.get(place.loc);
121
+
122
+ /*
123
+ * In some circumstances such as optional calls, we may first encounter a "hook may not be referenced as normal values" error.
124
+ * If that same place is also used as a conditional call, upgrade the error to a conditonal hook error
125
+ */
126
+ if (previousError === undefined || previousError.reason !== reason) {
127
+ errorsByPlace.set(
128
+ place.loc,
129
+ new CompilerErrorDetail({
130
+ description: null,
131
+ reason:
132
+ "Hooks must always be called in a consistent order, and may not be called conditionally. See the Rules of Hooks (https://react.dev/warnings/invalid-hook-call-warning)",
133
+ loc: place.loc,
134
+ severity: ErrorSeverity.InvalidReact,
135
+ suggestions: null,
136
+ })
137
+ );
138
+ }
139
}
140
function recordInvalidHookUsageError(place: Place): void {
127
- setKind(place, Kind.Error);
128
- errors.pushErrorDetail(
129
- new CompilerErrorDetail({
130
- description: null,
131
- reason:
132
- "Hooks may not be referenced as normal values, they must be called. See the Rules of Hooks (https://react.dev/warnings/invalid-hook-call-warning)",
133
- loc: typeof place.loc !== "symbol" ? place.loc : null,
134
- severity: ErrorSeverity.InvalidReact,
135
- suggestions: null,
136
- })
137
- );
141
+ if (!errorsByPlace.has(place.loc)) {
142
+ errorsByPlace.set(
143
+ place.loc,
144
+ new CompilerErrorDetail({
145
+ description: null,
146
+ reason:
147
+ "Hooks may not be referenced as normal values, they must be called. See the Rules of Hooks (https://react.dev/warnings/invalid-hook-call-warning)",
148
+ loc: place.loc,
149
+ severity: ErrorSeverity.InvalidReact,
150
+ suggestions: null,
151
+ })
152
+ );
153
+ }
154
}
155
156
const valueKinds = new Map<IdentifierId, Kind>();
@@ -374,6 +390,10 @@ export function validateHooksUsage(fn: HIRFunction): void {
390
}
391
}
392
393
+ const errors = new CompilerError();
394
+ for (const [, error] of errorsByPlace) {
395
+ errors.push(error);
396
+ }
397
if (errors.hasErrors()) {
398
throw errors;
399
}
compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.invalid-ternary-with-hook-values.expect.md
+4
@@ -16,6 +16,10 @@ function Component(props) {
16
[ReactForget] InvalidReact: Hooks may not be referenced as normal values, they must be called. See the Rules of Hooks (https://react.dev/warnings/invalid-hook-call-warning) (2:2)
17
18
[ReactForget] InvalidReact: Hooks may not be referenced as normal values, they must be called. See the Rules of Hooks (https://react.dev/warnings/invalid-hook-call-warning) (2:2)
19
+
20
+[ReactForget] InvalidReact: Hooks may not be referenced as normal values, they must be called. See the Rules of Hooks (https://react.dev/warnings/invalid-hook-call-warning) (2:2)
21
+
22
+[ReactForget] InvalidReact: Hooks may not be referenced as normal values, they must be called. See the Rules of Hooks (https://react.dev/warnings/invalid-hook-call-warning) (3:3)
23
```
24
25
\ No newline at end of file
compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.propertyload-hook.expect.md
+2
@@ -14,6 +14,8 @@ function Component() {
14
15
```
16
[ReactForget] InvalidReact: Hooks may not be referenced as normal values, they must be called. See the Rules of Hooks (https://react.dev/warnings/invalid-hook-call-warning) (2:2)
17
+
18
+[ReactForget] InvalidReact: Hooks may not be referenced as normal values, they must be called. See the Rules of Hooks (https://react.dev/warnings/invalid-hook-call-warning) (3:3)
19
```
20
21
\ No newline at end of file
compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/rules-of-hooks/error.invalid-call-phi-possibly-hook.expect.md
new
+28
@@ -0,0 +1,28 @@
1
+
2
+## Input
3
+
4
+```javascript
5
+function Component(props) {
6
+ // This is a violation of using a hook as a normal value rule:
7
+ const getUser = props.cond ? useGetUser : emptyFunction;
8
+
9
+ // Ideally we would report a "conditional hook call" error here.
10
+ // It's an unconditional call, but the value may or may not be a hook.
11
+ // TODO: report a conditional hook call error here
12
+ return getUser();
13
+}
14
+
15
+```
16
+
17
+
18
+## Error
19
+
20
+```
21
+[ReactForget] InvalidReact: Hooks may not be referenced as normal values, they must be called. See the Rules of Hooks (https://react.dev/warnings/invalid-hook-call-warning) (3:3)
22
+
23
+[ReactForget] InvalidReact: Hooks may not be referenced as normal values, they must be called. See the Rules of Hooks (https://react.dev/warnings/invalid-hook-call-warning) (3:3)
24
+
25
+[ReactForget] InvalidReact: Hooks may not be referenced as normal values, they must be called. See the Rules of Hooks (https://react.dev/warnings/invalid-hook-call-warning) (8:8)
26
+```
27
+
28
+
\ No newline at end of file
compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/rules-of-hooks/error.invalid-call-phi-possibly-hook.js
new
+9
@@ -0,0 +1,9 @@
1
+function Component(props) {
2
+ // This is a violation of using a hook as a normal value rule:
3
+ const getUser = props.cond ? useGetUser : emptyFunction;
4
+
5
+ // Ideally we would report a "conditional hook call" error here.
6
+ // It's an unconditional call, but the value may or may not be a hook.
7
+ // TODO: report a conditional hook call error here
8
+ return getUser();
9
+}
compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/rules-of-hooks/error.invalid-hook-as-conditional-test.expect.md
new
+19
@@ -0,0 +1,19 @@
1
+
2
+## Input
3
+
4
+```javascript
5
+function Component(props) {
6
+ const x = props.cond ? (useFoo ? 1 : 2) : 3;
7
+ return x;
8
+}
9
+
10
+```
11
+
12
+
13
+## Error
14
+
15
+```
16
+[ReactForget] InvalidReact: Hooks may not be referenced as normal values, they must be called. See the Rules of Hooks (https://react.dev/warnings/invalid-hook-call-warning) (2:2)
17
+```
18
+
19
+
\ No newline at end of file
compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/rules-of-hooks/error.invalid-hook-as-conditional-test.js
new
+4
@@ -0,0 +1,4 @@
1
+function Component(props) {
2
+ const x = props.cond ? (useFoo ? 1 : 2) : 3;
3
+ return x;
4
+}
compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/rules-of-hooks/error.invalid-hook-optional-methodcall.expect.md
new
+19
@@ -0,0 +1,19 @@
1
+
2
+## Input
3
+
4
+```javascript
5
+function Component() {
6
+ const { result } = Module.useConditionalHook?.() ?? {};
7
+ return result;
8
+}
9
+
10
+```
11
+
12
+
13
+## Error
14
+
15
+```
16
+[ReactForget] InvalidReact: Hooks must always be called in a consistent order, and may not be called conditionally. See the Rules of Hooks (https://react.dev/warnings/invalid-hook-call-warning) (2:2)
17
+```
18
+
19
+
\ No newline at end of file
compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/rules-of-hooks/error.invalid-hook-optional-methodcall.js
new
+4
@@ -0,0 +1,4 @@
1
+function Component() {
2
+ const { result } = Module.useConditionalHook?.() ?? {};
3
+ return result;
4
+}
compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/rules-of-hooks/error.invalid-hook-optional-property.expect.md
new
+19
@@ -0,0 +1,19 @@
1
+
2
+## Input
3
+
4
+```javascript
5
+function Component() {
6
+ const { result } = Module?.useConditionalHook() ?? {};
7
+ return result;
8
+}
9
+
10
+```
11
+
12
+
13
+## Error
14
+
15
+```
16
+[ReactForget] InvalidReact: Hooks must always be called in a consistent order, and may not be called conditionally. See the Rules of Hooks (https://react.dev/warnings/invalid-hook-call-warning) (2:2)
17
+```
18
+
19
+
\ No newline at end of file
compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/rules-of-hooks/error.invalid-hook-optional-property.js
new
+4
@@ -0,0 +1,4 @@
1
+function Component() {
2
+ const { result } = Module?.useConditionalHook() ?? {};
3
+ return result;
4
+}
compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/rules-of-hooks/error.invalid-hook-optionalcall.expect.md
new
+19
@@ -0,0 +1,19 @@
1
+
2
+## Input
3
+
4
+```javascript
5
+function Component() {
6
+ const { result } = useConditionalHook?.() ?? {};
7
+ return result;
8
+}
9
+
10
+```
11
+
12
+
13
+## Error
14
+
15
+```
16
+[ReactForget] InvalidReact: Hooks must always be called in a consistent order, and may not be called conditionally. See the Rules of Hooks (https://react.dev/warnings/invalid-hook-call-warning) (2:2)
17
+```
18
+
19
+
\ No newline at end of file
compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/rules-of-hooks/error.invalid-hook-optionalcall.js
new
+4
@@ -0,0 +1,4 @@
1
+function Component() {
2
+ const { result } = useConditionalHook?.() ?? {};
3
+ return result;
4
+}
compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/rules-of-hooks/error.invalid-hook-reassigned-in-conditional.expect.md
new
+24
@@ -0,0 +1,24 @@
1
+
2
+## Input
3
+
4
+```javascript
5
+function Component(props) {
6
+ let y;
7
+ props.cond ? (y = useFoo) : null;
8
+ return y();
9
+}
10
+
11
+```
12
+
13
+
14
+## Error
15
+
16
+```
17
+[ReactForget] InvalidReact: Hooks may not be referenced as normal values, they must be called. See the Rules of Hooks (https://react.dev/warnings/invalid-hook-call-warning) (3:3)
18
+
19
+[ReactForget] InvalidReact: Hooks may not be referenced as normal values, they must be called. See the Rules of Hooks (https://react.dev/warnings/invalid-hook-call-warning) (3:3)
20
+
21
+[ReactForget] InvalidReact: Hooks may not be referenced as normal values, they must be called. See the Rules of Hooks (https://react.dev/warnings/invalid-hook-call-warning) (4:4)
22
+```
23
+
24
+
\ No newline at end of file
compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/rules-of-hooks/error.invalid-hook-reassigned-in-conditional.js
new
+5
@@ -0,0 +1,5 @@
1
+function Component(props) {
2
+ let y;
3
+ props.cond ? (y = useFoo) : null;
4
+ return y();
5
+}