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.

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

[compiler] Lower JSXMemberExpression with LoadLocal #31201

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,6 @@ import {
mergeReactiveScopesThatInvalidateTogether,
promoteUsedTemporaries,
propagateEarlyReturns,
propagateScopeDependencies,
pruneHoistedContexts,
pruneNonEscapingScopes,
pruneNonReactiveDependencies,
Expand Down Expand Up @@ -348,14 +347,12 @@ function* runWithEnvironment(
});
assertTerminalSuccessorsExist(hir);
assertTerminalPredsExist(hir);
if (env.config.enablePropagateDepsInHIR) {
propagateScopeDependenciesHIR(hir);
yield log({
kind: 'hir',
name: 'PropagateScopeDependenciesHIR',
value: hir,
});
}
propagateScopeDependenciesHIR(hir);
yield log({
kind: 'hir',
name: 'PropagateScopeDependenciesHIR',
value: hir,
});

if (env.config.inlineJsxTransform) {
inlineJsxTransform(hir, env.config.inlineJsxTransform);
Expand Down Expand Up @@ -383,15 +380,6 @@ function* runWithEnvironment(
});
assertScopeInstructionsWithinScopes(reactiveFunction);

if (!env.config.enablePropagateDepsInHIR) {
propagateScopeDependencies(reactiveFunction);
yield log({
kind: 'reactive',
name: 'PropagateScopeDependencies',
value: reactiveFunction,
});
}

