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] Cleanup some feature flags #31807

Open
wants to merge 1 commit 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
[compiler] Cleanup some feature flags
Cleans up the flags to assume hooks follow the rules, and to transitively freeze function expressions. These have been the default for a very long time.
  • Loading branch information
josephsavona committed Dec 16, 2024
commit 597ce2c66d5ce950e7770c81ceeb2879c6ee70bb
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,6 @@ import {
} from './HIR';
import {
BuiltInMixedReadonlyId,
DefaultMutatingHook,
DefaultNonmutatingHook,
FunctionSignature,
ShapeRegistry,
Expand Down Expand Up @@ -337,23 +336,6 @@ const EnvironmentConfigSchema = z.object({
validateNoCapitalizedCalls: z.nullable(z.array(z.string())).default(null),
validateBlocklistedImports: z.nullable(z.array(z.string())).default(null),

/*
* When enabled, the compiler assumes that hooks follow the Rules of React:
* - Hooks may memoize computation based on any of their parameters, thus
* any arguments to a hook are assumed frozen after calling the hook.
* - Hooks may memoize the result they return, thus the return value is
* assumed frozen.
*/
enableAssumeHooksFollowRulesOfReact: z.boolean().default(true),

/**
* When enabled, the compiler assumes that any values are not subsequently
* modified after they are captured by a function passed to React. For example,
* if a value `x` is referenced inside a function expression passed to `useEffect`,
* then this flag will assume that `x` is not subusequently modified.
*/
enableTransitivelyFreezeFunctionExpressions: z.boolean().default(true),

/*
* Enables codegen mutability debugging. This emits a dev-mode only to log mutations
* to values that Forget assumes are immutable (for Forget compiled code).
Expand Down Expand Up @@ -919,19 +901,19 @@ export class Environment {
isHookName(match[1])
) {
const resolvedName = match[1];
return this.#globals.get(resolvedName) ?? this.#getCustomHookType();
return this.#globals.get(resolvedName) ?? DefaultNonmutatingHook;
}
}

switch (binding.kind) {
case 'ModuleLocal': {
// don't resolve module locals
return isHookName(binding.name) ? this.#getCustomHookType() : null;
return isHookName(binding.name) ? DefaultNonmutatingHook : null;
}
case 'Global': {
return (
this.#globals.get(binding.name) ??
(isHookName(binding.name) ? this.#getCustomHookType() : null)
(isHookName(binding.name) ? DefaultNonmutatingHook : null)
);
}
case 'ImportSpecifier': {
Expand All @@ -946,7 +928,7 @@ export class Environment {
return (
this.#globals.get(binding.imported) ??
(isHookName(binding.imported) || isHookName(binding.name)
? this.#getCustomHookType()
? DefaultNonmutatingHook
: null)
);
} else {
Expand Down Expand Up @@ -985,7 +967,7 @@ export class Environment {
* `import {foo as useHook} ...`
*/
return isHookName(binding.imported) || isHookName(binding.name)
? this.#getCustomHookType()
? DefaultNonmutatingHook
: null;
}
}
Expand All @@ -995,7 +977,7 @@ export class Environment {
// only resolve imports to modules we know about
return (
this.#globals.get(binding.name) ??
(isHookName(binding.name) ? this.#getCustomHookType() : null)
(isHookName(binding.name) ? DefaultNonmutatingHook : null)
);
} else {
const moduleType = this.#resolveModuleType(binding.module, loc);
Expand Down Expand Up @@ -1026,7 +1008,7 @@ export class Environment {
return importedType;
}
}
return isHookName(binding.name) ? this.#getCustomHookType() : null;
return isHookName(binding.name) ? DefaultNonmutatingHook : null;
}
}
}
Expand Down Expand Up @@ -1062,11 +1044,11 @@ export class Environment {
let value =
shape.properties.get(property) ?? shape.properties.get('*') ?? null;
if (value === null && isHookName(property)) {
value = this.#getCustomHookType();
value = DefaultNonmutatingHook;
}
return value;
} else if (isHookName(property)) {
return this.#getCustomHookType();
return DefaultNonmutatingHook;
} else {
return null;
}
Expand All @@ -1091,14 +1073,6 @@ export class Environment {
this.#contextIdentifiers.add(node);
this.#hoistedIdentifiers.add(node);
}

#getCustomHookType(): Global {
if (this.config.enableAssumeHooksFollowRulesOfReact) {
return DefaultNonmutatingHook;
} else {
return DefaultMutatingHook;
}
}
}

