Compare commits

...

4 Commits

Author SHA1 Message Date
Joe Savona
5eda822159 [compiler] Alternate take on ref validation
Some of the false positives we've seen have to do with the need to align our ref validation with our understanding of which functions may be called during render. The new mutability/aliasing model makes this much more explicit, with the ability to create Impure effects which we then throw as errors if they are reachable during render. This means we can now revisit ref validation by just emitting impure effects.

That's what this new pass does. It's a bit simpler: it implements the check for `ref.current == null` guarded if blocks. Otherwise it disallows access to `ref.current` specifically. Unlike before, we intentionally allow passing ref objects to functions — we just see a lot of many false positives on disallowing things like `children({ref})` or similar.

Open to feedback! This is also still WIP.
2025-06-25 09:49:17 -07:00
Joe Savona
2de9ddac73 [compiler] Consolidate HIRFunction return information
We now have `HIRFunction.returns: Place` as well as `returnType: Type`. I want to add additional return information, so as a first step i'm consolidating everything under an object at `HIRFunction.returns: {place: Place}`. We use the type of this place as the return type. Next step is to add more properties to this object to represent things like the return kind.
2025-06-25 09:49:17 -07:00
Joe Savona
6ef9e5bd49 [compiler] Avoid empty switch cases
Small cosmetic win, found this when i was looking at some code internally with lots of cases that all share the same logic. Previously, all the but last one would have an empty block.
2025-06-25 09:49:17 -07:00
Joe Savona
440d8c2876 [compiler] Fix bug with reassigning function param in destructuring
Closes #33577, a bug with ExtractScopeDeclarationsFromDestructuring and codegen when a function param is reassigned.
2025-06-25 09:49:17 -07:00
30 changed files with 289 additions and 84 deletions

View File

