Skip to content

Commit

Permalink
[wip] PropagateScopeDeps understands optionals
Browse files Browse the repository at this point in the history 
ghstack-source-id: 50d3523dbdb483cbd994a19a23a6ee262517bf04
Pull Request resolved: #30819
  • Loading branch information
@josephsavona
josephsavona committed Aug 26, 2024
1 parent 7666342 commit e0a4e1e
Show file tree
Hide file tree
Showing 5 changed files with 160 additions and 45 deletions.
2 changes: 1 addition & 1 deletion compiler/packages/babel-plugin-react-compiler/src/HIR/PrintHIR.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -191,7 +191,7 @@ export function printTerminal(terminal: Terminal): Array<string> | string {
case 'branch': {
value = `[${terminal.id}] Branch (${printPlace(terminal.test)}) then:bb${
terminal.consequent
} else:bb${terminal.alternate}`;
} else:bb${terminal.alternate} fallthrough:bb${terminal.fallthrough}`;
break;
}
case 'logical': {
Expand Down
22 changes: 22 additions & 0 deletions ...iler/packages/babel-plugin-react-compiler/src/ReactiveScopes/DeriveMinimalDependencies.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import {CompilerError} from '../CompilerError';
import {DependencyPath, Identifier, ReactiveScopeDependency} from '../HIR';
import {printIdentifier} from '../HIR/PrintHIR';
import {assertExhaustive} from '../Utils/utils';
import {printDependency} from './PrintReactiveFunction';

/*
* We need to understand optional member expressions only when determining
Expand Down Expand Up @@ -173,6 +174,27 @@ export class ReactiveScopeDependencyTree {
}
return res.flat().join('\n');
}

debug(): string {
const buf: Array<string> = [`tree() [`];
for (const [rootId, rootNode] of this.#roots) {
buf.push(`${printIdentifier(rootId)} (${rootNode.accessType}):`);
this.#debugImpl(buf, rootNode, 1);
}
buf.push(']');
return buf.length > 2 ? buf.join('\n') : buf.join('');
}

#debugImpl(
buf: Array<string>,
node: DependencyNode,
depth: number = 0,
): void {
for (const [property, childNode] of node.properties) {
buf.push(`${' '.repeat(depth)}.${property} (${childNode.accessType}):`);
this.#debugImpl(buf, childNode, depth + 1);
}
}
}

/*
Expand Down
2 changes: 1 addition & 1 deletion compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PrintReactiveFunction.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,7 @@ export function printDependency(dependency: ReactiveScopeDependency): string {
const identifier =
printIdentifier(dependency.identifier) +
printType(dependency.identifier.type);
return `${identifier}${dependency.path.map(token => `.${token.property}`).join('')}`;
return `${identifier}${dependency.path.map(token => `${token.optional ? '?.' : '.'}${token.property}`).join('')}`;
}

export function printReactiveInstructions(
Expand Down
176 changes: 134 additions & 42 deletions ...ler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PropagateScopeDependencies.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
* LICENSE file in the root directory of this source tree.
*/

import prettyFormat from 'pretty-format';
import {CompilerError} from '../CompilerError';
import {
areEqualPaths,
Expand All @@ -17,25 +18,29 @@ import {
isObjectMethodType,
isRefValueType,
isUseRefType,
LoadLocal,
makeInstructionId,
Place,
PrunedReactiveScopeBlock,
ReactiveFunction,
ReactiveInstruction,
ReactiveOptionalCallValue,
ReactiveScope,
ReactiveScopeBlock,
ReactiveScopeDependency,
ReactiveTerminalStatement,
ReactiveValue,
ScopeId,
} from '../HIR/HIR';
import {printIdentifier, printPlace} from '../HIR/PrintHIR';
import {eachInstructionValueOperand, eachPatternOperand} from '../HIR/visitors';
import {empty, Stack} from '../Utils/Stack';
import {assertExhaustive, Iterable_some} from '../Utils/utils';
import {
ReactiveScopeDependencyTree,
ReactiveScopePropertyDependency,
} from './DeriveMinimalDependencies';
import {printDependency, printReactiveFunction} from './PrintReactiveFunction';
import {ReactiveFunctionVisitor, visitReactiveFunction} from './visitors';

/*
Expand Down Expand Up @@ -302,6 +307,7 @@ class Context {
#properties: Map<Identifier, ReactiveScopePropertyDependency> = new Map();
#temporaries: Map<Identifier, Place> = new Map();
#inConditionalWithinScope: boolean = false;
inOptional: boolean = false;
/*
* Reactive dependencies used unconditionally in the current conditional.
* Composed of dependencies:
Expand Down Expand Up @@ -465,6 +471,7 @@ class Context {
#getProperty(
object: Place,
property: string,
optional: boolean,
): ReactiveScopePropertyDependency {
const resolvedObject = this.resolveTemporary(object);
const resolvedDependency = this.#properties.get(resolvedObject.identifier);
Expand All @@ -485,13 +492,18 @@ class Context {
};
}

objectDependency.path.push({property, optional: false});
objectDependency.path.push({property, optional});

return objectDependency;
}

declareProperty(lvalue: Place, object: Place, property: string): void {
const nextDependency = this.#getProperty(object, property);
declareProperty(
lvalue: Place,
object: Place,
property: string,
optional: boolean,
): void {
const nextDependency = this.#getProperty(object, property, optional);
this.#properties.set(lvalue.identifier, nextDependency);
}

Expand Down Expand Up @@ -571,8 +583,8 @@ class Context {
this.visitDependency(dependency);
}

visitProperty(object: Place, property: string): void {
const nextDependency = this.#getProperty(object, property);
visitProperty(object: Place, property: string, optional: boolean): void {
const nextDependency = this.#getProperty(object, property, optional);
this.visitDependency(nextDependency);
}

Expand Down Expand Up @@ -744,51 +756,102 @@ class PropagationVisitor extends ReactiveFunctionVisitor<Context> {
});
}

visitOptionalExpression(
context: Context,
id: InstructionId,
value: ReactiveOptionalCallValue,
lvalue: Place | null,
): void {
const wasWithinOptional = context.inOptional;
const isOptional = wasWithinOptional || value.optional;
const inner = value.value;
/*
* OptionalExpression value is a SequenceExpression where the instructions
* represent the code prior to the `?` and the final value represents the
* conditional code that follows.
*/
CompilerError.invariant(inner.kind === 'SequenceExpression', {
reason: 'Expected OptionalExpression value to be a SequenceExpression',
description: `Found a \`${value.kind}\``,
loc: value.loc,
});
// Instructions are the unconditionally executed portion before the `?`
context.inOptional = isOptional;
for (const instr of inner.instructions) {
this.visitInstruction(instr, context);
}
context.inOptional = wasWithinOptional;
const innerValue = inner.value;
if (
innerValue.kind !== 'SequenceExpression' ||
innerValue.value.kind !== 'LoadLocal'
) {
CompilerError.invariant(false, {
reason:
'Expected OptionalExpression inner value to be a SequenceExpression',
description: `Found a \`${innerValue.kind}\``,
loc: innerValue.loc,
});
}
if (
isOptional &&
innerValue.instructions.length === 1 &&
innerValue.instructions[0].value.kind === 'PropertyLoad' &&
innerValue.instructions[0].lvalue !== null &&
innerValue.instructions[0].lvalue.identifier.id ===
innerValue.value.place.identifier.id
) {
const propertyLoad = innerValue.instructions[0].value;
if (lvalue !== null && !context.isUsedOutsideDeclaringScope(lvalue)) {
context.declareProperty(
lvalue,
propertyLoad.object,
propertyLoad.property,
isOptional,
);
} else {
context.visitProperty(
propertyLoad.object,
propertyLoad.property,
isOptional,
);
}
if (isOptional && !wasWithinOptional && lvalue !== null) {
context.visitOperand(lvalue);
}
return;
}
context.enterConditional(() => {
this.visitReactiveValue(context, id, innerValue, lvalue);
});
}

