From 67b2a9f314bae522859cc7e0e67076d09e83524b Mon Sep 17 00:00:00 2001 From: Mofei Zhang Date: Mon, 27 Mar 2023 10:41:05 -0400 Subject: [PATCH] [rhir] Represent OptionalMemberExpression as a conditional dependency --- Every `OptionalMemberExpression` rvalue has the form `?.`. ``` // required = [a], optional: [b, c] props.a?.b.c; props.a?.b?.c; ``` When calculating reactive dependencies, recall that it is always correct to add a subpath of a dependency (e.g. we can always take `props.a` instead of `props.a.b` as a dependency). See comments in `DeriveMinimalDependencies` for a longer explanation. There are two ways we can deal with `OptionalMemberExpression`: - We can always truncate a OptionalMemberExpression dependency to its `requiredPath`, taking only the required path as a dependency. - this is the simpler approach, but it potentially loses granularity. e.g. ``` // here, since props.a is already unconditionally accessed, // we can safely add props.a.b as a dependency and preserve both // nullthrows and the correct dependency set. scope @0 { let x = []; x.push(props.a?.b); x.push(props.a.b); } ``` (See added test case `reduce-reactive-cond-memberexpr-join` + its comment block for a more detailed explanation` - (the approach taken by this PR) We can add the `requiredPath` as a potentially unconditional access (dependent on other control flow) and `requiredPath + optionalPath` as a conditional dependency. --- .../DeriveMinimalDependencies.ts | 89 +++++++++++++++---- .../PropagateScopeDependencies.ts | 88 +++++++++++------- ...ond-deps-conditional-member-expr.expect.md | 40 +++++++++ .../cond-deps-conditional-member-expr.js | 9 ++ .../nested-optional-member-expr.expect.md | 4 +- ...ce-reactive-cond-memberexpr-join.expect.md | 56 ++++++++++++ .../reduce-reactive-cond-memberexpr-join.js | 17 ++++ 7 files changed, 254 insertions(+), 49 deletions(-) create mode 100644 compiler/forget/src/__tests__/fixtures/compiler/cond-deps-conditional-member-expr.expect.md create mode 100644 compiler/forget/src/__tests__/fixtures/compiler/cond-deps-conditional-member-expr.js create mode 100644 compiler/forget/src/__tests__/fixtures/compiler/reduce-reactive-cond-memberexpr-join.expect.md create mode 100644 compiler/forget/src/__tests__/fixtures/compiler/reduce-reactive-cond-memberexpr-join.js diff --git a/compiler/forget/src/ReactiveScopes/DeriveMinimalDependencies.ts b/compiler/forget/src/ReactiveScopes/DeriveMinimalDependencies.ts index 454a8b3d6b..35c923acd2 100644 --- a/compiler/forget/src/ReactiveScopes/DeriveMinimalDependencies.ts +++ b/compiler/forget/src/ReactiveScopes/DeriveMinimalDependencies.ts @@ -3,6 +3,25 @@ import { Identifier, ReactiveScopeDependency } from "../HIR"; import { printIdentifier } from "../HIR/PrintHIR"; import { assertExhaustive } from "../Utils/utils"; +/** + * We need to understand optional member expressions only when determining + * dependencies of a ReactiveScope (i.e. in {@link PropagateScopeDependencies}), + * hence why this type lives here (not in HIR.ts) + * + * {@link ReactiveScopePropertyDependency.optionalPath} is populated only if the Property + * represents an optional member expression, and it represents the property path + * loaded conditionally. + * e.g. the member expr a.b.c?.d.e?.f is represented as + * { + * identifier: 'a'; + * path: ['b', 'c'], + * optionalPath: ['d', 'e', 'f']. + * } + */ +export type ReactiveScopePropertyDependency = ReactiveScopeDependency & { + optionalPath: Array; +}; + /** * Finalizes a set of ReactiveScopeDependencies to produce a set of minimal unconditional * dependencies, preserving granular accesses when possible. @@ -42,34 +61,54 @@ export class ReactiveScopeDependencyTree { return rootNode; } - add(dep: ReactiveScopeDependency, inConditional: boolean): void { - const { path } = dep; + add(dep: ReactiveScopePropertyDependency, inConditional: boolean): void { + const { path, optionalPath } = dep; let currNode = this.#getOrCreateRoot(dep.identifier); const accessType = inConditional ? PropertyAccessType.ConditionalAccess : PropertyAccessType.UnconditionalAccess; - const depType = inConditional - ? PropertyAccessType.ConditionalDependency - : PropertyAccessType.UnconditionalDependency; for (const property of path) { // all properties read 'on the way' to a dependency are marked as 'access' - let currChild = currNode.properties.get(property); - if (currChild == null) { - currChild = { - properties: new Map(), - accessType, - }; - currNode.properties.set(property, currChild); - } else { - currChild.accessType = merge(currChild.accessType, accessType); - } + let currChild = getOrMakeProperty(currNode, property); + currChild.accessType = merge(currChild.accessType, accessType); currNode = currChild; } - // final property read should be marked as `dependency` - currNode.accessType = merge(currNode.accessType, depType); + if (optionalPath.length === 0) { + // If this property does not have a conditional path (i.e. a.b.c), the + // final property node should be marked as an conditional/unconditional + // `dependency` as based on control flow. + const depType = inConditional + ? PropertyAccessType.ConditionalDependency + : PropertyAccessType.UnconditionalDependency; + + currNode.accessType = merge(currNode.accessType, depType); + } else { + // Technically, we only depend on whether unconditional path `dep.path` + // is nullish (not its actual value). As long as we preserve the nullthrows + // behavior of `dep.path`, we can keep it as an access (and not promote + // to a dependency). + // See test `reduce-reactive-cond-memberexpr-join` for example. + + // If this property has an optional path (i.e. a?.b.c), all optional + // nodes should be marked accordingly. + for (const property of optionalPath) { + let currChild = getOrMakeProperty(currNode, property); + currChild.accessType = merge( + currChild.accessType, + PropertyAccessType.ConditionalAccess + ); + currNode = currChild; + } + + // The final node should be marked as a conditional dependency. + currNode.accessType = merge( + currNode.accessType, + PropertyAccessType.ConditionalDependency + ); + } } deriveMinimalDependencies(): Set { @@ -176,6 +215,7 @@ enum PropertyAccessType { UnconditionalDependency = "UnconditionalDependency", } +const MIN_ACCESS_TYPE = PropertyAccessType.ConditionalAccess; function isUnconditional(access: PropertyAccessType): boolean { return ( access === PropertyAccessType.UnconditionalAccess || @@ -455,6 +495,21 @@ function printSubtree( return results; } +function getOrMakeProperty( + node: DependencyNode, + property: string +): DependencyNode { + let child = node.properties.get(property); + if (child == null) { + child = { + properties: new Map(), + accessType: MIN_ACCESS_TYPE, + }; + node.properties.set(property, child); + } + return child; +} + function mapNonNull, V, U>( arr: Array, fn: (arg0: U) => T | undefined | null diff --git a/compiler/forget/src/ReactiveScopes/PropagateScopeDependencies.ts b/compiler/forget/src/ReactiveScopes/PropagateScopeDependencies.ts index af84bd5c7e..9f62547056 100644 --- a/compiler/forget/src/ReactiveScopes/PropagateScopeDependencies.ts +++ b/compiler/forget/src/ReactiveScopes/PropagateScopeDependencies.ts @@ -24,7 +24,10 @@ import { eachPatternOperand, } from "../HIR/visitors"; import { assertExhaustive } from "../Utils/utils"; -import { ReactiveScopeDependencyTree } from "./DeriveMinimalDependencies"; +import { + ReactiveScopeDependencyTree, + ReactiveScopePropertyDependency, +} from "./DeriveMinimalDependencies"; /** * Infers the dependencies of each scope to include variables whose values @@ -68,7 +71,7 @@ class Context { // - a ReactiveScope (A) containing a PropertyLoad may differ from the // ReactiveScope (B) that uses the produced temporary. // - codegen will inline these PropertyLoads back into scope (B) - #properties: Map = new Map(); + #properties: Map = new Map(); #temporaries: Map = new Map(); #inConditionalWithinScope: boolean = false; // Reactive dependencies used unconditionally in the current conditional. @@ -186,21 +189,53 @@ class Context { this.#temporaries.set(lvalue.identifier, value); } - declareProperty(lvalue: Place, object: Place, property: string): void { + #getProperty( + object: Place, + property: string, + isConditional: boolean + ): ReactiveScopePropertyDependency { const resolvedObject = this.#temporaries.get(object.identifier) ?? object; - const objectDependency = this.#properties.get(resolvedObject.identifier); - let nextDependency: ReactiveScopeDependency; - if (objectDependency === undefined) { - nextDependency = { + const resolvedDependency = this.#properties.get(resolvedObject.identifier); + let objectDependency: ReactiveScopePropertyDependency; + // (1) Create the base property dependency as either a LoadLocal (from a temporary) + // or a deep copy of an existing property dependency. + if (resolvedDependency === undefined) { + objectDependency = { identifier: resolvedObject.identifier, - path: [property], + path: [], + optionalPath: [], }; } else { - nextDependency = { - identifier: objectDependency.identifier, - path: [...objectDependency.path, property], + objectDependency = { + identifier: resolvedDependency.identifier, + path: [...resolvedDependency.path], + optionalPath: [...resolvedDependency.optionalPath], }; } + + // (2) Determine whether property is an optional access + if (objectDependency.optionalPath.length > 0) { + // If the base property dependency represents a optional member expression, + // property is on the optionalPath (regardless of whether this PropertyLoad + // itself was conditional) + // e.g. for `a.b?.c.d`, `d` should be added to optionalPath + objectDependency.optionalPath.push(property); + } else if (isConditional) { + objectDependency.optionalPath.push(property); + } else { + objectDependency.path.push(property); + } + + return objectDependency; + } + + declareProperty( + lvalue: Place, + object: Place, + property: string, + isConditional: boolean + ): void { + const nextDependency = this.#getProperty(object, property, isConditional); this.#properties.set(lvalue.identifier, nextDependency); } @@ -234,9 +269,10 @@ class Context { // if this operand is a temporary created for a property load, try to resolve it to // the expanded Place. Fall back to using the operand as-is. - let dependency: ReactiveScopeDependency = { + let dependency: ReactiveScopePropertyDependency = { identifier: resolved.identifier, path: [], + optionalPath: [], }; if (resolved.identifier.name === null) { const propertyDependency = this.#properties.get(resolved.identifier); @@ -247,25 +283,12 @@ class Context { this.visitDependency(dependency); } - visitProperty(object: Place, property: string): void { - const resolvedObject = this.#temporaries.get(object.identifier) ?? object; - const objectDependency = this.#properties.get(resolvedObject.identifier); - let nextDependency: ReactiveScopeDependency; - if (objectDependency === undefined) { - nextDependency = { - identifier: resolvedObject.identifier, - path: [property], - }; - } else { - nextDependency = { - identifier: objectDependency.identifier, - path: [...objectDependency.path, property], - }; - } + visitProperty(object: Place, property: string, isConditional: boolean): void { + const nextDependency = this.#getProperty(object, property, isConditional); this.visitDependency(nextDependency); } - visitDependency(maybeDependency: ReactiveScopeDependency): void { + visitDependency(maybeDependency: ReactiveScopePropertyDependency): void { // Any value used after its originally defining scope has concluded must be added as an // output of its defining scope. Regardless of whether its a const or not, // some later code needs access to the value. If the current @@ -493,9 +516,14 @@ function visitInstructionValue( } } else if (value.kind === "PropertyLoad") { if (lvalue !== null) { - context.declareProperty(lvalue, value.object, value.property); + context.declareProperty( + lvalue, + value.object, + value.property, + value.optional + ); } else { - context.visitProperty(value.object, value.property); + context.visitProperty(value.object, value.property, value.optional); } } else if (value.kind === "StoreLocal") { context.visitOperand(value.value); diff --git a/compiler/forget/src/__tests__/fixtures/compiler/cond-deps-conditional-member-expr.expect.md b/compiler/forget/src/__tests__/fixtures/compiler/cond-deps-conditional-member-expr.expect.md new file mode 100644 index 0000000000..970bf36fa4 --- /dev/null +++ b/compiler/forget/src/__tests__/fixtures/compiler/cond-deps-conditional-member-expr.expect.md @@ -0,0 +1,40 @@ + +## Input + +```javascript +// To preserve the nullthrows behavior and reactive deps of this code, +// Forget needs to add `props.a` as a dependency (since `props.a.b` is +// a conditional dependency, i.e. gated behind control flow) + +function Component(props) { + let x = []; + x.push(props.a?.b); + return x; +} + +``` + +## Code + +```javascript +// To preserve the nullthrows behavior and reactive deps of this code, +// Forget needs to add `props.a` as a dependency (since `props.a.b` is +// a conditional dependency, i.e. gated behind control flow) + +function Component(props) { + const $ = React.unstable_useMemoCache(2); + const c_0 = $[0] !== props.a; + let x; + if (c_0) { + x = []; + x.push(props.a?.b); + $[0] = props.a; + $[1] = x; + } else { + x = $[1]; + } + return x; +} + +``` + \ No newline at end of file diff --git a/compiler/forget/src/__tests__/fixtures/compiler/cond-deps-conditional-member-expr.js b/compiler/forget/src/__tests__/fixtures/compiler/cond-deps-conditional-member-expr.js new file mode 100644 index 0000000000..c7bb2cd3c0 --- /dev/null +++ b/compiler/forget/src/__tests__/fixtures/compiler/cond-deps-conditional-member-expr.js @@ -0,0 +1,9 @@ +// To preserve the nullthrows behavior and reactive deps of this code, +// Forget needs to add `props.a` as a dependency (since `props.a.b` is +// a conditional dependency, i.e. gated behind control flow) + +function Component(props) { + let x = []; + x.push(props.a?.b); + return x; +} diff --git a/compiler/forget/src/__tests__/fixtures/compiler/nested-optional-member-expr.expect.md b/compiler/forget/src/__tests__/fixtures/compiler/nested-optional-member-expr.expect.md index 950ed1486a..d2bb09a4f8 100644 --- a/compiler/forget/src/__tests__/fixtures/compiler/nested-optional-member-expr.expect.md +++ b/compiler/forget/src/__tests__/fixtures/compiler/nested-optional-member-expr.expect.md @@ -18,11 +18,11 @@ function Component(props) { // (i.e. placing `?` in the correct PropertyLoad) function Component(props) { const $ = React.unstable_useMemoCache(2); - const c_0 = $[0] !== props.a.b.c.d; + const c_0 = $[0] !== props.a; let t0; if (c_0) { t0 = foo((props.a?.b).c.d); - $[0] = props.a.b.c.d; + $[0] = props.a; $[1] = t0; } else { t0 = $[1]; diff --git a/compiler/forget/src/__tests__/fixtures/compiler/reduce-reactive-cond-memberexpr-join.expect.md b/compiler/forget/src/__tests__/fixtures/compiler/reduce-reactive-cond-memberexpr-join.expect.md new file mode 100644 index 0000000000..614b7a4d24 --- /dev/null +++ b/compiler/forget/src/__tests__/fixtures/compiler/reduce-reactive-cond-memberexpr-join.expect.md @@ -0,0 +1,56 @@ + +## Input + +```javascript +// To preserve the nullthrows behavior and reactive deps of this code, +// Forget needs to add `props.a.b` or a subpath as a dependency. +// +// (1) Since the reactive block producing x unconditionally read props.a.<...>, +// reading `props.a.b` outside of the block would still preserve nullthrows +// semantics of source code +// (2) Technically, props.a, props.a.b, and props.a.b.c are all reactive deps. +// However, `props.a?.b` is only dependent on whether `props.a` is nullish, +// not its actual value. Since we already preserve nullthrows on `props.a`, +// we technically do not need to add `props.a` as a dependency. + +function Component(props) { + let x = []; + x.push(props.a?.b); + x.push(props.a.b.c); + return x; +} + +``` + +## Code + +```javascript +// To preserve the nullthrows behavior and reactive deps of this code, +// Forget needs to add `props.a.b` or a subpath as a dependency. +// +// (1) Since the reactive block producing x unconditionally read props.a.<...>, +// reading `props.a.b` outside of the block would still preserve nullthrows +// semantics of source code +// (2) Technically, props.a, props.a.b, and props.a.b.c are all reactive deps. +// However, `props.a?.b` is only dependent on whether `props.a` is nullish, +// not its actual value. Since we already preserve nullthrows on `props.a`, +// we technically do not need to add `props.a` as a dependency. + +function Component(props) { + const $ = React.unstable_useMemoCache(2); + const c_0 = $[0] !== props.a.b; + let x; + if (c_0) { + x = []; + x.push(props.a?.b); + x.push(props.a.b.c); + $[0] = props.a.b; + $[1] = x; + } else { + x = $[1]; + } + return x; +} + +``` + \ No newline at end of file diff --git a/compiler/forget/src/__tests__/fixtures/compiler/reduce-reactive-cond-memberexpr-join.js b/compiler/forget/src/__tests__/fixtures/compiler/reduce-reactive-cond-memberexpr-join.js new file mode 100644 index 0000000000..393653185b --- /dev/null +++ b/compiler/forget/src/__tests__/fixtures/compiler/reduce-reactive-cond-memberexpr-join.js @@ -0,0 +1,17 @@ +// To preserve the nullthrows behavior and reactive deps of this code, +// Forget needs to add `props.a.b` or a subpath as a dependency. +// +// (1) Since the reactive block producing x unconditionally read props.a.<...>, +// reading `props.a.b` outside of the block would still preserve nullthrows +// semantics of source code +// (2) Technically, props.a, props.a.b, and props.a.b.c are all reactive deps. +// However, `props.a?.b` is only dependent on whether `props.a` is nullish, +// not its actual value. Since we already preserve nullthrows on `props.a`, +// we technically do not need to add `props.a` as a dependency. + +function Component(props) { + let x = []; + x.push(props.a?.b); + x.push(props.a.b.c); + return x; +}