const REANIMATED_MODULE_NAME = 'react-native-reanimated';
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -664,19 +664,6 @@ addObject(BUILTIN_SHAPES, BuiltInMixedReadonlyId, [
addObject(BUILTIN_SHAPES, BuiltInJsxId, []);
addObject(BUILTIN_SHAPES, BuiltInFunctionId, []);

export const DefaultMutatingHook = addHook(
BUILTIN_SHAPES,
{
positionalParams: [],
restParam: Effect.ConditionallyMutate,
returnType: {kind: 'Poly'},
calleeEffect: Effect.Read,
hookKind: 'Custom',
returnValueKind: ValueKind.Mutable,
},
'DefaultMutatingHook',
);

export const DefaultNonmutatingHook = addHook(
BUILTIN_SHAPES,
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -396,10 +396,7 @@ class InferenceState {
context: new Set(),
});
if (value.kind === 'FunctionExpression') {
if (
this.#env.config.enablePreserveExistingMemoizationGuarantees ||
this.#env.config.enableTransitivelyFreezeFunctionExpressions
) {
if (this.#env.config.enablePreserveExistingMemoizationGuarantees) {
if (value.kind === 'FunctionExpression') {
/*
* We want to freeze the captured values, not mark the operands
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
## Input

```javascript
// @compilationMode(infer) @enableAssumeHooksFollowRulesOfReact:false @customMacros(cx)
// @compilationMode(infer) @customMacros(cx)
import {identity} from 'shared-runtime';

const DARK = 'dark';
Expand Down Expand Up @@ -47,26 +47,29 @@ export const FIXTURE_ENTRYPOINT = {
## Code

```javascript
import { c as _c } from "react/compiler-runtime"; // @compilationMode(infer) @enableAssumeHooksFollowRulesOfReact:false @customMacros(cx)
import { c as _c } from "react/compiler-runtime"; // @compilationMode(infer) @customMacros(cx)
import { identity } from "shared-runtime";

const DARK = "dark";

function Component() {
const $ = _c(2);
const $ = _c(4);
const theme = useTheme();

const t0 = cx({
"styles/light": true,
"styles/dark": theme.getTheme() === DARK,
});
let t0;
if ($[0] !== theme) {
t0 = cx({ "styles/light": true, "styles/dark": theme.getTheme() === DARK });
$[0] = theme;
$[1] = t0;
} else {
t0 = $[1];
}
let t1;
if ($[0] !== t0) {
if ($[2] !== t0) {
t1 = <div className={t0} />;
$[0] = t0;
$[1] = t1;
$[2] = t0;
$[3] = t1;
} else {
t1 = $[1];
t1 = $[3];
}
return t1;
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// @compilationMode(infer) @enableAssumeHooksFollowRulesOfReact:false @customMacros(cx)
// @compilationMode(infer) @customMacros(cx)
import {identity} from 'shared-runtime';

const DARK = 'dark';
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
## Input

```javascript
// @compilationMode(infer) @enableAssumeHooksFollowRulesOfReact:false @customMacros(cx)
// @compilationMode(infer) @customMacros(cx)
import {identity} from 'shared-runtime';

const DARK = 'dark';
Expand Down Expand Up @@ -49,26 +49,32 @@ export const FIXTURE_ENTRYPOINT = {
## Code

```javascript
import { c as _c } from "react/compiler-runtime"; // @compilationMode(infer) @enableAssumeHooksFollowRulesOfReact:false @customMacros(cx)
import { c as _c } from "react/compiler-runtime"; // @compilationMode(infer) @customMacros(cx)
import { identity } from "shared-runtime";

const DARK = "dark";

function Component() {
const $ = _c(2);
const $ = _c(4);
const theme = useTheme();

const t0 = cx.foo({
"styles/light": true,
"styles/dark": identity([theme.getTheme()]),
});
let t0;
if ($[0] !== theme) {
t0 = cx.foo({
"styles/light": true,
"styles/dark": identity([theme.getTheme()]),
});
$[0] = theme;
$[1] = t0;
} else {
t0 = $[1];
}
let t1;
if ($[0] !== t0) {
if ($[2] !== t0) {
t1 = <div className={t0} />;
$[0] = t0;
$[1] = t1;
$[2] = t0;
$[3] = t1;
} else {
t1 = $[1];
t1 = $[3];
}
return t1;
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// @compilationMode(infer) @enableAssumeHooksFollowRulesOfReact:false @customMacros(cx)
// @compilationMode(infer) @customMacros(cx)
import {identity} from 'shared-runtime';

const DARK = 'dark';
Expand Down
Loading