Skip to content

Fix(Compiler): prevent PropertyLoad from aliasing parent object as ref #422

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
everettbu wants to merge 3 commits into from
Closed

Fix(Compiler): prevent PropertyLoad from aliasing parent object as ref #422

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
25995dc
Fix(Compiler): prevent PropertyLoad from aliasing parent object as ref
SHIVIKA330 Jan 26, 2026
50087df
Test: update snapshots for property load fix
SHIVIKA330 Jan 26, 2026
a097252
Fix: restore license and documentation comments
SHIVIKA330 Jan 26, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
138 changes: 51 additions & 87 deletions compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoRefAccessInRender.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,15 +21,15 @@ import {
isRefValueType,
isUseRefType,
} from '../HIR';
import {BuiltInEventHandlerId} from '../HIR/ObjectShape';
import { BuiltInEventHandlerId } from '../HIR/ObjectShape';
import {
eachInstructionOperand,
eachInstructionValueOperand,
eachPatternOperand,
eachTerminalOperand,
} from '../HIR/visitors';
import {Err, Ok, Result} from '../Utils/Result';
import {retainWhere} from '../Utils/utils';
import { Err, Ok, Result } from '../Utils/Result';
import { retainWhere } from '../Utils/utils';

/**
* Validates that a function does not access a ref value during render. This includes a partial check
Expand All @@ -53,7 +53,7 @@ import {retainWhere} from '../Utils/utils';
*/

const opaqueRefId = Symbol();
type RefId = number & {[opaqueRefId]: 'RefId'};
type RefId = number & { [opaqueRefId]: 'RefId' };

