Skip to content

Commit

Permalink
Update on "[compiler] Transitively freezing functions marks values as…
Browse files Browse the repository at this point in the history
… frozen, not effects"

The fixture from the previous PR was getting inconsistent behavior because of the following:
1. Create an object in a useMemo
2. Create a callback in a useCallback, where the callback captures the object from (1) into a local object, then passes that local object into a logging method. We have to assume the logging method could modify the local object, and transitively, the object from (1).
3. Call the callback during render.
4. Pass the callback to JSX.

We correctly infer that the object from (1) is captured and modified in (2). However, in (4) we transitively freeze the callback. When transitively freezing functions we were previously doing two things: updating our internal abstract model of the program values to reflect the values as being frozen *and* also updating function operands to change their effects to freeze.

As the case above demonstrates, that can clobber over information about real potential mutability. The potential fix here is to only walk our abstract value model to mark values as frozen, but _not_ override operand effects. Conceptually, this is a forward data flow propagation — but walking backward to update effects is pushing information backwards in the algorithm. An alternative would be to mark that data was propagated backwards, and trigger another loop over the CFG to propagate information forward again given the updated effects. But the fix in this PR is more correct.

[ghstack-poisoned]
  • Loading branch information
josephsavona committed Aug 21, 2024
2 parents 28ee171 + f41935a commit 517951a
Show file tree
Hide file tree
Showing 2 changed files with 8 additions and 12 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@ import {
CallExpression,
Effect,
FunctionEffect,
FunctionExpression,
GeneratedSource,
HIRFunction,
IdentifierId,
Expand All @@ -36,7 +35,6 @@ import {
import {FunctionSignature} from '../HIR/ObjectShape';
import {
printIdentifier,
printInstruction,
printMixedHIR,
printPlace,
printSourceLocation,
Expand Down Expand Up @@ -468,10 +466,12 @@ class InferenceState {
this.#env.config.enableTransitivelyFreezeFunctionExpressions
) {
if (value.kind === 'FunctionExpression') {
// We want to freeze the captured values, not mark the operands
// themselves as frozen. There could be mutations that occur
// before the freeze we are processing, and it would be invalid
// to overwrite those mutations as a freeze.
/*
* We want to freeze the captured values, not mark the operands
* themselves as frozen. There could be mutations that occur
* before the freeze we are processing, and it would be invalid
* to overwrite those mutations as a freeze.
*/
for (const operand of eachInstructionValueOperand(value)) {
const operandValues = this.#variables.get(operand.identifier.id);
if (operandValues !== undefined) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
* LICENSE file in the root directory of this source tree.
*/

import {CompilerError, Effect, ErrorSeverity, printReactiveFunction} from '..';
import {CompilerError, Effect, ErrorSeverity} from '..';
import {
DeclarationId,
GeneratedSource,
Expand All @@ -23,11 +23,7 @@ import {
ScopeId,
SourceLocation,
} from '../HIR';
import {
printFunction,
printIdentifier,
printManualMemoDependency,
} from '../HIR/PrintHIR';
import {printIdentifier, printManualMemoDependency} from '../HIR/PrintHIR';
import {eachInstructionValueOperand} from '../HIR/visitors';
import {collectMaybeMemoDependencies} from '../Inference/DropManualMemoization';
import {
Expand Down

0 comments on commit 517951a

Please sign in to comment.