diff --git a/src/lib/mcp/autofixers.ts b/src/lib/mcp/autofixers.ts deleted file mode 100644 index a7e9c28..0000000 --- a/src/lib/mcp/autofixers.ts +++ /dev/null @@ -1,47 +0,0 @@ -import type { Node } from 'estree'; -import type { AST } from 'svelte-eslint-parser'; -import type { Visitors } from 'zimmerframe'; -import type { ParseResult } from '../server/analyze/parse.js'; - -export type Autofixer = Visitors< - Node | AST.SvelteNode, - { - output: { issues: string[]; suggestions: string[] }; - parsed: ParseResult; - desired_svelte_version: number; - } ->; - -export const assign_in_effect: Autofixer = { - AssignmentExpression(node, { path, state }) { - const in_effect = path.findLast( - (node) => - node.type === 'CallExpression' && - node.callee.type === 'Identifier' && - node.callee.name === '$effect', - ); - if ( - in_effect && - in_effect.type === 'CallExpression' && - (in_effect.callee.type === 'Identifier' || in_effect.callee.type === 'MemberExpression') - ) { - if (state.parsed.is_rune(in_effect, ['$effect', '$effect.pre'])) { - if (node.left.type === 'Identifier') { - const reference = state.parsed.find_reference_by_id(node.left); - const definition = reference?.resolved?.defs[0]; - if (definition && definition.type === 'Variable') { - const init = definition.node.init; - if ( - init?.type === 'CallExpression' && - state.parsed.is_rune(init, ['$state', '$state.raw']) - ) { - state.output.suggestions.push( - `The stateful variable "${node.left.name}" is assigned inside an $effect which is generally consider a malpractice. Consider using $derived if possible.`, - ); - } - } - } - } - } - }, -}; diff --git a/src/lib/mcp/autofixers/add-autofixers-issues.test.ts b/src/lib/mcp/autofixers/add-autofixers-issues.test.ts new file mode 100644 index 0000000..ac8c826 --- /dev/null +++ b/src/lib/mcp/autofixers/add-autofixers-issues.test.ts @@ -0,0 +1,113 @@ +import { describe, expect, it } from 'vitest'; +import { add_autofixers_issues } from './add-autofixers-issues'; + +describe('add_autofixers_issues', () => { + describe('assign_in_effect', () => { + it('should add suggestions when assigning to a stateful variable inside an effect', () => { + const content = { issues: [], suggestions: [] }; + const code = ` + `; + add_autofixers_issues(content, code, 5); + + expect(content.suggestions.length).toBeGreaterThanOrEqual(1); + expect(content.suggestions).toContain( + 'The stateful variable "count" is assigned inside an $effect which is generally consider a malpractice. Consider using $derived if possible.', + ); + }); + + it('should add a suggestion for each variable assigned within an effect', () => { + const content = { issues: [], suggestions: [] }; + const code = ` + `; + add_autofixers_issues(content, code, 5); + + expect(content.suggestions.length).toBeGreaterThanOrEqual(2); + expect(content.suggestions).toContain( + 'The stateful variable "count" is assigned inside an $effect which is generally consider a malpractice. Consider using $derived if possible.', + ); + expect(content.suggestions).toContain( + 'The stateful variable "count2" is assigned inside an $effect which is generally consider a malpractice. Consider using $derived if possible.', + ); + }); + it('should not add a suggestion for variables that are not assigned within an effect', () => { + const content = { issues: [], suggestions: [] }; + const code = ` + + + + `; + add_autofixers_issues(content, code, 5); + + expect(content.suggestions).not.toContain( + 'The stateful variable "count" is assigned inside an $effect which is generally consider a malpractice. Consider using $derived if possible.', + ); + }); + + it("should not add a suggestions for variables that are assigned within an effect but aren't stateful", () => { + const content = { issues: [], suggestions: [] }; + const code = ` + `; + add_autofixers_issues(content, code, 5); + + expect(content.suggestions).not.toContain( + 'The stateful variable "count" is assigned inside an $effect which is generally consider a malpractice. Consider using $derived if possible.', + ); + }); + + it('should add a suggestion for variables that are assigned within an effect with an update', () => { + const content = { issues: [], suggestions: [] }; + const code = ` + + `; + add_autofixers_issues(content, code, 5); + + expect(content.suggestions).toContain( + 'The stateful variable "count" is assigned inside an $effect which is generally consider a malpractice. Consider using $derived if possible.', + ); + }); + + it('should add a suggestion for variables that are mutated within an effect', () => { + const content = { issues: [], suggestions: [] }; + const code = ` + + `; + add_autofixers_issues(content, code, 5); + + expect(content.suggestions).toContain( + 'The stateful variable "count" is assigned inside an $effect which is generally consider a malpractice. Consider using $derived if possible.', + ); + }); + }); +}); diff --git a/src/lib/mcp/autofixers/add-autofixers-issues.ts b/src/lib/mcp/autofixers/add-autofixers-issues.ts new file mode 100644 index 0000000..3e21d59 --- /dev/null +++ b/src/lib/mcp/autofixers/add-autofixers-issues.ts @@ -0,0 +1,22 @@ +import { parse } from '../../parse/parse.js'; +import { walk } from '../../index.js'; +import type { Node } from 'estree'; +import * as autofixers from './visitors/index.js'; + +export function add_autofixers_issues( + content: { issues: string[]; suggestions: string[] }, + code: string, + desired_svelte_version: number, + filename = 'Component.svelte', +) { + const parsed = parse(code, filename); + + // Run each autofixer separately to avoid interrupting logic flow + for (const autofixer of Object.values(autofixers)) { + walk( + parsed.ast as unknown as Node, + { output: content, parsed, desired_svelte_version }, + autofixer, + ); + } +} diff --git a/src/lib/mcp/autofixers/add-compile-issues.ts b/src/lib/mcp/autofixers/add-compile-issues.ts new file mode 100644 index 0000000..2eb9efb --- /dev/null +++ b/src/lib/mcp/autofixers/add-compile-issues.ts @@ -0,0 +1,20 @@ +import { compile } from 'svelte/compiler'; + +export function add_compile_issues( + content: { issues: string[]; suggestions: string[] }, + code: string, + desired_svelte_version: number, + filename = 'Component.svelte', +) { + const compilation_result = compile(code, { + filename: filename || 'Component.svelte', + generate: false, + runes: desired_svelte_version >= 5, + }); + + for (const warning of compilation_result.warnings) { + content.issues.push( + `${warning.message} at line ${warning.start?.line}, column ${warning.start?.column}`, + ); + } +} diff --git a/src/lib/mcp/eslint.ts b/src/lib/mcp/autofixers/add-eslint-issues.ts similarity index 72% rename from src/lib/mcp/eslint.ts rename to src/lib/mcp/autofixers/add-eslint-issues.ts index 127ce3b..af9d2e4 100644 --- a/src/lib/mcp/eslint.ts +++ b/src/lib/mcp/autofixers/add-eslint-issues.ts @@ -48,7 +48,7 @@ function base_config(svelte_config: Config): ESLint.Options['baseConfig'] { ]; } -export function get_linter(version: number) { +function get_linter(version: number) { if (version < 5) { return (svelte_4_linter ??= new ESLint({ overrideConfigFile: true, @@ -68,3 +68,23 @@ export function get_linter(version: number) { }), })); } + +export async function add_eslint_issues( + content: { issues: string[]; suggestions: string[] }, + code: string, + desired_svelte_version: number, + filename = 'Component.svelte', +) { + const eslint = get_linter(desired_svelte_version); + const results = await eslint.lintText(code, { filePath: filename || './Component.svelte' }); + + for (const message of results[0].messages) { + if (message.severity === 2) { + content.issues.push(`${message.message} at line ${message.line}, column ${message.column}`); + } else if (message.severity === 1) { + content.suggestions.push( + `${message.message} at line ${message.line}, column ${message.column}`, + ); + } + } +} diff --git a/src/lib/mcp/autofixers/ast/utils.ts b/src/lib/mcp/autofixers/ast/utils.ts new file mode 100644 index 0000000..13ec632 --- /dev/null +++ b/src/lib/mcp/autofixers/ast/utils.ts @@ -0,0 +1,16 @@ +import type { Identifier, MemberExpression } from 'estree'; + +/** + * Gets the left-most identifier of a member expression or identifier. + */ +export function left_most_id(expression: MemberExpression | Identifier) { + while (expression.type === 'MemberExpression') { + expression = expression.object as MemberExpression | Identifier; + } + + if (expression.type !== 'Identifier') { + return null; + } + + return expression; +} diff --git a/src/lib/mcp/autofixers/visitors/assign-in-effect.ts b/src/lib/mcp/autofixers/visitors/assign-in-effect.ts new file mode 100644 index 0000000..67f41e7 --- /dev/null +++ b/src/lib/mcp/autofixers/visitors/assign-in-effect.ts @@ -0,0 +1,62 @@ +import type { AssignmentExpression, Identifier, Node, UpdateExpression } from 'estree'; +import type { Autofixer, AutofixerState } from '.'; +import { left_most_id } from '../ast/utils'; +import type { SvelteNode } from 'svelte-eslint-parser/lib/ast'; +import type { Context } from 'zimmerframe'; + +function run_if_in_effect(path: (Node | SvelteNode)[], state: AutofixerState, to_run: () => void) { + const in_effect = path.findLast( + (node) => + node.type === 'CallExpression' && + node.callee.type === 'Identifier' && + node.callee.name === '$effect', + ); + + if ( + in_effect && + in_effect.type === 'CallExpression' && + (in_effect.callee.type === 'Identifier' || in_effect.callee.type === 'MemberExpression') + ) { + if (state.parsed.is_rune(in_effect, ['$effect', '$effect.pre'])) { + to_run(); + } + } +} + +function visitor( + node: UpdateExpression | AssignmentExpression, + { state, path }: Context, +) { + run_if_in_effect(path, state, () => { + function check_if_stateful_id(id: Identifier) { + const reference = state.parsed.find_reference_by_id(id); + const definition = reference?.resolved?.defs[0]; + if (definition && definition.type === 'Variable') { + const init = definition.node.init; + if ( + init?.type === 'CallExpression' && + state.parsed.is_rune(init, ['$state', '$state.raw']) + ) { + state.output.suggestions.push( + `The stateful variable "${id.name}" is assigned inside an $effect which is generally consider a malpractice. Consider using $derived if possible.`, + ); + } + } + } + const variable = node.type === 'UpdateExpression' ? node.argument : node.left; + + if (variable.type === 'Identifier') { + check_if_stateful_id(variable); + } else if (variable.type === 'MemberExpression') { + const object = left_most_id(variable); + if (object) { + check_if_stateful_id(object); + } + } + }); +} + +export const assign_in_effect: Autofixer = { + UpdateExpression: visitor, + AssignmentExpression: visitor, +}; diff --git a/src/lib/mcp/autofixers/visitors/index.ts b/src/lib/mcp/autofixers/visitors/index.ts new file mode 100644 index 0000000..065f46f --- /dev/null +++ b/src/lib/mcp/autofixers/visitors/index.ts @@ -0,0 +1,14 @@ +import type { Node } from 'estree'; +import type { AST } from 'svelte-eslint-parser'; +import type { Visitors } from 'zimmerframe'; +import type { ParseResult } from '../../../parse/parse.js'; + +export type AutofixerState = { + output: { issues: string[]; suggestions: string[] }; + parsed: ParseResult; + desired_svelte_version: number; +}; + +export type Autofixer = Visitors; + +export * from './assign-in-effect.js'; diff --git a/src/lib/mcp/index.ts b/src/lib/mcp/index.ts index 65671fa..519726b 100644 --- a/src/lib/mcp/index.ts +++ b/src/lib/mcp/index.ts @@ -1,14 +1,11 @@ -import { walk } from '../index.js'; import { ValibotJsonSchemaAdapter } from '@tmcp/adapter-valibot'; import { HttpTransport } from '@tmcp/transport-http'; import { StdioTransport } from '@tmcp/transport-stdio'; -import type { Node } from 'estree'; import { McpServer } from 'tmcp'; import * as v from 'valibot'; -import { parse } from '../server/analyze/parse.js'; -import * as autofixers from './autofixers.js'; -import { get_linter } from './eslint.js'; -import { compile } from 'svelte/compiler'; +import { add_autofixers_issues } from './autofixers/add-autofixers-issues.js'; +import { add_compile_issues } from './autofixers/add-compile-issues.js'; +import { add_eslint_issues } from './autofixers/add-eslint-issues.js'; const server = new McpServer( { @@ -64,45 +61,11 @@ server.tool( require_another_tool_call_after_fixing: boolean; } = { issues: [], suggestions: [], require_another_tool_call_after_fixing: false }; try { - // compile without generating to get warnings and errors + add_compile_issues(content, code, desired_svelte_version, filename); - const compilation_result = compile(code, { - filename: filename || 'Component.svelte', - generate: false, - runes: desired_svelte_version >= 5, - }); + add_autofixers_issues(content, code, desired_svelte_version, filename); - for (const warning of compilation_result.warnings) { - content.issues.push( - `${warning.message} at line ${warning.start?.line}, column ${warning.start?.column}`, - ); - } - - const parsed = parse(code, filename ?? 'Component.svelte'); - - // Run each autofixer separately to avoid interrupting logic flow - for (const autofixer of Object.values(autofixers)) { - walk( - parsed.ast as unknown as Node, - { output: content, parsed, desired_svelte_version }, - autofixer, - ); - } - - const eslint = get_linter(desired_svelte_version); - const results = await eslint.lintText(code, { filePath: filename || './Component.svelte' }); - - for (const message of results[0].messages) { - if (message.severity === 2) { - content.issues.push( - `${message.message} at line ${message.line}, column ${message.column}`, - ); - } else if (message.severity === 1) { - content.suggestions.push( - `${message.message} at line ${message.line}, column ${message.column}`, - ); - } - } + await add_eslint_issues(content, code, desired_svelte_version, filename); } catch (e: unknown) { const error = e as Error & { start?: { line: number; column: number } }; content.issues.push( diff --git a/src/lib/server/analyze/parse.spec.ts b/src/lib/parse/parse.spec.ts similarity index 100% rename from src/lib/server/analyze/parse.spec.ts rename to src/lib/parse/parse.spec.ts diff --git a/src/lib/server/analyze/parse.ts b/src/lib/parse/parse.ts similarity index 100% rename from src/lib/server/analyze/parse.ts rename to src/lib/parse/parse.ts