function makeRefId(id: number): RefId {
CompilerError.invariant(id >= 0 && Number.isInteger(id), {
Expand All @@ -76,17 +76,17 @@ function nextRefId(): RefId {
}

type RefAccessType =
| {kind: 'None'}
| {kind: 'Nullable'}
| {kind: 'Guard'; refId: RefId}
| { kind: 'None' }
| { kind: 'Nullable' }
| { kind: 'Guard'; refId: RefId }
| RefAccessRefType;

type RefAccessRefType =
| {kind: 'Ref'; refId: RefId}
| {kind: 'RefValue'; loc?: SourceLocation; refId?: RefId}
| {kind: 'Structure'; value: null | RefAccessRefType; fn: null | RefFnType};
| { kind: 'Ref'; refId: RefId }
| { kind: 'RefValue'; loc?: SourceLocation; refId?: RefId }
| { kind: 'Structure'; value: null | RefAccessRefType; fn: null | RefFnType };

type RefFnType = {readRefEffect: boolean; returnType: RefAccessType};
type RefFnType = { readRefEffect: boolean; returnType: RefAccessType };

class Env {
#changed = false;
Expand Down Expand Up @@ -117,7 +117,7 @@ class Env {
set(key: IdentifierId, value: RefAccessType): this {
const operandId = this.#temporaries.get(key)?.identifier.id ?? key;
const cur = this.#data.get(operandId);
const widenedValue = joinRefAccessTypes(value, cur ?? {kind: 'None'});
const widenedValue = joinRefAccessTypes(value, cur ?? { kind: 'None' });
if (
!(cur == null && widenedValue.kind === 'None') &&
(cur == null || !tyEqual(cur, widenedValue))
Expand All @@ -140,7 +140,7 @@ export function validateNoRefAccessInRender(
function collectTemporariesSidemap(fn: HIRFunction, env: Env): void {
for (const block of fn.body.blocks.values()) {
for (const instr of block.instructions) {
const {lvalue, value} = instr;
const { lvalue, value } = instr;
switch (value.kind) {
case 'LoadLocal': {
const temp = env.lookup(value.place);
Expand All @@ -157,31 +157,18 @@ function collectTemporariesSidemap(fn: HIRFunction, env: Env): void {
}
break;
}
case 'PropertyLoad': {
if (
isUseRefType(value.object.identifier) &&
value.property === 'current'
) {
continue;
}
const temp = env.lookup(value.object);
if (temp != null) {
env.define(lvalue, temp);
}
break;
}
}
}
}
}

function refTypeOfType(place: Place): RefAccessType {
if (isRefValueType(place.identifier)) {
return {kind: 'RefValue'};
return { kind: 'RefValue' };
} else if (isUseRefType(place.identifier)) {
return {kind: 'Ref', refId: nextRefId()};
return { kind: 'Ref', refId: nextRefId() };
} else {
return {kind: 'None'};
return { kind: 'None' };
}
}

Expand Down Expand Up @@ -263,14 +250,14 @@ function joinRefAccessTypes(...types: Array<RefAccessType>): RefAccessType {
if (b.kind === 'RefValue' && a.refId === b.refId) {
return a;
}
return {kind: 'RefValue'};
return { kind: 'RefValue' };
} else if (b.kind === 'RefValue') {
return b;
} else if (a.kind === 'Ref' || b.kind === 'Ref') {
if (a.kind === 'Ref' && b.kind === 'Ref' && a.refId === b.refId) {
return a;
}
return {kind: 'Ref', refId: nextRefId()};
return { kind: 'Ref', refId: nextRefId() };
} else {
CompilerError.invariant(
a.kind === 'Structure' && b.kind === 'Structure',
Expand All @@ -292,12 +279,12 @@ function joinRefAccessTypes(...types: Array<RefAccessType>): RefAccessType {
: b.fn === null
? a.fn
: {
readRefEffect: a.fn.readRefEffect || b.fn.readRefEffect,
returnType: joinRefAccessTypes(
a.fn.returnType,
b.fn.returnType,
),
};
readRefEffect: a.fn.readRefEffect || b.fn.readRefEffect,
returnType: joinRefAccessTypes(
a.fn.returnType,
b.fn.returnType,
),
};
const value =
a.value === null
? b.value
Expand All @@ -322,13 +309,13 @@ function joinRefAccessTypes(...types: Array<RefAccessType>): RefAccessType {
if (b.kind === 'Guard' && a.refId === b.refId) {
return a;
} else if (b.kind === 'Nullable' || b.kind === 'Guard') {
return {kind: 'None'};
return { kind: 'None' };
} else {
return b;
}
} else if (b.kind === 'Guard') {
if (a.kind === 'Nullable') {
return {kind: 'None'};
return { kind: 'None' };
} else {
return b;
}
Expand All @@ -340,7 +327,7 @@ function joinRefAccessTypes(...types: Array<RefAccessType>): RefAccessType {
return joinRefAccessRefTypes(a, b);
}
},
{kind: 'None'},
{ kind: 'None' },
);
}

Expand All @@ -363,7 +350,7 @@ function validateNoRefAccessInRenderImpl(
const interpolatedAsJsx = new Set<IdentifierId>();
for (const block of fn.body.blocks.values()) {
for (const instr of block.instructions) {
const {value} = instr;
const { value } = instr;
if (value.kind === 'JsxExpression' || value.kind === 'JsxFragment') {
if (value.children != null) {
for (const child of value.children) {
Expand All @@ -377,7 +364,7 @@ function validateNoRefAccessInRenderImpl(
for (let i = 0; (i == 0 || env.hasChanged()) && i < 10; i++) {
env.resetChanged();
returnValues = [];
const safeBlocks: Array<{block: BlockId; ref: RefId}> = [];
const safeBlocks: Array<{ block: BlockId; ref: RefId }> = [];
const errors = new CompilerError();
for (const [, block] of fn.body.blocks) {
retainWhere(safeBlocks, entry => entry.block !== block.id);
Expand All @@ -387,7 +374,7 @@ function validateNoRefAccessInRenderImpl(
joinRefAccessTypes(
...Array(...phi.operands.values()).map(
operand =>
env.get(operand.identifier.id) ?? ({kind: 'None'} as const),
env.get(operand.identifier.id) ?? ({ kind: 'None' } as const),
),
),
);
Expand Down Expand Up @@ -428,7 +415,7 @@ function validateNoRefAccessInRenderImpl(
env.set(
instr.lvalue.identifier.id,
env.get(instr.value.value.identifier.id) ??
refTypeOfType(instr.lvalue),
refTypeOfType(instr.lvalue),
);
break;
}
Expand All @@ -437,7 +424,7 @@ function validateNoRefAccessInRenderImpl(
env.set(
instr.lvalue.identifier.id,
env.get(instr.value.place.identifier.id) ??
refTypeOfType(instr.lvalue),
refTypeOfType(instr.lvalue),
);
break;
}
Expand All @@ -446,12 +433,12 @@ function validateNoRefAccessInRenderImpl(
env.set(
instr.value.lvalue.place.identifier.id,
env.get(instr.value.value.identifier.id) ??
refTypeOfType(instr.value.lvalue.place),
refTypeOfType(instr.value.lvalue.place),
);
env.set(
instr.lvalue.identifier.id,
env.get(instr.value.value.identifier.id) ??
refTypeOfType(instr.lvalue),
refTypeOfType(instr.lvalue),
);
break;
}
Expand All @@ -472,7 +459,7 @@ function validateNoRefAccessInRenderImpl(
}
case 'ObjectMethod':
case 'FunctionExpression': {
let returnType: RefAccessType = {kind: 'None'};
let returnType: RefAccessType = { kind: 'None' };
let readRefEffect = false;
const result = validateNoRefAccessInRenderImpl(
instr.value.loweredFunc.func,
Expand Down Expand Up @@ -500,7 +487,7 @@ function validateNoRefAccessInRenderImpl(
? instr.value.callee
: instr.value.property;
const hookKind = getHookKindForType(fn.env, callee.identifier.type);
let returnType: RefAccessType = {kind: 'None'};
let returnType: RefAccessType = { kind: 'None' };
const fnType = env.get(callee.identifier.id);
let didError = false;
if (fnType?.kind === 'Structure' && fnType.fn !== null) {
Expand All @@ -520,42 +507,23 @@ function validateNoRefAccessInRenderImpl(
);
}
}
/*
* If we already reported an error on this instruction, don't report
* duplicate errors
*/
if (!didError) {
const isRefLValue = isUseRefType(instr.lvalue.identifier);
const isEventHandlerLValue = isEventHandlerType(
instr.lvalue.identifier,
);
for (const operand of eachInstructionValueOperand(instr.value)) {
/**
* By default we check that function call operands are not refs,
* ref values, or functions that can access refs.
*/
if (
isRefLValue ||
isEventHandlerLValue ||
(hookKind != null &&
hookKind !== 'useState' &&
hookKind !== 'useReducer')
) {
/**
* Allow passing refs or ref-accessing functions when:
* 1. lvalue is a ref (mergeRefs pattern: `mergeRefs(ref1, ref2)`)
* 2. lvalue is an event handler (DOM events execute outside render)
* 3. calling hooks (independently validated for ref safety)
*/

validateNoDirectRefValueAccess(errors, operand, env);
} else if (interpolatedAsJsx.has(instr.lvalue.identifier.id)) {
/**
* Special case: the lvalue is passed as a jsx child
*
* For example `<Foo>{renderHelper(ref)}</Foo>`. Here we have more
* context and infer that the ref is being passed to a component-like
* render function which attempts to obey the rules.
*/

validateNoRefValueAccess(errors, env, operand);
} else {
validateNoRefPassedToFunction(
Expand All @@ -575,15 +543,15 @@ function validateNoRefAccessInRenderImpl(
const types: Array<RefAccessType> = [];
for (const operand of eachInstructionValueOperand(instr.value)) {
validateNoDirectRefValueAccess(errors, operand, env);
types.push(env.get(operand.identifier.id) ?? {kind: 'None'});
types.push(env.get(operand.identifier.id) ?? { kind: 'None' });
}
const value = joinRefAccessTypes(...types);
if (
value.kind === 'None' ||
value.kind === 'Guard' ||
value.kind === 'Nullable'
) {
env.set(instr.lvalue.identifier.id, {kind: 'None'});
env.set(instr.lvalue.identifier.id, { kind: 'None' });
} else {
env.set(instr.lvalue.identifier.id, {
kind: 'Structure',
Expand Down Expand Up @@ -638,13 +606,13 @@ function validateNoRefAccessInRenderImpl(
break;
case 'LoadGlobal': {
if (instr.value.binding.name === 'undefined') {
env.set(instr.lvalue.identifier.id, {kind: 'Nullable'});
env.set(instr.lvalue.identifier.id, { kind: 'Nullable' });
}
break;
}
case 'Primitive': {
if (instr.value.value == null) {
env.set(instr.lvalue.identifier.id, {kind: 'Nullable'});
env.set(instr.lvalue.identifier.id, { kind: 'Nullable' });
}
break;
}
Expand All @@ -656,12 +624,8 @@ function validateNoRefAccessInRenderImpl(
? value.refId
: null;
if (refId !== null) {
/*
* Record an error suggesting the `if (ref.current == null)` pattern,
* but also record the lvalue as a guard so that we don't emit a second
* error for the write to the ref
*/
env.set(instr.lvalue.identifier.id, {kind: 'Guard', refId});

env.set(instr.lvalue.identifier.id, { kind: 'Guard', refId });
errors.pushDiagnostic(
CompilerDiagnostic.create({
category: ErrorCategory.Refs,
Expand Down Expand Up @@ -703,7 +667,7 @@ function validateNoRefAccessInRenderImpl(
}

if (refId !== null && nullish) {
env.set(instr.lvalue.identifier.id, {kind: 'Guard', refId});
env.set(instr.lvalue.identifier.id, { kind: 'Guard', refId });
} else {
for (const operand of eachInstructionValueOperand(instr.value)) {
validateNoRefValueAccess(errors, env, operand);
Expand All @@ -719,7 +683,7 @@ function validateNoRefAccessInRenderImpl(
}
}

// Guard values are derived from ref.current, so they can only be used in if statement targets

for (const operand of eachInstructionOperand(instr)) {
guardCheck(errors, operand, env);
}
Expand All @@ -731,8 +695,8 @@ function validateNoRefAccessInRenderImpl(
env.set(
instr.lvalue.identifier.id,
joinRefAccessTypes(
env.get(instr.lvalue.identifier.id) ?? {kind: 'None'},
{kind: 'Ref', refId: nextRefId()},
env.get(instr.lvalue.identifier.id) ?? { kind: 'None' },
{ kind: 'Ref', refId: nextRefId() },
),
);
}
Expand All @@ -743,8 +707,8 @@ function validateNoRefAccessInRenderImpl(
env.set(
instr.lvalue.identifier.id,
joinRefAccessTypes(
env.get(instr.lvalue.identifier.id) ?? {kind: 'None'},
{kind: 'RefValue', loc: instr.loc},
env.get(instr.lvalue.identifier.id) ?? { kind: 'None' },
{ kind: 'RefValue', loc: instr.loc },
),
);
}
Expand All @@ -756,7 +720,7 @@ function validateNoRefAccessInRenderImpl(
test?.kind === 'Guard' &&
safeBlocks.find(entry => entry.ref === test.refId) == null
) {
safeBlocks.push({block: block.terminal.fallthrough, ref: test.refId});
safeBlocks.push({ block: block.terminal.fallthrough, ref: test.refId });
}
}

Expand Down
2 changes: 1 addition & 1 deletion ...iler/src/__tests__/fixtures/compiler/codegen-emit-imports-same-source.expect.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ import { c as _c } from "react/compiler-runtime"; // @enableEmitFreeze @enableEm

function useFoo(props) {
if (DEV && shouldInstrument)
useRenderCounter("useFoo", "/codegen-emit-imports-same-source.ts");
useRenderCounter("useFoo", "C:\\codegen-emit-imports-same-source.ts");
const $ = _c(2);
let t0;
if ($[0] !== props.x) {
Expand Down
Loading
Loading