Skip to content

Commit 61b605f

Browse files
committed
[compiler] Add hint to name variables with "Ref" suffix
If you have a ref that the compiler doesn't know is a ref (say, a value returned from a custom hook) and try to assign its `.current = ...`, we currently fail with a generic error that hook return values are not mutable. However, an assignment to `.current` specifically is a very strong hint that the value is likely to be a ref. So in this PR, we track the reason for the mutation and if it ends up being an error, we use it to show an additional hint to the user. See the fixture for an example of the message.
1 parent fa992fd commit 61b605f

File tree

4 files changed

+107
-21
lines changed

4 files changed

+107
-21
lines changed

compiler/packages/babel-plugin-react-compiler/src/Inference/AliasingEffects.ts

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -50,7 +50,7 @@ export type AliasingEffect =
5050
/**
5151
* Mutate the value and any direct aliases (not captures). Errors if the value is not mutable.
5252
*/
53-
| {kind: 'Mutate'; value: Place}
53+
| {kind: 'Mutate'; value: Place; reason?: MutationReason | null}
5454
/**
5555
* Mutate the value and any direct aliases (not captures), but only if the value is known mutable.
5656
* This should be rare.
@@ -174,6 +174,8 @@ export type AliasingEffect =
174174
place: Place;
175175
};
176176

177+
export type MutationReason = {kind: 'AssignCurrentProperty'};
178+
177179
export function hashEffect(effect: AliasingEffect): string {
178180
switch (effect.kind) {
179181
case 'Apply': {

compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutationAliasingEffects.ts

Lines changed: 55 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -68,7 +68,12 @@ import {FunctionSignature} from '../HIR/ObjectShape';
6868
import {getWriteErrorReason} from './InferFunctionEffects';
6969
import prettyFormat from 'pretty-format';
7070
import {createTemporaryPlace} from '../HIR/HIRBuilder';
71-
import {AliasingEffect, AliasingSignature, hashEffect} from './AliasingEffects';
71+
import {
72+
AliasingEffect,
73+
AliasingSignature,
74+
hashEffect,
75+
MutationReason,
76+
} from './AliasingEffects';
7277

7378
const DEBUG = false;
7479

@@ -452,18 +457,29 @@ function applySignature(
452457
effect.value.identifier.name.kind === 'named'
453458
? `\`${effect.value.identifier.name.value}\``
454459
: 'value';
460+
const diagnostic = CompilerDiagnostic.create({
461+
severity: ErrorSeverity.InvalidReact,
462+
category: 'This value cannot be modified',
463+
description: `${reason}.`,
464+
}).withDetail({
465+
kind: 'error',
466+
loc: effect.value.loc,
467+
message: `${variable} cannot be modified`,
468+
});
469+
if (
470+
effect.kind === 'Mutate' &&
471+
effect.reason?.kind === 'AssignCurrentProperty'
472+
) {
473+
diagnostic.withDetail({
474+
kind: 'error',
475+
loc: effect.value.loc,
476+
message: `Hint: If this value is a Ref (value returned by \`useRef()\`), rename the variable to end in "Ref".`,
477+
});
478+
}
455479
effects.push({
456480
kind: 'MutateFrozen',
457481
place: effect.value,
458-
error: CompilerDiagnostic.create({
459-
severity: ErrorSeverity.InvalidReact,
460-
category: 'This value cannot be modified',
461-
description: `${reason}.`,
462-
}).withDetail({
463-
kind: 'error',
464-
loc: effect.value.loc,
465-
message: `${variable} cannot be modified`,
466-
}),
482+
error: diagnostic,
467483
});
468484
}
469485
}
@@ -1066,6 +1082,25 @@ function applyEffect(
10661082
effect.value.identifier.name.kind === 'named'
10671083
? `\`${effect.value.identifier.name.value}\``
10681084
: 'value';
1085+
const diagnostic = CompilerDiagnostic.create({
1086+
severity: ErrorSeverity.InvalidReact,
1087+
category: 'This value cannot be modified',
1088+
description: `${reason}.`,
1089+
}).withDetail({
1090+
kind: 'error',
1091+
loc: effect.value.loc,
1092+
message: `${variable} cannot be modified`,
1093+
});
1094+
if (
1095+
effect.kind === 'Mutate' &&
1096+
effect.reason?.kind === 'AssignCurrentProperty'
1097+
) {
1098+
diagnostic.withDetail({
1099+
kind: 'error',
1100+
loc: effect.value.loc,
1101+
message: `Hint: If this value is a Ref (value returned by \`useRef()\`), rename the variable to end in "Ref".`,
1102+
});
1103+
}
10691104
applyEffect(
10701105
context,
10711106
state,
@@ -1075,15 +1110,7 @@ function applyEffect(
10751110
? 'MutateFrozen'
10761111
: 'MutateGlobal',
10771112
place: effect.value,
1078-
error: CompilerDiagnostic.create({
1079-
severity: ErrorSeverity.InvalidReact,
1080-
category: 'This value cannot be modified',
1081-
description: `${reason}.`,
1082-
}).withDetail({
1083-
kind: 'error',
1084-
loc: effect.value.loc,
1085-
message: `${variable} cannot be modified`,
1086-
}),
1113+
error: diagnostic,
10871114
},
10881115
initialized,
10891116
effects,
@@ -1680,7 +1707,15 @@ function computeSignatureForInstruction(
16801707
}
16811708
case 'PropertyStore':
16821709
case 'ComputedStore': {
1683-
effects.push({kind: 'Mutate', value: value.object});
1710+
const mutationReason: MutationReason | null =
1711+
value.kind === 'PropertyStore' && value.property === 'current'
1712+
? {kind: 'AssignCurrentProperty'}
1713+
: null;
1714+
effects.push({
1715+
kind: 'Mutate',
1716+
value: value.object,
1717+
reason: mutationReason,
1718+
});
16841719
effects.push({
16851720
kind: 'Capture',
16861721
from: value.value,
Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,42 @@
1+
2+
## Input
3+
4+
```javascript
5+
// @flow
6+
7+
component Foo() {
8+
const foo = useFoo();
9+
foo.current = true;
10+
return <div />;
11+
}
12+
13+
```
14+
15+
16+
## Error
17+
18+
```
19+
Found 1 error:
20+
21+
Error: This value cannot be modified
22+
23+
Modifying a value returned from a hook is not allowed. Consider moving the modification into the hook where the value is constructed.
24+
25+
3 | component Foo() {
26+
4 | const foo = useFoo();
27+
> 5 | foo.current = true;
28+
| ^^^ value cannot be modified
29+
6 | return <div />;
30+
7 | }
31+
8 |
32+
33+
3 | component Foo() {
34+
4 | const foo = useFoo();
35+
> 5 | foo.current = true;
36+
| ^^^ Hint: If this value is a Ref (value returned by `useRef()`), rename the variable to end in "Ref".
37+
6 | return <div />;
38+
7 | }
39+
8 |
40+
```
41+
42+
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
// @flow
2+
3+
component Foo() {
4+
const foo = useFoo();
5+
foo.current = true;
6+
return <div />;
7+
}

0 commit comments

Comments
 (0)