From 515c33d2a650dbfa6158c709cfc96092e136789e Mon Sep 17 00:00:00 2001 From: Joseph Savona Date: Thu, 27 Oct 2022 16:29:09 -0700 Subject: [PATCH] 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'. --- .../packages/playground/lib/compilerDriver.ts | 4 +- compiler/forget/src/BackEnd/JSGen.ts | 12 + compiler/forget/src/BackEnd/index.ts | 1 - compiler/forget/src/CompilerDriver.ts | 3 +- .../src/Validation/NoUseBeforeDefineRule.ts | 372 ++++++++++++++++++ .../PostCodegenValidator.ts | 0 compiler/forget/src/Validation/index.ts | 9 + .../test-utils/validateNoUseBeforeDefine.ts | 11 +- compiler/forget/src/index.ts | 1 + 9 files changed, 407 insertions(+), 6 deletions(-) create mode 100644 compiler/forget/src/Validation/NoUseBeforeDefineRule.ts rename compiler/forget/src/{BackEnd => Validation}/PostCodegenValidator.ts (100%) create mode 100644 compiler/forget/src/Validation/index.ts diff --git a/compiler/forget/packages/playground/lib/compilerDriver.ts b/compiler/forget/packages/playground/lib/compilerDriver.ts index 5f2e757e22..a359ad9b71 100644 --- a/compiler/forget/packages/playground/lib/compilerDriver.ts +++ b/compiler/forget/packages/playground/lib/compilerDriver.ts @@ -8,6 +8,7 @@ import { createArrayLogger, createCompilerOutputs, getMostRecentCompilerContext, + NoUseBeforeDefineRule, OutputKind, type CompilerContext, type CompilerOptions, @@ -36,7 +37,7 @@ const ESLINT_CONFIG = { sourceType: "module", }, rules: { - "no-use-before-define": "error", + "custom-no-use-before-define": "error", }, }; @@ -48,6 +49,7 @@ const ESLINT_CONFIG = { */ function validateNoUseBeforeDefine(source: string) { const linter = new ESLint.index.Linter(); + linter.defineRule("custom-no-use-before-define", NoUseBeforeDefineRule); return linter.verify(source, ESLINT_CONFIG); } diff --git a/compiler/forget/src/BackEnd/JSGen.ts b/compiler/forget/src/BackEnd/JSGen.ts index 98adc902d7..ce16c691db 100644 --- a/compiler/forget/src/BackEnd/JSGen.ts +++ b/compiler/forget/src/BackEnd/JSGen.ts @@ -173,4 +173,16 @@ export function runFunc( // recover directives. funcBody.node.directives = directives; + if ( + funcBody.node.directives.length === 0 || + funcBody.node.directives[0]!.value?.value !== "use forget" + ) { + funcBody.node.directives.unshift({ + type: "Directive", + value: { + type: "DirectiveLiteral", + value: "use forget", + }, + }); + } } diff --git a/compiler/forget/src/BackEnd/index.ts b/compiler/forget/src/BackEnd/index.ts index 13da4bd632..2c2e1c49f5 100644 --- a/compiler/forget/src/BackEnd/index.ts +++ b/compiler/forget/src/BackEnd/index.ts @@ -14,5 +14,4 @@ export { default as DumpLIR } from "./DumpLIR"; export { default as JSGen } from "./JSGen"; export { default as LIRGen } from "./LIRGen"; export { default as MemoCacheAlloc } from "./MemoCacheAlloc"; -export { default as PostCodegenValidator } from "./PostCodegenValidator"; export { default as SanityCheck } from "./SanityCheck"; diff --git a/compiler/forget/src/CompilerDriver.ts b/compiler/forget/src/CompilerDriver.ts index 9516c884ec..2f83f05660 100644 --- a/compiler/forget/src/CompilerDriver.ts +++ b/compiler/forget/src/CompilerDriver.ts @@ -12,6 +12,7 @@ import { CompilerContext } from "./CompilerContext"; import { CompilerOptions } from "./CompilerOptions"; import * as ME from "./MiddleEnd"; import { PassManager } from "./PassManager"; +import * as Validation from "./Validation"; /** * Compiler Driver @@ -59,7 +60,7 @@ export function createCompilerDriver( passManager.addPass(BE.JSGen); // Optionally sanity-check the transformed output - passManager.addPass(BE.PostCodegenValidator); + passManager.addPass(Validation.PostCodegenValidator); passManager.runAll(); }, diff --git a/compiler/forget/src/Validation/NoUseBeforeDefineRule.ts b/compiler/forget/src/Validation/NoUseBeforeDefineRule.ts new file mode 100644 index 0000000000..34478356ec --- /dev/null +++ b/compiler/forget/src/Validation/NoUseBeforeDefineRule.ts @@ -0,0 +1,372 @@ +/** + * Copyright (c) Facebook, Inc. and its affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +// @ts-nocheck + +// NOTE: this file is forked from ESLint's no-use-before-define rule: +// https://github.com/eslint/eslint/blob/15814057fd69319b3744bdea5db2455f85d2e74f/lib/rules/no-use-before-define.js +// The only change is to treat the {functions:false} option similarly to {variables:false}, +// ie to disable validation for functions defined at module scope but still check for locally +// defined functions. + +/** + * @fileoverview Rule to flag use of variables before they are defined + * @author Ilya Volodin + */ + +("use strict"); + +//------------------------------------------------------------------------------ +// Helpers +//------------------------------------------------------------------------------ + +const SENTINEL_TYPE = + /^(?:(?:Function|Class)(?:Declaration|Expression)|ArrowFunctionExpression|CatchClause|ImportDeclaration|ExportNamedDeclaration)$/u; +const FOR_IN_OF_TYPE = /^For(?:In|Of)Statement$/u; + +/** + * Parses a given value as options. + * @param {any} options A value to parse. + * @returns {Object} The parsed options. + */ +function parseOptions(options) { + let functions = true; + let classes = true; + let variables = true; + let allowNamedExports = false; + + if (typeof options === "string") { + functions = options !== "nofunc"; + } else if (typeof options === "object" && options !== null) { + functions = options.functions !== false; + classes = options.classes !== false; + variables = options.variables !== false; + allowNamedExports = !!options.allowNamedExports; + } + + return { functions, classes, variables, allowNamedExports }; +} + +/** + * Checks whether or not a given location is inside of the range of a given node. + * @param {ASTNode} node An node to check. + * @param {number} location A location to check. + * @returns {boolean} `true` if the location is inside of the range of the node. + */ +function isInRange(node, location) { + return node && node.range[0] <= location && location <= node.range[1]; +} + +/** + * Checks whether or not a given location is inside of the range of a class static initializer. + * Static initializers are static blocks and initializers of static fields. + * @param {ASTNode} node `ClassBody` node to check static initializers. + * @param {number} location A location to check. + * @returns {boolean} `true` if the location is inside of a class static initializer. + */ +function isInClassStaticInitializerRange(node, location) { + return node.body.some( + (classMember) => + (classMember.type === "StaticBlock" && + isInRange(classMember, location)) || + (classMember.type === "PropertyDefinition" && + classMember.static && + classMember.value && + isInRange(classMember.value, location)) + ); +} + +/** + * Checks whether a given scope is the scope of a class static initializer. + * Static initializers are static blocks and initializers of static fields. + * @param {eslint-scope.Scope} scope A scope to check. + * @returns {boolean} `true` if the scope is a class static initializer scope. + */ +function isClassStaticInitializerScope(scope) { + if (scope.type === "class-static-block") { + return true; + } + + if (scope.type === "class-field-initializer") { + // `scope.block` is PropertyDefinition#value node + const propertyDefinition = scope.block.parent; + + return propertyDefinition.static; + } + + return false; +} + +/** + * Checks whether a given reference is evaluated in an execution context + * that isn't the one where the variable it refers to is defined. + * Execution contexts are: + * - top-level + * - functions + * - class field initializers (implicit functions) + * - class static blocks (implicit functions) + * Static class field initializers and class static blocks are automatically run during the class definition evaluation, + * and therefore we'll consider them as a part of the parent execution context. + * Example: + * + * const x = 1; + * + * x; // returns `false` + * () => x; // returns `true` + * + * class C { + * field = x; // returns `true` + * static field = x; // returns `false` + * + * method() { + * x; // returns `true` + * } + * + * static method() { + * x; // returns `true` + * } + * + * static { + * x; // returns `false` + * } + * } + * @param {eslint-scope.Reference} reference A reference to check. + * @returns {boolean} `true` if the reference is from a separate execution context. + */ +function isFromSeparateExecutionContext(reference) { + const variable = reference.resolved; + let scope = reference.from; + + // Scope#variableScope represents execution context + while (variable.scope.variableScope !== scope.variableScope) { + if (isClassStaticInitializerScope(scope.variableScope)) { + scope = scope.variableScope.upper; + } else { + return true; + } + } + + return false; +} + +/** + * Checks whether or not a given reference is evaluated during the initialization of its variable. + * + * This returns `true` in the following cases: + * + * var a = a + * var [a = a] = list + * var {a = a} = obj + * for (var a in a) {} + * for (var a of a) {} + * var C = class { [C]; }; + * var C = class { static foo = C; }; + * var C = class { static { foo = C; } }; + * class C extends C {} + * class C extends (class { static foo = C; }) {} + * class C { [C]; } + * @param {Reference} reference A reference to check. + * @returns {boolean} `true` if the reference is evaluated during the initialization. + */ +function isEvaluatedDuringInitialization(reference) { + if (isFromSeparateExecutionContext(reference)) { + /* + * Even if the reference appears in the initializer, it isn't evaluated during the initialization. + * For example, `const x = () => x;` is valid. + */ + return false; + } + + const location = reference.identifier.range[1]; + const definition = reference.resolved.defs[0]; + + if (definition.type === "ClassName") { + // `ClassDeclaration` or `ClassExpression` + const classDefinition = definition.node; + + return ( + isInRange(classDefinition, location) && + /* + * Class binding is initialized before running static initializers. + * For example, `class C { static foo = C; static { bar = C; } }` is valid. + */ + !isInClassStaticInitializerRange(classDefinition.body, location) + ); + } + + let node = definition.name.parent; + + while (node) { + if (node.type === "VariableDeclarator") { + if (isInRange(node.init, location)) { + return true; + } + if ( + FOR_IN_OF_TYPE.test(node.parent.parent.type) && + isInRange(node.parent.parent.right, location) + ) { + return true; + } + break; + } else if (node.type === "AssignmentPattern") { + if (isInRange(node.right, location)) { + return true; + } + } else if (SENTINEL_TYPE.test(node.type)) { + break; + } + + node = node.parent; + } + + return false; +} + +//------------------------------------------------------------------------------ +// Rule Definition +//------------------------------------------------------------------------------ + +/** @type {import('../shared/types').Rule} */ +const NoUseBeforeDefineRule = { + meta: { + type: "problem", + + docs: { + description: "Disallow the use of variables before they are defined", + recommended: false, + url: "https://eslint.org/docs/rules/no-use-before-define", + }, + + schema: [ + { + oneOf: [ + { + enum: ["nofunc"], + }, + { + type: "object", + properties: { + functions: { type: "boolean" }, + classes: { type: "boolean" }, + variables: { type: "boolean" }, + allowNamedExports: { type: "boolean" }, + }, + additionalProperties: false, + }, + ], + }, + ], + + messages: { + usedBeforeDefined: "'{{name}}' was used before it was defined.", + }, + }, + + create(context) { + const options = parseOptions(context.options[0]); + + /** + * Determines whether a given reference should be checked. + * + * Returns `false` if the reference is: + * - initialization's (e.g., `let a = 1`). + * - referring to an undefined variable (i.e., if it's an unresolved reference). + * - referring to a variable that is defined, but not in the given source code + * (e.g., global environment variable or `arguments` in functions). + * - allowed by options. + * @param {eslint-scope.Reference} reference The reference + * @returns {boolean} `true` if the reference should be checked + */ + function shouldCheck(reference) { + if (reference.init) { + return false; + } + + const { identifier } = reference; + + if ( + options.allowNamedExports && + identifier.parent.type === "ExportSpecifier" && + identifier.parent.local === identifier + ) { + return false; + } + + const variable = reference.resolved; + + if (!variable || variable.defs.length === 0) { + return false; + } + + const definitionType = variable.defs[0].type; + + if ( + ((!options.variables && definitionType === "Variable") || + (!options.classes && definitionType === "ClassName") || + (!options.functions && definitionType === "FunctionName")) && + // don't skip checking the reference if it's in the same execution context, because of TDZ + isFromSeparateExecutionContext(reference) + ) { + return false; + } + + return true; + } + + /** + * Finds and validates all references in a given scope and its child scopes. + * @param {eslint-scope.Scope} scope The scope object. + * @returns {void} + */ + function checkReferencesInScope(scope) { + scope.references.filter(shouldCheck).forEach((reference) => { + const variable = reference.resolved; + const definitionIdentifier = variable.defs[0].name; + + if ( + reference.identifier.range[1] < definitionIdentifier.range[1] || + isEvaluatedDuringInitialization(reference) + ) { + context.report({ + node: reference.identifier, + messageId: "usedBeforeDefined", + data: reference.identifier, + }); + } + }); + + scope.childScopes.forEach(checkReferencesInScope); + } + + function isReactFunction(node) { + return ( + node.body.type === "BlockStatement" && + node.body.body.some( + (stmt) => + stmt.type === "ExpressionStatement" && + stmt.expression.type === "Literal" && + stmt.expression.value === "use forget" + ) + ); + } + + return { + FunctionExpression(node) { + if (isReactFunction(node)) { + checkReferencesInScope(context.getScope()); + } + }, + FunctionDeclaration(node) { + if (isReactFunction(node)) { + checkReferencesInScope(context.getScope()); + } + }, + }; + }, +}; + +export default NoUseBeforeDefineRule; diff --git a/compiler/forget/src/BackEnd/PostCodegenValidator.ts b/compiler/forget/src/Validation/PostCodegenValidator.ts similarity index 100% rename from compiler/forget/src/BackEnd/PostCodegenValidator.ts rename to compiler/forget/src/Validation/PostCodegenValidator.ts diff --git a/compiler/forget/src/Validation/index.ts b/compiler/forget/src/Validation/index.ts new file mode 100644 index 0000000000..831092bb79 --- /dev/null +++ b/compiler/forget/src/Validation/index.ts @@ -0,0 +1,9 @@ +/** + * Copyright (c) Facebook, Inc. and its affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +export { default as NoUseBeforeDefineRule } from "./NoUseBeforeDefineRule"; +export { default as PostCodegenValidator } from "./PostCodegenValidator"; diff --git a/compiler/forget/src/__tests__/test-utils/validateNoUseBeforeDefine.ts b/compiler/forget/src/__tests__/test-utils/validateNoUseBeforeDefine.ts index 0da87409f1..b2e86a3da6 100644 --- a/compiler/forget/src/__tests__/test-utils/validateNoUseBeforeDefine.ts +++ b/compiler/forget/src/__tests__/test-utils/validateNoUseBeforeDefine.ts @@ -9,6 +9,8 @@ import { Linter } from "../../../node_modules/eslint/lib/linter"; // @ts-ignore-line import * as HermesESLint from "hermes-eslint"; +// @ts-ignore-line +import { NoUseBeforeDefineRule } from "../.."; const ESLINT_CONFIG: Linter.Config = { parser: "hermes-eslint", @@ -16,7 +18,10 @@ const ESLINT_CONFIG: Linter.Config = { sourceType: "module", }, rules: { - "no-use-before-define": "error", + "custom-no-use-before-define": [ + "error", + { variables: false, functions: false }, + ], }, }; @@ -31,6 +36,6 @@ export default function validateNoUseBeforeDefine( ): Array<{ line: number; column: number; message: string }> | null { const linter = new Linter(); linter.defineParser("hermes-eslint", HermesESLint); - const errors = linter.verify(source, ESLINT_CONFIG); - return errors; + linter.defineRule("custom-no-use-before-define", NoUseBeforeDefineRule); + return linter.verify(source, ESLINT_CONFIG); } diff --git a/compiler/forget/src/index.ts b/compiler/forget/src/index.ts index 5effbf5ff3..e9ca894cba 100644 --- a/compiler/forget/src/index.ts +++ b/compiler/forget/src/index.ts @@ -19,5 +19,6 @@ export * from "./CompilerOptions"; export * from "./CompilerOutputs"; export * from "./Diagnostic"; export * from "./Logger"; +export { NoUseBeforeDefineRule } from "./Validation"; export default BabelPlugin;