Conversation
Greptile OverviewGreptile SummaryThis PR adds constant propagation for ternary expressions, allowing the compiler to optimize expressions like The implementation splits the constant propagation pass into two phases: first collecting constants across all blocks, then applying optimizations. This enables cross-block propagation needed for ternaries, where the test value is in a different block than the ternary terminal. Critical Issues:
Architecture Note: Confidence Score: 1/5
Important Files ChangedFile Analysis
|
| instr.value.value.identifier.id, | ||
| ); | ||
| if (identifierValue != null) { | ||
| // constants.set(value.lvalue.place.identifier.id, placeValue); |
There was a problem hiding this comment.
style: commented-out code should be removed
| // constants.set(value.lvalue.place.identifier.id, placeValue); | |
| instr.value.value = identifierValue; |
Prompt To Fix With AI
This is a comment left during a code review.
Path: compiler/packages/babel-plugin-react-compiler/src/Optimization/ConstantPropagation.ts
Line: 144:144
Comment:
**style:** commented-out code should be removed
```suggestion
instr.value.value = identifierValue;
```
How can I resolve this? If you propose a fix, please make it concise.|
|
||
| /** | ||
| * If enabled, will validate useMemos that don't return any values: | ||
| * If enabled, ConstantPropgation will try to resolve ternaries. |
There was a problem hiding this comment.
syntax: typo: "ConstantPropgation" should be "ConstantPropagation"
| * If enabled, ConstantPropgation will try to resolve ternaries. | |
| * If enabled, ConstantPropagation will try to resolve ternaries. |
Prompt To Fix With AI
This is a comment left during a code review.
Path: compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts
Line: 629:629
Comment:
**syntax:** typo: "ConstantPropgation" should be "ConstantPropagation"
```suggestion
* If enabled, ConstantPropagation will try to resolve ternaries.
```
How can I resolve this? If you propose a fix, please make it concise.| : branchBlock.terminal.alternate; | ||
|
|
||
| const chosenBlock = fn.body.blocks.get(targetBlockId); | ||
| if (chosenBlock?.terminal.kind === 'goto') { |
There was a problem hiding this comment.
logic: this logic only handles the case where chosenBlock?.terminal.kind === 'goto', but doesn't update the terminal if this condition is false. This could leave the ternary terminal unresolved even when the test value is constant, potentially causing incomplete optimization.
Prompt To Fix With AI
This is a comment left during a code review.
Path: compiler/packages/babel-plugin-react-compiler/src/Optimization/ConstantPropagation.ts
Line: 200:200
Comment:
**logic:** this logic only handles the case where `chosenBlock?.terminal.kind === 'goto'`, but doesn't update the terminal if this condition is false. This could leave the ternary terminal unresolved even when the test value is constant, potentially causing incomplete optimization.
How can I resolve this? If you propose a fix, please make it concise.fdfa7a6 to
cb1fef4
Compare
cb1fef4 to
fdfa7a6
Compare
| @@ -121,6 +119,8 @@ function applyConstantPropagation( | |||
| } | |||
| } | |||
|
|
|||
| const localReassignments = new Map<IdentifierId, Place>(); | |||
There was a problem hiding this comment.
style: localReassignments only tracks reassignments within a single block, but StoreLocal values may depend on values from previous blocks. Since constants are already global across blocks, tracking local reassignments separately may miss cross-block propagation opportunities or create inconsistencies.
Prompt To Fix With AI
This is a comment left during a code review.
Path: compiler/packages/babel-plugin-react-compiler/src/Optimization/ConstantPropagation.ts
Line: 122:122
Comment:
**style:** `localReassignments` only tracks reassignments within a single block, but `StoreLocal` values may depend on values from previous blocks. Since constants are already global across blocks, tracking local reassignments separately may miss cross-block propagation opportunities or create inconsistencies.
How can I resolve this? If you propose a fix, please make it concise.fdfa7a6 to
cb1fef4
Compare
| branchBlock.terminal.fallthrough, | ||
| ); | ||
|
|
||
| if (fallthrough?.terminal.kind == 'goto') { |
There was a problem hiding this comment.
style: using loose equality == instead of strict equality ===
| if (fallthrough?.terminal.kind == 'goto') { | |
| if (fallthrough?.terminal.kind === 'goto') { |
Prompt To Fix With AI
This is a comment left during a code review.
Path: compiler/packages/babel-plugin-react-compiler/src/Optimization/ConstantPropagation.ts
Line: 197:197
Comment:
**style:** using loose equality `==` instead of strict equality `===`
```suggestion
if (fallthrough?.terminal.kind === 'goto') {
```
How can I resolve this? If you propose a fix, please make it concise.| // I think I can only set this if the block isn't | ||
| // used in a value position by its predecessor | ||
| block.kind = 'block'; |
There was a problem hiding this comment.
style: setting block.kind = 'block' unconditionally may be incorrect if the block is legitimately a sequence/value block for other reasons
Prompt To Fix With AI
This is a comment left during a code review.
Path: compiler/packages/babel-plugin-react-compiler/src/Optimization/ConstantPropagation.ts
Line: 181:183
Comment:
**style:** setting `block.kind = 'block'` unconditionally may be incorrect if the block is legitimately a sequence/value block for other reasons
How can I resolve this? If you propose a fix, please make it concise.| * const x = b; | ||
| */ | ||
| validateNoVoidUseMemo: z.boolean().default(true), | ||
| enableTernaryConstantPropagation: z.boolean().default(true), |
There was a problem hiding this comment.
style: flag defaults to true but PR description states "currently disabled"—verify the intended default value
Prompt To Fix With AI
This is a comment left during a code review.
Path: compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts
Line: 635:635
Comment:
**style:** flag defaults to `true` but PR description states "currently disabled"—verify the intended default value
How can I resolve this? If you propose a fix, please make it concise.cb1fef4 to
fdfa7a6
Compare
| if (testValue !== null && testValue.kind === 'Primitive') { | ||
| hasChanges = true; | ||
| const targetBlockId = testValue.value | ||
| ? branchBlock.terminal.consequent | ||
| : branchBlock.terminal.alternate; | ||
|
|
||
| const chosenBlock = fn.body.blocks.get(targetBlockId); | ||
| if (chosenBlock?.terminal.kind === 'goto') { | ||
| block.terminal = { | ||
| kind: 'goto', | ||
| variant: GotoVariant.Break, | ||
| block: targetBlockId, | ||
| id: terminal.id, | ||
| loc: terminal.loc, | ||
| }; | ||
| } | ||
| } |
There was a problem hiding this comment.
logic: hasChanges set to true before checking if optimization can be applied, causing infinite loop if chosenBlock?.terminal.kind !== 'goto'
| if (testValue !== null && testValue.kind === 'Primitive') { | |
| hasChanges = true; | |
| const targetBlockId = testValue.value | |
| ? branchBlock.terminal.consequent | |
| : branchBlock.terminal.alternate; | |
| const chosenBlock = fn.body.blocks.get(targetBlockId); | |
| if (chosenBlock?.terminal.kind === 'goto') { | |
| block.terminal = { | |
| kind: 'goto', | |
| variant: GotoVariant.Break, | |
| block: targetBlockId, | |
| id: terminal.id, | |
| loc: terminal.loc, | |
| }; | |
| } | |
| } | |
| if (testValue !== null && testValue.kind === 'Primitive') { | |
| const targetBlockId = testValue.value | |
| ? branchBlock.terminal.consequent | |
| : branchBlock.terminal.alternate; | |
| const chosenBlock = fn.body.blocks.get(targetBlockId); | |
| if (chosenBlock?.terminal.kind === 'goto') { | |
| hasChanges = true; | |
| block.terminal = { | |
| kind: 'goto', | |
| variant: GotoVariant.Break, | |
| block: targetBlockId, | |
| id: terminal.id, | |
| loc: terminal.loc, | |
| }; | |
| } | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: compiler/packages/babel-plugin-react-compiler/src/Optimization/ConstantPropagation.ts
Line: 193:209
Comment:
**logic:** `hasChanges` set to `true` before checking if optimization can be applied, causing infinite loop if `chosenBlock?.terminal.kind !== 'goto'`
```suggestion
if (testValue !== null && testValue.kind === 'Primitive') {
const targetBlockId = testValue.value
? branchBlock.terminal.consequent
: branchBlock.terminal.alternate;
const chosenBlock = fn.body.blocks.get(targetBlockId);
if (chosenBlock?.terminal.kind === 'goto') {
hasChanges = true;
block.terminal = {
kind: 'goto',
variant: GotoVariant.Break,
block: targetBlockId,
id: terminal.id,
loc: terminal.loc,
};
}
}
```
How can I resolve this? If you propose a fix, please make it concise.fdfa7a6 to
cb1fef4
Compare
| branchBlock.terminal.fallthrough, | ||
| ); | ||
|
|
||
| if (fallthrough?.terminal.kind == 'goto') { |
There was a problem hiding this comment.
style: inconsistent equality operator - use === instead of == to match lines 206 and 210
| if (fallthrough?.terminal.kind == 'goto') { | |
| if (fallthrough?.terminal.kind === 'goto') { |
Prompt To Fix With AI
This is a comment left during a code review.
Path: compiler/packages/babel-plugin-react-compiler/src/Optimization/ConstantPropagation.ts
Line: 197:197
Comment:
**style:** inconsistent equality operator - use `===` instead of `==` to match lines 206 and 210
```suggestion
if (fallthrough?.terminal.kind === 'goto') {
```
How can I resolve this? If you propose a fix, please make it concise.cb1fef4 to
fdfa7a6
Compare
| const chosenBlock = fn.body.blocks.get(targetBlockId); | ||
| if (chosenBlock?.terminal.kind === 'goto') { | ||
| block.terminal = { | ||
| kind: 'goto', | ||
| variant: GotoVariant.Break, | ||
| block: targetBlockId, | ||
| id: terminal.id, | ||
| loc: terminal.loc, | ||
| }; | ||
| } |
There was a problem hiding this comment.
logic: when chosenBlock?.terminal.kind !== 'goto', the block terminal remains a ternary, but hasChanges = true on line 194 causes an infinite loop since the fixpoint iteration never terminates
| const chosenBlock = fn.body.blocks.get(targetBlockId); | |
| if (chosenBlock?.terminal.kind === 'goto') { | |
| block.terminal = { | |
| kind: 'goto', | |
| variant: GotoVariant.Break, | |
| block: targetBlockId, | |
| id: terminal.id, | |
| loc: terminal.loc, | |
| }; | |
| } | |
| const chosenBlock = fn.body.blocks.get(targetBlockId); | |
| if (chosenBlock?.terminal.kind === 'goto') { | |
| hasChanges = true; | |
| block.terminal = { | |
| kind: 'goto', | |
| variant: GotoVariant.Break, | |
| block: targetBlockId, | |
| id: terminal.id, | |
| loc: terminal.loc, | |
| }; | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: compiler/packages/babel-plugin-react-compiler/src/Optimization/ConstantPropagation.ts
Line: 199:208
Comment:
**logic:** when `chosenBlock?.terminal.kind !== 'goto'`, the block terminal remains a ternary, but `hasChanges = true` on line 194 causes an infinite loop since the fixpoint iteration never terminates
```suggestion
const chosenBlock = fn.body.blocks.get(targetBlockId);
if (chosenBlock?.terminal.kind === 'goto') {
hasChanges = true;
block.terminal = {
kind: 'goto',
variant: GotoVariant.Break,
block: targetBlockId,
id: terminal.id,
loc: terminal.loc,
};
}
```
How can I resolve this? If you propose a fix, please make it concise.| switch (instr.value.kind) { | ||
| case 'StoreLocal': { | ||
| const identifierValue = localReassignments.get( | ||
| instr.value.value.identifier.id, | ||
| ); | ||
| if (identifierValue != null) { | ||
| // constants.set(value.lvalue.place.identifier.id, placeValue); | ||
| instr.value.value = identifierValue; | ||
| } | ||
|
|
||
| localReassignments.set( | ||
| instr.value.lvalue.place.identifier.id, | ||
| instr.value.value, | ||
| ); | ||
| } |
There was a problem hiding this comment.
style: localReassignments only tracks within a single block, but the ternary optimization needs constants from previous blocks. Since you're iterating all blocks to collect constants (lines 109-155), consider tracking StoreLocal propagation in the global constants map instead of a local map that gets discarded after each block
Prompt To Fix With AI
This is a comment left during a code review.
Path: compiler/packages/babel-plugin-react-compiler/src/Optimization/ConstantPropagation.ts
Line: 138:152
Comment:
**style:** `localReassignments` only tracks within a single block, but the ternary optimization needs constants from previous blocks. Since you're iterating all blocks to collect constants (lines 109-155), consider tracking `StoreLocal` propagation in the global `constants` map instead of a local map that gets discarded after each block
How can I resolve this? If you propose a fix, please make it concise.| const wasTernaryConstantPropagationEnabled = | ||
| env.config.enableTernaryConstantPropagation; | ||
| env.config.enableTernaryConstantPropagation = false; | ||
| constantPropagation(hir); | ||
| log({kind: 'hir', name: 'ConstantPropagation', value: hir}); | ||
|
|
||
| env.config.enableTernaryConstantPropagation = | ||
| wasTernaryConstantPropagationEnabled; | ||
| constantPropagation(hir); | ||
| log({kind: 'hir', name: 'ConstantPropagationTernary', value: hir}); |
There was a problem hiding this comment.
style: running constant propagation twice doubles the cost. Consider a single-pass approach where ternary optimization is integrated into the main constant propagation loop, or use a flag to skip the first pass when ternary propagation is disabled
Prompt To Fix With AI
This is a comment left during a code review.
Path: compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts
Line: 193:202
Comment:
**style:** running constant propagation twice doubles the cost. Consider a single-pass approach where ternary optimization is integrated into the main constant propagation loop, or use a flag to skip the first pass when ternary propagation is disabled
How can I resolve this? If you propose a fix, please make it concise.fdfa7a6 to
cb1fef4
Compare
cb1fef4 to
fdfa7a6
Compare
| hasChanges = true; | ||
| const targetBlockId = testValue.value | ||
| ? branchBlock.terminal.consequent | ||
| : branchBlock.terminal.alternate; | ||
|
|
||
| const chosenBlock = fn.body.blocks.get(targetBlockId); | ||
| if (chosenBlock?.terminal.kind === 'goto') { | ||
| block.terminal = { | ||
| kind: 'goto', | ||
| variant: GotoVariant.Break, | ||
| block: targetBlockId, | ||
| id: terminal.id, | ||
| loc: terminal.loc, | ||
| }; | ||
| } |
There was a problem hiding this comment.
logic: hasChanges = true on line 194 occurs before verifying optimization is possible on line 200. When chosenBlock?.terminal.kind !== 'goto', the ternary terminal remains unchanged but hasChanges = true triggers infinite loop in the fixpoint iteration.
| hasChanges = true; | |
| const targetBlockId = testValue.value | |
| ? branchBlock.terminal.consequent | |
| : branchBlock.terminal.alternate; | |
| const chosenBlock = fn.body.blocks.get(targetBlockId); | |
| if (chosenBlock?.terminal.kind === 'goto') { | |
| block.terminal = { | |
| kind: 'goto', | |
| variant: GotoVariant.Break, | |
| block: targetBlockId, | |
| id: terminal.id, | |
| loc: terminal.loc, | |
| }; | |
| } | |
| const testValue = read(constants, branchBlock.terminal.test); | |
| if (testValue !== null && testValue.kind === 'Primitive') { | |
| const targetBlockId = testValue.value | |
| ? branchBlock.terminal.consequent | |
| : branchBlock.terminal.alternate; | |
| const chosenBlock = fn.body.blocks.get(targetBlockId); | |
| if (chosenBlock?.terminal.kind === 'goto') { | |
| hasChanges = true; | |
| block.terminal = { | |
| kind: 'goto', | |
| variant: GotoVariant.Break, | |
| block: targetBlockId, | |
| id: terminal.id, | |
| loc: terminal.loc, | |
| }; | |
| } | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: compiler/packages/babel-plugin-react-compiler/src/Optimization/ConstantPropagation.ts
Line: 194:208
Comment:
**logic:** `hasChanges = true` on line 194 occurs before verifying optimization is possible on line 200. When `chosenBlock?.terminal.kind !== 'goto'`, the ternary terminal remains unchanged but `hasChanges = true` triggers infinite loop in the fixpoint iteration.
```suggestion
const testValue = read(constants, branchBlock.terminal.test);
if (testValue !== null && testValue.kind === 'Primitive') {
const targetBlockId = testValue.value
? branchBlock.terminal.consequent
: branchBlock.terminal.alternate;
const chosenBlock = fn.body.blocks.get(targetBlockId);
if (chosenBlock?.terminal.kind === 'goto') {
hasChanges = true;
block.terminal = {
kind: 'goto',
variant: GotoVariant.Break,
block: targetBlockId,
id: terminal.id,
loc: terminal.loc,
};
}
}
```
How can I resolve this? If you propose a fix, please make it concise.| const chosenBlock = fn.body.blocks.get(targetBlockId); | ||
| if (chosenBlock?.terminal.kind === 'goto') { | ||
| block.terminal = { | ||
| kind: 'goto', | ||
| variant: GotoVariant.Break, | ||
| block: targetBlockId, | ||
| id: terminal.id, | ||
| loc: terminal.loc, | ||
| }; | ||
| } |
There was a problem hiding this comment.
style: When chosenBlock?.terminal.kind !== 'goto', the ternary isn't resolved but remains in the HIR. Since the ternary test value is constant but the target block isn't a simple goto, the ternary block stays unoptimized. Consider whether you should either transform the block differently or continue with the unoptimized ternary
Prompt To Fix With AI
This is a comment left during a code review.
Path: compiler/packages/babel-plugin-react-compiler/src/Optimization/ConstantPropagation.ts
Line: 199:208
Comment:
**style:** When `chosenBlock?.terminal.kind !== 'goto'`, the ternary isn't resolved but remains in the HIR. Since the ternary test value is constant but the target block isn't a simple goto, the ternary block stays unoptimized. Consider whether you should either transform the block differently or continue with the unoptimized ternary
How can I resolve this? If you propose a fix, please make it concise.| switch (instr.value.kind) { | ||
| case 'StoreLocal': { | ||
| const identifierValue = localReassignments.get( | ||
| instr.value.value.identifier.id, | ||
| ); | ||
| if (identifierValue != null) { | ||
| // constants.set(value.lvalue.place.identifier.id, placeValue); | ||
| instr.value.value = identifierValue; | ||
| } | ||
|
|
||
| localReassignments.set( | ||
| instr.value.lvalue.place.identifier.id, | ||
| instr.value.value, | ||
| ); | ||
| } |
There was a problem hiding this comment.
style: localReassignments only tracks within a single block but is discarded after each block iteration. Since ternary optimization needs constants from previous blocks (which are in the global constants map), verify this local tracking integrates correctly with the cross-block ternary logic
Prompt To Fix With AI
This is a comment left during a code review.
Path: compiler/packages/babel-plugin-react-compiler/src/Optimization/ConstantPropagation.ts
Line: 138:152
Comment:
**style:** `localReassignments` only tracks within a single block but is discarded after each block iteration. Since ternary optimization needs constants from previous blocks (which are in the global `constants` map), verify this local tracking integrates correctly with the cross-block ternary logic
How can I resolve this? If you propose a fix, please make it concise.fdfa7a6 to
cb1fef4
Compare
| hasChanges = true; | ||
| const targetBlockId = testValue.value | ||
| ? branchBlock.terminal.consequent | ||
| : branchBlock.terminal.alternate; | ||
|
|
||
| const chosenBlock = fn.body.blocks.get(targetBlockId); | ||
| if (chosenBlock?.terminal.kind === 'goto') { | ||
| block.terminal = { | ||
| kind: 'goto', | ||
| variant: GotoVariant.Break, | ||
| block: targetBlockId, | ||
| id: terminal.id, | ||
| loc: terminal.loc, | ||
| }; | ||
| } |
There was a problem hiding this comment.
logic: infinite loop: hasChanges = true set on line 194 before verifying the terminal actually gets updated on line 201. when chosenBlock?.terminal.kind !== 'goto' (line 200 check fails), the ternary terminal stays unchanged but hasChanges = true causes the fixpoint loop at line 63 to never terminate
move line 194 inside the if block after line 207:
| hasChanges = true; | |
| const targetBlockId = testValue.value | |
| ? branchBlock.terminal.consequent | |
| : branchBlock.terminal.alternate; | |
| const chosenBlock = fn.body.blocks.get(targetBlockId); | |
| if (chosenBlock?.terminal.kind === 'goto') { | |
| block.terminal = { | |
| kind: 'goto', | |
| variant: GotoVariant.Break, | |
| block: targetBlockId, | |
| id: terminal.id, | |
| loc: terminal.loc, | |
| }; | |
| } | |
| const testValue = read(constants, branchBlock.terminal.test); | |
| if (testValue !== null && testValue.kind === 'Primitive') { | |
| const targetBlockId = testValue.value | |
| ? branchBlock.terminal.consequent | |
| : branchBlock.terminal.alternate; | |
| const chosenBlock = fn.body.blocks.get(targetBlockId); | |
| if (chosenBlock?.terminal.kind === 'goto') { | |
| hasChanges = true; | |
| block.terminal = { | |
| kind: 'goto', | |
| variant: GotoVariant.Break, | |
| block: targetBlockId, | |
| id: terminal.id, | |
| loc: terminal.loc, | |
| }; | |
| } | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: compiler/packages/babel-plugin-react-compiler/src/Optimization/ConstantPropagation.ts
Line: 194:208
Comment:
**logic:** infinite loop: `hasChanges = true` set on line 194 before verifying the terminal actually gets updated on line 201. when `chosenBlock?.terminal.kind !== 'goto'` (line 200 check fails), the ternary terminal stays unchanged but `hasChanges = true` causes the fixpoint loop at line 63 to never terminate
move line 194 inside the if block after line 207:
```suggestion
const testValue = read(constants, branchBlock.terminal.test);
if (testValue !== null && testValue.kind === 'Primitive') {
const targetBlockId = testValue.value
? branchBlock.terminal.consequent
: branchBlock.terminal.alternate;
const chosenBlock = fn.body.blocks.get(targetBlockId);
if (chosenBlock?.terminal.kind === 'goto') {
hasChanges = true;
block.terminal = {
kind: 'goto',
variant: GotoVariant.Break,
block: targetBlockId,
id: terminal.id,
loc: terminal.loc,
};
}
}
```
How can I resolve this? If you propose a fix, please make it concise.|
|
||
| /** | ||
| * If enabled, will validate useMemos that don't return any values: | ||
| * If enabled, ConstantPropgation will try to resolve ternaries. |
There was a problem hiding this comment.
syntax: typo: "ConstantPropgation" missing 'a'
| * If enabled, ConstantPropgation will try to resolve ternaries. | |
| * If enabled, ConstantPropagation will try to resolve ternaries. |
Prompt To Fix With AI
This is a comment left during a code review.
Path: compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts
Line: 629:629
Comment:
**syntax:** typo: "ConstantPropgation" missing 'a'
```suggestion
* If enabled, ConstantPropagation will try to resolve ternaries.
```
How can I resolve this? If you propose a fix, please make it concise.| switch (instr.value.kind) { | ||
| case 'StoreLocal': { | ||
| const identifierValue = localReassignments.get( | ||
| instr.value.value.identifier.id, | ||
| ); | ||
| if (identifierValue != null) { | ||
| // constants.set(value.lvalue.place.identifier.id, placeValue); | ||
| instr.value.value = identifierValue; | ||
| } | ||
|
|
||
| localReassignments.set( | ||
| instr.value.lvalue.place.identifier.id, | ||
| instr.value.value, | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
syntax: missing break statement after StoreLocal case - if additional cases added later, this will fall through
| switch (instr.value.kind) { | |
| case 'StoreLocal': { | |
| const identifierValue = localReassignments.get( | |
| instr.value.value.identifier.id, | |
| ); | |
| if (identifierValue != null) { | |
| // constants.set(value.lvalue.place.identifier.id, placeValue); | |
| instr.value.value = identifierValue; | |
| } | |
| localReassignments.set( | |
| instr.value.lvalue.place.identifier.id, | |
| instr.value.value, | |
| ); | |
| } | |
| } | |
| switch (instr.value.kind) { | |
| case 'StoreLocal': { | |
| const identifierValue = localReassignments.get( | |
| instr.value.value.identifier.id, | |
| ); | |
| if (identifierValue != null) { | |
| // constants.set(value.lvalue.place.identifier.id, placeValue); | |
| instr.value.value = identifierValue; | |
| } | |
| localReassignments.set( | |
| instr.value.lvalue.place.identifier.id, | |
| instr.value.value, | |
| ); | |
| break; | |
| } | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: compiler/packages/babel-plugin-react-compiler/src/Optimization/ConstantPropagation.ts
Line: 138:153
Comment:
**syntax:** missing `break` statement after `StoreLocal` case - if additional cases added later, this will fall through
```suggestion
switch (instr.value.kind) {
case 'StoreLocal': {
const identifierValue = localReassignments.get(
instr.value.value.identifier.id,
);
if (identifierValue != null) {
// constants.set(value.lvalue.place.identifier.id, placeValue);
instr.value.value = identifierValue;
}
localReassignments.set(
instr.value.lvalue.place.identifier.id,
instr.value.value,
);
break;
}
}
```
How can I resolve this? If you propose a fix, please make it concise.| const identifierValue = localReassignments.get( | ||
| instr.value.value.identifier.id, | ||
| ); | ||
| if (identifierValue != null) { |
There was a problem hiding this comment.
style: use !== instead of != for consistency with strict equality checks elsewhere in the codebase
| if (identifierValue != null) { | |
| if (identifierValue !== null) { |
Prompt To Fix With AI
This is a comment left during a code review.
Path: compiler/packages/babel-plugin-react-compiler/src/Optimization/ConstantPropagation.ts
Line: 143:143
Comment:
**style:** use `!==` instead of `!=` for consistency with strict equality checks elsewhere in the codebase
```suggestion
if (identifierValue !== null) {
```
How can I resolve this? If you propose a fix, please make it concise.| instr.value.value.identifier.id, | ||
| ); | ||
| if (identifierValue != null) { | ||
| // constants.set(value.lvalue.place.identifier.id, placeValue); |
There was a problem hiding this comment.
style: remove commented-out code
Prompt To Fix With AI
This is a comment left during a code review.
Path: compiler/packages/babel-plugin-react-compiler/src/Optimization/ConstantPropagation.ts
Line: 144:144
Comment:
**style:** remove commented-out code
How can I resolve this? If you propose a fix, please make it concise.| const wasTernaryConstantPropagationEnabled = | ||
| env.config.enableTernaryConstantPropagation; | ||
| env.config.enableTernaryConstantPropagation = false; | ||
| constantPropagation(hir); | ||
| log({kind: 'hir', name: 'ConstantPropagation', value: hir}); | ||
|
|
||
| env.config.enableTernaryConstantPropagation = | ||
| wasTernaryConstantPropagationEnabled; | ||
| constantPropagation(hir); | ||
| log({kind: 'hir', name: 'ConstantPropagationTernary', value: hir}); |
There was a problem hiding this comment.
style: running constant propagation twice doubles compilation cost for every function. the first pass (lines 196-197) executes the full fixpoint loop even when enableTernaryConstantPropagation is false. consider integrating ternary optimization into a single pass or refactoring to avoid redundant work
Prompt To Fix With AI
This is a comment left during a code review.
Path: compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts
Line: 193:202
Comment:
**style:** running constant propagation twice doubles compilation cost for every function. the first pass (lines 196-197) executes the full fixpoint loop even when `enableTernaryConstantPropagation` is `false`. consider integrating ternary optimization into a single pass or refactoring to avoid redundant work
How can I resolve this? If you propose a fix, please make it concise.|
|
||
| /** | ||
| * If enabled, will validate useMemos that don't return any values: | ||
| * If enabled, ConstantPropgation will try to resolve ternaries. |
There was a problem hiding this comment.
syntax: typo: "ConstantPropgation" missing 'a'
| * If enabled, ConstantPropgation will try to resolve ternaries. | |
| * If enabled, ConstantPropagation will try to resolve ternaries. |
Prompt To Fix With AI
This is a comment left during a code review.
Path: compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts
Line: 627:627
Comment:
**syntax:** typo: "ConstantPropgation" missing 'a'
```suggestion
* If enabled, ConstantPropagation will try to resolve ternaries.
```
How can I resolve this? If you propose a fix, please make it concise.| branchBlock.terminal.fallthrough, | ||
| ); | ||
|
|
||
| if (fallthrough?.terminal.kind == 'goto') { |
There was a problem hiding this comment.
style: use === instead of == for consistency with lines 206 and 210
Prompt To Fix With AI
This is a comment left during a code review.
Path: compiler/packages/babel-plugin-react-compiler/src/Optimization/ConstantPropagation.ts
Line: 197:197
Comment:
**style:** use `===` instead of `==` for consistency with lines 206 and 210
How can I resolve this? If you propose a fix, please make it concise.| // I think I can only set this if the block isn't | ||
| // used in a value position by its predecessor | ||
| block.kind = 'block'; |
There was a problem hiding this comment.
style: comment expresses uncertainty but code sets block.kind = 'block' unconditionally - verify this is correct for all ternary blocks or make it conditional
Prompt To Fix With AI
This is a comment left during a code review.
Path: compiler/packages/babel-plugin-react-compiler/src/Optimization/ConstantPropagation.ts
Line: 181:183
Comment:
**style:** comment expresses uncertainty but code sets `block.kind = 'block'` unconditionally - verify this is correct for all ternary blocks or make it conditional
How can I resolve this? If you propose a fix, please make it concise.| const wasTernaryConstantPropagationEnabled = | ||
| env.config.enableTernaryConstantPropagation; | ||
| env.config.enableTernaryConstantPropagation = false; | ||
| constantPropagation(hir); | ||
| log({kind: 'hir', name: 'ConstantPropagation', value: hir}); | ||
|
|
||
| env.config.enableTernaryConstantPropagation = | ||
| wasTernaryConstantPropagationEnabled; | ||
| constantPropagation(hir); | ||
| log({kind: 'hir', name: 'ConstantPropagationTernary', value: hir}); |
There was a problem hiding this comment.
style: running constant propagation twice doubles compilation cost for every function. the first pass (lines 195-196) executes the full fixpoint loop even when enableTernaryConstantPropagation is false. consider integrating ternary optimization into a single pass
Prompt To Fix With AI
This is a comment left during a code review.
Path: compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts
Line: 192:201
Comment:
**style:** running constant propagation twice doubles compilation cost for every function. the first pass (lines 195-196) executes the full fixpoint loop even when `enableTernaryConstantPropagation` is `false`. consider integrating ternary optimization into a single pass
How can I resolve this? If you propose a fix, please make it concise.|
|
||
| const testValue = read(constants, branchBlock.terminal.test); | ||
| if (testValue !== null && testValue.kind === 'Primitive') { | ||
| hasChanges = true; |
There was a problem hiding this comment.
logic: infinite loop: hasChanges = true set before verifying terminal actually gets modified. when chosenBlock?.terminal.kind !== 'goto' check fails on line 200, the ternary terminal stays unchanged but the fixpoint loop at line 63 never terminates
move this line inside the if block after line 207 to only set hasChanges when optimization succeeds:
| hasChanges = true; | |
| const chosenBlock = fn.body.blocks.get(targetBlockId); | |
| if (chosenBlock?.terminal.kind === 'goto') { | |
| hasChanges = true; | |
| block.terminal = { |
Prompt To Fix With AI
This is a comment left during a code review.
Path: compiler/packages/babel-plugin-react-compiler/src/Optimization/ConstantPropagation.ts
Line: 194:194
Comment:
**logic:** infinite loop: `hasChanges = true` set before verifying terminal actually gets modified. when `chosenBlock?.terminal.kind !== 'goto'` check fails on line 200, the ternary terminal stays unchanged but the fixpoint loop at line 63 never terminates
move this line inside the if block after line 207 to only set `hasChanges` when optimization succeeds:
```suggestion
const chosenBlock = fn.body.blocks.get(targetBlockId);
if (chosenBlock?.terminal.kind === 'goto') {
hasChanges = true;
block.terminal = {
```
How can I resolve this? If you propose a fix, please make it concise.| switch (instr.value.kind) { | ||
| case 'StoreLocal': { | ||
| const identifierValue = localReassignments.get( | ||
| instr.value.value.identifier.id, | ||
| ); | ||
| if (identifierValue != null) { | ||
| // constants.set(value.lvalue.place.identifier.id, placeValue); | ||
| instr.value.value = identifierValue; | ||
| } | ||
|
|
||
| localReassignments.set( | ||
| instr.value.lvalue.place.identifier.id, | ||
| instr.value.value, | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
syntax: missing break statement after StoreLocal case - if additional cases are added later, this will fall through
| switch (instr.value.kind) { | |
| case 'StoreLocal': { | |
| const identifierValue = localReassignments.get( | |
| instr.value.value.identifier.id, | |
| ); | |
| if (identifierValue != null) { | |
| // constants.set(value.lvalue.place.identifier.id, placeValue); | |
| instr.value.value = identifierValue; | |
| } | |
| localReassignments.set( | |
| instr.value.lvalue.place.identifier.id, | |
| instr.value.value, | |
| ); | |
| } | |
| } | |
| switch (instr.value.kind) { | |
| case 'StoreLocal': { | |
| const identifierValue = localReassignments.get( | |
| instr.value.value.identifier.id, | |
| ); | |
| if (identifierValue != null) { | |
| // constants.set(value.lvalue.place.identifier.id, placeValue); | |
| instr.value.value = identifierValue; | |
| } | |
| localReassignments.set( | |
| instr.value.lvalue.place.identifier.id, | |
| instr.value.value, | |
| ); | |
| break; | |
| } | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: compiler/packages/babel-plugin-react-compiler/src/Optimization/ConstantPropagation.ts
Line: 138:153
Comment:
**syntax:** missing `break` statement after `StoreLocal` case - if additional cases are added later, this will fall through
```suggestion
switch (instr.value.kind) {
case 'StoreLocal': {
const identifierValue = localReassignments.get(
instr.value.value.identifier.id,
);
if (identifierValue != null) {
// constants.set(value.lvalue.place.identifier.id, placeValue);
instr.value.value = identifierValue;
}
localReassignments.set(
instr.value.lvalue.place.identifier.id,
instr.value.value,
);
break;
}
}
```
How can I resolve this? If you propose a fix, please make it concise.| @@ -671,56 +626,208 @@ export const EnvironmentConfigSchema = z.object({ | |||
| lowerContextAccess: ExternalFunctionSchema.nullable().default(null), | |||
|
|
|||
| /** | |||
There was a problem hiding this comment.
syntax: typo: missing 'a' in "ConstantPropgation"
| /** | |
| * If enabled, ConstantPropagation will try to resolve ternaries. |
Prompt To Fix With AI
This is a comment left during a code review.
Path: compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts
Line: 628:628
Comment:
**syntax:** typo: missing 'a' in "ConstantPropgation"
```suggestion
* If enabled, ConstantPropagation will try to resolve ternaries.
```
How can I resolve this? If you propose a fix, please make it concise.| const wasTernaryConstantPropagationEnabled = | ||
| env.config.enableTernaryConstantPropagation; | ||
| env.config.enableTernaryConstantPropagation = false; | ||
| constantPropagation(hir); | ||
| log({kind: 'hir', name: 'ConstantPropagation', value: hir}); | ||
|
|
||
| env.config.enableTernaryConstantPropagation = | ||
| wasTernaryConstantPropagationEnabled; | ||
| constantPropagation(hir); | ||
| log({kind: 'hir', name: 'ConstantPropagationTernary', value: hir}); |
There was a problem hiding this comment.
style: running constant propagation twice doubles compilation cost for every function. the first pass (lines 196-197) executes the full fixpoint loop even when enableTernaryConstantPropagation is false
consider integrating ternary optimization into a single pass or refactoring to avoid redundant work
Prompt To Fix With AI
This is a comment left during a code review.
Path: compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts
Line: 193:202
Comment:
**style:** running constant propagation twice doubles compilation cost for every function. the first pass (lines 196-197) executes the full fixpoint loop even when `enableTernaryConstantPropagation` is `false`
consider integrating ternary optimization into a single pass or refactoring to avoid redundant work
How can I resolve this? If you propose a fix, please make it concise.| const identifierValue = localReassignments.get( | ||
| instr.value.value.identifier.id, | ||
| ); | ||
| if (identifierValue != null) { |
There was a problem hiding this comment.
style: use !== instead of != for consistency with strict equality checks elsewhere in the codebase
| if (identifierValue != null) { | |
| if (identifierValue !== null) { |
Prompt To Fix With AI
This is a comment left during a code review.
Path: compiler/packages/babel-plugin-react-compiler/src/Optimization/ConstantPropagation.ts
Line: 143:143
Comment:
**style:** use `!==` instead of `!=` for consistency with strict equality checks elsewhere in the codebase
```suggestion
if (identifierValue !== null) {
```
How can I resolve this? If you propose a fix, please make it concise.|
|
||
| /** | ||
| * If enabled, will validate useMemos that don't return any values: | ||
| * If enabled, ConstantPropgation will try to resolve ternaries. |
There was a problem hiding this comment.
syntax: typo: missing 'a' in "ConstantPropgation"
| * If enabled, ConstantPropgation will try to resolve ternaries. | |
| * If enabled, ConstantPropagation will try to resolve ternaries. |
Prompt To Fix With AI
This is a comment left during a code review.
Path: compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts
Line: 627:627
Comment:
**syntax:** typo: missing 'a' in "ConstantPropgation"
```suggestion
* If enabled, ConstantPropagation will try to resolve ternaries.
```
How can I resolve this? If you propose a fix, please make it concise.| branchBlock.terminal.fallthrough, | ||
| ); | ||
|
|
||
| if (fallthrough?.terminal.kind == 'goto') { |
There was a problem hiding this comment.
style: inconsistent equality operator - use === instead of == to match lines 206 and 210
| if (fallthrough?.terminal.kind == 'goto') { | |
| if (fallthrough?.terminal.kind === 'goto') { |
Prompt To Fix With AI
This is a comment left during a code review.
Path: compiler/packages/babel-plugin-react-compiler/src/Optimization/ConstantPropagation.ts
Line: 197:197
Comment:
**style:** inconsistent equality operator - use `===` instead of `==` to match lines 206 and 210
```suggestion
if (fallthrough?.terminal.kind === 'goto') {
```
How can I resolve this? If you propose a fix, please make it concise.| const wasTernaryConstantPropagationEnabled = | ||
| env.config.enableTernaryConstantPropagation; | ||
| env.config.enableTernaryConstantPropagation = false; | ||
| constantPropagation(hir); | ||
| log({kind: 'hir', name: 'ConstantPropagation', value: hir}); | ||
|
|
||
| env.config.enableTernaryConstantPropagation = | ||
| wasTernaryConstantPropagationEnabled; | ||
| constantPropagation(hir); | ||
| log({kind: 'hir', name: 'ConstantPropagationTernary', value: hir}); |
There was a problem hiding this comment.
style: running constant propagation twice doubles compilation cost for every function. the first pass (lines 195-196) executes the full fixpoint loop even when enableTernaryConstantPropagation is false. consider integrating ternary optimization into a single pass or refactoring to avoid redundant work
Prompt To Fix With AI
This is a comment left during a code review.
Path: compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts
Line: 192:201
Comment:
**style:** running constant propagation twice doubles compilation cost for every function. the first pass (lines 195-196) executes the full fixpoint loop even when `enableTernaryConstantPropagation` is `false`. consider integrating ternary optimization into a single pass or refactoring to avoid redundant work
How can I resolve this? If you propose a fix, please make it concise.| // I think I can only set this if the block isn't | ||
| // used in a value position by its predecessor | ||
| block.kind = 'block'; |
There was a problem hiding this comment.
style: comment expresses uncertainty but code sets block.kind = 'block' unconditionally - verify this is correct for all ternary blocks or make it conditional based on whether the block is used in a value position by its predecessor
Prompt To Fix With AI
This is a comment left during a code review.
Path: compiler/packages/babel-plugin-react-compiler/src/Optimization/ConstantPropagation.ts
Line: 181:183
Comment:
**style:** comment expresses uncertainty but code sets `block.kind = 'block'` unconditionally - verify this is correct for all ternary blocks or make it conditional based on whether the block is used in a value position by its predecessor
How can I resolve this? If you propose a fix, please make it concise.|
|
||
| const testValue = read(constants, branchBlock.terminal.test); | ||
| if (testValue !== null && testValue.kind === 'Primitive') { | ||
| hasChanges = true; |
There was a problem hiding this comment.
logic: infinite loop: hasChanges = true set before verifying terminal actually gets modified. when chosenBlock?.terminal.kind !== 'goto' check fails (line 200), ternary stays unchanged but fixpoint loop at line 63 never terminates
move this line inside the if block after line 207 to only set when optimization succeeds
Prompt To Fix With AI
This is a comment left during a code review.
Path: compiler/packages/babel-plugin-react-compiler/src/Optimization/ConstantPropagation.ts
Line: 194:194
Comment:
**logic:** infinite loop: `hasChanges = true` set before verifying terminal actually gets modified. when `chosenBlock?.terminal.kind !== 'goto'` check fails (line 200), ternary stays unchanged but fixpoint loop at line 63 never terminates
move this line inside the if block after line 207 to only set when optimization succeeds
How can I resolve this? If you propose a fix, please make it concise.| switch (instr.value.kind) { | ||
| case 'StoreLocal': { | ||
| const identifierValue = localReassignments.get( | ||
| instr.value.value.identifier.id, | ||
| ); | ||
| if (identifierValue != null) { | ||
| // constants.set(value.lvalue.place.identifier.id, placeValue); | ||
| instr.value.value = identifierValue; | ||
| } | ||
|
|
||
| localReassignments.set( | ||
| instr.value.lvalue.place.identifier.id, | ||
| instr.value.value, | ||
| ); | ||
| } |
There was a problem hiding this comment.
syntax: missing break - if additional cases added later, will fall through
Prompt To Fix With AI
This is a comment left during a code review.
Path: compiler/packages/babel-plugin-react-compiler/src/Optimization/ConstantPropagation.ts
Line: 138:152
Comment:
**syntax:** missing `break` - if additional cases added later, will fall through
How can I resolve this? If you propose a fix, please make it concise.|
|
||
| const testValue = read(constants, branchBlock.terminal.test); | ||
| if (testValue !== null && testValue.kind === 'Primitive') { | ||
| hasChanges = true; |
There was a problem hiding this comment.
logic: infinite loop: hasChanges = true before verifying terminal gets updated. when chosenBlock?.terminal.kind !== 'goto' check fails (line 200), ternary stays unchanged but fixpoint loop at line 63 never terminates
move this line inside the if block after line 207 to only set when optimization succeeds
Prompt To Fix With AI
This is a comment left during a code review.
Path: compiler/packages/babel-plugin-react-compiler/src/Optimization/ConstantPropagation.ts
Line: 194:194
Comment:
**logic:** infinite loop: `hasChanges = true` before verifying terminal gets updated. when `chosenBlock?.terminal.kind !== 'goto'` check fails (line 200), ternary stays unchanged but fixpoint loop at line 63 never terminates
move this line inside the if block after line 207 to only set when optimization succeeds
How can I resolve this? If you propose a fix, please make it concise.|
|
||
| /** | ||
| * If enabled, will validate useMemos that don't return any values: | ||
| * If enabled, ConstantPropgation will try to resolve ternaries. |
There was a problem hiding this comment.
syntax: typo: missing 'a' in "ConstantPropgation"
Prompt To Fix With AI
This is a comment left during a code review.
Path: compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts
Line: 629:629
Comment:
**syntax:** typo: missing 'a' in "ConstantPropgation"
How can I resolve this? If you propose a fix, please make it concise.| hasChanges = true; | ||
| const targetBlockId = testValue.value | ||
| ? branchBlock.terminal.consequent | ||
| : branchBlock.terminal.alternate; | ||
|
|
||
| const chosenBlock = fn.body.blocks.get(targetBlockId); | ||
| if (chosenBlock?.terminal.kind === 'goto') { | ||
| block.terminal = { | ||
| kind: 'goto', | ||
| variant: GotoVariant.Break, | ||
| block: targetBlockId, | ||
| id: terminal.id, | ||
| loc: terminal.loc, | ||
| }; | ||
| } |
There was a problem hiding this comment.
logic: setting hasChanges = true on line 194 before verifying the terminal transformation succeeds causes infinite loop. when the check chosenBlock?.terminal.kind !== 'goto' fails on line 200, the ternary terminal remains unchanged but the fixpoint loop at line 63 never terminates.
| hasChanges = true; | |
| const targetBlockId = testValue.value | |
| ? branchBlock.terminal.consequent | |
| : branchBlock.terminal.alternate; | |
| const chosenBlock = fn.body.blocks.get(targetBlockId); | |
| if (chosenBlock?.terminal.kind === 'goto') { | |
| block.terminal = { | |
| kind: 'goto', | |
| variant: GotoVariant.Break, | |
| block: targetBlockId, | |
| id: terminal.id, | |
| loc: terminal.loc, | |
| }; | |
| } | |
| const testValue = read(constants, branchBlock.terminal.test); | |
| if (testValue !== null && testValue.kind === 'Primitive') { | |
| const targetBlockId = testValue.value | |
| ? branchBlock.terminal.consequent | |
| : branchBlock.terminal.alternate; | |
| const chosenBlock = fn.body.blocks.get(targetBlockId); | |
| if (chosenBlock?.terminal.kind === 'goto') { | |
| hasChanges = true; | |
| block.terminal = { | |
| kind: 'goto', | |
| variant: GotoVariant.Break, | |
| block: targetBlockId, | |
| id: terminal.id, | |
| loc: terminal.loc, | |
| }; | |
| } | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: compiler/packages/babel-plugin-react-compiler/src/Optimization/ConstantPropagation.ts
Line: 194:208
Comment:
**logic:** setting `hasChanges = true` on line 194 before verifying the terminal transformation succeeds causes infinite loop. when the check `chosenBlock?.terminal.kind !== 'goto'` fails on line 200, the ternary terminal remains unchanged but the fixpoint loop at line 63 never terminates.
```suggestion
const testValue = read(constants, branchBlock.terminal.test);
if (testValue !== null && testValue.kind === 'Primitive') {
const targetBlockId = testValue.value
? branchBlock.terminal.consequent
: branchBlock.terminal.alternate;
const chosenBlock = fn.body.blocks.get(targetBlockId);
if (chosenBlock?.terminal.kind === 'goto') {
hasChanges = true;
block.terminal = {
kind: 'goto',
variant: GotoVariant.Break,
block: targetBlockId,
id: terminal.id,
loc: terminal.loc,
};
}
}
```
How can I resolve this? If you propose a fix, please make it concise.|
|
||
| const testValue = read(constants, branchBlock.terminal.test); | ||
| if (testValue !== null && testValue.kind === 'Primitive') { | ||
| hasChanges = true; |
There was a problem hiding this comment.
logic: infinite loop: hasChanges = true set before verifying terminal actually gets modified. when chosenBlock?.terminal.kind !== 'goto' check fails on line 200, the ternary terminal stays unchanged but the fixpoint loop at line 63 never terminates
move this line inside the if block after line 207 to only set when optimization succeeds
Prompt To Fix With AI
This is a comment left during a code review.
Path: compiler/packages/babel-plugin-react-compiler/src/Optimization/ConstantPropagation.ts
Line: 194:194
Comment:
**logic:** infinite loop: `hasChanges = true` set before verifying terminal actually gets modified. when `chosenBlock?.terminal.kind !== 'goto'` check fails on line 200, the ternary terminal stays unchanged but the fixpoint loop at line 63 never terminates
move this line inside the if block after line 207 to only set when optimization succeeds
How can I resolve this? If you propose a fix, please make it concise.|
|
||
| /** | ||
| * If enabled, will validate useMemos that don't return any values: | ||
| * If enabled, ConstantPropgation will try to resolve ternaries. |
There was a problem hiding this comment.
syntax: typo: "ConstantPropgation" missing 'a'
| * If enabled, ConstantPropgation will try to resolve ternaries. | |
| * If enabled, ConstantPropagation will try to resolve ternaries. |
Prompt To Fix With AI
This is a comment left during a code review.
Path: compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts
Line: 629:629
Comment:
**syntax:** typo: "ConstantPropgation" missing 'a'
```suggestion
* If enabled, ConstantPropagation will try to resolve ternaries.
```
How can I resolve this? If you propose a fix, please make it concise.| const testValue = read(constants, branchBlock.terminal.test); | ||
| if (testValue !== null && testValue.kind === 'Primitive') { | ||
| hasChanges = true; | ||
| const targetBlockId = testValue.value |
There was a problem hiding this comment.
style: use === instead of == for consistency with lines 206 and 210
Prompt To Fix With AI
This is a comment left during a code review.
Path: compiler/packages/babel-plugin-react-compiler/src/Optimization/ConstantPropagation.ts
Line: 195:195
Comment:
**style:** use `===` instead of `==` for consistency with lines 206 and 210
How can I resolve this? If you propose a fix, please make it concise.| constantPropagation(hir); | ||
| log({kind: 'hir', name: 'ConstantPropagation', value: hir}); | ||
|
|
||
| env.config.enableTernaryConstantPropagation = | ||
| wasTernaryConstantPropagationEnabled; | ||
| constantPropagation(hir); |
There was a problem hiding this comment.
style: running constant propagation twice doubles compilation cost for every function. the first pass still executes the full fixpoint loop even when enableTernaryConstantPropagation is false. consider integrating ternary optimization into a single pass or refactoring to avoid redundant work
Prompt To Fix With AI
This is a comment left during a code review.
Path: compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts
Line: 196:201
Comment:
**style:** running constant propagation twice doubles compilation cost for every function. the first pass still executes the full fixpoint loop even when `enableTernaryConstantPropagation` is `false`. consider integrating ternary optimization into a single pass or refactoring to avoid redundant work
How can I resolve this? If you propose a fix, please make it concise.| switch (instr.value.kind) { | ||
| case 'StoreLocal': { | ||
| const identifierValue = localReassignments.get( | ||
| instr.value.value.identifier.id, | ||
| ); | ||
| if (identifierValue != null) { | ||
| // constants.set(value.lvalue.place.identifier.id, placeValue); | ||
| instr.value.value = identifierValue; | ||
| } | ||
|
|
||
| localReassignments.set( | ||
| instr.value.lvalue.place.identifier.id, | ||
| instr.value.value, | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
syntax: missing break statement after StoreLocal case - if additional cases are added later, this will fall through
Prompt To Fix With AI
This is a comment left during a code review.
Path: compiler/packages/babel-plugin-react-compiler/src/Optimization/ConstantPropagation.ts
Line: 138:153
Comment:
**syntax:** missing `break` statement after `StoreLocal` case - if additional cases are added later, this will fall through
How can I resolve this? If you propose a fix, please make it concise.|
|
||
| const testValue = read(constants, branchBlock.terminal.test); | ||
| if (testValue !== null && testValue.kind === 'Primitive') { | ||
| hasChanges = true; |
There was a problem hiding this comment.
logic: infinite loop: setting hasChanges = true before verifying the terminal actually gets modified causes the fixpoint loop at line 63 to run forever when the optimization can't be applied. when chosenBlock?.terminal.kind !== 'goto' (line 200 check fails), the ternary terminal remains unchanged but the loop never terminates.
move this line inside the if block after line 207 to only set hasChanges when the optimization actually succeeds:
| hasChanges = true; | |
| const testValue = read(constants, branchBlock.terminal.test); | |
| if (testValue !== null && testValue.kind === 'Primitive') { | |
| const targetBlockId = testValue.value | |
| ? branchBlock.terminal.consequent | |
| : branchBlock.terminal.alternate; | |
| const chosenBlock = fn.body.blocks.get(targetBlockId); | |
| if (chosenBlock?.terminal.kind === 'goto') { | |
| hasChanges = true; | |
| block.terminal = { |
Prompt To Fix With AI
This is a comment left during a code review.
Path: compiler/packages/babel-plugin-react-compiler/src/Optimization/ConstantPropagation.ts
Line: 194:194
Comment:
**logic:** infinite loop: setting `hasChanges = true` before verifying the terminal actually gets modified causes the fixpoint loop at line 63 to run forever when the optimization can't be applied. when `chosenBlock?.terminal.kind !== 'goto'` (line 200 check fails), the ternary terminal remains unchanged but the loop never terminates.
move this line inside the if block after line 207 to only set `hasChanges` when the optimization actually succeeds:
```suggestion
const testValue = read(constants, branchBlock.terminal.test);
if (testValue !== null && testValue.kind === 'Primitive') {
const targetBlockId = testValue.value
? branchBlock.terminal.consequent
: branchBlock.terminal.alternate;
const chosenBlock = fn.body.blocks.get(targetBlockId);
if (chosenBlock?.terminal.kind === 'goto') {
hasChanges = true;
block.terminal = {
```
How can I resolve this? If you propose a fix, please make it concise.| branchBlock.terminal.fallthrough, | ||
| ); | ||
|
|
||
| if (fallthrough?.terminal.kind == 'goto') { |
There was a problem hiding this comment.
style: inconsistent equality operator - use === instead of == to match lines 206 and 210
Prompt To Fix With AI
This is a comment left during a code review.
Path: compiler/packages/babel-plugin-react-compiler/src/Optimization/ConstantPropagation.ts
Line: 197:197
Comment:
**style:** inconsistent equality operator - use `===` instead of `==` to match lines 206 and 210
How can I resolve this? If you propose a fix, please make it concise.|
|
||
| /** | ||
| * If enabled, will validate useMemos that don't return any values: | ||
| * If enabled, ConstantPropgation will try to resolve ternaries. |
There was a problem hiding this comment.
syntax: typo: "ConstantPropgation" missing 'a'
| * If enabled, ConstantPropgation will try to resolve ternaries. | |
| * If enabled, ConstantPropagation will try to resolve ternaries. |
Prompt To Fix With AI
This is a comment left during a code review.
Path: compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts
Line: 627:627
Comment:
**syntax:** typo: "ConstantPropgation" missing 'a'
```suggestion
* If enabled, ConstantPropagation will try to resolve ternaries.
```
How can I resolve this? If you propose a fix, please make it concise.|
|
||
| const testValue = read(constants, branchBlock.terminal.test); | ||
| if (testValue !== null && testValue.kind === 'Primitive') { | ||
| hasChanges = true; |
There was a problem hiding this comment.
logic: infinite loop: hasChanges = true before verifying terminal gets updated. when chosenBlock?.terminal.kind !== 'goto' check fails (line 200), ternary stays unchanged but fixpoint loop at line 63 never terminates
move this line inside the if block after line 207
Prompt To Fix With AI
This is a comment left during a code review.
Path: compiler/packages/babel-plugin-react-compiler/src/Optimization/ConstantPropagation.ts
Line: 194:194
Comment:
**logic:** infinite loop: `hasChanges = true` before verifying terminal gets updated. when `chosenBlock?.terminal.kind !== 'goto'` check fails (line 200), ternary stays unchanged but fixpoint loop at line 63 never terminates
move this line inside the if block after line 207
How can I resolve this? If you propose a fix, please make it concise.|
|
||
| /** | ||
| * If enabled, will validate useMemos that don't return any values: | ||
| * If enabled, ConstantPropgation will try to resolve ternaries. |
There was a problem hiding this comment.
syntax: typo: missing 'a' in "ConstantPropgation"
Prompt To Fix With AI
This is a comment left during a code review.
Path: compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts
Line: 629:629
Comment:
**syntax:** typo: missing 'a' in "ConstantPropgation"
How can I resolve this? If you propose a fix, please make it concise.| instr.value.lvalue.place.identifier.id, | ||
| instr.value.value, | ||
| ); | ||
| } |
There was a problem hiding this comment.
syntax: missing break - if additional cases added later, will fall through
Prompt To Fix With AI
This is a comment left during a code review.
Path: compiler/packages/babel-plugin-react-compiler/src/Optimization/ConstantPropagation.ts
Line: 152:152
Comment:
**syntax:** missing `break` - if additional cases added later, will fall through
How can I resolve this? If you propose a fix, please make it concise.| instr.value.value.identifier.id, | ||
| ); | ||
| if (identifierValue != null) { | ||
| // constants.set(value.lvalue.place.identifier.id, placeValue); |
There was a problem hiding this comment.
style: remove commented-out code
Prompt To Fix With AI
This is a comment left during a code review.
Path: compiler/packages/babel-plugin-react-compiler/src/Optimization/ConstantPropagation.ts
Line: 144:144
Comment:
**style:** remove commented-out code
How can I resolve this? If you propose a fix, please make it concise.| hasChanges = true; | ||
| const targetBlockId = testValue.value | ||
| ? branchBlock.terminal.consequent | ||
| : branchBlock.terminal.alternate; | ||
|
|
||
| const chosenBlock = fn.body.blocks.get(targetBlockId); | ||
| if (chosenBlock?.terminal.kind === 'goto') { | ||
| block.terminal = { | ||
| kind: 'goto', | ||
| variant: GotoVariant.Break, | ||
| block: targetBlockId, | ||
| id: terminal.id, | ||
| loc: terminal.loc, | ||
| }; | ||
| } |
There was a problem hiding this comment.
logic: infinite loop: hasChanges = true on line 194 before verifying terminal actually gets updated on line 201. when chosenBlock?.terminal.kind !== 'goto' (line 200 check fails), the ternary terminal stays unchanged but hasChanges = true causes the fixpoint loop at line 63 to never terminate.
| hasChanges = true; | |
| const targetBlockId = testValue.value | |
| ? branchBlock.terminal.consequent | |
| : branchBlock.terminal.alternate; | |
| const chosenBlock = fn.body.blocks.get(targetBlockId); | |
| if (chosenBlock?.terminal.kind === 'goto') { | |
| block.terminal = { | |
| kind: 'goto', | |
| variant: GotoVariant.Break, | |
| block: targetBlockId, | |
| id: terminal.id, | |
| loc: terminal.loc, | |
| }; | |
| } | |
| const testValue = read(constants, branchBlock.terminal.test); | |
| if (testValue !== null && testValue.kind === 'Primitive') { | |
| const targetBlockId = testValue.value | |
| ? branchBlock.terminal.consequent | |
| : branchBlock.terminal.alternate; | |
| const chosenBlock = fn.body.blocks.get(targetBlockId); | |
| if (chosenBlock?.terminal.kind === 'goto') { | |
| hasChanges = true; | |
| block.terminal = { | |
| kind: 'goto', | |
| variant: GotoVariant.Break, | |
| block: targetBlockId, | |
| id: terminal.id, | |
| loc: terminal.loc, | |
| }; | |
| } | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: compiler/packages/babel-plugin-react-compiler/src/Optimization/ConstantPropagation.ts
Line: 194:208
Comment:
**logic:** infinite loop: `hasChanges = true` on line 194 before verifying terminal actually gets updated on line 201. when `chosenBlock?.terminal.kind !== 'goto'` (line 200 check fails), the ternary terminal stays unchanged but `hasChanges = true` causes the fixpoint loop at line 63 to never terminate.
```suggestion
const testValue = read(constants, branchBlock.terminal.test);
if (testValue !== null && testValue.kind === 'Primitive') {
const targetBlockId = testValue.value
? branchBlock.terminal.consequent
: branchBlock.terminal.alternate;
const chosenBlock = fn.body.blocks.get(targetBlockId);
if (chosenBlock?.terminal.kind === 'goto') {
hasChanges = true;
block.terminal = {
kind: 'goto',
variant: GotoVariant.Break,
block: targetBlockId,
id: terminal.id,
loc: terminal.loc,
};
}
}
```
How can I resolve this? If you propose a fix, please make it concise.| hasChanges = true; | ||
| const targetBlockId = testValue.value | ||
| ? branchBlock.terminal.consequent | ||
| : branchBlock.terminal.alternate; | ||
|
|
||
| const chosenBlock = fn.body.blocks.get(targetBlockId); | ||
| if (chosenBlock?.terminal.kind === 'goto') { | ||
| block.terminal = { | ||
| kind: 'goto', | ||
| variant: GotoVariant.Break, | ||
| block: targetBlockId, | ||
| id: terminal.id, | ||
| loc: terminal.loc, | ||
| }; | ||
| } |
There was a problem hiding this comment.
logic: infinite loop: setting hasChanges = true on line 194 before verifying the terminal actually gets updated causes the fixpoint loop to never terminate when the optimization can't be applied. when the check chosenBlock?.terminal.kind !== 'goto' fails on line 200, the ternary terminal remains unchanged but hasChanges = true causes the loop at line 63 to run forever.
move hasChanges = true to after line 207, inside the if block:
| hasChanges = true; | |
| const targetBlockId = testValue.value | |
| ? branchBlock.terminal.consequent | |
| : branchBlock.terminal.alternate; | |
| const chosenBlock = fn.body.blocks.get(targetBlockId); | |
| if (chosenBlock?.terminal.kind === 'goto') { | |
| block.terminal = { | |
| kind: 'goto', | |
| variant: GotoVariant.Break, | |
| block: targetBlockId, | |
| id: terminal.id, | |
| loc: terminal.loc, | |
| }; | |
| } | |
| const targetBlockId = testValue.value | |
| ? branchBlock.terminal.consequent | |
| : branchBlock.terminal.alternate; | |
| const chosenBlock = fn.body.blocks.get(targetBlockId); | |
| if (chosenBlock?.terminal.kind === 'goto') { | |
| hasChanges = true; | |
| block.terminal = { | |
| kind: 'goto', | |
| variant: GotoVariant.Break, | |
| block: targetBlockId, | |
| id: terminal.id, | |
| loc: terminal.loc, | |
| }; | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: compiler/packages/babel-plugin-react-compiler/src/Optimization/ConstantPropagation.ts
Line: 194:208
Comment:
**logic:** infinite loop: setting `hasChanges = true` on line 194 before verifying the terminal actually gets updated causes the fixpoint loop to never terminate when the optimization can't be applied. when the check `chosenBlock?.terminal.kind !== 'goto'` fails on line 200, the ternary terminal remains unchanged but `hasChanges = true` causes the loop at line 63 to run forever.
move `hasChanges = true` to after line 207, inside the if block:
```suggestion
const targetBlockId = testValue.value
? branchBlock.terminal.consequent
: branchBlock.terminal.alternate;
const chosenBlock = fn.body.blocks.get(targetBlockId);
if (chosenBlock?.terminal.kind === 'goto') {
hasChanges = true;
block.terminal = {
kind: 'goto',
variant: GotoVariant.Break,
block: targetBlockId,
id: terminal.id,
loc: terminal.loc,
};
}
```
How can I resolve this? If you propose a fix, please make it concise.| @@ -671,56 +626,208 @@ export const EnvironmentConfigSchema = z.object({ | |||
| lowerContextAccess: ExternalFunctionSchema.nullable().default(null), | |||
|
|
|||
| /** | |||
There was a problem hiding this comment.
syntax: typo: "ConstantPropgation" missing 'a'
| /** | |
| * If enabled, ConstantPropagation will try to resolve ternaries. |
Prompt To Fix With AI
This is a comment left during a code review.
Path: compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts
Line: 628:628
Comment:
**syntax:** typo: "ConstantPropgation" missing 'a'
```suggestion
* If enabled, ConstantPropagation will try to resolve ternaries.
```
How can I resolve this? If you propose a fix, please make it concise.| switch (instr.value.kind) { | ||
| case 'StoreLocal': { | ||
| const identifierValue = localReassignments.get( | ||
| instr.value.value.identifier.id, | ||
| ); | ||
| if (identifierValue != null) { | ||
| // constants.set(value.lvalue.place.identifier.id, placeValue); | ||
| instr.value.value = identifierValue; | ||
| } | ||
|
|
||
| localReassignments.set( | ||
| instr.value.lvalue.place.identifier.id, | ||
| instr.value.value, | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
syntax: missing break statement after StoreLocal case - will fall through if additional cases are added later
Prompt To Fix With AI
This is a comment left during a code review.
Path: compiler/packages/babel-plugin-react-compiler/src/Optimization/ConstantPropagation.ts
Line: 138:153
Comment:
**syntax:** missing `break` statement after `StoreLocal` case - will fall through if additional cases are added later
How can I resolve this? If you propose a fix, please make it concise.| const testValue = read(constants, branchBlock.terminal.test); | ||
| if (testValue !== null && testValue.kind === 'Primitive') { | ||
| hasChanges = true; | ||
| const targetBlockId = testValue.value |
There was a problem hiding this comment.
style: use === instead of == for consistency with strict equality checks on lines 206 and 210
Prompt To Fix With AI
This is a comment left during a code review.
Path: compiler/packages/babel-plugin-react-compiler/src/Optimization/ConstantPropagation.ts
Line: 195:195
Comment:
**style:** use `===` instead of `==` for consistency with strict equality checks on lines 206 and 210
How can I resolve this? If you propose a fix, please make it concise.|
|
||
| /** | ||
| * If enabled, will validate useMemos that don't return any values: | ||
| * If enabled, ConstantPropgation will try to resolve ternaries. |
There was a problem hiding this comment.
syntax: typo: missing 'a' in "ConstantPropgation"
Prompt To Fix With AI
This is a comment left during a code review.
Path: compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts
Line: 629:629
Comment:
**syntax:** typo: missing 'a' in "ConstantPropgation"
How can I resolve this? If you propose a fix, please make it concise.| instr.value.lvalue.place.identifier.id, | ||
| instr.value.value, | ||
| ); | ||
| } |
There was a problem hiding this comment.
syntax: missing break - if additional cases added later, will fall through
Prompt To Fix With AI
This is a comment left during a code review.
Path: compiler/packages/babel-plugin-react-compiler/src/Optimization/ConstantPropagation.ts
Line: 152:152
Comment:
**syntax:** missing `break` - if additional cases added later, will fall through
How can I resolve this? If you propose a fix, please make it concise.|
|
||
| const testValue = read(constants, branchBlock.terminal.test); | ||
| if (testValue !== null && testValue.kind === 'Primitive') { | ||
| hasChanges = true; |
There was a problem hiding this comment.
logic: infinite loop: hasChanges = true set before verifying terminal gets modified. when chosenBlock?.terminal.kind !== 'goto' check fails (line 200), ternary stays unchanged but fixpoint loop at line 63 never terminates
move this line inside the if block after line 207 to only set when optimization succeeds
Prompt To Fix With AI
This is a comment left during a code review.
Path: compiler/packages/babel-plugin-react-compiler/src/Optimization/ConstantPropagation.ts
Line: 194:194
Comment:
**logic:** infinite loop: `hasChanges = true` set before verifying terminal gets modified. when `chosenBlock?.terminal.kind !== 'goto'` check fails (line 200), ternary stays unchanged but fixpoint loop at line 63 never terminates
move this line inside the if block after line 207 to only set when optimization succeeds
How can I resolve this? If you propose a fix, please make it concise.| instr.value.value.identifier.id, | ||
| ); | ||
| if (identifierValue != null) { | ||
| // constants.set(value.lvalue.place.identifier.id, placeValue); |
There was a problem hiding this comment.
style: commented-out code
Prompt To Fix With AI
This is a comment left during a code review.
Path: compiler/packages/babel-plugin-react-compiler/src/Optimization/ConstantPropagation.ts
Line: 144:144
Comment:
**style:** commented-out code
How can I resolve this? If you propose a fix, please make it concise.| const wasTernaryConstantPropagationEnabled = | ||
| env.config.enableTernaryConstantPropagation; | ||
| env.config.enableTernaryConstantPropagation = false; | ||
| constantPropagation(hir); | ||
| log({kind: 'hir', name: 'ConstantPropagation', value: hir}); | ||
|
|
||
| env.config.enableTernaryConstantPropagation = | ||
| wasTernaryConstantPropagationEnabled; | ||
| constantPropagation(hir); | ||
| log({kind: 'hir', name: 'ConstantPropagationTernary', value: hir}); |
There was a problem hiding this comment.
style: running constant propagation twice doubles compilation cost for every function. the first pass (lines 196-197) executes the full fixpoint loop even when enableTernaryConstantPropagation is false. consider integrating ternary optimization into a single pass or refactoring to avoid redundant work
Prompt To Fix With AI
This is a comment left during a code review.
Path: compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts
Line: 193:202
Comment:
**style:** running constant propagation twice doubles compilation cost for every function. the first pass (lines 196-197) executes the full fixpoint loop even when `enableTernaryConstantPropagation` is `false`. consider integrating ternary optimization into a single pass or refactoring to avoid redundant work
How can I resolve this? If you propose a fix, please make it concise.|
|
||
| const testValue = read(constants, branchBlock.terminal.test); | ||
| if (testValue !== null && testValue.kind === 'Primitive') { | ||
| hasChanges = true; |
There was a problem hiding this comment.
logic: infinite loop: hasChanges = true before verifying terminal gets modified. when chosenBlock?.terminal.kind !== 'goto' check fails on line 200, ternary stays unchanged but fixpoint loop at line 63 never terminates
move this inside the if block after line 207:
| hasChanges = true; | |
| const chosenBlock = fn.body.blocks.get(targetBlockId); | |
| if (chosenBlock?.terminal.kind === 'goto') { | |
| hasChanges = true; | |
| block.terminal = { |
Prompt To Fix With AI
This is a comment left during a code review.
Path: compiler/packages/babel-plugin-react-compiler/src/Optimization/ConstantPropagation.ts
Line: 194:194
Comment:
**logic:** infinite loop: `hasChanges = true` before verifying terminal gets modified. when `chosenBlock?.terminal.kind !== 'goto'` check fails on line 200, ternary stays unchanged but fixpoint loop at line 63 never terminates
move this inside the if block after line 207:
```suggestion
const chosenBlock = fn.body.blocks.get(targetBlockId);
if (chosenBlock?.terminal.kind === 'goto') {
hasChanges = true;
block.terminal = {
```
How can I resolve this? If you propose a fix, please make it concise.| switch (instr.value.kind) { | ||
| case 'StoreLocal': { | ||
| const identifierValue = localReassignments.get( | ||
| instr.value.value.identifier.id, | ||
| ); | ||
| if (identifierValue != null) { | ||
| // constants.set(value.lvalue.place.identifier.id, placeValue); | ||
| instr.value.value = identifierValue; | ||
| } | ||
|
|
||
| localReassignments.set( | ||
| instr.value.lvalue.place.identifier.id, | ||
| instr.value.value, | ||
| ); | ||
| } |
There was a problem hiding this comment.
syntax: missing break - if additional cases added later, will fall through
Prompt To Fix With AI
This is a comment left during a code review.
Path: compiler/packages/babel-plugin-react-compiler/src/Optimization/ConstantPropagation.ts
Line: 138:152
Comment:
**syntax:** missing `break` - if additional cases added later, will fall through
How can I resolve this? If you propose a fix, please make it concise.| instr.value.value.identifier.id, | ||
| ); | ||
| if (identifierValue != null) { | ||
| // constants.set(value.lvalue.place.identifier.id, placeValue); |
There was a problem hiding this comment.
style: commented-out code
Prompt To Fix With AI
This is a comment left during a code review.
Path: compiler/packages/babel-plugin-react-compiler/src/Optimization/ConstantPropagation.ts
Line: 144:144
Comment:
**style:** commented-out code
How can I resolve this? If you propose a fix, please make it concise.|
|
||
| /** | ||
| * If enabled, will validate useMemos that don't return any values: | ||
| * If enabled, ConstantPropgation will try to resolve ternaries. |
There was a problem hiding this comment.
syntax: typo: missing 'a' in "ConstantPropgation"
Prompt To Fix With AI
This is a comment left during a code review.
Path: compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts
Line: 629:629
Comment:
**syntax:** typo: missing 'a' in "ConstantPropgation"
How can I resolve this? If you propose a fix, please make it concise.| const wasTernaryConstantPropagationEnabled = | ||
| env.config.enableTernaryConstantPropagation; | ||
| env.config.enableTernaryConstantPropagation = false; | ||
| constantPropagation(hir); | ||
| log({kind: 'hir', name: 'ConstantPropagation', value: hir}); | ||
|
|
||
| env.config.enableTernaryConstantPropagation = | ||
| wasTernaryConstantPropagationEnabled; | ||
| constantPropagation(hir); | ||
| log({kind: 'hir', name: 'ConstantPropagationTernary', value: hir}); |
There was a problem hiding this comment.
style: running constant propagation twice doubles compilation cost for every function. the first pass still executes the full fixpoint loop even when enableTernaryConstantPropagation is false. consider integrating ternary optimization into a single pass or refactoring to avoid redundant work
Prompt To Fix With AI
This is a comment left during a code review.
Path: compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts
Line: 193:202
Comment:
**style:** running constant propagation twice doubles compilation cost for every function. the first pass still executes the full fixpoint loop even when `enableTernaryConstantPropagation` is `false`. consider integrating ternary optimization into a single pass or refactoring to avoid redundant work
How can I resolve this? If you propose a fix, please make it concise.| const testValue = read(constants, branchBlock.terminal.test); | ||
|
|
||
| if (testValue !== null && testValue.kind === 'Primitive') { | ||
| hasChanges = true; |
There was a problem hiding this comment.
logic: infinite loop: setting hasChanges = true before verifying the terminal actually gets modified causes the fixpoint loop at line 63 to run forever when the optimization can't be applied
when the ternary test value is constant but the subsequent checks fail (e.g., branch block not found, terminal not a branch, etc.), the ternary terminal remains unchanged but hasChanges = true causes infinite iteration
move this line after all checks pass and the terminal is successfully updated (after line 191)
Prompt To Fix With AI
This is a comment left during a code review.
Path: compiler/packages/babel-plugin-react-compiler/src/Optimization/ConstantPropagation.ts
Line: 176:176
Comment:
**logic:** infinite loop: setting `hasChanges = true` before verifying the terminal actually gets modified causes the fixpoint loop at line 63 to run forever when the optimization can't be applied
when the ternary test value is constant but the subsequent checks fail (e.g., branch block not found, terminal not a branch, etc.), the ternary terminal remains unchanged but `hasChanges = true` causes infinite iteration
move this line after all checks pass and the terminal is successfully updated (after line 191)
How can I resolve this? If you propose a fix, please make it concise.| branchBlock.terminal.fallthrough, | ||
| ); | ||
|
|
||
| if (fallthrough?.terminal.kind == 'goto') { |
There was a problem hiding this comment.
style: use === instead of == for consistency with lines 206 and 210
Prompt To Fix With AI
This is a comment left during a code review.
Path: compiler/packages/babel-plugin-react-compiler/src/Optimization/ConstantPropagation.ts
Line: 197:197
Comment:
**style:** use `===` instead of `==` for consistency with lines 206 and 210
How can I resolve this? If you propose a fix, please make it concise.| const wasTernaryConstantPropagationEnabled = | ||
| env.config.enableTernaryConstantPropagation; | ||
| env.config.enableTernaryConstantPropagation = false; | ||
| constantPropagation(hir); | ||
| log({kind: 'hir', name: 'ConstantPropagation', value: hir}); | ||
|
|
||
| env.config.enableTernaryConstantPropagation = | ||
| wasTernaryConstantPropagationEnabled; | ||
| constantPropagation(hir); | ||
| log({kind: 'hir', name: 'ConstantPropagationTernary', value: hir}); |
There was a problem hiding this comment.
style: running constant propagation twice doubles compilation cost for every function. the first pass (lines 195-196) executes the full fixpoint loop even when enableTernaryConstantPropagation is false
consider integrating ternary optimization into a single pass or refactoring to avoid redundant work
Prompt To Fix With AI
This is a comment left during a code review.
Path: compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts
Line: 192:201
Comment:
**style:** running constant propagation twice doubles compilation cost for every function. the first pass (lines 195-196) executes the full fixpoint loop even when `enableTernaryConstantPropagation` is `false`
consider integrating ternary optimization into a single pass or refactoring to avoid redundant work
How can I resolve this? If you propose a fix, please make it concise.|
|
||
| /** | ||
| * If enabled, will validate useMemos that don't return any values: | ||
| * If enabled, ConstantPropgation will try to resolve ternaries. |
There was a problem hiding this comment.
syntax: typo: "ConstantPropgation" missing 'a'
| * If enabled, ConstantPropgation will try to resolve ternaries. | |
| * If enabled, ConstantPropagation will try to resolve ternaries. |
Prompt To Fix With AI
This is a comment left during a code review.
Path: compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts
Line: 627:627
Comment:
**syntax:** typo: "ConstantPropgation" missing 'a'
```suggestion
* If enabled, ConstantPropagation will try to resolve ternaries.
```
How can I resolve this? If you propose a fix, please make it concise.| // I think I can only set this if the block isn't | ||
| // used in a value position by its predecessor | ||
| block.kind = 'block'; |
There was a problem hiding this comment.
style: comment expresses uncertainty but code sets block.kind = 'block' unconditionally
verify this is correct for all ternary blocks or make it conditional based on whether the block is used in a value position by its predecessor
Prompt To Fix With AI
This is a comment left during a code review.
Path: compiler/packages/babel-plugin-react-compiler/src/Optimization/ConstantPropagation.ts
Line: 181:183
Comment:
**style:** comment expresses uncertainty but code sets `block.kind = 'block'` unconditionally
verify this is correct for all ternary blocks or make it conditional based on whether the block is used in a value position by its predecessor
How can I resolve this? If you propose a fix, please make it concise.| const testValue = read(constants, branchBlock.terminal.test); | ||
|
|
||
| if (testValue !== null && testValue.kind === 'Primitive') { | ||
| hasChanges = true; |
There was a problem hiding this comment.
logic: hasChanges = true set before verifying optimization succeeds. when the chosen block isn't a goto (line 206-207 checks), terminal stays unchanged but fixpoint loop at line 63 runs infinitely.
move after line 212 inside the transformation logic
Prompt To Fix With AI
This is a comment left during a code review.
Path: compiler/packages/babel-plugin-react-compiler/src/Optimization/ConstantPropagation.ts
Line: 176:176
Comment:
**logic:** `hasChanges = true` set before verifying optimization succeeds. when the chosen block isn't a goto (line 206-207 checks), terminal stays unchanged but fixpoint loop at line 63 runs infinitely.
move after line 212 inside the transformation logic
How can I resolve this? If you propose a fix, please make it concise.| branchBlock.terminal.fallthrough, | ||
| ); | ||
|
|
||
| if (fallthrough?.terminal.kind == 'goto') { |
There was a problem hiding this comment.
style: loose equality == instead of strict === (inconsistent with lines 206 and 210)
Prompt To Fix With AI
This is a comment left during a code review.
Path: compiler/packages/babel-plugin-react-compiler/src/Optimization/ConstantPropagation.ts
Line: 197:197
Comment:
**style:** loose equality `==` instead of strict `===` (inconsistent with lines 206 and 210)
How can I resolve this? If you propose a fix, please make it concise.|
|
||
| /** | ||
| * If enabled, will validate useMemos that don't return any values: | ||
| * If enabled, ConstantPropgation will try to resolve ternaries. |
There was a problem hiding this comment.
syntax: typo: "ConstantPropgation" missing 'a'
| * If enabled, ConstantPropgation will try to resolve ternaries. | |
| * If enabled, ConstantPropagation will try to resolve ternaries. |
Prompt To Fix With AI
This is a comment left during a code review.
Path: compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts
Line: 627:627
Comment:
**syntax:** typo: "ConstantPropgation" missing 'a'
```suggestion
* If enabled, ConstantPropagation will try to resolve ternaries.
```
How can I resolve this? If you propose a fix, please make it concise.| constantPropagation(hir); | ||
| log({kind: 'hir', name: 'ConstantPropagation', value: hir}); |
There was a problem hiding this comment.
style: first pass runs full fixpoint loop even when enableTernaryConstantPropagation is false, doubling compilation cost for every function
Prompt To Fix With AI
This is a comment left during a code review.
Path: compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts
Line: 195:196
Comment:
**style:** first pass runs full fixpoint loop even when `enableTernaryConstantPropagation` is `false`, doubling compilation cost for every function
How can I resolve this? If you propose a fix, please make it concise.
Mirror of facebook/react#32222
Original author: elicwhite
Summary
Some basic constant propagation for ternaries.
This has a feature flag, currently disabled.
This change, even when the flag is turned off, does double the number of iterations that needs to be taken through the blocks in the file.
The previous logic was to detect all the local values within the current block (storing in constants), and if you get to an if statement, and that test is in your lookup, use it.
However, with ternaries, the test is in a different block that follows the if statement so it's not clear to me if you can do the single for loop with instructions and blocks together.
Now, it iterates through all the blocks and stores the constants, then iterates over the blocks again to try to inline.
This doesn't seem great, I'm curious if there is a better approach.
How did you test this change?
Jest with the flag disabled, only enabled for the single test. When the flag is enabled globally, there are some other cases it fails on that need to be addressed.
Playground: https://react-compiler-playground-git-fork-elicwhit-fcc939-fbopensource.vercel.app