diff --git a/src/lib/constants.ts b/src/lib/constants.ts new file mode 100644 index 0000000..17b7122 --- /dev/null +++ b/src/lib/constants.ts @@ -0,0 +1,23 @@ +export const base_runes = [ + '$state', + '$effect', + '$derived', + '$inspect', + '$props', + '$bindable', + '$host', +] as const; + +export const nested_runes = [ + '$state.raw', + '$state.snapshot', + '$effect.pre', + '$effect.tracking', + '$effect.pending', + '$effect.root', + '$derived.by', + '$inspect.trace', + '$props.id', +] as const; + +export const runes = [...base_runes, ...nested_runes] as const; diff --git a/src/lib/mcp/autofixers/add-autofixers-issues.test.ts b/src/lib/mcp/autofixers/add-autofixers-issues.test.ts index ac8c826..fa8e56a 100644 --- a/src/lib/mcp/autofixers/add-autofixers-issues.test.ts +++ b/src/lib/mcp/autofixers/add-autofixers-issues.test.ts @@ -1,18 +1,25 @@ import { describe, expect, it } from 'vitest'; -import { add_autofixers_issues } from './add-autofixers-issues'; +import { add_autofixers_issues } from './add-autofixers-issues.js'; +import { base_runes } from '../../constants.js'; + +const dollarless_runes = base_runes.map((r) => ({ rune: r.replace('$', '') })); + +function run_autofixers_on_code(code: string, desired_svelte_version = 5) { + const content = { issues: [], suggestions: [] }; + add_autofixers_issues(content, code, desired_svelte_version); + return content; +} 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 = ` + it(`should add suggestions when assigning to a stateful variable inside an effect`, () => { + const content = run_autofixers_on_code(` `; - add_autofixers_issues(content, code, 5); + `); expect(content.suggestions.length).toBeGreaterThanOrEqual(1); expect(content.suggestions).toContain( @@ -20,9 +27,8 @@ describe('add_autofixers_issues', () => { ); }); - it('should add a suggestion for each variable assigned within an effect', () => { - const content = { issues: [], suggestions: [] }; - const code = ` + it(`should add a suggestion for each variable assigned within an effect`, () => { + const content = run_autofixers_on_code(` `; - add_autofixers_issues(content, code, 5); + `); expect(content.suggestions.length).toBeGreaterThanOrEqual(2); expect(content.suggestions).toContain( @@ -41,16 +46,14 @@ describe('add_autofixers_issues', () => { '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 = ` + it(`should not add a suggestion for variables that are not assigned within an effect`, () => { + const content = run_autofixers_on_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.', @@ -58,25 +61,22 @@ describe('add_autofixers_issues', () => { }); it("should not add a suggestions for variables that are assigned within an effect but aren't stateful", () => { - const content = { issues: [], suggestions: [] }; - const code = ` + const content = run_autofixers_on_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 = ` + it(`should add a suggestion for variables that are assigned within an effect with an update`, () => { + const content = run_autofixers_on_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 = ` + it(`should add a suggestion for variables that are mutated within an effect`, () => { + const content = run_autofixers_on_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.', ); }); }); + + describe.each([{ method: 'set' }, { method: 'update' }])( + 'set_or_update_state ($method)', + ({ method }) => { + it(`should add suggestions when using .${method}() on a stateful variable with a literal init`, () => { + const content = run_autofixers_on_code(` + `); + + expect(content.suggestions.length).toBeGreaterThanOrEqual(1); + expect(content.suggestions).toContain( + `You are trying to update the stateful variable "count" using "${method}". stateful variables should be updated with a normal assignment/mutation, do not use methods to update them.`, + ); + }); + + it(`should add suggestions when using .${method}() on a stateful variable with an array init`, () => { + const content = run_autofixers_on_code(` + `); + + expect(content.suggestions.length).toBeGreaterThanOrEqual(1); + expect(content.suggestions).toContain( + `You are trying to update the stateful variable "count" using "${method}". stateful variables should be updated with a normal assignment/mutation, do not use methods to update them.`, + ); + }); + + it(`should add suggestions when using .${method}() on a stateful variable with conditional if it's not sure if the method could actually be present on the variable ($state({}))`, () => { + const content = run_autofixers_on_code(` + `); + + expect(content.suggestions.length).toBeGreaterThanOrEqual(1); + expect(content.suggestions).toContain( + `You are trying to update the stateful variable "count" using "${method}". stateful variables should be updated with a normal assignment/mutation, do not use methods to update them. However I can't verify if "count" is a state variable of an object or a class with a "${method}" method on it. Please verify that before updating the code to use a normal assignment`, + ); + }); + + it(`should add suggestions when using .${method}() on a stateful variable with conditional if it's not sure if the method could actually be present on the variable ($state(new Class()))`, () => { + const content = run_autofixers_on_code(` + `); + + expect(content.suggestions.length).toBeGreaterThanOrEqual(1); + expect(content.suggestions).toContain( + `You are trying to update the stateful variable "count" using "${method}". stateful variables should be updated with a normal assignment/mutation, do not use methods to update them. However I can't verify if "count" is a state variable of an object or a class with a "${method}" method on it. Please verify that before updating the code to use a normal assignment`, + ); + }); + + it(`should add suggestions when using .${method}() on a stateful variable with conditional if it's not sure if the method could actually be present on the variable ($state(variable_name))`, () => { + const content = run_autofixers_on_code(` + `); + + expect(content.suggestions.length).toBeGreaterThanOrEqual(1); + expect(content.suggestions).toContain( + `You are trying to update the stateful variable "count" using "${method}". stateful variables should be updated with a normal assignment/mutation, do not use methods to update them. However I can't verify if "count" is a state variable of an object or a class with a "${method}" method on it. Please verify that before updating the code to use a normal assignment`, + ); + }); + + it(`should not add suggestions when using .${method} on a stateful variable if it's not a method call`, () => { + const content = run_autofixers_on_code(` + `); + + expect(content.suggestions).not.toContain( + `You are trying to update the stateful variable "count" using "${method}". stateful variables should be updated with a normal assignment/mutation, do not use methods to update them. However I can't verify if "count" is a state variable of an object or a class with a "${method}" method on it. Please verify that before updating the code to use a normal assignment`, + ); + }); + }, + ); + + describe('imported_runes', () => { + describe.each([{ source: 'svelte' }, { source: 'svelte/runes' }])( + 'from "$source"', + ({ source }) => { + describe.each(dollarless_runes)('single import ($rune)', ({ rune }) => { + it(`should add suggestions when importing '${rune}' from '${source}'`, () => { + const content = run_autofixers_on_code(` + `); + + expect(content.suggestions.length).toBeGreaterThanOrEqual(1); + expect(content.suggestions).toContain( + `You are importing "${rune}" from "${source}". This is not necessary, all runes are globally available. Please remove this import and use "$${rune}" directly.`, + ); + }); + + it(`should add suggestions when importing "${rune}" as the default export from '${source}'`, () => { + const content = run_autofixers_on_code(` + `); + + expect(content.suggestions.length).toBeGreaterThanOrEqual(1); + expect(content.suggestions).toContain( + `You are importing "${rune}" from "${source}". This is not necessary, all runes are globally available. Please remove this import and use "$${rune}" directly.`, + ); + }); + + it(`should add suggestions when importing '${rune}' as the namespace export from '${source}'`, () => { + const content = run_autofixers_on_code(` + `); + + expect(content.suggestions.length).toBeGreaterThanOrEqual(1); + expect(content.suggestions).toContain( + `You are importing "${rune}" from "${source}". This is not necessary, all runes are globally available. Please remove this import and use "$${rune}" directly.`, + ); + }); + }); + + it(`should add suggestions when importing multiple runes from '${source}'`, () => { + const content = run_autofixers_on_code(` + `); + + expect(content.suggestions.length).toBeGreaterThanOrEqual(2); + expect(content.suggestions).toContain( + `You are importing "state" from "${source}". This is not necessary, all runes are globally available. Please remove this import and use "$state" directly.`, + ); + expect(content.suggestions).toContain( + `You are importing "effect" from "${source}". This is not necessary, all runes are globally available. Please remove this import and use "$effect" directly.`, + ); + }); + + it(`should not add suggestions when importing other identifiers from '${source}'`, () => { + const content = run_autofixers_on_code(` + `); + + expect(content.suggestions).not.toContain( + `You are importing "onMount" from "${source}". This is not necessary, all runes are globally available. Please remove this import and use "$onMount" directly.`, + ); + }); + }, + ); + }); + + describe('derived_with_function', () => { + it(`should add suggestions when using a function as the first argument to $derived`, () => { + const content = run_autofixers_on_code(` + `); + + expect(content.suggestions.length).toBeGreaterThanOrEqual(1); + expect(content.suggestions).toContain( + 'You are passing a function to $derived when declaring "value" but $derived expects an expression. You can use $derived.by instead.', + ); + }); + + it(`should add suggestions when using a function as the first argument to $derived in classes`, () => { + const content = run_autofixers_on_code(` + `); + + expect(content.suggestions.length).toBeGreaterThanOrEqual(1); + expect(content.suggestions).toContain( + 'You are passing a function to $derived when declaring "value" but $derived expects an expression. You can use $derived.by instead.', + ); + }); + + it(`should add suggestions when using a function as the first argument to $derived in classes constructors`, () => { + const content = run_autofixers_on_code(` + `); + + expect(content.suggestions.length).toBeGreaterThanOrEqual(1); + expect(content.suggestions).toContain( + 'You are passing a function to $derived when declaring "value" but $derived expects an expression. You can use $derived.by instead.', + ); + }); + + it(`should add suggestions when using a function as the first argument to $derived without the declaring part if it's not an identifier`, () => { + const content = run_autofixers_on_code(` + `); + + expect(content.suggestions.length).toBeGreaterThanOrEqual(1); + expect(content.suggestions).toContain( + 'You are passing a function to $derived but $derived expects an expression. You can use $derived.by instead.', + ); + }); + + it(`should add suggestions when using a function as the first argument to $derived.by`, () => { + const content = run_autofixers_on_code(` + `); + + expect(content.suggestions).not.toContain( + 'You are passing a function to $derived but $derived expects an expression. You can use $derived.by instead.', + ); + }); + }); }); diff --git a/src/lib/mcp/autofixers/visitors/assign-in-effect.ts b/src/lib/mcp/autofixers/visitors/assign-in-effect.ts index 67f41e7..ffe1b16 100644 --- a/src/lib/mcp/autofixers/visitors/assign-in-effect.ts +++ b/src/lib/mcp/autofixers/visitors/assign-in-effect.ts @@ -1,6 +1,6 @@ import type { AssignmentExpression, Identifier, Node, UpdateExpression } from 'estree'; import type { Autofixer, AutofixerState } from '.'; -import { left_most_id } from '../ast/utils'; +import { left_most_id } from '../ast/utils.js'; import type { SvelteNode } from 'svelte-eslint-parser/lib/ast'; import type { Context } from 'zimmerframe'; diff --git a/src/lib/mcp/autofixers/visitors/derived-with-function.ts b/src/lib/mcp/autofixers/visitors/derived-with-function.ts new file mode 100644 index 0000000..46c4f6f --- /dev/null +++ b/src/lib/mcp/autofixers/visitors/derived-with-function.ts @@ -0,0 +1,41 @@ +import type { Identifier, PrivateIdentifier } from 'estree'; +import type { Autofixer } from '.'; + +export const derived_with_function: Autofixer = { + CallExpression(node, { state, path }) { + if ( + node.callee.type === 'Identifier' && + node.callee.name === '$derived' && + state.parsed.is_rune(node, ['$derived']) && + (node.arguments[0].type === 'ArrowFunctionExpression' || + node.arguments[0].type === 'FunctionExpression') + ) { + const parent = path[path.length - 1]; + let variable_id: Identifier | PrivateIdentifier | undefined; + if (parent.type === 'VariableDeclarator' && parent.id.type === 'Identifier') { + // const something = $derived(...) + variable_id = parent.id; + } else if (parent.type === 'PropertyDefinition') { + // class X { something = $derived(...) } + variable_id = + parent.key.type === 'Identifier' + ? parent.key + : parent.key.type === 'PrivateIdentifier' + ? parent.key + : undefined; + } else if (parent.type === 'AssignmentExpression') { + // this.something = $derived(...) + variable_id = + parent.left.type === 'MemberExpression' + ? parent.left.property.type === 'Identifier' + ? parent.left.property + : undefined + : undefined; + } + + state.output.suggestions.push( + `You are passing a function to $derived ${variable_id ? `when declaring "${variable_id.name}" ` : ''}but $derived expects an expression. You can use $derived.by instead.`, + ); + } + }, +}; diff --git a/src/lib/mcp/autofixers/visitors/imported-runes.ts b/src/lib/mcp/autofixers/visitors/imported-runes.ts new file mode 100644 index 0000000..10b62f4 --- /dev/null +++ b/src/lib/mcp/autofixers/visitors/imported-runes.ts @@ -0,0 +1,28 @@ +import { base_runes } from '../../../constants.js'; +import type { Autofixer } from '.'; + +const dollarless_runes = base_runes.map((r) => r.replace('$', '')); + +export const imported_runes: Autofixer = { + ImportDeclaration(node, { state, next }) { + const source = (node.source.value || node.source.raw?.slice(1, -1))?.toString(); + if (source && source.startsWith('svelte')) { + for (const specifier of node.specifiers) { + const id = + specifier.type === 'ImportDefaultSpecifier' + ? specifier.local + : specifier.type === 'ImportNamespaceSpecifier' + ? specifier.local + : specifier.type === 'ImportSpecifier' + ? specifier.imported + : null; + if (id && id.type === 'Identifier' && dollarless_runes.includes(id.name)) { + state.output.suggestions.push( + `You are importing "${id.name}" from "${source}". This is not necessary, all runes are globally available. Please remove this import and use "$${id.name}" directly.`, + ); + } + } + } + next(); + }, +}; diff --git a/src/lib/mcp/autofixers/visitors/index.ts b/src/lib/mcp/autofixers/visitors/index.ts index 065f46f..5f2e3e2 100644 --- a/src/lib/mcp/autofixers/visitors/index.ts +++ b/src/lib/mcp/autofixers/visitors/index.ts @@ -12,3 +12,6 @@ export type AutofixerState = { export type Autofixer = Visitors; export * from './assign-in-effect.js'; +export * from './set-or-update-state.js'; +export * from './imported-runes.js'; +export * from './derived-with-function.js'; diff --git a/src/lib/mcp/autofixers/visitors/set-or-update-state.ts b/src/lib/mcp/autofixers/visitors/set-or-update-state.ts new file mode 100644 index 0000000..2ec074f --- /dev/null +++ b/src/lib/mcp/autofixers/visitors/set-or-update-state.ts @@ -0,0 +1,37 @@ +import type { Autofixer } from '.'; +import { left_most_id } from '../ast/utils.js'; + +const UPDATE_PROPERTIES = ['set', 'update']; + +export const set_or_update_state: Autofixer = { + MemberExpression(node, { state, next, path }) { + const parent = path[path.length - 1]; + if ( + parent.type === 'CallExpression' && + parent.callee === node && + node.property.type === 'Identifier' && + UPDATE_PROPERTIES.includes(node.property.name) + ) { + const id = left_most_id(node); + if (id) { + 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']) + ) { + let suggestion = `You are trying to update the stateful variable "${id.name}" using "${node.property.name}". stateful variables should be updated with a normal assignment/mutation, do not use methods to update them.`; + const argument = init.arguments[0]; + if (!argument || (argument.type !== 'Literal' && argument.type !== 'ArrayExpression')) { + suggestion += ` However I can't verify if "${id.name}" is a state variable of an object or a class with a "${node.property.name}" method on it. Please verify that before updating the code to use a normal assignment`; + } + state.output.suggestions.push(suggestion); + } + } + } + } + next(); + }, +}; diff --git a/src/lib/parse/parse.ts b/src/lib/parse/parse.ts index d3c4854..53fb9e1 100644 --- a/src/lib/parse/parse.ts +++ b/src/lib/parse/parse.ts @@ -2,6 +2,7 @@ import ts_parser from '@typescript-eslint/parser'; import type { CallExpression, Identifier } from 'estree'; import type { Reference, Variable } from 'eslint-scope'; import { parseForESLint as svelte_eslint_parse } from 'svelte-eslint-parser'; +import { runes } from '../constants.js'; type Scope = { variables?: Variable[]; @@ -18,25 +19,6 @@ function collect_scopes(scope: Scope, acc: Scope[] = []) { return acc; } -const runes = [ - '$state', - '$state.raw', - '$state.snapshot', - '$effect', - '$effect.pre', - '$effect.tracking', - '$effect.pending', - '$effect.root', - '$derived', - '$derived.by', - '$inspect', - '$inspect.trace', - '$props', - '$props.id', - '$bindable', - '$host', -] as const; - export type ParseResult = ReturnType; export function parse(code: string, file_path: string) {