Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/shy-countries-carry.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'eslint-plugin-svelte': patch
---

fix: preventing infinite loops in multiple rules
13 changes: 12 additions & 1 deletion packages/eslint-plugin-svelte/eslint.config.mjs
Original file line number Diff line number Diff line change
@@ -1,5 +1,12 @@
import * as myPlugin from '@ota-meshi/eslint-plugin';
import * as tseslint from 'typescript-eslint';
import { createJiti } from 'jiti';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is jiti really needed here? I don't understand this much, but why not just import the rule straight away?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The rule is implemented in Typescript, so we need to import them via jiti. This will no longer be necessary once we drop support for older Node.

const jiti = createJiti(import.meta.url);
const internal = {
rules: {
'prefer-find-variable-safe': await jiti.import('./internal-rules/prefer-find-variable-safe.ts')
}
};

/**
* @type {import('eslint').Linter.Config[]}
Expand Down Expand Up @@ -54,6 +61,9 @@ const config = [
},
{
files: ['src/**'],
plugins: {
internal
},
rules: {
'@typescript-eslint/no-restricted-imports': [
'error',
Expand All @@ -80,7 +90,8 @@ const config = [
{ object: 'context', property: 'getCwd', message: 'Use `context.cwd`' },
{ object: 'context', property: 'getScope', message: 'Use src/utils/compat.ts' },
{ object: 'context', property: 'parserServices', message: 'Use src/utils/compat.ts' }
]
],
'internal/prefer-find-variable-safe': 'error'
}
},
...tseslint.config({
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,171 @@
import type { Rule } from 'eslint';
import { ReferenceTracker, findVariable } from '@eslint-community/eslint-utils';
import path from 'node:path';
import type { TSESTree } from '@typescript-eslint/types';
import type { Variable } from '@typescript-eslint/scope-manager';

export default {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd still prefer just merging findVariableSafe into findVariable. Then the whole complication with this, and jiti can go away. And if I understand correctly, it's entirely possible, especially if we switch to marker: Symbol or marker: string.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's redundant to have more arguments that we won't use.
So I think it's better to leave it as it is.

meta: {
docs: {
description: 'enforce to use FindVariableContext to avoid infinite recursion',
category: 'Best Practices',
recommended: false,
conflictWithPrettier: false,
url: 'https://github.com/sveltejs/eslint-plugin-svelte/blob/v3.12.3/docs/rules/prefer-find-variable-safe.md'
},
messages: {
preferFindVariableSafe: 'Prefer to use FindVariableContext to avoid infinite recursion.'
},
schema: [],
type: 'suggestion'
},
create(context: Rule.RuleContext): Rule.RuleListener {
const referenceTracker = new ReferenceTracker(
context.sourceCode.scopeManager.globalScope as never
);
let astUtilsPath = path.relative(
path.dirname(context.physicalFilename),
path.join(import.meta.dirname, '..', 'src', 'utils', 'ast-utils')
);
if (!astUtilsPath.startsWith('.')) {
astUtilsPath = `./${astUtilsPath}`;
}
const findVariableCalls = [
...referenceTracker.iterateEsmReferences({
[astUtilsPath]: {
[ReferenceTracker.ESM]: true,
findVariable: {
[ReferenceTracker.CALL]: true
}
},
[`${astUtilsPath}.js`]: {
[ReferenceTracker.ESM]: true,
findVariable: {
[ReferenceTracker.CALL]: true
}
},
[`${astUtilsPath}.ts`]: {
[ReferenceTracker.ESM]: true,
findVariable: {
[ReferenceTracker.CALL]: true
}
}
})
];
type FunctionContext = {
node:
| TSESTree.FunctionDeclaration
| TSESTree.FunctionExpression
| TSESTree.ArrowFunctionExpression;
identifier: TSESTree.Identifier | null;
findVariableCall?: TSESTree.CallExpression;
calls: Set<TSESTree.Identifier>;
upper: FunctionContext | null;
};
let functionStack: FunctionContext | null = null;
const functionContexts: FunctionContext[] = [];

function getFunctionVariableName(
node:
| TSESTree.FunctionDeclaration
| TSESTree.FunctionExpression
| TSESTree.ArrowFunctionExpression
) {
if (node.type === 'FunctionDeclaration') {
return node.id;
}
if (node.parent?.type === 'VariableDeclarator' && node.parent.id.type === 'Identifier') {
return node.parent.id;
}
return null;
}

function* iterateVariables(node: TSESTree.Identifier) {
const visitedNodes = new Set<TSESTree.Identifier>();
let currentNode: TSESTree.Identifier | null = node;
while (currentNode) {
if (visitedNodes.has(currentNode)) break;
const variable = findVariable(
context.sourceCode.getScope(currentNode),
currentNode
) as Variable | null;
if (!variable) break;
yield variable;
const def = variable.defs[0];
if (!def) break;
if (def.type !== 'Variable' || !def.node.init) break;
if (def.node.init.type !== 'Identifier') break;
currentNode = def.node.init;
visitedNodes.add(currentNode);
}
}

/**
* Verify a function context to report if necessary.
* Reports when a function contains a call to findVariable and the function is recursive.
*/
function verifyFunctionContext(functionContext: FunctionContext) {
if (!functionContext.findVariableCall) return;
if (!hasRecursive(functionContext)) return;
context.report({
node: functionContext.findVariableCall,
messageId: 'preferFindVariableSafe'
});
}

