Skip to content

[compiler][fix] mutableOnlyIfOperandsAreMutable does not apply when operands are globals #32695

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

Merged
merged 1 commit into from
Mar 24, 2025
Merged

[compiler][fix] mutableOnlyIfOperandsAreMutable does not apply when operands are globals #32695

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
110 changes: 98 additions & 12 deletions compiler/packages/babel-plugin-react-compiler/src/HIR/Globals.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import {
BuiltInArrayId,
BuiltInFireId,
BuiltInMixedReadonlyId,
BuiltInObjectId,
BuiltInUseActionStateId,
BuiltInUseContextHookId,
BuiltInUseEffectHookId,
Expand Down Expand Up @@ -45,21 +46,17 @@ export const DEFAULT_SHAPES: ShapeRegistry = new Map(BUILTIN_SHAPES);

// Hack until we add ObjectShapes for all globals
const UNTYPED_GLOBALS: Set<string> = new Set([
'String',
'Object',
'Function',
'Number',
'RegExp',
'Date',
'Error',
'Function',
'TypeError',
'RangeError',
'ReferenceError',
'SyntaxError',
'URIError',
'EvalError',
'Boolean',
'DataView',
'Float32Array',
'Float64Array',
Expand All @@ -75,16 +72,8 @@ const UNTYPED_GLOBALS: Set<string> = new Set([
'Uint32Array',
'ArrayBuffer',
'JSON',
'parseFloat',
'parseInt',
'console',
'isNaN',
'eval',
'isFinite',
'encodeURI',
'decodeURI',
'encodeURIComponent',
'decodeURIComponent',
]);

const TYPED_GLOBALS: Array<[string, BuiltInType]> = [
Expand All @@ -101,6 +90,23 @@ const TYPED_GLOBALS: Array<[string, BuiltInType]> = [
returnValueKind: ValueKind.Mutable,
}),
],
[
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added this definition for unit test (see newly added repro-array-map-known-mutate-shape.tsx). It was hard finding a global function that (1) has no mutable receiver, (2) only takes one argument, and (3) mutates its argument.

/**
* Object.fromEntries(iterable)
* iterable: An iterable, such as an Array or Map, containing a list of
* objects. Each object should have two properties.
* Returns a new object whose properties are given by the entries of the
* iterable.
*/
'fromEntries',
addFunction(DEFAULT_SHAPES, [], {
positionalParams: [Effect.ConditionallyMutate],
restParam: null,
returnType: {kind: 'Object', shapeId: BuiltInObjectId},
calleeEffect: Effect.Read,
returnValueKind: ValueKind.Mutable,
}),
],
]),
],
[
Expand Down Expand Up @@ -372,6 +378,86 @@ const TYPED_GLOBALS: Array<[string, BuiltInType]> = [
returnValueKind: ValueKind.Primitive,
}),
],
[
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added these drive-by (similar to Boolean) to try to limit scope of deopt.

'parseInt',
addFunction(DEFAULT_SHAPES, [], {
positionalParams: [],
restParam: Effect.Read,
returnType: {kind: 'Primitive'},
calleeEffect: Effect.Read,
returnValueKind: ValueKind.Primitive,
}),
],
[
'parseFloat',
addFunction(DEFAULT_SHAPES, [], {
positionalParams: [],
restParam: Effect.Read,
returnType: {kind: 'Primitive'},
calleeEffect: Effect.Read,
returnValueKind: ValueKind.Primitive,
}),
],
[
'isNaN',
addFunction(DEFAULT_SHAPES, [], {
positionalParams: [],
restParam: Effect.Read,
returnType: {kind: 'Primitive'},
calleeEffect: Effect.Read,
returnValueKind: ValueKind.Primitive,
}),
],
[
'isFinite',
addFunction(DEFAULT_SHAPES, [], {
positionalParams: [],
restParam: Effect.Read,
returnType: {kind: 'Primitive'},
calleeEffect: Effect.Read,
returnValueKind: ValueKind.Primitive,
}),
],
[
'encodeURI',
addFunction(DEFAULT_SHAPES, [], {
positionalParams: [],
restParam: Effect.Read,
returnType: {kind: 'Primitive'},
calleeEffect: Effect.Read,
returnValueKind: ValueKind.Primitive,
}),
],
[
'encodeURIComponent',
addFunction(DEFAULT_SHAPES, [], {
positionalParams: [],
restParam: Effect.Read,
returnType: {kind: 'Primitive'},
calleeEffect: Effect.Read,
returnValueKind: ValueKind.Primitive,
}),
],
[
'decodeURI',
addFunction(DEFAULT_SHAPES, [], {
positionalParams: [],
restParam: Effect.Read,
returnType: {kind: 'Primitive'},
calleeEffect: Effect.Read,
returnValueKind: ValueKind.Primitive,
}),
],
[
'decodeURIComponent',
addFunction(DEFAULT_SHAPES, [], {
positionalParams: [],
restParam: Effect.Read,
returnType: {kind: 'Primitive'},
calleeEffect: Effect.Read,
returnValueKind: ValueKind.Primitive,
}),
],
// TODO: rest of Global objects
];