pruneNonEscapingScopes(reactiveFunction);
yield log({
kind: 'reactive',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3186,7 +3186,13 @@ function lowerJsxMemberExpression(
loc: object.node.loc ?? null,
suggestions: null,
});
objectPlace = lowerIdentifier(builder, object);

const kind = getLoadKind(builder, object);
objectPlace = lowerValueToTemporary(builder, {
kind: kind,
place: lowerIdentifier(builder, object),
loc: exprPath.node.loc ?? GeneratedSource,
});
}
const property = exprPath.get('property').node.name;
return lowerValueToTemporary(builder, {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ import {
Set_union,
getOrInsertDefault,
} from '../Utils/utils';
import {collectOptionalChainSidemap} from './CollectOptionalChainDependencies';
import {
BasicBlock,
BlockId,
Expand All @@ -22,7 +21,6 @@ import {
ReactiveScopeDependency,
ScopeId,
} from './HIR';
import {collectTemporariesSidemap} from './PropagateScopeDependenciesHIR';

const DEBUG_PRINT = false;

Expand Down Expand Up @@ -373,17 +371,10 @@ function collectNonNullsInBlocks(
!fn.env.config.enableTreatFunctionDepsAsConditional
) {
const innerFn = instr.value.loweredFunc;
const innerTemporaries = collectTemporariesSidemap(
innerFn.func,
new Set(),
);
const innerOptionals = collectOptionalChainSidemap(innerFn.func);
const innerHoistableMap = collectHoistablePropertyLoadsImpl(
innerFn.func,
{
...context,
temporaries: innerTemporaries, // TODO: remove in later PR
hoistableFromOptionals: innerOptionals.hoistableObjects, // TODO: remove in later PR
nestedFnImmutableContext:
context.nestedFnImmutableContext ??
new Set(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ import {assertNonNull} from './CollectHoistablePropertyLoads';
import {
BlockId,
BasicBlock,
InstructionId,
IdentifierId,
ReactiveScopeDependency,
BranchTerminal,
Expand All @@ -15,32 +14,23 @@ import {
OptionalTerminal,
HIRFunction,
DependencyPathEntry,
Instruction,
Terminal,
} from './HIR';
import {printIdentifier} from './PrintHIR';

export function collectOptionalChainSidemap(
fn: HIRFunction,
): OptionalChainSidemap {
const context: OptionalTraversalContext = {
currFn: fn,
blocks: fn.body.blocks,
seenOptionals: new Set(),
processedInstrsInOptional: new Set(),
temporariesReadInOptional: new Map(),
hoistableObjects: new Map(),
};
for (const [_, block] of fn.body.blocks) {
if (
block.terminal.kind === 'optional' &&
!context.seenOptionals.has(block.id)
) {
traverseOptionalBlock(
block as TBasicBlock<OptionalTerminal>,
context,
null,
);
}
}

traverseFunction(fn, context);
return {
temporariesReadInOptional: context.temporariesReadInOptional,
processedInstrsInOptional: context.processedInstrsInOptional,
Expand Down Expand Up @@ -96,8 +86,10 @@ export type OptionalChainSidemap = {
* bb5:
* $5 = MethodCall $2.$4() <--- here, we want to take a dep on $2 and $4!
* ```
*
* Also note that InstructionIds are not unique across inner functions.
*/
processedInstrsInOptional: ReadonlySet<InstructionId>;
processedInstrsInOptional: ReadonlySet<Instruction | Terminal>;
/**
* Records optional chains for which we can safely evaluate non-optional
* PropertyLoads. e.g. given `a?.b.c`, we can evaluate any load from `a?.b` at
Expand All @@ -115,16 +107,46 @@ export type OptionalChainSidemap = {
};

type OptionalTraversalContext = {
currFn: HIRFunction;
blocks: ReadonlyMap<BlockId, BasicBlock>;

// Track optional blocks to avoid outer calls into nested optionals
seenOptionals: Set<BlockId>;

processedInstrsInOptional: Set<InstructionId>;
processedInstrsInOptional: Set<Instruction | Terminal>;
temporariesReadInOptional: Map<IdentifierId, ReactiveScopeDependency>;
hoistableObjects: Map<BlockId, ReactiveScopeDependency>;
};

function traverseFunction(
fn: HIRFunction,
context: OptionalTraversalContext,
): void {
for (const [_, block] of fn.body.blocks) {
for (const instr of block.instructions) {
if (
instr.value.kind === 'FunctionExpression' ||
instr.value.kind === 'ObjectMethod'
) {
traverseFunction(instr.value.loweredFunc.func, {
...context,
currFn: instr.value.loweredFunc.func,
blocks: instr.value.loweredFunc.func.body.blocks,
});
}
}
if (
block.terminal.kind === 'optional' &&
!context.seenOptionals.has(block.id)
) {
traverseOptionalBlock(
block as TBasicBlock<OptionalTerminal>,
context,
null,
);
}
}
}
/**
* Match the consequent and alternate blocks of an optional.
* @returns propertyload computed by the consequent block, or null if the
Expand All @@ -137,7 +159,7 @@ function matchOptionalTestBlock(
consequentId: IdentifierId;
property: string;
propertyId: IdentifierId;
storeLocalInstrId: InstructionId;
storeLocalInstr: Instruction;
consequentGoto: BlockId;
} | null {
const consequentBlock = assertNonNull(blocks.get(terminal.consequent));
Expand All @@ -149,7 +171,7 @@ function matchOptionalTestBlock(
const propertyLoad: TInstruction<PropertyLoad> = consequentBlock
.instructions[0] as TInstruction<PropertyLoad>;
const storeLocal: StoreLocal = consequentBlock.instructions[1].value;
const storeLocalInstrId = consequentBlock.instructions[1].id;
const storeLocalInstr = consequentBlock.instructions[1];
CompilerError.invariant(
propertyLoad.value.object.identifier.id === terminal.test.identifier.id,
{
Expand Down Expand Up @@ -189,7 +211,7 @@ function matchOptionalTestBlock(
consequentId: storeLocal.lvalue.place.identifier.id,
property: propertyLoad.value.property,
propertyId: propertyLoad.lvalue.identifier.id,
storeLocalInstrId,
storeLocalInstr,
consequentGoto: consequentBlock.terminal.block,
};
}
Expand Down Expand Up @@ -369,10 +391,8 @@ function traverseOptionalBlock(
},
],
};
context.processedInstrsInOptional.add(
matchConsequentResult.storeLocalInstrId,
);
context.processedInstrsInOptional.add(test.id);
context.processedInstrsInOptional.add(matchConsequentResult.storeLocalInstr);
context.processedInstrsInOptional.add(test);
context.temporariesReadInOptional.set(
matchConsequentResult.consequentId,
load,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -231,15 +231,7 @@ const EnvironmentConfigSchema = z.object({
*/
enableUseTypeAnnotations: z.boolean().default(false),

enablePropagateDepsInHIR: z.boolean().default(false),

/**
* Enables inference of optional dependency chains. Without this flag
* a property chain such as `props?.items?.foo` will infer as a dep on
* just `props`. With this flag enabled, we'll infer that full path as
* the dependency.
*/
enableOptionalDependencies: z.boolean().default(true),
enableFunctionDependencyRewrite: z.boolean().default(true),

/**
* Enables inlining ReactElement object literals in place of JSX
Expand Down
Loading
Loading