Add validation against instructions not part of their scope
Adds an internal compiler assertion pass which checks that all the instructions which are necessary for constructing a given scope correctly end up within the corresponding ReactiveScopeBlock. All known cases where this can occur are fixed earlier in the stack, but this assertion will help us catch any other cases we haven't thought of. See docblock comment for more info. ## Test Plan I manually reverted the fixes from the previous PRs while keeping the new fixtures, and verified that this new assertion pass flags the fixtures as invalid.
Joe Savona committed
Nov 6, 2023 at 08:33 UTC
c424cbbfa7822b47fc73b112514426d149d3c98a
4 files changed
+87
-13
compiler/packages/babel-plugin-react-forget/src/Entrypoint/Pipeline.ts
+3
@@ -36,6 +36,7 @@ import {
36
import {
37
CodegenFunction,
38
alignReactiveScopesToBlockScopes,
39
+ assertScopeInstructionsWithinScopes,
40
buildReactiveBlocks,
41
buildReactiveFunction,
42
codegenReactiveFunction,
@@ -261,6 +262,8 @@ function* runWithEnvironment(
262
value: reactiveFunction,
263
});
264
265
+ assertScopeInstructionsWithinScopes(reactiveFunction);
266
+
267
flattenScopesWithHooks(reactiveFunction);
268
yield log({
269
kind: "reactive",
compiler/packages/babel-plugin-react-forget/src/ReactiveScopes/AlignReactiveScopesToBlockScopes.ts
+7
-13
@@ -82,7 +82,7 @@ class Visitor extends ReactiveFunctionVisitor<Context> {
82
override visitBlock(block: ReactiveBlock, state: Context): void {
83
state.enter(() => {
84
this.traverseBlock(block, state);
85
- }, "block");
85
+ });
86
}
87
}
88
@@ -91,10 +91,7 @@ type PendingReactiveScope = { active: boolean; scope: ReactiveScope };
91
class Context {
92
// For each block scope (outer array) stores a list of ReactiveScopes that start
93
// in that block scope.
94
- #blockScopes: Array<{
95
- kind: "block" | "value";
96
- scopes: Array<PendingReactiveScope>;
97
- }> = [];
94
+ #blockScopes: Array<Array<PendingReactiveScope>> = [];
95
96
// ReactiveScopes whose declaring block scope has ended but may still need to
97
// be "closed" (ie have their range.end be updated). A given scope can be in
@@ -105,11 +102,11 @@ class Context {
102
// the above data structures they're in, to avoid tracking the same scope twice.
103
#seenScopes: Set<ScopeId> = new Set();
104
108
- enter(fn: () => void, kind: "block" | "value" = "block"): void {
109
- this.#blockScopes.push({ kind, scopes: [] });
105
+ enter(fn: () => void): void {
106
+ this.#blockScopes.push([]);
107
fn();
108
const lastScope = this.#blockScopes.pop()!;
112
- for (const scope of lastScope.scopes) {
109
+ for (const scope of lastScope) {
110
if (scope.active) {
111
this.#unclosedScopes.push(scope);
112
}
@@ -118,10 +115,7 @@ class Context {
115
116
visitId(id: InstructionId): void {
117
const currentScopes = this.#blockScopes.at(-1)!;
121
- if (currentScopes.kind === "value") {
122
- return;
123
- }
124
- const scopes = [...currentScopes.scopes, ...this.#unclosedScopes];
118
+ const scopes = [...currentScopes, ...this.#unclosedScopes];
119
for (const pending of scopes) {
120
if (!pending.active) {
121
continue;
@@ -137,7 +131,7 @@ class Context {
131
if (!this.#seenScopes.has(scope.id)) {
132
const currentScopes = this.#blockScopes.at(-1)!;
133
this.#seenScopes.add(scope.id);
140
- currentScopes.scopes.push({
134
+ currentScopes.push({
135
active: true,
136
scope,
137
});
compiler/packages/babel-plugin-react-forget/src/ReactiveScopes/AssertScopeInstructionsWithinScope.ts
new
+76
@@ -0,0 +1,76 @@
1
+/**
2
+ * Copyright (c) Meta Platforms, Inc. and affiliates.
3
+ *
4
+ * This source code is licensed under the MIT license found in the
5
+ * LICENSE file in the root directory of this source tree.
6
+ */
7
+
8
+import { visitReactiveFunction } from ".";
9
+import { CompilerError } from "..";
10
+import {
11
+ InstructionId,
12
+ Place,
13
+ ReactiveFunction,
14
+ ReactiveScopeBlock,
15
+ ScopeId,
16
+} from "../HIR";
17
+import { getPlaceScope } from "./BuildReactiveBlocks";
18
+import { ReactiveFunctionVisitor } from "./visitors";
19
+
20
+/**
21
+ * Internal validation pass that checks all the instructions involved in creating
22
+ * values for a given scope are within the corresponding ReactiveScopeBlock. Errors
23
+ * in HIR/ReactiveFunction structure and alias analysis could theoretically create
24
+ * a structure such as:
25
+ *
26
+ * Function
27
+ * LabelTerminal
28
+ * Instruction in scope 0
29
+ * Instruction in scope 0
30
+ *
31
+ * Because ReactiveScopeBlocks are closed when their surrounding block ends, this
32
+ * structure would create reactive scopes as follows:
33
+ *
34
+ * Function
35
+ * LabelTerminal
36
+ * ReactiveScopeBlock scope=0
37
+ * Instruction in scope 0
38
+ * Instruction in scope 0
39
+ *
40
+ * This pass asserts we didn't accidentally end up with such a structure, as a guard
41
+ * against compiler coding mistakes in earlier passes.
42
+ */
43
+export function assertScopeInstructionsWithinScopes(
44
+ fn: ReactiveFunction
45
+): void {
46
+ visitReactiveFunction(fn, new Visitor(), undefined);
47
+}
48
+
49
+class Visitor extends ReactiveFunctionVisitor<void> {
50
+ seenScopes: Set<ScopeId> = new Set();
51
+ activeScopes: Set<ScopeId> = new Set();
52
+
53
+ override visitPlace(id: InstructionId, place: Place, _state: void): void {
54
+ const scope = getPlaceScope(id, place);
55
+ if (
56
+ scope !== null &&
57
+ this.seenScopes.has(scope.id) &&
58
+ !this.activeScopes.has(scope.id)
59
+ ) {
60
+ CompilerError.invariant(false, {
61
+ description: `Instruction [${id}] is part of scope @${scope.id}, but that scope has already completed.`,
62
+ loc: place.loc,
63
+ reason:
64
+ "Encountered an instruction that should be part of a scope, but where that scope has already completed",
65
+ suggestions: null,
66
+ });
67
+ }
68
+ }
69
+
70
+ override visitScope(block: ReactiveScopeBlock, state: void): void {
71
+ this.seenScopes.add(block.scope.id);
72
+ this.activeScopes.add(block.scope.id);
73
+ this.traverseScope(block, state);
74
+ this.activeScopes.delete(block.scope.id);
75
+ }
76
+}
compiler/packages/babel-plugin-react-forget/src/ReactiveScopes/index.ts
+1
@@ -6,6 +6,7 @@
6
*/
7
8
export { alignReactiveScopesToBlockScopes } from "./AlignReactiveScopesToBlockScopes";
9
+export { assertScopeInstructionsWithinScopes } from "./AssertScopeInstructionsWithinScope";
10
export { buildReactiveBlocks } from "./BuildReactiveBlocks";
11
export { buildReactiveFunction } from "./BuildReactiveFunction";
12
export {