From 7c8498bc2ad7604aff4580f51f6ed87fbe025e81 Mon Sep 17 00:00:00 2001 From: loki5512344 Date: Tue, 15 Sep 2026 17:07:46 +0200 Subject: [PATCH] =?UTF-8?q?feat(ts):=20ts/unused-import=20rule=20=E2=80=94?= =?UTF-8?q?=20whole-statement=20delete=20of=20dead=20TS/JS=20imports?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - binds names via tree-sitter (default, namespace, named + alias, type-only), keeps a statement when ANY bound name occurs outside it (word scan over comments/strings included): under-delete is the safe direction, mirroring the Java rule's asymmetry - ESM caveat encoded in the policy: mixed live/dead statements are not touched — removing a specifier would still drop the module's side effects - side-effect imports and 'export .. from' re-exports are never candidates - parser/ts became a folder (mod/extract/unused_imports) to keep the 4-entries-per-module rule; word_occurs shared via crate::parser - java/rules/unused_imports now reuses the shared word scanner (DRY) --- README.md | 4 +- docs/SKILL.md | 6 +- src/parser/java/rules/unused_imports.rs | 27 +-- src/parser/mod.rs | 28 ++- src/parser/{ts_extract.rs => ts/extract.rs} | 4 +- src/parser/{ts.rs => ts/mod.rs} | 9 +- src/parser/ts/unused_imports.rs | 250 ++++++++++++++++++++ tests/cli_fix.rs | 24 ++ tests/typescript/unused/package.json | 1 + tests/typescript/unused/src/app.ts | 10 + tests/typescript/unused/src/lib.ts | 6 + tests/typescript/unused/src/logger.ts | 3 + tests/typescript/unused/src/side-effects.ts | 0 tests/typescript/unused/src/types.ts | 0 todo.md | 6 +- 15 files changed, 340 insertions(+), 38 deletions(-) rename src/parser/{ts_extract.rs => ts/extract.rs} (96%) rename src/parser/{ts.rs => ts/mod.rs} (96%) create mode 100644 src/parser/ts/unused_imports.rs create mode 100644 tests/typescript/unused/package.json create mode 100644 tests/typescript/unused/src/app.ts create mode 100644 tests/typescript/unused/src/lib.ts create mode 100644 tests/typescript/unused/src/logger.ts create mode 100644 tests/typescript/unused/src/side-effects.ts create mode 100644 tests/typescript/unused/src/types.ts diff --git a/README.md b/README.md index 113f034..3c733a9 100644 --- a/README.md +++ b/README.md @@ -80,8 +80,8 @@ See [docs/EXAMPLES.md](docs/EXAMPLES.md) for more, and TypeScript/JavaScript and Java (the open niche) are in — real-world tested on `google/guava`. `fix` auto-repairs small breakages on the same -dry-run/atomic engine (unused, missing and misordered Java imports; -TS rules next), then Python, Go. `split` (automatic file decomposition) is +dry-run/atomic engine (Java: unused, missing and +misordered imports; TS: unused imports), then Python, Go. `split` (automatic file decomposition) is planned — no tool does it. Full plan: [docs/PLAN.md](docs/PLAN.md). ## License diff --git a/docs/SKILL.md b/docs/SKILL.md index f3e0cf5..c2b61b4 100644 --- a/docs/SKILL.md +++ b/docs/SKILL.md @@ -53,7 +53,11 @@ exit codes). Current Java rules: `java/unused-import` (deletes single-type imports whose name is provably unreferenced), `java/missing-import` (inserts the import of a project class used by simple name — unique FQN candidate required) and `java/import-order` (Google style: statics first, -then single-type, ASCII-sorted, duplicates dropped). Unknown `--rule` +then single-type, ASCII-sorted, duplicates dropped). TS/JS: +`ts/unused-import` deletes whole statements whose every bound name is +unreferenced; mixed statements (one name live) stay untouched because ESM +imports carry module side effects, so partial specifier surgery is +deliberately off. Unknown `--rule` fails with `INVALID_ARGUMENT` and lists the known ids. A candidate the engine cannot prove safe is reported with `"applied": false` and a `candidates` array of FQN options — resolve it yourself (pick one, add diff --git a/src/parser/java/rules/unused_imports.rs b/src/parser/java/rules/unused_imports.rs index 63e412f..c76c34f 100644 --- a/src/parser/java/rules/unused_imports.rs +++ b/src/parser/java/rules/unused_imports.rs @@ -13,7 +13,6 @@ //! column zero, and anything else is the generator's problem, not the //! engine's (see [`crate::core::Edit`]). -use std::ops::Range; use std::path::Path; use tree_sitter::Node; @@ -22,7 +21,7 @@ use super::is_static; use crate::core::Edit; use crate::core::index::Index; use crate::parser::java::{TreeSitterJava, find_child_kind, has_child_kind, line_end, text}; -use crate::parser::{Fix, FixCandidate, Severity}; +use crate::parser::{Fix, FixCandidate, Severity, word_occurs}; /// Rule id accepted by `jmove fix --rule`. pub const RULE: &str = "java/unused-import"; @@ -100,30 +99,6 @@ fn unused_import(node: Node, source: &str) -> Option { }) } -// `word` as a standalone Java identifier token outside `skip`. Matches -// overlapping the import statement itself never count as usage. Bytes -// >= 0x80 count as identifier parts: treating a possibly-mojibake -// neighbour as "part of a bigger word" can only keep an import, never -// drop one. -fn word_occurs(source: &[u8], word: &[u8], skip: &Range) -> bool { - if word.is_empty() { - return false; - } - source.windows(word.len()).enumerate().any(|(at, found)| { - let end = at + word.len(); - if at < skip.end && end > skip.start { - return false; - } - let before = at == 0 || !is_ident(source[at - 1]); - let after = end == source.len() || !is_ident(source[end]); - *found == *word && before && after - }) -} - -fn is_ident(byte: u8) -> bool { - byte.is_ascii_alphanumeric() || matches!(byte, b'_' | b'$') || byte >= 0x80 -} - #[cfg(test)] mod tests { use super::{JavaUnusedImports, RULE}; diff --git a/src/parser/mod.rs b/src/parser/mod.rs index 5c8e44c..385c375 100644 --- a/src/parser/mod.rs +++ b/src/parser/mod.rs @@ -188,6 +188,29 @@ pub trait Fix: Send + Sync { fn fixes(&self, path: &Path, source: &str, index: &Index) -> Vec; } +/// `word` as a standalone identifier token outside `skip`. Shared by the +/// deletion-safety scans of every `unused-import` rule. Bytes >= 0x80 +/// count as identifier parts: treating a possibly-mojibake neighbour as +/// "part of a bigger word" can only keep an import, never drop one. +pub(crate) fn word_occurs(source: &[u8], word: &[u8], skip: &Range) -> bool { + if word.is_empty() { + return false; + } + source.windows(word.len()).enumerate().any(|(at, found)| { + let end = at + word.len(); + if at < skip.end && end > skip.start { + return false; + } + let before = at == 0 || !is_ident_byte(source[at - 1]); + let after = end == source.len() || !is_ident_byte(source[end]); + *found == *word && before && after + }) +} + +fn is_ident_byte(byte: u8) -> bool { + byte.is_ascii_alphanumeric() || matches!(byte, b'_' | b'$') || byte >= 0x80 +} + /// Default rule set for `lang` (empty for languages without rules yet). #[must_use] pub fn fixers_for(lang: SourceLanguage) -> Vec> { @@ -197,7 +220,9 @@ pub fn fixers_for(lang: SourceLanguage) -> Vec> { Box::new(java::rules::missing_imports::JavaMissingImports::new()), Box::new(java::rules::import_order::JavaImportOrder::new()), ], - SourceLanguage::TypeScript | SourceLanguage::Tsx | SourceLanguage::JavaScript => Vec::new(), + SourceLanguage::TypeScript | SourceLanguage::Tsx | SourceLanguage::JavaScript => { + vec![Box::new(ts::unused_imports::TsUnusedImports::new())] + } } } @@ -208,5 +233,6 @@ pub fn rule_ids() -> &'static [&'static str] { java::rules::unused_imports::RULE, java::rules::missing_imports::RULE, java::rules::import_order::RULE, + ts::unused_imports::RULE, ] } diff --git a/src/parser/ts_extract.rs b/src/parser/ts/extract.rs similarity index 96% rename from src/parser/ts_extract.rs rename to src/parser/ts/extract.rs index a5f4eb4..8f90933 100644 --- a/src/parser/ts_extract.rs +++ b/src/parser/ts/extract.rs @@ -1,4 +1,4 @@ -//! Tree-sitter traversal backing [`crate::parser::ts`]. +//! Tree-sitter traversal backing [`crate::parser::ts`] and its rules. //! //! Finds every module reference: `import`/`export … from` statements (the //! grammar exposes the specifier via the `source` field), `require("…")` @@ -63,7 +63,7 @@ fn visit(node: Node, source: &str, out: &mut Vec) { /// plain JavaScript; only JSX needs the TSX variant. Java never reaches /// this frontend ([`crate::parser::frontend_for`] routes it to /// [`crate::parser::java`]), so everything else maps to plain TypeScript. -fn grammar(lang: SourceLanguage) -> Language { +pub(super) fn grammar(lang: SourceLanguage) -> Language { match lang { SourceLanguage::Tsx => tree_sitter_typescript::LANGUAGE_TSX.into(), _ => tree_sitter_typescript::LANGUAGE_TYPESCRIPT.into(), diff --git a/src/parser/ts.rs b/src/parser/ts/mod.rs similarity index 96% rename from src/parser/ts.rs rename to src/parser/ts/mod.rs index b1be26b..06728ed 100644 --- a/src/parser/ts.rs +++ b/src/parser/ts/mod.rs @@ -1,10 +1,9 @@ //! Tree-sitter based frontend for TypeScript/JavaScript. //! -//! CONTRACT: see [`crate::parser`]. Traversal lives in [`ts_extract`]. +//! CONTRACT: see [`crate::parser`]. Traversal lives in [`extract`]. -// Inline module so the folder stays at `mod.rs` + 3 files. -#[path = "ts_extract.rs"] -mod ts_extract; +mod extract; +pub mod unused_imports; use super::{ImportRecord, Language, SourceLanguage}; @@ -27,7 +26,7 @@ impl Language for TreeSitterTs { } fn extract_imports(&self, source: &str) -> Vec { - ts_extract::extract(self.lang, source) + extract::extract(self.lang, source) } } diff --git a/src/parser/ts/unused_imports.rs b/src/parser/ts/unused_imports.rs new file mode 100644 index 0000000..7747e26 --- /dev/null +++ b/src/parser/ts/unused_imports.rs @@ -0,0 +1,250 @@ +//! TS/JS rule: drop imported bindings that are never referenced. +//! +//! Safety is the same asymmetry as [`crate::parser::java::rules::unused_imports`]: +//! a value is kept when its name occurs as a standalone identifier anywhere +//! outside its own `import` statement (comments and strings included), so the +//! rule can only *under*-delete. That conservatism is load-bearing: a JSX +//! component, a decorator, a `typeof x` or an object-literal shorthand all +//! count as uses, and a bare `foo` mention in a comment keeps an otherwise +//! dead import alive — a false "used" is harmless, a false "unused" deletes +//! live code. +//! +//! Two TS-specific facts the whole-statement Java approach cannot carry over: +//! - a statement can bind several names (`import D, { A, B as C }`), so the +//! edit removes individual *specifiers*, and only the whole statement when +//! its last binding (default or namespace) goes; +//! - `import "./side-effect"` has no binding to be "unused" — the statement +//! exists for its effects and is never a candidate, exactly like a re-export +//! `export { A } from "./a"` (whose `A` is re-exposed, not used locally). + +use std::path::Path; + +use tree_sitter::{Node, Parser}; + +use crate::core::Edit; +use crate::core::index::Index; +use crate::parser::java::line_end; +use crate::parser::{Fix, FixCandidate, Severity, SourceLanguage, word_occurs}; + +use super::extract::grammar; + +/// Rule id accepted by `jmove fix --rule`. +pub const RULE: &str = "ts/unused-import"; + +/// Unused imported-binding remover for TS/JS/TSX. +pub struct TsUnusedImports; + +impl TsUnusedImports { + /// Create the rule. + #[must_use] + pub fn new() -> Self { + Self + } +} + +impl Default for TsUnusedImports { + fn default() -> Self { + Self::new() + } +} + +impl Fix for TsUnusedImports { + fn rule(&self) -> &'static str { + RULE + } + + fn fixes(&self, path: &Path, source: &str, _index: &Index) -> Vec { + let lang = SourceLanguage::for_path(path).unwrap_or(SourceLanguage::TypeScript); + let Some(tree) = parse(lang, source) else { + return Vec::new(); + }; + let bytes = source.as_bytes(); + let mut cursor = tree.root_node().walk(); + tree.root_node() + .children(&mut cursor) + .filter(|node| node.kind() == "import_statement") + .filter_map(|node| unused_bindings(node, source, bytes)) + .collect() + } +} + +fn parse(lang: SourceLanguage, source: &str) -> Option { + let mut parser = Parser::new(); + if parser.set_language(&grammar(lang)).is_err() { + return None; + } + parser.parse(source, None) +} + +// One candidate when an import binds at least one name and none of its names +// occur outside the statement itself. +fn unused_bindings(node: Node, source: &str, bytes: &[u8]) -> Option { + let mut c = node.walk(); + let clause = node + .children(&mut c) + .find(|n| n.kind() == "import_clause")?; + let names = binding_names(clause, source); + if names.is_empty() { + return None; // `import "./side"` — effects only, never unused. + } + let stmt = node.start_byte()..node.end_byte(); + if names + .iter() + .any(|n| word_occurs(bytes, n.as_bytes(), &stmt)) + { + return None; // keep the whole statement on any live binding. + } + // Every binding is dead, so the whole statement goes — including its line + // ending. Partial specifier removal (some names live) is deliberately out + // of v1 scope: safe under-delete, comma surgery is a formatter job. + let span = stmt.start..line_end(bytes, stmt.end); + let old_text = source[span.clone()].to_owned(); + Some(FixCandidate { + rule: RULE, + message: format!("unused import [{}]", names.join(", ")), + severity: Severity::Warning, + auto_fixable: true, + span: span.clone(), + edits: vec![Edit { + span, + old_text, + new_text: String::new(), + }], + candidates: Vec::new(), + }) +} + +// Every local name the clause introduces (`D`, the alias of `B as C`, the +// `ns` of `* as ns`), in source order. +fn binding_names(clause: Node, source: &str) -> Vec { + let mut c = clause.walk(); + clause + .children(&mut c) + .filter_map(|child| match child.kind() { + // Default import: `import D from ...` + "identifier" => Some(vec![text(child, source)]), + // Namespace import: `import * as ns from ...` + "namespace_import" => last_identifier(child).map(|n| vec![text(n, source)]), + // Named imports: `import { A, B as C } from ...` + "named_imports" => { + let mut s = child.walk(); + Some( + child + .children(&mut s) + .filter(|n| n.kind() == "import_specifier") + .filter_map(last_identifier) + .map(|n| text(n, source)) + .collect(), + ) + } + _ => None, + }) + .flatten() + .collect() +} + +// The last `identifier` under a node is the local name: for `B as C` and +// `* as ns` it is the alias; for a lone specifier it is the name itself. +// (tree-sitter's child iterator is forward-only, so "last" is `.last()`.) +fn last_identifier(node: Node) -> Option { + let mut c = node.walk(); + node.children(&mut c) + .filter(|n| n.kind() == "identifier") + .last() +} + +fn text(node: Node, source: &str) -> String { + source[node.byte_range()].to_owned() +} + +#[cfg(test)] +mod tests { + use super::TsUnusedImports; + use crate::core::index::Index; + use crate::parser::{Fix, FixCandidate}; + use std::path::Path; + + fn fixes(source: &str) -> Vec { + TsUnusedImports::new().fixes(Path::new("a.ts"), source, &Index::default()) + } + + #[test] + fn unused_named_import_is_deleted_with_its_line() { + let src = "import { A } from './a';\nexport const b = 1;\n"; + let found = fixes(src); + assert_eq!(found.len(), 1); + assert!(found[0].auto_fixable); + assert_eq!(found[0].message, "unused import [A]"); + assert_eq!( + &src[found[0].edits[0].span.clone()], + "import { A } from './a';\n" + ); + } + + #[test] + fn any_live_binding_keeps_the_whole_statement() { + // `A` used, `B` dead => under-delete: keep both. + let src = "import { A, B } from './a';\nconst x: A = A();\n"; + assert!(fixes(src).is_empty()); + } + + #[test] + fn default_and_namespace_and_alias_bindings() { + let src = "import React from 'react';\nconst x = 1;\n"; + assert_eq!(fixes(src).len(), 1); + let src = "import * as ns from './a';\nconst x = 1;\n"; + assert_eq!(fixes(src).len(), 1); + // `B as C`: the local name is `C`; using `C` keeps it. + let src = "import { B as C } from './a';\nconst x = C;\n"; + assert!(fixes(src).is_empty()); + let src = "import { B as C } from './a';\nconst B = 1;\n"; + // `B` here is a different declaration, not the import alias `C`. + assert_eq!(fixes(src).len(), 1); + } + + #[test] + fn side_effect_and_reexport_are_never_candidates() { + let src = "import './styles.css';\nexport { A } from './a';\n"; + assert!(fixes(src).is_empty()); + } + + #[test] + fn comment_or_string_or_jsx_mention_keeps_the_import() { + let src = "import Foo from './foo';\n// see Foo\nconst s = \"Foo\";\n"; + assert!(fixes(src).is_empty()); + // JSX usage of the bound component. + let tsx = "import Foo from './foo';\nconst x = ;\n"; + assert!( + TsUnusedImports::new() + .fixes(Path::new("a.tsx"), tsx, &Index::default()) + .is_empty() + ); + } + + #[test] + fn type_only_import_is_deleted_like_any_binding() { + let src = "import type { A } from './a';\nconst x = 1;\n"; + assert_eq!(fixes(src).len(), 1); + // Referenced through `typeof`/annotation => kept. + let src = "import type { A } from './a';\nconst x: A = 1;\n"; + assert!(fixes(src).is_empty()); + } + + #[test] + fn crlf_line_is_removed_wholly() { + let src = "import { A } from './a';\r\nexport const b = 1;\r\n"; + let found = fixes(src); + assert_eq!( + &src[found[0].edits[0].span.clone()], + "import { A } from './a';\r\n" + ); + } + + #[test] + fn used_side_effect_and_dynamic_are_untouched() { + // require/dynamic import are separate records, not import_statement + // nodes, so this rule never proposes deleting them. + let src = "const a = require('./a');\nimport { A } from './a';\nexport const x = A;\n"; + assert!(fixes(src).is_empty(), "A is used by `export const x = A`"); + } +} diff --git a/tests/cli_fix.rs b/tests/cli_fix.rs index cb7cbe8..1ecb885 100644 --- a/tests/cli_fix.rs +++ b/tests/cli_fix.rs @@ -247,3 +247,27 @@ fn three_rules_converge_on_one_project() { .stdout(predicate::str::contains("nothing to change")); jmove(&tmp, &["check"]).success(); } + +fn ts_unused() -> tempfile::TempDir { + common::copy_fixture("typescript", "unused") +} + +#[test] +fn ts_unused_import_deletes_dead_type_import_and_keeps_the_rest() { + let tmp = ts_unused(); + // The whole-statement delete must fire for the dead `Ghost` type import. + jmove(&tmp, &["fix", "--rule", "ts/unused-import", "--json"]) + .success() + .stdout( + predicate::str::contains("\"rule\": \"ts/unused-import\"") + .and(predicate::str::contains("\"applied\": true")), + ); + let app = read(&in_root(tmp.path(), "src/app.ts")); + assert!(!app.contains("Ghost"), "{app}"); + // Side-effect import, the mixed used/unused statement and the default + // class import all stay (under-delete safety). + assert!(app.contains("import './side-effects';"), "{app}"); + assert!(app.contains("unused"), "{app}"); + assert!(app.contains("import Logger"), "{app}"); + jmove(&tmp, &["check"]).success(); +} diff --git a/tests/typescript/unused/package.json b/tests/typescript/unused/package.json new file mode 100644 index 0000000..79a6b69 --- /dev/null +++ b/tests/typescript/unused/package.json @@ -0,0 +1 @@ +{ "name": "unused-fixture" } diff --git a/tests/typescript/unused/src/app.ts b/tests/typescript/unused/src/app.ts new file mode 100644 index 0000000..98aa77f --- /dev/null +++ b/tests/typescript/unused/src/app.ts @@ -0,0 +1,10 @@ +import { used, unused } from './lib'; +import './side-effects'; +import Logger from './logger'; // used below, keep it +import type { Ghost } from './types'; + +export function run(): number { + const log = new Logger(); + log.write('hello'); + return used(1, 2); +} diff --git a/tests/typescript/unused/src/lib.ts b/tests/typescript/unused/src/lib.ts new file mode 100644 index 0000000..0acbc6b --- /dev/null +++ b/tests/typescript/unused/src/lib.ts @@ -0,0 +1,6 @@ +export function used(a: number, b: number): number { + return a + b; +} +export function unused(a: number): number { + return a; +} diff --git a/tests/typescript/unused/src/logger.ts b/tests/typescript/unused/src/logger.ts new file mode 100644 index 0000000..cebefaa --- /dev/null +++ b/tests/typescript/unused/src/logger.ts @@ -0,0 +1,3 @@ +export default class Logger { + write(_s: string) {} +} diff --git a/tests/typescript/unused/src/side-effects.ts b/tests/typescript/unused/src/side-effects.ts new file mode 100644 index 0000000..e69de29 diff --git a/tests/typescript/unused/src/types.ts b/tests/typescript/unused/src/types.ts new file mode 100644 index 0000000..e69de29 diff --git a/todo.md b/todo.md index 28fdd7b..2d62afb 100644 --- a/todo.md +++ b/todo.md @@ -56,7 +56,11 @@ AI оставляем СНАРУЖИ: при неоднозначности jmov insert `import pkg.Type;` at the import-block end; ambiguous/wildcard → `--json` `candidates`, applied:false; закрыт guava-разрыв «перенесли файл, соседняя ссылка без импорта умерла» — проверено mv+fix+javac SUCCESS), import-order (DONE: Google-стиль — statics первыми, ASCII-сортировка, дедуп; конфликтующие с другими правилами откладываются (prune_overlaps по severity) и сходятся за 2-3 прогона), class-name-mismatch -- [ ] TS v1: unused-imports, import-order; add-import требует индекс экспортов (символ→файл) +- [x] TS v1: unused-imports (DONE: whole-statement delete, все биндинги мертвы → строка уходит; + mixed used/unused НЕ трогаем — в ESM импорт исполняет побочные эффекты модуля, + partial-удаление specifier'ов отложено осознанно) +- [ ] TS v1: import-order (нет кэнона без eslint-config — grouping-конвенции плавающие; ждать запроса), + add-import требует индекс экспортов (символ→файл) - [ ] Форматирование: свой cargo-fmt НЕ строим (вечный long-tail). Только «import formatting» (порядок/группировка — у нас уже есть spans). Опционально `--format-after ` (prettier / google-java-format), не зависимость