Skip to content
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

Jump to bottom

[compiler] Propogate effects stored in object props #30457

Closed
wants to merge 2 commits into from
Closed

[compiler] Propogate effects stored in object props #30457

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c9043f5
[compiler] Propogate effects stored in object props
gsathya Jul 25, 2024
f7443ac
Update on "[compiler] Propogate effects stored in object props"
gsathya Jul 25, 2024
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
111 changes: 68 additions & 43 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 @@ -401,59 +401,84 @@ class InferenceState {

// Propagate effects of function expressions to the outer (ie current) effect context
for (const value of values) {
if (
(value.kind === 'FunctionExpression' ||
value.kind === 'ObjectMethod') &&
value.loweredFunc.func.effects != null
) {
for (const effect of value.loweredFunc.func.effects) {
if (
effect.kind === 'GlobalMutation' ||
effect.kind === 'ReactMutation'
) {
// Known effects are always propagated upwards
functionEffects.push(effect);
} else {
/**
* Contextual effects need to be replayed against the current inference
* state, which may know more about the value to which the effect applied.
* The main cases are:
* 1. The mutated context value is _still_ a context value in the current scope,
* so we have to continue propagating the original context mutation.
* 2. The mutated context value is a mutable value in the current scope,
* so the context mutation was fine and we can skip propagating the effect.
* 3. The mutated context value is an immutable value in the current scope,
* resulting in a non-ContextMutation FunctionEffect. We propagate that new,
* more detailed effect to the current function context.
*/
for (const place of effect.places) {
if (this.isDefined(place)) {
const replayedEffect = this.reference(
{...place, loc: effect.loc},
effect.effect,
reason,
);
if (replayedEffect != null) {
if (replayedEffect.kind === 'ContextMutation') {
// Case 1, still a context value so propagate the original effect
functionEffects.push(effect);
} else {
// Case 3, immutable value so propagate the more precise effect
functionEffects.push(replayedEffect);
}
} // else case 2, local mutable value so this effect was fine
}
}
if (value.kind === 'ObjectExpression') {
for (const prop of value.properties) {
const propValues = this.#variables.get(prop.place.identifier.id);
CompilerError.invariant(propValues !== undefined, {
reason:
'[InferReferenceEffects] Every object property must have been initialised',
description: null,
loc: place.loc,
suggestions: null,
});
for (const propValue of propValues) {
this.propogateEffect(propValue, functionEffects, reason);
}
}
} else {
this.propogateEffect(value, functionEffects, reason);
}
}

const functionEffect = this.reference(place, effectKind, reason);
if (functionEffect !== null) {
functionEffects.push(functionEffect);
}
}

propogateEffect(
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
propogateEffect(
propagateEffect(

very minor spelling nit

value: InstructionValue,
functionEffects: Array<FunctionEffect>,
reason: ValueReason,
): void {
if (value.kind !== 'FunctionExpression' && value.kind !== 'ObjectMethod') {
return;
}

const effects = value.loweredFunc.func.effects;
if (!effects) {
return;
}

for (const effect of effects) {
if (effect.kind === 'GlobalMutation' || effect.kind === 'ReactMutation') {
// Known effects are always propagated upwards
functionEffects.push(effect);
} else {
/**
* Contextual effects need to be replayed against the current inference
* state, which may know more about the value to which the effect applied.
* The main cases are:
* 1. The mutated context value is _still_ a context value in the current scope,
* so we have to continue propagating the original context mutation.
* 2. The mutated context value is a mutable value in the current scope,
* so the context mutation was fine and we can skip propagating the effect.
* 3. The mutated context value is an immutable value in the current scope,
* resulting in a non-ContextMutation FunctionEffect. We propagate that new,
* more detailed effect to the current function context.
*/
for (const place of effect.places) {
if (this.isDefined(place)) {
const replayedEffect = this.reference(
{...place, loc: effect.loc},
effect.effect,
reason,
);
if (replayedEffect != null) {
if (replayedEffect.kind === 'ContextMutation') {
// Case 1, still a context value so propagate the original effect
functionEffects.push(effect);
} else {
// Case 3, immutable value so propagate the more precise effect
functionEffects.push(replayedEffect);
}
} // else case 2, local mutable value so this effect was fine
}
}
}
}
}

reference(
place: Place,
effectKind: Effect,
Expand Down