function hasRecursive(functionContext: FunctionContext) {
const buffer = [functionContext];
const visitedContext = new Set<FunctionContext>();
let current;
while ((current = buffer.shift())) {
if (visitedContext.has(current)) continue;
visitedContext.add(current);
if (!current.identifier) continue;
for (const variable of iterateVariables(current.identifier)) {
for (const { identifier } of variable.references) {
if (identifier.type !== 'Identifier') continue;
if (functionContext.calls.has(identifier)) {
return true;
}
buffer.push(...functionContexts.filter((ctx) => ctx.calls.has(identifier)));
}
}
}
return false;
}

return {
':function'(
node:
| TSESTree.FunctionDeclaration
| TSESTree.FunctionExpression
| TSESTree.ArrowFunctionExpression
) {
functionStack = {
node,
identifier: getFunctionVariableName(node),
calls: new Set(),
upper: functionStack
};
functionContexts.push(functionStack);
},
':function:exit'() {
functionStack = functionStack?.upper || null;
},
CallExpression(node) {
if (!functionStack) return;
if (findVariableCalls.some((call) => call.node === node)) {
functionStack.findVariableCall = node;
}
if (node.callee.type === 'Identifier') {
functionStack.calls.add(node.callee);
}
},
'Program:exit'() {
for (const functionContext of functionContexts) {
verifyFunctionContext(functionContext);
}
}
};
}
};
1 change: 1 addition & 0 deletions packages/eslint-plugin-svelte/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,7 @@
"eslint-typegen": "^2.3.0",
"eslint-visitor-keys": "^4.2.1",
"espree": "^10.4.0",
"jiti": "^2.5.1",
"less": "^4.4.1",
"mocha": "~11.7.2",
"postcss-nested": "^7.0.2",
Expand Down
15 changes: 7 additions & 8 deletions packages/eslint-plugin-svelte/src/rules/no-dynamic-slot-name.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import type { AST } from 'svelte-eslint-parser';
import type { TSESTree } from '@typescript-eslint/types';
import { createRule } from '../utils/index.js';
import {
findVariable,
FindVariableContext,
getAttributeValueQuoteAndRange,
getStringIfConstant
} from '../utils/ast-utils.js';
Expand Down Expand Up @@ -67,28 +67,27 @@ export default createRule('no-dynamic-slot-name', {
* Get static text from given expression
*/
function getStaticText(node: TSESTree.Expression) {
const expr = findRootExpression(node);
const expr = findRootExpression(new FindVariableContext(context), node);
return getStringIfConstant(expr);
}

/** Find data expression */
function findRootExpression(
node: TSESTree.Expression,
already = new Set<TSESTree.Identifier>()
ctx: FindVariableContext,
node: TSESTree.Expression
): TSESTree.Expression {
if (node.type !== 'Identifier' || already.has(node)) {
if (node.type !== 'Identifier') {
return node;
}
already.add(node);
const variable = findVariable(context, node);
const variable = ctx.findVariable(node);
if (!variable || variable.defs.length !== 1) {
return node;
}
const def = variable.defs[0];
if (def.type === 'Variable') {
if (def.parent.kind === 'const' && def.node.init) {
const init = def.node.init;
return findRootExpression(init, already);
return findRootExpression(ctx, init);
}
}
return node;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import type { AST } from 'svelte-eslint-parser';
import { createRule } from '../utils/index.js';
import type { Scope, Variable, Reference, Definition } from '@typescript-eslint/scope-manager';
import type { TSESTree } from '@typescript-eslint/types';
import { findVariable, iterateIdentifiers } from '../utils/ast-utils.js';
import { FindVariableContext, iterateIdentifiers } from '../utils/ast-utils.js';

export default createRule('no-immutable-reactive-statements', {
meta: {
Expand Down Expand Up @@ -91,7 +91,7 @@ export default createRule('no-immutable-reactive-statements', {
return true;
}
}
return hasWrite(variable);
return hasWrite(new FindVariableContext(context), variable);
}
return false;
});
Expand All @@ -100,7 +100,7 @@ export default createRule('no-immutable-reactive-statements', {
}

/** Checks whether the given variable has a write or reactive store reference or not. */
function hasWrite(variable: Variable) {
function hasWrite(ctx: FindVariableContext, variable: Variable) {
const defIds = variable.defs.map((def: Definition) => def.name);
for (const reference of variable.references) {
if (
Expand All @@ -113,7 +113,7 @@ export default createRule('no-immutable-reactive-statements', {
) {
return true;
}
if (hasWriteMember(reference.identifier)) {
if (hasWriteMember(ctx, reference.identifier)) {
return true;
}
}
Expand All @@ -122,6 +122,7 @@ export default createRule('no-immutable-reactive-statements', {

/** Checks whether the given expression has writing to a member or not. */
function hasWriteMember(
ctx: FindVariableContext,
expr: TSESTree.Identifier | TSESTree.JSXIdentifier | TSESTree.MemberExpression
): boolean {
if (expr.type === 'JSXIdentifier') return false;
Expand All @@ -136,25 +137,30 @@ export default createRule('no-immutable-reactive-statements', {
return parent.operator === 'delete' && parent.argument === expr;
}
if (parent.type === 'MemberExpression') {
return parent.object === expr && hasWriteMember(parent);
return parent.object === expr && hasWriteMember(ctx, parent);
}
if (parent.type === 'SvelteDirective') {
return parent.kind === 'Binding' && parent.expression === expr;
}
if (parent.type === 'SvelteEachBlock') {
return (
parent.context !== null && parent.expression === expr && hasWriteReference(parent.context)
parent.context !== null &&
parent.expression === expr &&
hasWriteReference(ctx, parent.context)
);
}

return false;
}

/** Checks whether the given pattern has writing or not. */
function hasWriteReference(pattern: TSESTree.DestructuringPattern): boolean {
function hasWriteReference(
ctx: FindVariableContext,
pattern: TSESTree.DestructuringPattern
): boolean {
for (const id of iterateIdentifiers(pattern)) {
const variable = findVariable(context, id);
if (variable && hasWrite(variable)) return true;
const variable = ctx.findVariable(id);
if (variable && hasWrite(ctx, variable)) return true;
}

return false;
Expand Down
Loading
Loading