visitReactiveValue(
context: Context,
id: InstructionId,
value: ReactiveValue,
lvalue: Place | null,
): void {
switch (value.kind) {
case 'OptionalExpression': {
const inner = value.value;
/*
* OptionalExpression value is a SequenceExpression where the instructions
* represent the code prior to the `?` and the final value represents the
* conditional code that follows.
*/
CompilerError.invariant(inner.kind === 'SequenceExpression', {
reason:
'Expected OptionalExpression value to be a SequenceExpression',
description: `Found a \`${value.kind}\``,
loc: value.loc,
suggestions: null,
});
// Instructions are the unconditionally executed portion before the `?`
for (const instr of inner.instructions) {
this.visitInstruction(instr, context);
}
// The final value is the conditional portion following the `?`
context.enterConditional(() => {
this.visitReactiveValue(context, id, inner.value);
});
this.visitOptionalExpression(context, id, value, lvalue);
break;
}
case 'LogicalExpression': {
this.visitReactiveValue(context, id, value.left);
this.visitReactiveValue(context, id, value.left, null);
context.enterConditional(() => {
this.visitReactiveValue(context, id, value.right);
this.visitReactiveValue(context, id, value.right, lvalue);
});
break;
}
case 'ConditionalExpression': {
this.visitReactiveValue(context, id, value.test);
this.visitReactiveValue(context, id, value.test, null);

const consequentDeps = context.enterConditional(() => {
this.visitReactiveValue(context, id, value.consequent);
this.visitReactiveValue(context, id, value.consequent, null);
});
const alternateDeps = context.enterConditional(() => {
this.visitReactiveValue(context, id, value.alternate);
this.visitReactiveValue(context, id, value.alternate, null);
});
context.promoteDepsFromExhaustiveConditionals([
consequentDeps,
Expand All @@ -797,10 +860,34 @@ class PropagationVisitor extends ReactiveFunctionVisitor<Context> {
break;
}
case 'SequenceExpression': {
/**
* Sequence
* <lvalue> = Sequence <-- we're here
* <lvalue> = ...
* LoadLocal <lvalue>
*/
if (
lvalue !== null &&
value.instructions.length === 1 &&
value.value.kind === 'LoadLocal' &&
value.value.place.identifier.id === lvalue.identifier.id &&
value.instructions[0].lvalue !== null &&
value.instructions[0].lvalue.identifier.id ===
value.value.place.identifier.id
) {
this.visitInstructionValue(
context,
value.instructions[0].id,
value.instructions[0].value,
lvalue,
);
break;
}

for (const instr of value.instructions) {
this.visitInstruction(instr, context);
}
this.visitInstructionValue(context, id, value.value, null);
this.visitInstructionValue(context, id, value.value, lvalue);
break;
}
case 'FunctionExpression': {
Expand Down Expand Up @@ -851,9 +938,9 @@ class PropagationVisitor extends ReactiveFunctionVisitor<Context> {
}
} else if (value.kind === 'PropertyLoad') {
if (lvalue !== null && !context.isUsedOutsideDeclaringScope(lvalue)) {
context.declareProperty(lvalue, value.object, value.property);
context.declareProperty(lvalue, value.object, value.property, false);
} else {
context.visitProperty(value.object, value.property);
context.visitProperty(value.object, value.property, false);
}
} else if (value.kind === 'StoreLocal') {
context.visitOperand(value.value);
Expand Down Expand Up @@ -896,7 +983,7 @@ class PropagationVisitor extends ReactiveFunctionVisitor<Context> {
});
}
} else {
this.visitReactiveValue(context, id, value);
this.visitReactiveValue(context, id, value, lvalue);
}
}

Expand Down Expand Up @@ -947,25 +1034,30 @@ class PropagationVisitor extends ReactiveFunctionVisitor<Context> {
break;
}
case 'for': {
this.visitReactiveValue(context, terminal.id, terminal.init);
this.visitReactiveValue(context, terminal.id, terminal.test);
this.visitReactiveValue(context, terminal.id, terminal.init, null);
this.visitReactiveValue(context, terminal.id, terminal.test, null);
context.enterConditional(() => {
this.visitBlock(terminal.loop, context);
if (terminal.update !== null) {
this.visitReactiveValue(context, terminal.id, terminal.update);
this.visitReactiveValue(
context,
terminal.id,
terminal.update,
null,
);
}
});
break;
}
case 'for-of': {
this.visitReactiveValue(context, terminal.id, terminal.init);
this.visitReactiveValue(context, terminal.id, terminal.init, null);
context.enterConditional(() => {
this.visitBlock(terminal.loop, context);
});
break;
}
case 'for-in': {
this.visitReactiveValue(context, terminal.id, terminal.init);
this.visitReactiveValue(context, terminal.id, terminal.init, null);
context.enterConditional(() => {
this.visitBlock(terminal.loop, context);
});
Expand All @@ -974,12 +1066,12 @@ class PropagationVisitor extends ReactiveFunctionVisitor<Context> {
case 'do-while': {
this.visitBlock(terminal.loop, context);
context.enterConditional(() => {
this.visitReactiveValue(context, terminal.id, terminal.test);
this.visitReactiveValue(context, terminal.id, terminal.test, null);
});
break;
}
case 'while': {
this.visitReactiveValue(context, terminal.id, terminal.test);
this.visitReactiveValue(context, terminal.id, terminal.test, null);
context.enterConditional(() => {
this.visitBlock(terminal.loop, context);
});
Expand Down
3 changes: 2 additions & 1 deletion ...iler/src/__tests__/fixtures/compiler/error.todo-optional-member-expression-as-memo-dep.js
View file
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
// @validatePreserveExistingMemoizationGuarantees
function Component(props) {
const data = useMemo(() => {
return props.items?.edges?.nodes ?? [];
return props?.items.edges?.nodes.map();
}, [props.items?.edges?.nodes]);
// const data = props?.item.edges?.nodes.map();
return <Foo data={data} />;
}

0 comments on commit e0a4e1e

Please sign in to comment.