Expand Down
54 changes: 47 additions & 7 deletions compiler/packages/babel-plugin-react-compiler/src/Inference/InferReferenceEffects.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -251,7 +251,7 @@ type FreezeAction = {values: Set<InstructionValue>; reason: Set<ValueReason>};

// Maintains a mapping of top-level variables to the kind of value they hold
class InferenceState {
#env: Environment;
env: Environment;

// The kind of each value, based on its allocation site
#values: Map<InstructionValue, AbstractValue>;
Expand All @@ -267,7 +267,7 @@ class InferenceState {
values: Map<InstructionValue, AbstractValue>,
variables: Map<IdentifierId, Set<InstructionValue>>,
) {
this.#env = env;
this.env = env;
this.#values = values;
this.#variables = variables;
}
Expand Down Expand Up @@ -409,8 +409,8 @@ class InferenceState {
});
if (
value.kind === 'FunctionExpression' &&
(this.#env.config.enablePreserveExistingMemoizationGuarantees ||
this.#env.config.enableTransitivelyFreezeFunctionExpressions)
(this.env.config.enablePreserveExistingMemoizationGuarantees ||
this.env.config.enableTransitivelyFreezeFunctionExpressions)
) {
for (const operand of value.loweredFunc.func.context) {
const operandValues = this.#variables.get(operand.identifier.id);
Expand Down Expand Up @@ -590,7 +590,7 @@ class InferenceState {
return null;
} else {
return new InferenceState(
this.#env,
this.env,
nextValues ?? new Map(this.#values),
nextVariables ?? new Map(this.#variables),
);
Expand All @@ -604,7 +604,7 @@ class InferenceState {
*/
clone(): InferenceState {
return new InferenceState(
this.#env,
this.env,
new Map(this.#values),
new Map(this.#variables),
);
Expand Down Expand Up @@ -2012,6 +2012,32 @@ export function getFunctionEffects(
return results;
}

export function isKnownMutableEffect(effect: Effect): boolean {
switch (effect) {
case Effect.Store:
case Effect.ConditionallyMutate:
case Effect.Mutate: {
return true;
}

case Effect.Unknown: {
CompilerError.invariant(false, {
reason: 'Unexpected unknown effect',
description: null,
loc: GeneratedSource,
suggestions: null,
});
}
case Effect.Read:
case Effect.Capture:
case Effect.Freeze: {
return false;
}
default: {
assertExhaustive(effect, `Unexpected effect \`${effect}\``);
}
}
}
/**
* Returns true if all of the arguments are both non-mutable (immutable or frozen)
* _and_ are not functions which might mutate their arguments. Note that function
Expand All @@ -2023,10 +2049,20 @@ function areArgumentsImmutableAndNonMutating(
args: MethodCall['args'],
): boolean {
for (const arg of args) {
if (arg.kind === 'Identifier' && arg.identifier.type.kind === 'Function') {
const fnShape = state.env.getFunctionSignature(arg.identifier.type);
if (fnShape != null) {
return (
!fnShape.positionalParams.some(isKnownMutableEffect) &&
(fnShape.restParam == null ||
!isKnownMutableEffect(fnShape.restParam))
);
}
}
const place = arg.kind === 'Identifier' ? arg : arg.place;

const kind = state.kind(place).kind;
switch (kind) {
case ValueKind.Global:
case ValueKind.Primitive:
case ValueKind.Frozen: {
/*
Expand All @@ -2037,6 +2073,10 @@ function areArgumentsImmutableAndNonMutating(
break;
}
default: {
/**
* Globals, module locals, and other locally defined functions may
* mutate their arguments.
*/
return false;
}
}
Expand Down
39 changes: 29 additions & 10 deletions ...compiler/src/__tests__/fixtures/compiler/array-from-maybemutates-arg0.expect.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,12 @@ function Component({value}) {
const arr = [{value: 'foo'}, {value: 'bar'}, {value}];
useIdentity();
const derived = Array.from(arr, mutateAndReturn);
return <Stringify>{derived.at(-1)}</Stringify>;
return (
<Stringify>
{derived.at(0)}
{derived.at(-1)}
</Stringify>
);
}

export const FIXTURE_ENTRYPOINT = {
Expand All @@ -26,28 +31,42 @@ import { c as _c } from "react/compiler-runtime";
import { mutateAndReturn, Stringify, useIdentity } from "shared-runtime";

function Component(t0) {
const $ = _c(4);
const $ = _c(7);
const { value } = t0;
const arr = [{ value: "foo" }, { value: "bar" }, { value }];
useIdentity();
const derived = Array.from(arr, mutateAndReturn);
let t1;
if ($[0] !== derived) {
t1 = derived.at(-1);
t1 = derived.at(0);
$[0] = derived;
$[1] = t1;
} else {
t1 = $[1];
}
let t2;
if ($[2] !== t1) {
t2 = <Stringify>{t1}</Stringify>;
$[2] = t1;
if ($[2] !== derived) {
t2 = derived.at(-1);
$[2] = derived;
$[3] = t2;
} else {
t2 = $[3];
}
return t2;
let t3;
if ($[4] !== t1 || $[5] !== t2) {
t3 = (
<Stringify>
{t1}
{t2}
</Stringify>
);
$[4] = t1;
$[5] = t2;
$[6] = t3;
} else {
t3 = $[6];
}
return t3;
}

export const FIXTURE_ENTRYPOINT = {
Expand All @@ -59,6 +78,6 @@ export const FIXTURE_ENTRYPOINT = {
```

### Eval output
(kind: ok) <div>{"children":{"value":5,"wat0":"joe"}}</div>
<div>{"children":{"value":6,"wat0":"joe"}}</div>
<div>{"children":{"value":6,"wat0":"joe"}}</div>
(kind: ok) <div>{"children":[{"value":"foo","wat0":"joe"},{"value":5,"wat0":"joe"}]}</div>
<div>{"children":[{"value":"foo","wat0":"joe"},{"value":6,"wat0":"joe"}]}</div>
<div>{"children":[{"value":"foo","wat0":"joe"},{"value":6,"wat0":"joe"}]}</div>
7 changes: 6 additions & 1 deletion ...bel-plugin-react-compiler/src/__tests__/fixtures/compiler/array-from-maybemutates-arg0.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,12 @@ function Component({value}) {
const arr = [{value: 'foo'}, {value: 'bar'}, {value}];
useIdentity();
const derived = Array.from(arr, mutateAndReturn);
return <Stringify>{derived.at(-1)}</Stringify>;
return (
<Stringify>
{derived.at(0)}
{derived.at(-1)}
</Stringify>
);
}

export const FIXTURE_ENTRYPOINT = {
Expand Down
Loading
Loading