@samitouri / QOS-React-2 / commits / 515c33d2a6

Custom version of no-use-before-define rule

## Proper Detection of Out-of-order Functions The no-use-before-define rule from ESLint has a strange behavior in which it treats variables differently than functions: ```javascript function foo() { return bar(X); } const X = null; function bar(x) {} ``` By default, `bar(x)` has two errors: one because X is used before defined, and once because `bar` is used before defined. The rule has an option `{variables: false}` which only enables validation when the variable is from the same "scope" as the reference, the net result of which is it means it doesn't report spurious errors such as X being undefined. There is _also_ a `{functions: false}` option, but for some reason that doesn't work the same way, it just turns off all validation of references that came from functions. So enabling that option would suppress the (spurious) error on invoking `bar()` above, but causes the rule to miss invalid code such as: ``` function foo() { return bar(); function bar() {} } ``` This PR adds a fork of the rule that makes `{functions: false}` behave similarly to `{variables: false}`, which should help avoid some of the spurious errors i saw internally. The rule is exported from Forget itself, which will make it easier to consume internally, in tests, and in the playground. ## Targeting the validation to Forget functions Even with the above, there are still some false positives coming from code such as: ```javascript const x = foo(); function foo() {} ``` This PR changes codegen to ensure that the output of a function _always_ has the body starting with 'use forget'. The ESLint rule then only looks at function declarations/expressions whose body starts with that expression. The new unit test confirms that the validation finds invalid reorderings even on functions that weren't explicitly tagged as 'use forget'.