@@ -285,7 +285,7 @@ function runWithEnvironment(
}
if (env.config.validateRefAccessDuringRender) {
validateNoRefAccessInRender(hir).unwrap();
// validateNoRefAccessInRender(hir).unwrap();
}
if (env.config.validateNoSetStateInRender) {

View File

@@ -221,7 +221,6 @@ export function lower(
params,
fnType: bindings == null ? env.fnType : 'Other',
returnTypeAnnotation: null, // TODO: extract the actual return type node if present
returnType: makeType(),
returns: createTemporaryPlace(env, func.node.loc ?? GeneratedSource),
body: builder.build(),
context,

View File

@@ -279,7 +279,6 @@ export type HIRFunction = {
env: Environment;
params: Array<Place | SpreadPattern>;
returnTypeAnnotation: t.FlowType | t.TSType | null;
returnType: Type;
returns: Place;
context: Array<Place>;
effects: Array<FunctionEffect> | null;

View File

@@ -54,6 +54,8 @@ export function printFunction(fn: HIRFunction): string {
let definition = '';
if (fn.id !== null) {
definition += fn.id;
} else {
definition += '<<anonymous>>';
}
if (fn.params.length !== 0) {
definition +=
@@ -71,10 +73,8 @@ export function printFunction(fn: HIRFunction): string {
} else {
definition += '()';
}
if (definition.length !== 0) {
output.push(definition);
}
output.push(`: ${printType(fn.returnType)} @ ${printPlace(fn.returns)}`);
definition += `: ${printPlace(fn.returns)}`;
output.push(definition);
output.push(...fn.directives);
output.push(printHIR(fn.body));
return output.join('\n');

View File

@@ -28,7 +28,9 @@ import {
isMapType,
isPrimitiveType,
isRefOrRefValue,
isRefValueType,
isSetType,
isUseRefType,
makeIdentifierId,
Phi,
Place,
@@ -219,6 +221,9 @@ export function inferMutationAliasingEffects(
}
}
}
if (fn.env.config.validateRefAccessDuringRender) {
inferRefAccessEffects(fn, isFunctionExpression);
}
return Ok(undefined);
}
@@ -2513,3 +2518,127 @@ export type AbstractValue = {
kind: ValueKind;
reason: ReadonlySet<ValueReason>;
};
function inferRefAccessEffects(
fn: HIRFunction,
_isFunctionExpression: boolean,
): void {
const nullish = new Set<IdentifierId>();
const nullishTest = new Map<IdentifierId, Place>();
let guard: {ref: IdentifierId; fallthrough: BlockId} | null = null;
const temporaries: Map<IdentifierId, Place> = new Map();
function visitOperand(operand: Place): AliasingEffect | null {
const nullTestRef = nullishTest.get(operand.identifier.id);
if (isRefValueType(operand.identifier) || nullTestRef != null) {
const refOperand = nullTestRef ?? operand;
return {
kind: 'Impure',
error: {
severity: ErrorSeverity.InvalidReact,
reason:
'Ref values (the `current` property) may not be accessed during render. (https://react.dev/reference/react/useRef)',
loc: refOperand.loc,
description:
refOperand.identifier.name !== null &&
refOperand.identifier.name.kind === 'named'
? `Cannot access ref value \`${refOperand.identifier.name.value}\``
: null,
suggestions: null,
},
place: refOperand,
};
}
return null;
}
for (const block of fn.body.blocks.values()) {
if (guard !== null && guard.fallthrough === block.id) {
guard = null;
}
for (const instr of block.instructions) {
const {lvalue, value} = instr;
if (value.kind === 'LoadLocal' && isUseRefType(value.place.identifier)) {
temporaries.set(lvalue.identifier.id, value.place);
} else if (
value.kind === 'StoreLocal' &&
isUseRefType(value.value.identifier)
) {
temporaries.set(value.lvalue.place.identifier.id, value.value);
temporaries.set(lvalue.identifier.id, value.value);
} else if (
value.kind === 'BinaryExpression' &&
((isRefValueType(value.left.identifier) &&
nullish.has(value.right.identifier.id)) ||
(nullish.has(value.left.identifier.id) &&
isRefValueType(value.right.identifier)))
) {
const refOperand = isRefValueType(value.left.identifier)
? value.left
: value.right;
const operand = temporaries.get(refOperand.identifier.id) ?? refOperand;
nullishTest.set(lvalue.identifier.id, operand);
} else if (value.kind === 'Primitive' && value.value == null) {
nullish.add(lvalue.identifier.id);
} else if (
value.kind === 'PropertyLoad' &&
isUseRefType(value.object.identifier) &&
value.property === 'current'
) {
const refOperand =
temporaries.get(value.object.identifier.id) ?? value.object;
temporaries.set(lvalue.identifier.id, refOperand);
} else if (
value.kind === 'PropertyStore' &&
value.property === 'current' &&
isUseRefType(value.object.identifier)
) {
const refOperand =
temporaries.get(value.object.identifier.id) ?? value.object;
if (guard != null && refOperand.identifier.id === guard.ref) {
// Allow a single write within the guard
guard = null;
} else {
instr.effects ??= [];
instr.effects.push({
kind: 'Impure',
error: {
severity: ErrorSeverity.InvalidReact,
reason:
'Ref values (the `current` property) may not be accessed during render. (https://react.dev/reference/react/useRef)',
loc: value.loc,
description:
value.object.identifier.name !== null &&
value.object.identifier.name.kind === 'named'
? `Cannot access ref value \`${value.object.identifier.name.value}\``
: null,
suggestions: null,
},
place: value.object,
});
}
} else {
for (const operand of eachInstructionValueOperand(value)) {
const error = visitOperand(operand);
if (error) {
instr.effects ??= [];
instr.effects.push(error);
}
}
}
}
if (
guard == null &&
block.terminal.kind === 'if' &&
nullishTest.has(block.terminal.test.identifier.id)
) {
const ref = nullishTest.get(block.terminal.test.identifier.id)!;
guard = {ref: ref.identifier.id, fallthrough: block.terminal.fallthrough};
} else {
for (const operand of eachTerminalOperand(block.terminal)) {
const _effect = visitOperand(operand);
// TODO: need a place to store terminal effects generically
}
}
}
}

View File

@@ -18,6 +18,7 @@ import {
ValueKind,
ValueReason,
Place,
isPrimitiveType,
} from '../HIR/HIR';
import {
eachInstructionLValue,
@@ -471,15 +472,15 @@ export function inferMutationAliasingRanges(
* Here we populate an effect to create the return value as well as populating alias/capture
* effects for how data flows between the params, context vars, and return.
*/
const returns = fn.returns.identifier;
functionEffects.push({
kind: 'Create',
into: fn.returns,
value:
fn.returnType.kind === 'Primitive'
? ValueKind.Primitive
: isJsxType(fn.returnType)
? ValueKind.Frozen
: ValueKind.Mutable,
value: isPrimitiveType(returns)
? ValueKind.Primitive
: isJsxType(returns.type)
? ValueKind.Frozen
: ValueKind.Mutable,
reason: ValueReason.KnownReturnSignature,
});
/**

View File

@@ -25,7 +25,6 @@ import {
makeBlockId,
makeInstructionId,
makePropertyLiteral,
makeType,
markInstructionIds,
promoteTemporary,
reversePostorderBlocks,
@@ -253,7 +252,6 @@ function emitSelectorFn(env: Environment, keys: Array<string>): Instruction {
env,
params: [obj],
returnTypeAnnotation: null,
returnType: makeType(),
returns: createTemporaryPlace(env, GeneratedSource),
context: [],
effects: null,

View File

@@ -21,7 +21,6 @@ import {
makeBlockId,
makeIdentifierName,
makeInstructionId,
makeType,
ObjectProperty,
Place,
promoteTemporary,
@@ -368,7 +367,6 @@ function emitOutlinedFn(
env,
params: [propsObj],
returnTypeAnnotation: null,
returnType: makeType(),
returns: createTemporaryPlace(env, GeneratedSource),
context: [],
effects: null,

View File

@@ -349,11 +349,9 @@ function codegenReactiveFunction(
fn: ReactiveFunction,
): Result<CodegenFunction, CompilerError> {
for (const param of fn.params) {
if (param.kind === 'Identifier') {
cx.temp.set(param.identifier.declarationId, null);
} else {
cx.temp.set(param.place.identifier.declarationId, null);
}
const place = param.kind === 'Identifier' ? param : param.place;
cx.temp.set(place.identifier.declarationId, null);
cx.declare(place.identifier);
}
const params = fn.params.map(param => convertParameter(param));
@@ -1183,7 +1181,7 @@ function codegenTerminal(
? codegenPlaceToExpression(cx, case_.test)
: null;
const block = codegenBlock(cx, case_.block!);
return t.switchCase(test, [block]);
return t.switchCase(test, block.body.length === 0 ? [] : [block]);
}),
);
}

View File

@@ -79,6 +79,10 @@ export function extractScopeDeclarationsFromDestructuring(
fn: ReactiveFunction,
): void {
const state = new State(fn.env);
for (const param of fn.params) {
const place = param.kind === 'Identifier' ? param : param.place;
state.declared.add(place.identifier.declarationId);
}
visitReactiveFunction(fn, new Visitor(), state);
}

View File

@@ -90,7 +90,8 @@ function apply(func: HIRFunction, unifier: Unifier): void {
}
}
}
func.returnType = unifier.get(func.returnType);
const returns = func.returns.identifier;
returns.type = unifier.get(returns.type);
}
type TypeEquation = {
@@ -143,12 +144,12 @@ function* generate(
}
}
if (returnTypes.length > 1) {
yield equation(func.returnType, {
yield equation(func.returns.identifier.type, {
kind: 'Phi',
operands: returnTypes,
});
} else if (returnTypes.length === 1) {
yield equation(func.returnType, returnTypes[0]!);
yield equation(func.returns.identifier.type, returnTypes[0]!);
}
}
@@ -407,7 +408,7 @@ function* generateInstructionTypes(
yield equation(left, {
kind: 'Function',
shapeId: BuiltInFunctionId,
return: value.loweredFunc.func.returnType,
return: value.loweredFunc.func.returns.identifier.type,
isConstructor: false,
});
break;

View File

@@ -50,8 +50,7 @@ function Component(props) {
console.log(handlers.value);
break bb0;
}
default: {
}
default:
}
t0 = handlers;

View File

@@ -67,8 +67,7 @@ function Component(props) {
case "b": {
break bb1;
}
case "c": {
}
case "c":
default: {
x = 6;
}

View File

@@ -24,8 +24,6 @@ export const FIXTURE_ENTRYPOINT = {
4 | const ref = useRef();
> 5 | useEffect(() => {}, [ref.current]);
| ^^^^^^^^^^^ InvalidReact: Ref values (the `current` property) may not be accessed during render. (https://react.dev/reference/react/useRef) (5:5)
InvalidReact: Ref values (the `current` property) may not be accessed during render. (https://react.dev/reference/react/useRef) (5:5)
6 | }
7 |
8 | export const FIXTURE_ENTRYPOINT = {

View File

@@ -19,6 +19,8 @@ function Component(props) {
3 | const ref = useRef(null);
> 4 | const value = ref.current;
| ^^^^^^^^^^^ InvalidReact: Ref values (the `current` property) may not be accessed during render. (https://react.dev/reference/react/useRef) (4:4)
InvalidReact: Ref values (the `current` property) may not be accessed during render. (https://react.dev/reference/react/useRef). Cannot access ref value `value` (5:5)
5 | return value;
6 | }
7 |

View File

@@ -19,12 +19,17 @@ function Component(props) {
## Error
```
7 | return <Foo item={item} current={current} />;
8 | };
> 9 | return <Items>{props.items.map(item => renderItem(item))}</Items>;
| ^^^^^^^^^^^^^^^^^^^^^^^^ InvalidReact: Ref values (the `current` property) may not be accessed during render. (https://react.dev/reference/react/useRef) (9:9)
10 | }
11 |
4 | const renderItem = item => {
5 | const aliasedRef = ref;
> 6 | const current = aliasedRef.current;
| ^^^^^^^^^^^^^^^^^^ InvalidReact: Ref values (the `current` property) may not be accessed during render. (https://react.dev/reference/react/useRef) (6:6)
InvalidReact: Ref values (the `current` property) may not be accessed during render. (https://react.dev/reference/react/useRef). Cannot access ref value `current` (7:7)
InvalidReact: Ref values (the `current` property) may not be accessed during render. (https://react.dev/reference/react/useRef) (7:7)
7 | return <Foo item={item} current={current} />;
8 | };
9 | return <Items>{props.items.map(item => renderItem(item))}</Items>;
```

View File

@@ -21,15 +21,13 @@ function Component() {
## Error
```
7 | };
8 | const changeRef = setRef;
> 9 | changeRef();
| ^^^^^^^^^ InvalidReact: This function accesses a ref value (the `current` property), which may not be accessed during render. (https://react.dev/reference/react/useRef) (9:9)
InvalidReact: Ref values (the `current` property) may not be accessed during render. (https://react.dev/reference/react/useRef) (9:9)
10 |
11 | return <button ref={ref} />;
12 | }
4 |
5 | const setRef = () => {
> 6 | ref.current = false;
| ^^^^^^^^^^^ InvalidReact: Ref values (the `current` property) may not be accessed during render. (https://react.dev/reference/react/useRef) (6:6)
7 | };
8 | const changeRef = setRef;
9 | changeRef();
```

View File

@@ -18,6 +18,10 @@ function Component({ref}) {
2 | function Component({ref}) {
> 3 | const value = ref.current;
| ^^^^^^^^^^^ InvalidReact: Ref values (the `current` property) may not be accessed during render. (https://react.dev/reference/react/useRef) (3:3)
InvalidReact: Ref values (the `current` property) may not be accessed during render. (https://react.dev/reference/react/useRef). Cannot access ref value `value` (4:4)
InvalidReact: Ref values (the `current` property) may not be accessed during render. (https://react.dev/reference/react/useRef) (4:4)
4 | return <div>{value}</div>;
5 | }
6 |

View File

@@ -18,6 +18,10 @@ function Component(props) {
2 | function Component(props) {
> 3 | const value = props.ref.current;
| ^^^^^^^^^^^^^^^^^ InvalidReact: Ref values (the `current` property) may not be accessed during render. (https://react.dev/reference/react/useRef) (3:3)
InvalidReact: Ref values (the `current` property) may not be accessed during render. (https://react.dev/reference/react/useRef). Cannot access ref value `value` (4:4)
InvalidReact: Ref values (the `current` property) may not be accessed during render. (https://react.dev/reference/react/useRef) (4:4)
4 | return <div>{value}</div>;
5 | }
6 |

View File

@@ -18,12 +18,17 @@ function Component(props) {
## Error
```
6 | return <Foo item={item} current={current} />;
7 | };
> 8 | return <Items>{props.items.map(item => renderItem(item))}</Items>;
| ^^^^^^^^^^^^^^^^^^^^^^^^ InvalidReact: Ref values (the `current` property) may not be accessed during render. (https://react.dev/reference/react/useRef) (8:8)
9 | }
10 |
3 | const ref = useRef(null);
4 | const renderItem = item => {
> 5 | const current = ref.current;
| ^^^^^^^^^^^ InvalidReact: Ref values (the `current` property) may not be accessed during render. (https://react.dev/reference/react/useRef) (5:5)
InvalidReact: Ref values (the `current` property) may not be accessed during render. (https://react.dev/reference/react/useRef). Cannot access ref value `current` (6:6)
InvalidReact: Ref values (the `current` property) may not be accessed during render. (https://react.dev/reference/react/useRef) (6:6)
6 | return <Foo item={item} current={current} />;
7 | };
8 | return <Items>{props.items.map(item => renderItem(item))}</Items>;
```

View File

@@ -19,8 +19,6 @@ function Component(props) {
3 | const ref = useRef(null);
> 4 | ref.current = props.value;
| ^^^^^^^^^^^ InvalidReact: Ref values (the `current` property) may not be accessed during render. (https://react.dev/reference/react/useRef) (4:4)
InvalidReact: Ref values (the `current` property) may not be accessed during render. (https://react.dev/reference/react/useRef) (5:5)
5 | return ref.current;
6 | }
7 |

View File

@@ -27,9 +27,9 @@ export const FIXTURE_ENTRYPOINT = {
4 | component C() {
5 | const r = useRef(null);
> 6 | const guard = r.current == null;
| ^^^^^^^^^^^^^^^^^ InvalidReact: Ref values (the `current` property) may not be accessed during render. (https://react.dev/reference/react/useRef) (6:6)
| ^ InvalidReact: Ref values (the `current` property) may not be accessed during render. (https://react.dev/reference/react/useRef). Cannot access ref value `r` (6:6)
InvalidReact: Ref values (the `current` property) may not be accessed during render. (https://react.dev/reference/react/useRef). Cannot access ref value `guard` (7:7)
InvalidReact: Ref values (the `current` property) may not be accessed during render. (https://react.dev/reference/react/useRef) (8:8)
7 | if (guard) {
8 | r.current = 1;
9 | }

View File

@@ -34,15 +34,13 @@ export const FIXTURE_ENTRYPOINT = {
## Error
```
15 | ref.current.inner = null;
13 | // The ref is modified later, extending its range and preventing memoization of onChange
14 | const reset = () => {
> 15 | ref.current.inner = null;
| ^^^^^^^^^^^ InvalidReact: Ref values (the `current` property) may not be accessed during render. (https://react.dev/reference/react/useRef) (15:15)
16 | };
> 17 | reset();
| ^^^^^ InvalidReact: This function accesses a ref value (the `current` property), which may not be accessed during render. (https://react.dev/reference/react/useRef) (17:17)
InvalidReact: Ref values (the `current` property) may not be accessed during render. (https://react.dev/reference/react/useRef) (17:17)
17 | reset();
18 |
19 | return <input onChange={onChange} />;
20 | }
```

View File

@@ -2,7 +2,7 @@
## Input
```javascript
// @validateRefAccessDuringRender false
// @validateRefAccessDuringRender:false
function VideoTab() {
const ref = useRef();
const t = ref.current;
@@ -18,7 +18,7 @@ function VideoTab() {
## Code
```javascript
import { c as _c } from "react/compiler-runtime"; // @validateRefAccessDuringRender false
import { c as _c } from "react/compiler-runtime"; // @validateRefAccessDuringRender:false
function VideoTab() {
const $ = _c(1);
const ref = useRef();

View File

@@ -1,4 +1,4 @@
// @validateRefAccessDuringRender false
// @validateRefAccessDuringRender:false
function VideoTab() {
const ref = useRef();
const t = ref.current;

View File

@@ -0,0 +1,65 @@
## Input
```javascript
import {Stringify, useIdentity} from 'shared-runtime';
function Component({other, ...props}, ref) {
[props, ref] = useIdentity([props, ref]);
return <Stringify props={props} />;
}
export const FIXTURE_ENTRYPOINT = {
fn: Component,
params: [{a: 0, b: 'hello', children: <div>Hello</div>}],
};
```
## Code
```javascript
import { c as _c } from "react/compiler-runtime";
import { Stringify, useIdentity } from "shared-runtime";
function Component(t0, ref) {
const $ = _c(7);
let props;
if ($[0] !== t0) {
let { other, ...t1 } = t0;
props = t1;
$[0] = t0;
$[1] = props;
} else {
props = $[1];
}
let t1;
if ($[2] !== props || $[3] !== ref) {
t1 = [props, ref];
$[2] = props;
$[3] = ref;
$[4] = t1;
} else {
t1 = $[4];
}
[props, ref] = useIdentity(t1);
let t2;
if ($[5] !== props) {
t2 = <Stringify props={props} />;
$[5] = props;
$[6] = t2;
} else {
t2 = $[6];
}
return t2;
}
export const FIXTURE_ENTRYPOINT = {
fn: Component,
params: [{ a: 0, b: "hello", children: <div>Hello</div> }],
};
```
### Eval output
(kind: ok) <div>{"props":{"a":0,"b":"hello","children":{"type":"div","key":null,"props":{"children":"Hello"},"_owner":"[[ cyclic ref *3 ]]","_store":{}}}}</div>

View File

@@ -0,0 +1,11 @@
import {Stringify, useIdentity} from 'shared-runtime';
function Component({other, ...props}, ref) {
[props, ref] = useIdentity([props, ref]);
return <Stringify props={props} />;
}
export const FIXTURE_ENTRYPOINT = {
fn: Component,
params: [{a: 0, b: 'hello', children: <div>Hello</div>}],
};

View File

@@ -50,10 +50,8 @@ function Component(props) {
case 1: {
break bb0;
}
case 2: {
}
default: {
}
case 2:
default:
}
} else {
if (props.cond2) {

View File

@@ -41,8 +41,7 @@ function foo() {
case 2: {
break bb0;
}
default: {
}
default:
}
}

View File

@@ -43,22 +43,17 @@ export const FIXTURE_ENTRYPOINT = {
```javascript
function foo(x) {
bb0: switch (x) {
case 0: {
}
case 1: {
}
case 0:
case 1:
case 2: {
break bb0;
}
case 3: {
break bb0;
}
case 4: {
}
case 5: {
}
default: {
}
case 4:
case 5:
default:
}
}