diff --git a/docs/rules/isolated-functions.md b/docs/rules/isolated-functions.md new file mode 100644 index 0000000000..642b8af6c3 --- /dev/null +++ b/docs/rules/isolated-functions.md @@ -0,0 +1,271 @@ +# Prevent usage of variables from outside the scope of isolated functions + +πŸ’ΌπŸš« This rule is enabled in the βœ… `recommended` [config](https://github.com/sindresorhus/eslint-plugin-unicorn#recommended-config). This rule is _disabled_ in the β˜‘οΈ `unopinionated` [config](https://github.com/sindresorhus/eslint-plugin-unicorn#recommended-config). + + + + +Some functions need to be isolated from their surrounding scope due to execution context constraints. For example, functions passed to [`makeSynchronous()`](https://github.com/sindresorhus/make-synchronous) are executed in a worker or subprocess and cannot access variables from outside their scope. This rule helps identify when functions are using external variables that may cause runtime errors. + +Common scenarios where functions must be isolated: + +- Functions passed to `makeSynchronous()` (executed in worker) +- Functions that will be serialized via `Function.prototype.toString()` +- Server actions or other remote execution contexts +- Functions with specific JSDoc annotations + +By default, this rule uses ESLint's language options globals and allows global variables (like `console`, `fetch`, etc.) in isolated functions, but prevents usage of variables from the surrounding scope. + +## Examples + +```js +import makeSynchronous from 'make-synchronous'; + +const url = 'https://example.com'; + +const getText = makeSynchronous(async () => { + const response = await fetch(url); // ❌ 'url' is not defined in isolated function scope + return response.text(); +}); + +// βœ… Define all variables within isolated function's scope +const getText = makeSynchronous(async () => { + const url = 'https://example.com'; // Variable defined within function scope + const response = await fetch(url); + return response.text(); +}); + +// βœ… Alternative: Pass as parameter +const getText = makeSynchronous(async (url) => { // Variable passed as parameter + const response = await fetch(url); + return response.text(); +}); + +console.log(getText('https://example.com')); +``` + +```js +const foo = 'hi'; + +/** @isolated */ +function abc() { + return foo.slice(); // ❌ 'foo' is not defined in isolated function scope +} + +// βœ… +/** @isolated */ +function abc() { + const foo = 'hi'; // Variable defined within function scope + return foo.slice(); +} +``` + +## Options + +Type: `object` + +### functions + +Type: `string[]`\ +Default: `['makeSynchronous']` + +Array of function names that create isolated execution contexts. Functions passed as arguments to these functions will be considered isolated. + +### selectors + +Type: `string[]`\ +Default: `[]` + +Array of [ESLint selectors](https://eslint.org/docs/developer-guide/selectors) to identify isolated functions. Useful for custom naming conventions or framework-specific patterns. + +```js +{ + 'unicorn/isolated-functions': [ + 'error', + { + selectors: [ + 'FunctionDeclaration[id.name=/lambdaHandler.*/]' + ] + } + ] +} +``` + +### comments + +Type: `string[]`\ +Default: `['@isolated']` + +Array of comment strings that mark functions as isolated. Functions with inline, block, or JSDoc comments tagged with these strings will be considered isolated. (Definition of "tagged": either the comment consists solely of the tag, or starts with it, and has an explanation following a hyphen, like `// @isolated - this function will be stringified`). + +```js +{ + 'unicorn/isolated-functions': [ + 'error', + { + comments: [ + '@isolated', + '@remote' + ] + } + ] +} +``` + +### overrideGlobals + +Type: `object`\ +Default: `undefined` (uses ESLint's language options globals) + +Controls how global variables are handled. When not specified, uses ESLint's language options globals. When specified as an object, each key is a global variable name and the value controls its behavior: + +- `'readonly'`: Global variable is allowed but cannot be written to +- `'writable'`: Global variable is allowed and can be read/written +- `'off'`: Global variable is not allowed + +```js +{ + 'unicorn/isolated-functions': [ + 'error', + { + overrideGlobals: { + console: 'writable', // Allowed and writable + fetch: 'readonly', // Allowed but readonly + process: 'off' // Not allowed + } + } + ] +} +``` + +## Examples + +### Custom function names + +```js +{ + 'unicorn/isolated-functions': [ + 'error', + { + functions: [ + 'makeSynchronous', + 'createWorker', + 'serializeFunction' + ] + } + ] +} +``` + +### Lambda function naming convention + +```js +{ + 'unicorn/isolated-functions': [ + 'error', + { + selectors: [ + 'FunctionDeclaration[id.name=/lambdaHandler.*/]' + ] + } + ] +} +``` + +```js +const foo = 'hi'; + +function lambdaHandlerFoo() { // ❌ Will be flagged as isolated + return foo.slice(); +} + +function someOtherFunction() { // βœ… Not flagged + return foo.slice(); +} + +createLambda({ + name: 'fooLambda', + code: lambdaHandlerFoo.toString(), // Function will be serialized +}); +``` + +### Default behavior (using ESLint's language options) + +```js +// Uses ESLint's language options globals by default +makeSynchronous(async () => { + console.log('Starting...'); // βœ… Allowed if console is in language options + const response = await fetch('https://api.example.com'); // βœ… Allowed if fetch is in language options + return response.text(); +}); +``` + +### Allowing specific globals + +```js +{ + 'unicorn/isolated-functions': [ + 'error', + { + overrideGlobals: { + console: 'writable', // Allowed and writable + fetch: 'readonly', // Allowed but readonly + URL: 'readonly' // Allowed but readonly + } + } + ] +} +``` + +```js +// βœ… All globals used are explicitly allowed +makeSynchronous(async () => { + console.log('Starting...'); // βœ… Allowed global + const response = await fetch('https://api.example.com'); // βœ… Allowed global + const url = new URL(response.url); // βœ… Allowed global + return response.text(); +}); + +makeSynchronous(async () => { + const response = await fetch('https://api.example.com', { + headers: { + 'Authorization': `Bearer ${process.env.API_TOKEN}` // ❌ 'process' is not in allowed globals + } + }); + + const url = new URL(response.url); + + return response.text(); +}); + +// ❌ Attempting to write to readonly global +makeSynchronous(async () => { + fetch = null; // ❌ 'fetch' is readonly + console.log('Starting...'); +}); +``` + +### Predefined global variables + +To enable a predefined set of globals, use the [`globals` package](https://npmjs.com/package/globals) similarly to how you would use it in `languageOptions` (see [ESLint docs on globals](https://eslint.org/docs/latest/use/configure/language-options#predefined-global-variables)): + +```js +import globals from 'globals' + +export default [ + { + rules: { + 'unicorn/isolated-functions': [ + 'error', + { + globals: { + ...globals.builtin, + ...globals.applescript, + ...globals.greasemonkey, + }, + }, + ], + }, + }, +] +``` diff --git a/readme.md b/readme.md index eaf3113b9c..3dc0b9edde 100644 --- a/readme.md +++ b/readme.md @@ -73,6 +73,7 @@ export default [ | [explicit-length-check](docs/rules/explicit-length-check.md) | Enforce explicitly comparing the `length` or `size` property of a value. | βœ… | πŸ”§ | πŸ’‘ | | [filename-case](docs/rules/filename-case.md) | Enforce a case style for filenames. | βœ… | | | | [import-style](docs/rules/import-style.md) | Enforce specific import styles per module. | βœ… β˜‘οΈ | | | +| [isolated-functions](docs/rules/isolated-functions.md) | Prevent usage of variables from outside the scope of isolated functions. | βœ… | | | | [new-for-builtins](docs/rules/new-for-builtins.md) | Enforce the use of `new` for all builtins, except `String`, `Number`, `Boolean`, `Symbol` and `BigInt`. | βœ… β˜‘οΈ | πŸ”§ | πŸ’‘ | | [no-abusive-eslint-disable](docs/rules/no-abusive-eslint-disable.md) | Enforce specifying rules to disable in `eslint-disable` comments. | βœ… β˜‘οΈ | | | | [no-accessor-recursion](docs/rules/no-accessor-recursion.md) | Disallow recursive access to `this` within getters and setters. | βœ… β˜‘οΈ | | | diff --git a/rules/index.js b/rules/index.js index 0bb7fb36eb..f28bfe7015 100644 --- a/rules/index.js +++ b/rules/index.js @@ -16,6 +16,7 @@ export {default as 'expiring-todo-comments'} from './expiring-todo-comments.js'; export {default as 'explicit-length-check'} from './explicit-length-check.js'; export {default as 'filename-case'} from './filename-case.js'; export {default as 'import-style'} from './import-style.js'; +export {default as 'isolated-functions'} from './isolated-functions.js'; export {default as 'new-for-builtins'} from './new-for-builtins.js'; export {default as 'no-abusive-eslint-disable'} from './no-abusive-eslint-disable.js'; export {default as 'no-accessor-recursion'} from './no-accessor-recursion.js'; diff --git a/rules/isolated-functions.js b/rules/isolated-functions.js new file mode 100644 index 0000000000..4f38c53e8c --- /dev/null +++ b/rules/isolated-functions.js @@ -0,0 +1,199 @@ +import globals from 'globals'; +import esquery from 'esquery'; +import {functionTypes} from './ast/index.js'; + +const MESSAGE_ID_EXTERNALLY_SCOPED_VARIABLE = 'externally-scoped-variable'; +const messages = { + [MESSAGE_ID_EXTERNALLY_SCOPED_VARIABLE]: 'Variable {{name}} not defined in scope of isolated function. Function is isolated because: {{reason}}.', +}; + +const parsedEsquerySelectors = new Map(); +const parseEsquerySelector = selector => { + if (!parsedEsquerySelectors.has(selector)) { + parsedEsquerySelectors.set(selector, esquery.parse(selector)); + } + + return parsedEsquerySelectors.get(selector); +}; + +/** @type {{functions: string[], selectors: string[], comments: string[], overrideGlobals?: import('eslint').Linter.Globals}} */ +const defaultOptions = { + functions: ['makeSynchronous'], + selectors: [], + comments: ['@isolated'], + overrideGlobals: {}, +}; + +/** @param {import('eslint').Rule.RuleContext} context */ +const create = context => { + const {sourceCode} = context; + /** @type {typeof defaultOptions} */ + const options = { + ...defaultOptions, + ...context.options[0], + }; + + options.comments = options.comments.map(comment => comment.toLowerCase()); + + const allowedGlobals = { + ...(globals[`es${context.languageOptions.ecmaVersion}`] ?? globals.builtins), + ...context.languageOptions.globals, + ...options.overrideGlobals, + }; + + /** @param {import('estree').Node} node */ + const checkForExternallyScopedVariables = node => { + let reason = reasonForBeingIsolatedFunction(node); + if (!reason) { + return; + } + + const nodeScope = sourceCode.getScope(node); + + // `through`: "The array of references which could not be resolved in this scope" https://eslint.org/docs/latest/extend/scope-manager-interface#scope-interface + for (const reference of nodeScope.through) { + const {identifier} = reference; + if (identifier.name in allowedGlobals && allowedGlobals[identifier.name] !== 'off') { + if (reference.isReadOnly()) { + continue; + } + + const globalsValue = allowedGlobals[identifier.name]; + const isGlobalWritable = globalsValue === true || globalsValue === 'writable' || globalsValue === 'writeable'; + if (isGlobalWritable) { + continue; + } + + reason += ' (global variable is not writable)'; + } + + // Could consider checking for typeof operator here, like in no-undef? + + context.report({ + node: identifier, + messageId: MESSAGE_ID_EXTERNALLY_SCOPED_VARIABLE, + data: {name: identifier.name, reason}, + }); + } + }; + + const isComment = token => token?.type === 'Block' || token?.type === 'Line'; + + /** + Find a comment on this node or its parent, in cases where the node passed is part of a variable or export declaration. + @param {import('estree').Node} node + */ + const findComment = node => { + let previousToken = sourceCode.getTokenBefore(node, {includeComments: true}); + let commentableNode = node; + while ( + !isComment(previousToken) + && (commentableNode.parent.type === 'VariableDeclarator' + || commentableNode.parent.type === 'VariableDeclaration' + || commentableNode.parent.type === 'ExportNamedDeclaration' + || commentableNode.parent.type === 'ExportDefaultDeclaration') + ) { + commentableNode = commentableNode.parent; + previousToken = sourceCode.getTokenBefore(commentableNode, {includeComments: true}); + } + + if (isComment(previousToken)) { + return previousToken.value; + } + }; + + /** + Find the string "reason" that a function (node) should be considered isolated. For passing in to `context.report(...)` when out-of-scope variables are found. Returns undefined if the function should not be considered isolated. + @param {import('estree').Node & {parent?: import('estree').Node}} node + */ + const reasonForBeingIsolatedFunction = node => { + if (options.comments.length > 0) { + let previousComment = findComment(node); + + if (previousComment) { + previousComment = previousComment + .replace(/(?:\*\s*)*/, '') // JSDoc comments like `/** @isolated */` are parsed into `* @isolated`. And `/**\n * @isolated */` is parsed into `*\n * @isolated` + .trim() + .toLowerCase(); + const match = options.comments.find(comment => previousComment === comment || previousComment.startsWith(`${comment} - `) || previousComment.startsWith(`${comment} -- `)); + if (match) { + return `follows comment ${JSON.stringify(match)}`; + } + } + } + + if ( + options.functions.length > 0 + && node.parent.type === 'CallExpression' + && node.parent.arguments.includes(node) + && node.parent.callee.type === 'Identifier' + && options.functions.includes(node.parent.callee.name) + ) { + return `callee of function named ${JSON.stringify(node.parent.callee.name)}`; + } + + if (options.selectors.length > 0) { + const ancestors = sourceCode.getAncestors(node); + const matchedSelector = options.selectors.find(selector => esquery.matches(node, parseEsquerySelector(selector), ancestors)); + if (matchedSelector) { + return `matches selector ${JSON.stringify(matchedSelector)}`; + } + } + }; + + context.on( + functionTypes.map(type => `${type}:exit`), + checkForExternallyScopedVariables, + ); +}; + +/** @type {import('json-schema').JSONSchema7[]} */ +const schema = [ + { + type: 'object', + additionalProperties: false, + properties: { + overrideGlobals: { + additionalProperties: { + anyOf: [{type: 'boolean'}, {type: 'string', enum: ['readonly', 'writable', 'writeable', 'off']}], + }, + }, + functions: { + type: 'array', + uniqueItems: true, + items: { + type: 'string', + }, + }, + selectors: { + type: 'array', + uniqueItems: true, + items: { + type: 'string', + }, + }, + comments: { + type: 'array', + uniqueItems: true, + items: { + type: 'string', + }, + }, + }, + }, +]; + +/** @type {import('eslint').Rule.RuleModule} */ +export default { + create, + meta: { + type: 'problem', + docs: { + description: 'Prevent usage of variables from outside the scope of isolated functions.', + recommended: true, + }, + schema, + defaultOptions: [defaultOptions], + messages, + }, +}; diff --git a/test/isolated-functions.js b/test/isolated-functions.js new file mode 100644 index 0000000000..a493caf65c --- /dev/null +++ b/test/isolated-functions.js @@ -0,0 +1,278 @@ +import outdent from 'outdent'; +import {getTester} from './utils/test.js'; + +const {test} = getTester(import.meta); + +const error = data => ({messageId: 'externally-scoped-variable', data}); +const fooInMakeSynchronousError = error({name: 'foo', reason: 'callee of function named "makeSynchronous"'}); + +test({ + /** @type {import('eslint').RuleTester.ValidTestCase[]} */ + valid: [ + { + name: 'variable defined in scope of isolated function', + code: outdent` + makeSynchronous(() => { + const foo = 'hi'; + return foo.slice(); + }); + `, + }, + { + name: 'variable defined as parameter of isolated function', + code: outdent` + makeSynchronous(foo => { + return foo.slice(); + }); + `, + }, + { + name: 'inner function can access outer function parameters', + code: outdent` + /** @isolated */ + function abc () { + const foo = 'hi'; + const slice = () => foo.slice(); + return slice(); + } + `, + }, + { + name: 'variable defined as parameter of isolated function (async)', + code: outdent` + makeSynchronous(async function (foo) { + return foo.slice(); + }); + `, + }, + { + name: 'default global variables come from language options', + code: 'makeSynchronous(() => process.env.MAP ? new Map() : new URL("https://example.com"))', + }, + { + name: 'global Array', + code: 'makeSynchronous(() => new Array())', + }, + { + name: 'global Array w overrideGlobals: {} still works', + code: 'makeSynchronous(() => new Array())', + options: [{overrideGlobals: {}}], + }, + { + name: 'can implicitly allow global variables from language options', + languageOptions: {globals: {foo: true}}, + code: outdent` + makeSynchronous(function () { + return foo.slice(); + }); + `, + }, + { + name: 'allow global variables separate from language options', + languageOptions: {globals: {abc: true}}, + options: [{overrideGlobals: {foo: true}}], + code: outdent` + makeSynchronous(function () { + return foo.slice(); + }); + `, + }, + ], + /** @type {import('eslint').RuleTester.InvalidTestCase[]} */ + invalid: [ + { + name: 'out of scope variable under makeSynchronous (arrow function)', + code: outdent` + const foo = 'hi'; + makeSynchronous(() => foo.slice()); + `, + errors: [fooInMakeSynchronousError], + }, + { + name: 'out of scope variable under makeSynchronous (async arrow function)', + code: outdent` + const foo = 'hi'; + makeSynchronous(async () => foo.slice()); + `, + errors: [fooInMakeSynchronousError], + }, + { + name: 'out of scope variable under makeSynchronous (function expression)', + code: outdent` + const foo = 'hi'; + makeSynchronous(function () { + return foo.slice(); + }); + `, + errors: [fooInMakeSynchronousError], + }, + { + name: 'out of scope variable under makeSynchronous (async function expression)', + code: outdent` + const foo = 'hi'; + makeSynchronous(async function () { + return foo.slice(); + }); + `, + errors: [fooInMakeSynchronousError], + }, + { + name: 'out of scope variable under makeSynchronous (named function expression)', + code: outdent` + const foo = 'hi'; + makeSynchronous(function abc () { + return foo.slice(); + }); + `, + errors: [fooInMakeSynchronousError], + }, + { + name: 'out of scope variable under makeSynchronous (named async function expression)', + code: outdent` + const foo = 'hi'; + makeSynchronous(async function abc () { + return foo.slice(); + }); + `, + errors: [fooInMakeSynchronousError], + }, + { + name: '@isolated comment on function declaration', + code: outdent` + const foo = 'hi'; + /** @isolated */ + function abc () { + return foo.slice(); + } + `, + errors: [error({name: 'foo', reason: 'follows comment "@isolated"'})], + }, + { + name: '@isolated comment on arrow function', + code: outdent` + const foo = 'hi'; + /** @isolated */ + const abc = () => foo.slice(); + `, + errors: [error({name: 'foo', reason: 'follows comment "@isolated"'})], + }, + { + name: '@isolated comments with explanations', + code: outdent` + const foo = 'hi'; + // @isolated - explanation + const abc1 = () => foo.slice(); + + // @isolated -- explanation + const abc2 = () => foo.slice(); + `, + errors: [ + error({name: 'foo', reason: 'follows comment "@isolated"'}), + error({name: 'foo', reason: 'follows comment "@isolated"'}), + ], + }, + { + name: '@isolated block comments', + code: outdent` + const foo = 'hi'; + /* @isolated */ + const abc1 = () => foo.slice(); + + /** @isolated */ + const abc2 = () => foo.slice(); + + /** + * @isolated + */ + const abc3 = () => foo.slice(); + `, + errors: [ + error({name: 'foo', reason: 'follows comment "@isolated"'}), + error({name: 'foo', reason: 'follows comment "@isolated"'}), + error({name: 'foo', reason: 'follows comment "@isolated"'}), + ], + }, + { + name: '@isolated inline comment', + code: outdent` + const foo = 'hi'; + // @isolated + const abc = () => foo.slice(); + `, + errors: [error({name: 'foo', reason: 'follows comment "@isolated"'})], + }, + { + name: '@isolated comment on exports', + code: outdent` + const foo = 'hi'; + + // @isolated + export const abc = () => foo.slice(); + + // @isolated + export default () => foo.slice(); + `, + errors: [ + error({name: 'foo', reason: 'follows comment "@isolated"'}), + error({name: 'foo', reason: 'follows comment "@isolated"'}), + ], + }, + { + name: 'individual global variables can be explicitly disallowed', + options: [{overrideGlobals: {URLSearchParams: 'readonly', URL: 'off'}}], + code: outdent` + makeSynchronous(function () { + return new URL('https://example.com?') + new URLSearchParams({a: 'b'}).toString(); + }); + `, + errors: [error({name: 'URL', reason: 'callee of function named "makeSynchronous"'})], + }, + { + name: 'check globals writability', + code: outdent` + makeSynchronous(function () { + location = new URL('https://example.com'); + process = {env: {}}; + process.env.FOO = 'bar'; + }); + `, + errors: [ + // Only one error, `location = new URL('https://example.com')` and `process.env.FOO = 'bar'` are fine, the problem is `process = {...}`. + error({ + name: 'process', + reason: 'callee of function named "makeSynchronous" (global variable is not writable)', + }), + ], + }, + { + name: 'make a function isolated by a selector', + // In this case, we're imagining some naming convention for lambda functions that will be created via `fn.toString()` + options: [{selectors: ['FunctionDeclaration[id.name=/lambdaHandler.*/]']}], + code: outdent` + const foo = 'hi'; + + function lambdaHandlerFoo() { + return foo.slice(); + } + + function someOtherFunction() { + return foo.slice(); + } + + createLambda({ + name: 'fooLambda', + code: lambdaHandlerFoo.toString(), + }); + `, + errors: [ + error({name: 'foo', reason: 'matches selector "FunctionDeclaration[id.name=/lambdaHandler.*/]"'}), + ], + }, + { + name: 'can explicitly turn off ecmascript globals', + code: 'makeSynchronous(() => new Array())', + options: [{overrideGlobals: {Array: 'off'}}], + errors: [error({name: 'Array', reason: 'callee of function named "makeSynchronous"'})], + }, + ], +});