Joseph Savona committed Oct 27, 2022 at 16:29 UTC 515c33d2a650dbfa6158c709cfc96092e136789e
9 files changed +407 -6
compiler/forget/packages/playground/lib/compilerDriver.ts
+3 -1
@@ -8,6 +8,7 @@ import {
8 createArrayLogger,
9 createCompilerOutputs,
10 getMostRecentCompilerContext,
11 + NoUseBeforeDefineRule,
12 OutputKind,
13 type CompilerContext,
14 type CompilerOptions,
@@ -36,7 +37,7 @@ const ESLINT_CONFIG = {
37 sourceType: "module",
38 },
39 rules: {
39 - "no-use-before-define": "error",
40 + "custom-no-use-before-define": "error",
41 },
42 };
43
@@ -48,6 +49,7 @@ const ESLINT_CONFIG = {
49 */
50 function validateNoUseBeforeDefine(source: string) {
51 const linter = new ESLint.index.Linter();
52 + linter.defineRule("custom-no-use-before-define", NoUseBeforeDefineRule);
53 return linter.verify(source, ESLINT_CONFIG);
54 }
55
compiler/forget/src/BackEnd/JSGen.ts
+12
@@ -173,4 +173,16 @@ export function runFunc(
173
174 // recover directives.
175 funcBody.node.directives = directives;
176 + if (
177 + funcBody.node.directives.length === 0 ||
178 + funcBody.node.directives[0]!.value?.value !== "use forget"
179 + ) {
180 + funcBody.node.directives.unshift({
181 + type: "Directive",
182 + value: {
183 + type: "DirectiveLiteral",
184 + value: "use forget",
185 + },
186 + });
187 + }
188 }
compiler/forget/src/BackEnd/index.ts
-1
@@ -14,5 +14,4 @@ export { default as DumpLIR } from "./DumpLIR";
14 export { default as JSGen } from "./JSGen";
15 export { default as LIRGen } from "./LIRGen";
16 export { default as MemoCacheAlloc } from "./MemoCacheAlloc";
17 -export { default as PostCodegenValidator } from "./PostCodegenValidator";
17 export { default as SanityCheck } from "./SanityCheck";
compiler/forget/src/CompilerDriver.ts
+2 -1
@@ -12,6 +12,7 @@ import { CompilerContext } from "./CompilerContext";
12 import { CompilerOptions } from "./CompilerOptions";
13 import * as ME from "./MiddleEnd";
14 import { PassManager } from "./PassManager";
15 +import * as Validation from "./Validation";
16
17 /**
18 * Compiler Driver
@@ -59,7 +60,7 @@ export function createCompilerDriver(
60 passManager.addPass(BE.JSGen);
61
62 // Optionally sanity-check the transformed output
62 - passManager.addPass(BE.PostCodegenValidator);
63 + passManager.addPass(Validation.PostCodegenValidator);
64
65 passManager.runAll();
66 },
compiler/forget/src/Validation/NoUseBeforeDefineRule.ts new
+372
@@ -0,0 +1,372 @@
1 +/**
2 + * Copyright (c) Facebook, Inc. and its 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 +// @ts-nocheck
9 +
10 +// NOTE: this file is forked from ESLint's no-use-before-define rule:
11 +// https://github.com/eslint/eslint/blob/15814057fd69319b3744bdea5db2455f85d2e74f/lib/rules/no-use-before-define.js
12 +// The only change is to treat the {functions:false} option similarly to {variables:false},
13 +// ie to disable validation for functions defined at module scope but still check for locally
14 +// defined functions.
15 +
16 +/**
17 + * @fileoverview Rule to flag use of variables before they are defined
18 + * @author Ilya Volodin
19 + */
20 +
21 +("use strict");
22 +
23 +//------------------------------------------------------------------------------
24 +// Helpers
25 +//------------------------------------------------------------------------------
26 +
27 +const SENTINEL_TYPE =
28 + /^(?:(?:Function|Class)(?:Declaration|Expression)|ArrowFunctionExpression|CatchClause|ImportDeclaration|ExportNamedDeclaration)$/u;
29 +const FOR_IN_OF_TYPE = /^For(?:In|Of)Statement$/u;
30 +
31 +/**
32 + * Parses a given value as options.
33 + * @param {any} options A value to parse.
34 + * @returns {Object} The parsed options.
35 + */
36 +function parseOptions(options) {
37 + let functions = true;
38 + let classes = true;
39 + let variables = true;
40 + let allowNamedExports = false;
41 +
42 + if (typeof options === "string") {
43 + functions = options !== "nofunc";
44 + } else if (typeof options === "object" && options !== null) {
45 + functions = options.functions !== false;
46 + classes = options.classes !== false;
47 + variables = options.variables !== false;
48 + allowNamedExports = !!options.allowNamedExports;
49 + }
50 +
51 + return { functions, classes, variables, allowNamedExports };
52 +}
53 +
54 +/**
55 + * Checks whether or not a given location is inside of the range of a given node.
56 + * @param {ASTNode} node An node to check.
57 + * @param {number} location A location to check.
58 + * @returns {boolean} `true` if the location is inside of the range of the node.
59 + */
60 +function isInRange(node, location) {
61 + return node && node.range[0] <= location && location <= node.range[1];
62 +}
63 +
64 +/**
65 + * Checks whether or not a given location is inside of the range of a class static initializer.
66 + * Static initializers are static blocks and initializers of static fields.
67 + * @param {ASTNode} node `ClassBody` node to check static initializers.
68 + * @param {number} location A location to check.
69 + * @returns {boolean} `true` if the location is inside of a class static initializer.
70 + */
71 +function isInClassStaticInitializerRange(node, location) {
72 + return node.body.some(
73 + (classMember) =>
74 + (classMember.type === "StaticBlock" &&
75 + isInRange(classMember, location)) ||
76 + (classMember.type === "PropertyDefinition" &&
77 + classMember.static &&
78 + classMember.value &&
79 + isInRange(classMember.value, location))
80 + );
81 +}
82 +
83 +/**
84 + * Checks whether a given scope is the scope of a class static initializer.
85 + * Static initializers are static blocks and initializers of static fields.
86 + * @param {eslint-scope.Scope} scope A scope to check.
87 + * @returns {boolean} `true` if the scope is a class static initializer scope.
88 + */
89 +function isClassStaticInitializerScope(scope) {
90 + if (scope.type === "class-static-block") {
91 + return true;
92 + }
93 +
94 + if (scope.type === "class-field-initializer") {
95 + // `scope.block` is PropertyDefinition#value node
96 + const propertyDefinition = scope.block.parent;
97 +
98 + return propertyDefinition.static;
99 + }
100 +
101 + return false;
102 +}
103 +
104 +/**
105 + * Checks whether a given reference is evaluated in an execution context
106 + * that isn't the one where the variable it refers to is defined.
107 + * Execution contexts are:
108 + * - top-level
109 + * - functions
110 + * - class field initializers (implicit functions)
111 + * - class static blocks (implicit functions)
112 + * Static class field initializers and class static blocks are automatically run during the class definition evaluation,
113 + * and therefore we'll consider them as a part of the parent execution context.
114 + * Example:
115 + *
116 + * const x = 1;
117 + *
118 + * x; // returns `false`
119 + * () => x; // returns `true`
120 + *
121 + * class C {
122 + * field = x; // returns `true`
123 + * static field = x; // returns `false`
124 + *
125 + * method() {
126 + * x; // returns `true`
127 + * }
128 + *
129 + * static method() {
130 + * x; // returns `true`
131 + * }
132 + *
133 + * static {
134 + * x; // returns `false`
135 + * }
136 + * }
137 + * @param {eslint-scope.Reference} reference A reference to check.
138 + * @returns {boolean} `true` if the reference is from a separate execution context.
139 + */
140 +function isFromSeparateExecutionContext(reference) {
141 + const variable = reference.resolved;
142 + let scope = reference.from;
143 +
144 + // Scope#variableScope represents execution context
145 + while (variable.scope.variableScope !== scope.variableScope) {
146 + if (isClassStaticInitializerScope(scope.variableScope)) {
147 + scope = scope.variableScope.upper;
148 + } else {
149 + return true;
150 + }
151 + }
152 +
153 + return false;
154 +}
155 +
156 +/**
157 + * Checks whether or not a given reference is evaluated during the initialization of its variable.
158 + *
159 + * This returns `true` in the following cases:
160 + *
161 + * var a = a
162 + * var [a = a] = list
163 + * var {a = a} = obj
164 + * for (var a in a) {}
165 + * for (var a of a) {}
166 + * var C = class { [C]; };
167 + * var C = class { static foo = C; };
168 + * var C = class { static { foo = C; } };
169 + * class C extends C {}
170 + * class C extends (class { static foo = C; }) {}
171 + * class C { [C]; }
172 + * @param {Reference} reference A reference to check.
173 + * @returns {boolean} `true` if the reference is evaluated during the initialization.
174 + */
175 +function isEvaluatedDuringInitialization(reference) {
176 + if (isFromSeparateExecutionContext(reference)) {
177 + /*
178 + * Even if the reference appears in the initializer, it isn't evaluated during the initialization.
179 + * For example, `const x = () => x;` is valid.
180 + */
181 + return false;
182 + }
183 +
184 + const location = reference.identifier.range[1];
185 + const definition = reference.resolved.defs[0];
186 +
187 + if (definition.type === "ClassName") {
188 + // `ClassDeclaration` or `ClassExpression`
189 + const classDefinition = definition.node;
190 +
191 + return (
192 + isInRange(classDefinition, location) &&
193 + /*
194 + * Class binding is initialized before running static initializers.
195 + * For example, `class C { static foo = C; static { bar = C; } }` is valid.
196 + */
197 + !isInClassStaticInitializerRange(classDefinition.body, location)
198 + );
199 + }
200 +
201 + let node = definition.name.parent;
202 +
203 + while (node) {
204 + if (node.type === "VariableDeclarator") {
205 + if (isInRange(node.init, location)) {
206 + return true;
207 + }
208 + if (
209 + FOR_IN_OF_TYPE.test(node.parent.parent.type) &&
210 + isInRange(node.parent.parent.right, location)
211 + ) {
212 + return true;
213 + }
214 + break;
215 + } else if (node.type === "AssignmentPattern") {
216 + if (isInRange(node.right, location)) {
217 + return true;
218 + }
219 + } else if (SENTINEL_TYPE.test(node.type)) {
220 + break;
221 + }
222 +
223 + node = node.parent;
224 + }
225 +
226 + return false;
227 +}
228 +
229 +//------------------------------------------------------------------------------
230 +// Rule Definition
231 +//------------------------------------------------------------------------------
232 +
233 +/** @type {import('../shared/types').Rule} */
234 +const NoUseBeforeDefineRule = {
235 + meta: {
236 + type: "problem",
237 +
238 + docs: {
239 + description: "Disallow the use of variables before they are defined",
240 + recommended: false,
241 + url: "https://eslint.org/docs/rules/no-use-before-define",
242 + },
243 +
244 + schema: [
245 + {
246 + oneOf: [
247 + {
248 + enum: ["nofunc"],
249 + },
250 + {
251 + type: "object",
252 + properties: {
253 + functions: { type: "boolean" },
254 + classes: { type: "boolean" },
255 + variables: { type: "boolean" },
256 + allowNamedExports: { type: "boolean" },
257 + },
258 + additionalProperties: false,
259 + },
260 + ],
261 + },
262 + ],
263 +
264 + messages: {
265 + usedBeforeDefined: "'{{name}}' was used before it was defined.",
266 + },
267 + },
268 +
269 + create(context) {
270 + const options = parseOptions(context.options[0]);
271 +
272 + /**
273 + * Determines whether a given reference should be checked.
274 + *
275 + * Returns `false` if the reference is:
276 + * - initialization's (e.g., `let a = 1`).
277 + * - referring to an undefined variable (i.e., if it's an unresolved reference).
278 + * - referring to a variable that is defined, but not in the given source code
279 + * (e.g., global environment variable or `arguments` in functions).
280 + * - allowed by options.
281 + * @param {eslint-scope.Reference} reference The reference
282 + * @returns {boolean} `true` if the reference should be checked
283 + */
284 + function shouldCheck(reference) {
285 + if (reference.init) {
286 + return false;
287 + }
288 +
289 + const { identifier } = reference;
290 +
291 + if (
292 + options.allowNamedExports &&
293 + identifier.parent.type === "ExportSpecifier" &&
294 + identifier.parent.local === identifier
295 + ) {
296 + return false;
297 + }
298 +
299 + const variable = reference.resolved;
300 +
301 + if (!variable || variable.defs.length === 0) {
302 + return false;
303 + }
304 +
305 + const definitionType = variable.defs[0].type;
306 +
307 + if (
308 + ((!options.variables && definitionType === "Variable") ||
309 + (!options.classes && definitionType === "ClassName") ||
310 + (!options.functions && definitionType === "FunctionName")) &&
311 + // don't skip checking the reference if it's in the same execution context, because of TDZ
312 + isFromSeparateExecutionContext(reference)
313 + ) {
314 + return false;
315 + }
316 +
317 + return true;
318 + }
319 +
320 + /**
321 + * Finds and validates all references in a given scope and its child scopes.
322 + * @param {eslint-scope.Scope} scope The scope object.
323 + * @returns {void}
324 + */
325 + function checkReferencesInScope(scope) {
326 + scope.references.filter(shouldCheck).forEach((reference) => {
327 + const variable = reference.resolved;
328 + const definitionIdentifier = variable.defs[0].name;
329 +
330 + if (
331 + reference.identifier.range[1] < definitionIdentifier.range[1] ||
332 + isEvaluatedDuringInitialization(reference)
333 + ) {
334 + context.report({
335 + node: reference.identifier,
336 + messageId: "usedBeforeDefined",
337 + data: reference.identifier,
338 + });
339 + }
340 + });
341 +
342 + scope.childScopes.forEach(checkReferencesInScope);
343 + }
344 +
345 + function isReactFunction(node) {
346 + return (
347 + node.body.type === "BlockStatement" &&
348 + node.body.body.some(
349 + (stmt) =>
350 + stmt.type === "ExpressionStatement" &&
351 + stmt.expression.type === "Literal" &&
352 + stmt.expression.value === "use forget"
353 + )
354 + );
355 + }
356 +
357 + return {
358 + FunctionExpression(node) {
359 + if (isReactFunction(node)) {
360 + checkReferencesInScope(context.getScope());
361 + }
362 + },
363 + FunctionDeclaration(node) {
364 + if (isReactFunction(node)) {
365 + checkReferencesInScope(context.getScope());
366 + }
367 + },
368 + };
369 + },
370 +};
371 +
372 +export default NoUseBeforeDefineRule;
compiler/forget/src/Validation/PostCodegenValidator.ts renamed
compiler/forget/src/Validation/index.ts new
+9
@@ -0,0 +1,9 @@
1 +/**
2 + * Copyright (c) Facebook, Inc. and its 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 +export { default as NoUseBeforeDefineRule } from "./NoUseBeforeDefineRule";
9 +export { default as PostCodegenValidator } from "./PostCodegenValidator";
compiler/forget/src/__tests__/test-utils/validateNoUseBeforeDefine.ts
+8 -3
@@ -9,6 +9,8 @@
9 import { Linter } from "../../../node_modules/eslint/lib/linter";
10 // @ts-ignore-line
11 import * as HermesESLint from "hermes-eslint";
12 +// @ts-ignore-line
13 +import { NoUseBeforeDefineRule } from "../..";
14
15 const ESLINT_CONFIG: Linter.Config = {
16 parser: "hermes-eslint",
@@ -16,7 +18,10 @@ const ESLINT_CONFIG: Linter.Config = {
18 sourceType: "module",
19 },
20 rules: {
19 - "no-use-before-define": "error",
21 + "custom-no-use-before-define": [
22 + "error",
23 + { variables: false, functions: false },
24 + ],
25 },
26 };
27
@@ -31,6 +36,6 @@ export default function validateNoUseBeforeDefine(
36 ): Array<{ line: number; column: number; message: string }> | null {
37 const linter = new Linter();
38 linter.defineParser("hermes-eslint", HermesESLint);
34 - const errors = linter.verify(source, ESLINT_CONFIG);
35 - return errors;
39 + linter.defineRule("custom-no-use-before-define", NoUseBeforeDefineRule);
40 + return linter.verify(source, ESLINT_CONFIG);
41 }
compiler/forget/src/index.ts
+1
@@ -19,5 +19,6 @@ export * from "./CompilerOptions";
19 export * from "./CompilerOutputs";
20 export * from "./Diagnostic";
21 export * from "./Logger";
22 +export { NoUseBeforeDefineRule } from "./Validation";
23
24 export default BabelPlugin;