diff --git a/src/cli/fix.rs b/src/cli/fix.rs new file mode 100644 index 0000000..961458b --- /dev/null +++ b/src/cli/fix.rs @@ -0,0 +1,203 @@ +//! `jmove fix` — auto-repair small import problems with the same safety +//! engine as `mv`: index → rules → plan → optional dry-run diff → atomic +//! apply. Deterministic by design; ambiguous findings are reported as +//! non-auto `candidates` for an agent to resolve, never applied blindly. + +use std::collections::BTreeMap; +use std::path::{Path, PathBuf}; + +use serde::Serialize; + +use crate::core::apply::{self, render_edits_diff}; +use crate::core::fix::{FixPlan, plan_fix}; +use crate::core::index::Index; +use crate::core::{JmoveError, JmoveResult, rel_str}; +use crate::parser; + +use super::json::Envelope; +use super::{Flow, exit, fail, flow, json, output}; + +/// One reported candidate (JSON element). `applied` is false for +/// ambiguous findings the engine will not touch. +#[derive(Debug, Serialize)] +pub struct FixEntry { + /// Rule that produced this candidate, e.g. `java/unused-import`. + pub rule: &'static str, + /// 1-based line of the first edit. + pub line: usize, + /// `error` | `warning` | `info`. + pub severity: &'static str, + /// `true` = auto-applied; `false` = needs an agent/human decision. + pub applied: bool, + /// Human explanation, mirrors `FixCandidate::message`. + pub message: String, + /// FQN options for an ambiguous finding (omitted when empty): pick one, + /// add the import yourself and re-run. + #[serde(skip_serializing_if = "Vec::is_empty")] + pub candidates: Vec, +} + +/// A file touched (or inspectable) by `fix`. +#[derive(Debug, Serialize)] +pub struct FixedFile { + /// Project-relative path. + pub path: String, + /// Candidates for this file, in source order. + pub fixes: Vec, +} + +/// Apply-mode payload (`status: "ok"`). +#[derive(Debug, Serialize)] +pub struct FixData { + /// Candidates applied to disk. + pub fixes: usize, + /// Files rewritten. + pub files_changed: usize, + /// Per-file detail, sorted by path. + pub changed_files: Vec, +} + +/// Dry-run payload (`status: "dry_run"`). +#[derive(Debug, Serialize)] +pub struct FixDryRunData { + /// Candidates that would be applied. + pub would_fix: usize, + /// Files that would be rewritten, sorted. + pub affected_files: Vec, + /// Unified diff of the whole plan. + pub diff: String, + /// Per-file candidate detail, sorted by path. + pub files: Vec, +} + +/// `fix` handler: validate rule, index, plan, then dry-run or apply. +pub fn fix(root: &Path, rule: Option<&str>, dry_run: bool, json: bool) -> Flow { + let root = flow(json, "fix", root.canonicalize().map_err(JmoveError::from))?; + if let Some(rejected) = fix_reject(rule) { + return Err(fail(json, "fix", rejected)); + } + let index = flow(json, "fix", Index::build(&root))?; + let plan = plan_fix(&index, rule); + if plan.is_empty() { + if json { + json::print(&Envelope::ok("fix", empty_data())); + } else { + println!("fix: nothing to change"); + } + return Ok(exit::OK); + } + if dry_run { + return fix_dry_run(&root, json, &plan); + } + // Line numbers use spans against the original contents, so the JSON + // payload is assembled before any edit reaches the disk. + let detail = flow(json, "fix", describe(&root, &plan))?; + let edits = plan.auto_edits(); + let files_changed = flow(json, "fix", apply::apply_edits(&root, &edits))?; + if json { + let fixes = edits.values().map(Vec::len).sum(); + json::print(&Envelope::ok( + "fix", + FixData { + fixes, + files_changed, + changed_files: detail, + }, + )); + } else { + let files = edits.len(); + let fixes = edits.values().map(Vec::len).sum(); + if files == 0 { + // Manual-only plan (e.g. ambiguous `java/missing-import`): + // nothing applied, the agent decides via --json candidates. + let manual = plan.total(); + println!( + "fix: nothing to change; {manual} manual {} (see --json)", + output::plural(manual, "candidate") + ); + } else { + println!( + "fixed {} {} in {} {}", + fixes, + output::plural(fixes, "issue"), + files, + output::plural(files, "file") + ); + } + } + Ok(exit::OK) +} + +/// Reject an unknown `--rule` before doing any work. +fn fix_reject(rule: Option<&str>) -> Option { + let name = rule?; + if parser::rule_ids().contains(&name) { + return None; + } + let known = parser::rule_ids().join(", "); + Some(super::json::ErrorData::new( + "INVALID_ARGUMENT", + format!("unknown fix rule '{name}'"), + Some(format!("available rules: {known}")), + )) +} + +/// Dry-run branch: unified diff for humans, structured preview for agents. +fn fix_dry_run(root: &Path, json: bool, plan: &FixPlan) -> Flow { + let edits = plan.auto_edits(); + let diff = flow(json, "fix", render_edits_diff(root, &edits))?; + if json { + let detail = flow(json, "fix", describe(root, plan))?; + let affected = edits.keys().map(|p| rel_str(p)).collect(); + json::print(&Envelope::dry_run( + "fix", + FixDryRunData { + would_fix: edits.values().map(Vec::len).sum(), + affected_files: affected, + diff, + files: detail, + }, + )); + } else { + print!("{diff}"); + } + Ok(exit::OK) +} + +/// Empty-plan payload with zero counters. +fn empty_data() -> FixData { + FixData { + fixes: 0, + files_changed: 0, + changed_files: Vec::new(), + } +} + +// Build per-file candidate detail with 1-based line numbers resolved +// against the on-disk contents at call time (before any apply). +fn describe(root: &Path, plan: &FixPlan) -> JmoveResult> { + let mut lines: BTreeMap<&PathBuf, String> = BTreeMap::new(); + let mut out = Vec::new(); + for (file, candidates) in &plan.files { + if !lines.contains_key(file) { + lines.insert(file, std::fs::read_to_string(root.join(file))?); + } + let source = &lines[file]; + let fixes = candidates + .iter() + .map(|c| FixEntry { + rule: c.rule, + line: output::line_of(source, c.span.start), + severity: c.severity.as_str(), + applied: c.auto_fixable, + message: c.message.clone(), + candidates: c.candidates.clone(), + }) + .collect(); + out.push(FixedFile { + path: rel_str(file), + fixes, + }); + } + Ok(out) +} diff --git a/src/cli/json.rs b/src/cli/json.rs index 2c4a0f4..79f673d 100644 --- a/src/cli/json.rs +++ b/src/cli/json.rs @@ -7,8 +7,8 @@ use serde::Serialize; -use crate::core::JmoveError; use crate::core::plan::MovePlan; +use crate::core::{JmoveError, rel_str}; use super::output; @@ -98,6 +98,10 @@ impl ErrorData { "PLAN_REJECTED", "run `jmove check --json` to inspect the import graph", ), + JmoveError::Git(_) => ( + "GIT_ERROR", + "retry with --no-git to move without touching git", + ), }; Self::new(code, err.to_string(), Some(hint.to_owned())) } @@ -136,18 +140,21 @@ pub struct MvData { pub moved: usize, /// Total specifiers rewritten across all importers. pub updated_imports: usize, + /// Rename backend: `"git"` (staged in the index) or `"fs"`. + pub moved_via: &'static str, } impl MvData { /// Assemble the payload from an applied plan and its change details. #[must_use] - pub fn new(plan: &MovePlan, changed_files: Vec) -> Self { + pub fn new(plan: &MovePlan, changed_files: Vec, via_git: bool) -> Self { Self { - source: plan.source.display().to_string(), - target: plan.target.display().to_string(), + source: rel_str(&plan.source), + target: rel_str(&plan.target), changed_files, moved: 1, updated_imports: plan.rewrites.len(), + moved_via: if via_git { "git" } else { "fs" }, } } } @@ -166,21 +173,24 @@ pub struct MvDryRunData { pub affected_files: Vec, /// Unified diff (rewrites + rename) of the whole plan. pub diff: String, + /// Rename backend a real run would use: `"git"` or `"fs"`. + pub would_move_via: &'static str, } impl MvDryRunData { /// Assemble the preview payload from a plan and its rendered diff. #[must_use] - pub fn new(plan: &MovePlan, diff: String) -> Self { + pub fn new(plan: &MovePlan, diff: String, via_git: bool) -> Self { Self { - would_move: plan.source.display().to_string(), - target: plan.target.display().to_string(), + would_move: rel_str(&plan.source), + target: rel_str(&plan.target), would_update: plan.rewrites.len(), affected_files: output::group_by_file(&plan.rewrites) .into_iter() - .map(|(file, _)| file.display().to_string()) + .map(|(file, _)| rel_str(file)) .collect(), diff, + would_move_via: if via_git { "git" } else { "fs" }, } } } diff --git a/src/cli/mod.rs b/src/cli/mod.rs index d1dfab1..8d3a470 100644 --- a/src/cli/mod.rs +++ b/src/cli/mod.rs @@ -3,11 +3,12 @@ //! Exit codes (mirrored in `docs/SKILL.md`): //! `0` success · `1` operation error · `2` broken imports found. +pub mod fix; pub mod json; pub mod output; use std::convert::identity; -use std::path::{Component, Path, PathBuf}; +use std::path::{Path, PathBuf}; use clap::{Parser, Subcommand}; @@ -45,6 +46,9 @@ pub enum Command { /// Machine-readable JSON output (for AI agents). #[arg(long)] json: bool, + /// Plain filesystem rename even for git-tracked files. + #[arg(long)] + no_git: bool, }, /// Report broken imports in the project. Check { @@ -52,6 +56,18 @@ pub enum Command { #[arg(long)] json: bool, }, + /// Auto-fix small import problems (same engine as `mv`). + Fix { + /// Run only this rule id (see docs/SKILL.md), e.g. java/unused-import. + #[arg(long)] + rule: Option, + /// Preview changes without touching the disk. + #[arg(long)] + dry_run: bool, + /// Machine-readable JSON output (for AI agents). + #[arg(long)] + json: bool, + }, } /// Process exit codes documented for humans and agents alike. @@ -78,18 +94,32 @@ pub fn run() -> anyhow::Result { target, dry_run, json, - } => mv(&args.root, &source, &target, dry_run, json), + no_git, + } => mv(&args.root, &source, &target, dry_run, json, no_git), Command::Check { json } => check(&args.root, json), + Command::Fix { + rule, + dry_run, + json, + } => fix::fix(&args.root, rule.as_deref(), dry_run, json), }; // Handlers report their own failures; both arms carry an exit code. Ok(outcome.unwrap_or_else(identity)) } /// `mv` handler: normalize paths, validate, index, plan, then dry-run or apply. -fn mv(root: &Path, source: &Path, target: &Path, dry_run: bool, json: bool) -> Flow { +fn mv( + root: &Path, + source: &Path, + target: &Path, + dry_run: bool, + json: bool, + no_git: bool, +) -> Flow { + let git = apply::GitMode::from_no_git(no_git); let root = flow(json, "mv", root.canonicalize().map_err(JmoveError::from))?; - let source = flow(json, "mv", rel_from_root(&root, source))?; - let target = flow(json, "mv", rel_from_root(&root, target))?; + let source = flow(json, "mv", core::rel_from_root(&root, source))?; + let target = flow(json, "mv", core::rel_from_root(&root, target))?; if let Some(rejected) = mv_reject(&root, &source, &target) { return Err(fail(json, "mv", rejected)); } @@ -97,7 +127,7 @@ fn mv(root: &Path, source: &Path, target: &Path, dry_run: bool, json: bool) -> F let index = flow(json, "mv", Index::build(&root))?; let plan = flow(json, "mv", plan::plan_move(&index, &source, &target))?; if dry_run { - return mv_dry_run(&root, json, &plan); + return mv_dry_run(&root, json, &plan, git); } // Line numbers use spans against the *original* contents, so the JSON // payload is assembled before the rewrites hit the disk. @@ -106,12 +136,15 @@ fn mv(root: &Path, source: &Path, target: &Path, dry_run: bool, json: bool) -> F } else { Vec::new() }; - flow(json, "mv", apply::apply(&root, &plan))?; + let applied = flow(json, "mv", apply::apply(&root, &plan, git))?; if json { - json::print(&Envelope::ok("mv", MvData::new(&plan, changed))); + json::print(&Envelope::ok( + "mv", + MvData::new(&plan, changed, applied.via_git), + )); } else { - println!("{}", output::mv_summary(&plan)); + println!("{}", output::mv_summary(&plan, applied.via_git)); } Ok(exit::OK) } @@ -127,7 +160,7 @@ fn mv_reject(root: &Path, source: &Path, target: &Path) -> Option { return bad("INVALID_ARGUMENT", msg, "pick a different destination"); } if !root.join(source).is_file() { - let msg = format!("source file '{}' does not exist", source.display()); + let msg = format!("source file '{}' does not exist", core::rel_str(source)); return bad( "SOURCE_NOT_FOUND", msg, @@ -135,7 +168,7 @@ fn mv_reject(root: &Path, source: &Path, target: &Path) -> Option { ); } if root.join(target).exists() { - let msg = format!("target path '{}' already exists", target.display()); + let msg = format!("target path '{}' already exists", core::rel_str(target)); return bad( "TARGET_EXISTS", msg, @@ -145,7 +178,10 @@ fn mv_reject(root: &Path, source: &Path, target: &Path) -> Option { // `target` names a file, so `parent()` always yields the directory part. let parent = root.join(target.parent().unwrap_or(Path::new(""))); if parent.exists() && !parent.is_dir() { - let msg = format!("target parent of '{}' is not a directory", target.display()); + let msg = format!( + "target parent of '{}' is not a directory", + core::rel_str(target) + ); return bad( "INVALID_ARGUMENT", msg, @@ -156,10 +192,14 @@ fn mv_reject(root: &Path, source: &Path, target: &Path) -> Option { } /// Dry-run branch: unified diff for humans, structured preview for agents. -fn mv_dry_run(root: &Path, json: bool, plan: &MovePlan) -> Flow { +fn mv_dry_run(root: &Path, json: bool, plan: &MovePlan, git: apply::GitMode) -> Flow { let diff = flow(json, "mv", apply::render_diff(root, plan))?; + let via_git = apply::would_use_git(root, &plan.source, git); if json { - json::print(&Envelope::dry_run("mv", MvDryRunData::new(plan, diff))); + json::print(&Envelope::dry_run( + "mv", + MvDryRunData::new(plan, diff, via_git), + )); } else { print!("{diff}"); } @@ -208,41 +248,3 @@ fn fail(json: bool, operation: &'static str, err: ErrorData) -> i32 { } exit::ERROR } - -/// Convert a user path to a normalized project-relative path. Relative -/// paths are taken against `root`; absolute ones must live underneath it. -fn rel_from_root(root: &Path, path: &Path) -> JmoveResult { - let joined = if path.is_absolute() { - path.into() - } else { - root.join(path) - }; - let outside = || { - JmoveError::InvalidArgument(format!( - "path '{}' is outside the project root", - path.display() - )) - }; - let abs = collapse(&joined); - let rel = abs.strip_prefix(root).map_err(|_| outside())?; - core::normalize_rel_path(rel).ok_or_else(|| { - JmoveError::InvalidArgument(format!("invalid project path '{}'", path.display())) - }) -} - -/// Lexically normalize a path: drop `.` segments, apply `..` where possible. -fn collapse(path: &Path) -> PathBuf { - let mut stack: Vec> = Vec::new(); - for comp in path.components() { - match comp { - Component::CurDir => {} - Component::ParentDir => { - if stack.last() != Some(&Component::ParentDir) { - stack.pop(); - } - } - other => stack.push(other), - } - } - stack.into_iter().collect() -} diff --git a/src/cli/output.rs b/src/cli/output.rs index e68939d..4fa23d6 100644 --- a/src/cli/output.rs +++ b/src/cli/output.rs @@ -7,9 +7,9 @@ use std::collections::BTreeMap; use std::path::{Path, PathBuf}; -use crate::core::JmoveResult; use crate::core::index::Index; use crate::core::plan::{MovePlan, Rewrite}; +use crate::core::{JmoveResult, rel_str}; use super::json::{BrokenImport, Change, ChangedFile}; @@ -57,7 +57,7 @@ pub fn broken_imports(root: &Path, index: &Index) -> JmoveResult JmoveResult tgt, updated N imports in M files` success summary. +/// `moved src -> tgt, updated N imports in M files` success summary, +/// noting when the rename went through `git mv`. #[must_use] -pub fn mv_summary(plan: &MovePlan) -> String { +pub fn mv_summary(plan: &MovePlan, via_git: bool) -> String { let imports = plan.rewrites.len(); let files = group_by_file(&plan.rewrites).len(); + let git = if via_git { " (via git mv)" } else { "" }; format!( - "moved {} -> {}, updated {} {} in {} {}", - plan.source.display(), - plan.target.display(), + "moved {} -> {}{git}, updated {} {} in {} {}", + rel_str(&plan.source), + rel_str(&plan.target), imports, plural(imports, "import"), files, @@ -136,7 +138,7 @@ pub fn print_error(message: &str, hint: Option<&str>) { } /// `N noun` with a naive English plural. -fn plural(count: usize, noun: &str) -> String { +pub(crate) fn plural(count: usize, noun: &str) -> String { if count == 1 { noun.to_owned() } else { diff --git a/src/core/apply/diff.rs b/src/core/apply/diff.rs index bd0b5b4..7d36dc6 100644 --- a/src/core/apply/diff.rs +++ b/src/core/apply/diff.rs @@ -1,30 +1,45 @@ -//! Unified-diff rendering of a plan, used by `mv --dry-run`. +//! Unified-diff rendering of edit plans, used by `--dry-run` of every +//! command: [`render_edits_diff`] is the generic engine over per-file edit +//! groups, [`render_diff`] adds the `mv` rename line. use similar::TextDiff; -use crate::core::JmoveResult; +use std::collections::BTreeMap; +use std::path::{Path, PathBuf}; + +use crate::core::Edit; use crate::core::apply::fsops::{group_by_file, rewrite_bytes}; use crate::core::plan::MovePlan; -use std::path::Path; +use crate::core::{JmoveResult, rel_str}; /// Render the plan as a unified diff per rewritten file plus a final /// `move -> ` line, for dry-run. A plan without rewrites /// renders the empty string. pub fn render_diff(root: &Path, plan: &MovePlan) -> JmoveResult { + let mut out = render_edits_diff(root, &group_by_file(plan))?; + if !out.is_empty() { + let (src, dst) = (rel_str(&plan.source), rel_str(&plan.target)); + out.push_str(&format!("move {src} -> {dst}\n")); + } + Ok(out) +} + +/// Render pre-grouped edits (the `fix` dry-run shape) as one unified diff +/// per changed file; files with empty edit groups render nothing. +pub fn render_edits_diff( + root: &Path, + by_file: &BTreeMap>, +) -> JmoveResult { let mut out = String::new(); - for (file, rewrites) in group_by_file(plan) { - let original = std::fs::read(root.join(&file))?; - let patched = rewrite_bytes(&original, &rewrites)?; - let name = file.display().to_string(); + for (file, edits) in by_file { + let original = std::fs::read(root.join(file))?; + let patched = rewrite_bytes(file, &original, edits)?; + let name = rel_str(file); let old = String::from_utf8_lossy(&original); let new = String::from_utf8_lossy(&patched); let text = TextDiff::from_lines(&old, &new); out.push_str(&text.unified_diff().header(&name, &name).to_string()); } - if !out.is_empty() { - let (src, dst) = (plan.source.display(), plan.target.display()); - out.push_str(&format!("move {src} -> {dst}\n")); - } Ok(out) } diff --git a/src/core/apply/fsops.rs b/src/core/apply/fsops.rs index cb741e8..eb00d9e 100644 --- a/src/core/apply/fsops.rs +++ b/src/core/apply/fsops.rs @@ -5,14 +5,16 @@ use std::fs; use std::io::Write; use std::path::{Path, PathBuf}; -use crate::core::plan::{MovePlan, Rewrite}; -use crate::core::{JmoveError, JmoveResult}; +use crate::core::Edit; +use crate::core::plan::MovePlan; +use crate::core::{JmoveError, JmoveResult, rel_str}; -// Group rewrites by file; the BTreeMap keeps the order deterministic. -pub(super) fn group_by_file(plan: &MovePlan) -> BTreeMap> { - let mut m: BTreeMap> = BTreeMap::new(); +// Group plan rewrites as generic per-file edits; the BTreeMap keeps the +// order deterministic. +pub(super) fn group_by_file(plan: &MovePlan) -> BTreeMap> { + let mut m: BTreeMap> = BTreeMap::new(); for r in &plan.rewrites { - m.entry(r.file.clone()).or_default().push(r); + m.entry(r.file.clone()).or_default().push(r.into()); } m } @@ -24,24 +26,46 @@ pub(super) fn sibling_temp(path: &Path) -> PathBuf { PathBuf::from(temp) } -// Apply byte-span replacements in reverse offset order so earlier spans -// stay valid; a span/content mismatch means the plan is stale. Valid -// UTF-8 needles can only match on char boundaries, so a successful -// rewrite of valid UTF-8 stays valid UTF-8. -pub(super) fn rewrite_bytes(original: &[u8], rewrites: &[&Rewrite]) -> JmoveResult> { +// Byte `i` is a UTF-8 char boundary iff it sits at/after the end or is +// not a continuation byte (`10xx_xxxx`). +fn is_char_boundary(bytes: &[u8], i: usize) -> bool { + bytes.get(i).is_none_or(|b| b & 0xC0 != 0x80) +} + +// Apply edits in reverse span order so earlier offsets stay valid; a +// span/content mismatch means the plan is stale. Equal starts (insertions +// at one offset) keep their input order in the output because the +// stable-ascending sort is applied back-to-front. Overlapping or +// malformed spans are generator bugs and rejected before any byte of +// `out` changes. Non-empty needles can only match valid UTF-8 on char +// boundaries; the explicit boundary check pins insertions the same way, +// so patching UTF-8 stays UTF-8. +pub(super) fn rewrite_bytes(file: &Path, original: &[u8], edits: &[Edit]) -> JmoveResult> { + let mut sorted: Vec<&Edit> = edits.iter().collect(); + sorted.sort_by_key(|e| (e.span.start, e.span.end)); + if sorted.iter().any(|e| e.span.start > e.span.end) + || sorted.windows(2).any(|w| w[1].span.start < w[0].span.end) + { + return Err(JmoveError::PlanRejected(format!( + "overlapping or malformed edits in '{}': {:?}", + rel_str(file), + edits.iter().map(|e| e.span.clone()).collect::>() + ))); + } let mut out = original.to_vec(); - let mut sorted = rewrites.to_vec(); - sorted.sort_by_key(|r| std::cmp::Reverse(r.span.start)); - for r in sorted { - if r.span.end > out.len() || &out[r.span.clone()] != r.old_text.as_bytes() { + for e in sorted.iter().rev() { + if !is_char_boundary(&out, e.span.start) + || e.span.end > out.len() + || &out[e.span.clone()] != e.old_text.as_bytes() + { return Err(JmoveError::StaleIndex(format!( "'{}' changed since indexing (expected {:?} at {:?})", - r.file.display(), - r.old_text, - r.span + rel_str(file), + e.old_text, + e.span ))); } - out.splice(r.span.clone(), r.new_text.as_bytes().iter().copied()); + out.splice(e.span.clone(), e.new_text.as_bytes().iter().copied()); } Ok(out) } @@ -69,3 +93,69 @@ pub(super) fn create_missing_dirs(dst: &Path) -> JmoveResult> { } Ok(created) } + +#[cfg(test)] +mod tests { + use super::rewrite_bytes; + use crate::core::{Edit, JmoveError}; + use std::path::Path; + + fn file() -> &'static Path { + Path::new("f.txt") + } + + fn edit(start: usize, end: usize, old: &str, new: &str) -> Edit { + Edit { + span: start..end, + old_text: old.into(), + new_text: new.into(), + } + } + + fn text(bytes: &[u8]) -> String { + String::from_utf8(bytes.to_vec()).expect("valid utf-8") + } + + #[test] + fn replaces_inserts_and_deletes_in_one_pass() { + // "one\ntwo\nthree\n": insert a line at 0, replace `two`, delete + // the whole `three` line via its span absorbing the trailing `\n`. + let src = b"one\ntwo\nthree\n"; + let edits = [ + edit(4, 7, "two", "TWO"), + edit(8, 14, "three\n", ""), + edit(0, 0, "", "zero\n"), + ]; + let out = rewrite_bytes(file(), src, &edits).unwrap(); + assert_eq!(text(&out), "zero\none\nTWO\n"); + } + + #[test] + fn insertions_at_one_offset_keep_input_order() { + let src = b"head\ntail\n"; + let edits = [edit(5, 5, "", "b\n"), edit(5, 5, "", "a\n")]; + let out = rewrite_bytes(file(), src, &edits).unwrap(); + assert_eq!(text(&out), "head\nb\na\ntail\n"); + } + + #[test] + fn overlapping_or_malformed_spans_are_rejected() { + let src = b"abcdefgh"; + let overlap = [edit(2, 6, "cdef", "X"), edit(4, 8, "efgh", "Y")]; + let err = rewrite_bytes(file(), src, &overlap).unwrap_err(); + assert!(matches!(err, JmoveError::PlanRejected(_)), "{err}"); + let malformed = [edit(6, 2, "", "X")]; + let err = rewrite_bytes(file(), src, &malformed).unwrap_err(); + assert!(matches!(err, JmoveError::PlanRejected(_)), "{err}"); + } + + #[test] + fn stale_or_off_boundary_edits_are_rejected() { + // Wrong expectation at the span => the file moved under the plan. + let err = rewrite_bytes(file(), b"abcd", &[edit(0, 4, "wrong", "X")]).unwrap_err(); + assert!(matches!(err, JmoveError::StaleIndex(_)), "{err}"); + // Insertion inside the 2-byte `ä` is not a char boundary. + let err = rewrite_bytes(file(), "ä\n".as_bytes(), &[edit(1, 1, "", "x")]).unwrap_err(); + assert!(matches!(err, JmoveError::StaleIndex(_)), "{err}"); + } +} diff --git a/src/core/apply/git.rs b/src/core/apply/git.rs new file mode 100644 index 0000000..f4faefc --- /dev/null +++ b/src/core/apply/git.rs @@ -0,0 +1,164 @@ +//! Git integration: move tracked files through `git mv` so the rename +//! lands in the index (staged, with history detection) instead of being a +//! plain filesystem rename. Shells out to the system `git` — no new +//! dependency, and `git` is the only sane implementation of its own index. + +use std::path::Path; +use std::process::Command; + +use crate::core::{JmoveError, JmoveResult, rel_str}; + +/// Whether `mv` may involve git in the physical rename. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Default)] +pub enum GitMode { + /// `git mv` when the source is tracked; plain rename otherwise. + #[default] + Auto, + /// Never touch git (CLI `--no-git`). + Disabled, +} + +impl GitMode { + /// CLI mapping: the `--no-git` flag switches to [`GitMode::Disabled`]. + #[must_use] + pub const fn from_no_git(no_git: bool) -> Self { + if no_git { Self::Disabled } else { Self::Auto } + } +} + +/// True when `rel` (root-relative) is tracked in the git index of the +/// repository containing `root`. A missing git binary, a non-repository +/// root or an untracked file all count as "not tracked". +pub(super) fn is_tracked(root: &Path, rel: &Path) -> bool { + let rel = rel_str(rel); + git_output(root, &["ls-files", "--", &rel]).is_some_and(|out| !out.trim().is_empty()) +} + +/// True when a rename of `rel` under `root` would go through `git mv`. +#[must_use] +pub fn would_use_git(root: &Path, rel: &Path, mode: GitMode) -> bool { + mode == GitMode::Auto && is_tracked(root, rel) +} + +/// `git mv ` with root-relative paths; stderr on failure is +/// surfaced as [`JmoveError::Git`]. +pub(super) fn mv(root: &Path, src: &Path, dst: &Path) -> JmoveResult<()> { + let (src, dst) = (rel_str(src), rel_str(dst)); + git_run(root, &["mv", "--", &src, &dst]) +} + +// Run git inside `root`; None when git is absent or exits non-zero. +fn git_output(root: &Path, args: &[&str]) -> Option { + let ok = git_command(root, args).output().ok()?; + ok.status + .success() + .then(|| String::from_utf8_lossy(&ok.stdout).into_owned()) +} + +fn git_run(root: &Path, args: &[&str]) -> JmoveResult<()> { + let out = git_command(root, args).output()?; + if out.status.success() { + return Ok(()); + } + let err = String::from_utf8_lossy(&out.stderr).trim().to_owned(); + Err(JmoveError::Git(format!( + "`git {}` failed: {err}", + args.first().copied().unwrap_or("") + ))) +} + +fn git_command(root: &Path, args: &[&str]) -> Command { + let mut cmd = Command::new("git"); + cmd.arg("-C").arg(root).args(args); + cmd +} + +#[cfg(test)] +mod tests { + use super::{GitMode, is_tracked, mv, would_use_git}; + use std::fs; + use std::path::Path; + use std::process::Command; + + // A real throwaway repository: init, one committed file, no user + // identity needed beyond the inline -c overrides. + fn repo() -> tempfile::TempDir { + let dir = tempfile::TempDir::new().expect("tempdir"); + git(dir.path(), &["init", "-q", "-b", "main", "."]); + fs::write(dir.path().join("a.txt"), "one\n").expect("write"); + git(dir.path(), &["add", "a.txt"]); + git( + dir.path(), + &[ + "-c", + "user.name=t", + "-c", + "user.email=t@t", + "commit", + "-qm", + "init", + ], + ); + dir + } + + fn git(root: &Path, args: &[&str]) { + let status = Command::new("git") + .arg("-C") + .arg(root) + .args(args) + .status() + .expect("git"); + assert!(status.success(), "`git {args:?}` failed"); + } + + fn stdout(root: &Path, args: &[&str]) -> String { + let out = Command::new("git") + .arg("-C") + .arg(root) + .args(args) + .output() + .expect("git"); + assert!(out.status.success(), "`git {args:?}` failed"); + String::from_utf8(out.stdout).expect("utf-8") + } + + #[test] + fn tracks_only_indexed_files() { + let dir = repo(); + let root = dir.path(); + assert!(is_tracked(root, Path::new("a.txt"))); + assert!(!is_tracked(root, Path::new("untracked.txt"))); + // Not a repository at all. + let plain = tempfile::TempDir::new().expect("tempdir"); + assert!(!is_tracked(plain.path(), Path::new("a.txt"))); + } + + #[test] + fn would_use_git_respects_mode_and_tracking() { + let dir = repo(); + let root = dir.path(); + assert!(would_use_git(root, Path::new("a.txt"), GitMode::Auto)); + assert!(!would_use_git(root, Path::new("a.txt"), GitMode::Disabled)); + assert!(!would_use_git(root, Path::new("nope.txt"), GitMode::Auto)); + } + + #[test] + fn from_no_git_maps_the_cli_flag() { + assert_eq!(GitMode::from_no_git(false), GitMode::Auto); + assert_eq!(GitMode::from_no_git(true), GitMode::Disabled); + assert_eq!(GitMode::default(), GitMode::Auto); + } + + #[test] + fn mv_renames_on_disk_and_stages_the_rename() { + let dir = repo(); + let root = dir.path(); + fs::create_dir_all(root.join("sub")).expect("mkdir"); + mv(root, Path::new("a.txt"), Path::new("sub/a.txt")).expect("git mv"); + assert!(!root.join("a.txt").exists()); + assert!(root.join("sub/a.txt").is_file()); + let staged = stdout(root, &["diff", "--cached", "-M", "--name-status"]); + assert_eq!(staged, "R100\ta.txt\tsub/a.txt\n"); + } +} diff --git a/src/core/apply/mod.rs b/src/core/apply/mod.rs index 4595861..5e1f679 100644 --- a/src/core/apply/mod.rs +++ b/src/core/apply/mod.rs @@ -1,22 +1,29 @@ //! Atomic apply with rollback, plus unified-diff rendering for dry-run. //! -//! Rewrites land on importer files first (each atomically via temp-file + -//! rename), the `source -> target` rename happens last, and any failure +//! Two entry points share one engine: [`apply`] writes a `mv` plan (its +//! rewrites land on importer files first, each atomically via temp-file + +//! rename, and the `source -> target` rename happens last — through +//! `git mv` for tracked files, see [`GitMode`]), [`apply_edits`] writes +//! plain edit groups without a move (the `fix` flavour). Any failure //! mid-way rolls back everything already written. mod diff; mod fsops; +mod git; -pub use diff::render_diff; +pub use diff::{render_diff, render_edits_diff}; +pub use git::{GitMode, would_use_git}; +use std::collections::BTreeMap; use std::fs; use std::path::{Path, PathBuf}; +use crate::core::Edit; use crate::core::apply::fsops::{ create_missing_dirs, group_by_file, rewrite_bytes, sibling_temp, write_durable, }; -use crate::core::plan::{MovePlan, Rewrite}; -use crate::core::{JmoveError, JmoveResult}; +use crate::core::plan::MovePlan; +use crate::core::{JmoveError, JmoveResult, rel_str}; /// Summary of a successfully applied plan. #[derive(Debug, Clone)] @@ -25,6 +32,8 @@ pub struct Applied { pub files_rewritten: usize, /// The moved file's new project-relative path. pub new_path: PathBuf, + /// Whether the physical rename went through `git mv`. + pub via_git: bool, } // Rollback state for one run: originals of rewritten files (newest last), @@ -34,46 +43,80 @@ struct Run { root: PathBuf, backups: Vec<(PathBuf, Vec)>, dirs: Vec, + // Root-relative (src, dst) of the executed rename. moved: Option<(PathBuf, PathBuf)>, + via_git: bool, } /// Apply `plan` under `root` atomically (see module docs). Rollback is /// best-effort: on restore failure the error names the files that need -/// manual recovery. -pub fn apply(root: &Path, plan: &MovePlan) -> JmoveResult { +/// manual recovery. `git` selects between `git mv` and plain rename. +pub fn apply(root: &Path, plan: &MovePlan, git: GitMode) -> JmoveResult { let mut run = Run { root: root.to_path_buf(), ..Default::default() }; - match run.try_apply(plan) { + match run.try_apply(plan, git) { Ok(applied) => Ok(applied), Err(err) => Err(run.undo(err)), } } +/// Apply pre-grouped edits under `root` atomically, without any move; +/// returns the number of files written. Same rollback contract as +/// [`apply`]. +pub fn apply_edits(root: &Path, by_file: &BTreeMap>) -> JmoveResult { + let mut run = Run { + root: root.to_path_buf(), + ..Default::default() + }; + match run.try_fix(by_file) { + Ok(files) => Ok(files), + Err(err) => Err(run.undo(err)), + } +} + impl Run { - fn try_apply(&mut self, plan: &MovePlan) -> JmoveResult { + fn try_apply(&mut self, plan: &MovePlan, mode: GitMode) -> JmoveResult { let by_file = group_by_file(plan); - for (file, rewrites) in &by_file { - self.rewrite_one(file, rewrites)?; - } + self.write_all(&by_file)?; // The move comes last, after every importer was rewritten. let (src, dst) = (self.root.join(&plan.source), self.root.join(&plan.target)); self.dirs = create_missing_dirs(&dst)?; - fs::rename(&src, &dst)?; - self.moved = Some((src, dst)); + // git mv needs the destination dir to exist; tracked sources are + // renamed through git so the change lands staged in the index. + self.via_git = would_use_git(&self.root, &plan.source, mode); + if self.via_git { + git::mv(&self.root, &plan.source, &plan.target)?; + } else { + fs::rename(&src, &dst)?; + } + self.moved = Some((plan.source.clone(), plan.target.clone())); Ok(Applied { files_rewritten: by_file.len(), new_path: plan.target.clone(), + via_git: self.via_git, }) } + fn try_fix(&mut self, by_file: &BTreeMap>) -> JmoveResult { + self.write_all(by_file)?; + Ok(by_file.len()) + } + + fn write_all(&mut self, by_file: &BTreeMap>) -> JmoveResult<()> { + for (file, edits) in by_file { + self.rewrite_one(file, edits)?; + } + Ok(()) + } + // Patch one file in memory, then temp-file + fsync + rename over it; // the original bytes go to `backups` for rollback. - fn rewrite_one(&mut self, file: &Path, rewrites: &[&Rewrite]) -> JmoveResult<()> { + fn rewrite_one(&mut self, file: &Path, edits: &[Edit]) -> JmoveResult<()> { let path = self.root.join(file); let original = fs::read(&path)?; - let patched = rewrite_bytes(&original, rewrites)?; + let patched = rewrite_bytes(file, &original, edits)?; self.backups.push((file.to_path_buf(), original)); let temp = sibling_temp(&path); // same dir => rename stays atomic write_durable(&temp, &patched)?; @@ -88,14 +131,19 @@ impl Run { // problems to its message. fn undo(&mut self, err: JmoveError) -> JmoveError { let mut problems = Vec::new(); - if let Some((src, dst)) = self.moved.take() - && let Err(e) = fs::rename(&dst, &src) - { - problems.push(format!("could not move back {}: {e}", dst.display())); + if let Some((src, dst)) = self.moved.take() { + let back = if self.via_git { + git::mv(&self.root, &dst, &src) + } else { + fs::rename(self.root.join(&dst), self.root.join(&src)).map_err(Into::into) + }; + if let Err(e) = back { + problems.push(format!("could not move back {}: {e}", rel_str(&dst))); + } } for (file, bytes) in self.backups.drain(..).rev() { if let Err(e) = fs::write(self.root.join(&file), &bytes) { - problems.push(format!("could not restore {}: {e}", file.display())); + problems.push(format!("could not restore {}: {e}", rel_str(&file))); } } for dir in self.dirs.drain(..).rev() { @@ -111,7 +159,7 @@ impl Run { #[cfg(test)] mod tests { - use super::apply; + use super::{GitMode, apply}; use crate::core::JmoveResult; use crate::core::plan::{MovePlan, Rewrite}; use std::fs; @@ -151,7 +199,7 @@ mod tests { let root = dir.path(); mk(root, "src/app.ts", OLD)?; mk(root, "lib/fmt.ts", "export const fmt = 1;\n")?; - let applied = apply(root, &plan())?; + let applied = apply(root, &plan(), GitMode::Disabled)?; assert_eq!( (applied.files_rewritten, &applied.new_path), (1, &PathBuf::from("deep/fmt.ts")) @@ -172,7 +220,7 @@ mod tests { // Missing source: the last rename fails after the rewrite landed. let dir = tempfile::TempDir::new()?; mk(dir.path(), "src/app.ts", OLD)?; - let err = apply(dir.path(), &plan()).expect_err("missing source"); + let err = apply(dir.path(), &plan(), GitMode::Disabled).expect_err("missing source"); assert!(matches!(err, crate::core::JmoveError::Io(_)), "{err}"); // Importer restored to its exact original bytes; created dirs gone. assert_eq!(fs::read_to_string(dir.path().join("src/app.ts"))?, OLD); @@ -189,7 +237,7 @@ mod tests { let root = dir.path(); mk(root, "src/app.ts", other)?; mk(root, "lib/fmt.ts", "export const fmt = 1;\n")?; - let err = apply(root, &plan()).expect_err("span mismatch"); + let err = apply(root, &plan(), GitMode::Disabled).expect_err("span mismatch"); assert!( matches!(err, crate::core::JmoveError::StaleIndex(_)), "{err}" diff --git a/src/core/fix/mod.rs b/src/core/fix/mod.rs new file mode 100644 index 0000000..6697f9f --- /dev/null +++ b/src/core/fix/mod.rs @@ -0,0 +1,212 @@ +//! Fix planning: run the language rules over the indexed files and gather +//! their candidates. Like a [`crate::core::plan::MovePlan`], a [`FixPlan`] +//! is pure data, so `--dry-run` and `--json` render it without any write, +//! and applying reuses the atomic engine behind `mv`. +//! +//! Overlap policy: candidates from different rules may touch the same +//! bytes (`import-order` rewrites the whole block a `unused-import` +//! deletion sits in). [`prune_overlaps`] resolves that per file — urgent +//! rules win, the loser is downgraded to a manual "skipped" report and +//! converges on the next run — instead of rejecting the whole plan. The +//! apply engine's `PLAN_REJECTED` guard stays as the backstop. + +use std::collections::BTreeMap; +use std::fs; +use std::ops::Range; +use std::path::PathBuf; + +use crate::core::Edit; +use crate::core::index::Index; +use crate::parser::{FixCandidate, SourceLanguage, fixers_for}; + +/// Every candidate found by one `jmove fix` run, grouped per file: files +/// in sorted order, candidates of a file in source order. +#[derive(Debug, Default)] +pub struct FixPlan { + /// Project-relative file → its candidates (non-empty groups only). + pub files: BTreeMap>, +} + +impl FixPlan { + /// `true` when no rule matched anything. + #[must_use] + pub fn is_empty(&self) -> bool { + self.files.is_empty() + } + + /// Total candidates across all files. + #[must_use] + pub fn total(&self) -> usize { + self.files.values().map(Vec::len).sum() + } + + /// The auto-fixable candidates as per-file edit groups, ready for the + /// apply/diff engine; files with only manual candidates drop out. + #[must_use] + pub fn auto_edits(&self) -> BTreeMap> { + self.files + .iter() + .map(|(file, candidates)| { + let edits: Vec = candidates + .iter() + .filter(|c| c.auto_fixable) + .flat_map(|c| c.edits.iter().cloned()) + .collect(); + (file.clone(), edits) + }) + .filter(|(_, edits)| !edits.is_empty()) + .collect() + } +} + +/// Run every rule for each indexed file's language — or only the rule +/// named by `rule` — over freshly-read contents. Files that vanished or +/// are not valid UTF-8 after indexing are skipped, mirroring the scanner. +#[must_use] +pub fn plan_fix(index: &Index, rule: Option<&str>) -> FixPlan { + let mut plan = FixPlan::default(); + for file in index.files.sorted() { + let Some(lang) = SourceLanguage::for_path(&file) else { + continue; + }; + let Ok(source) = fs::read_to_string(index.root.join(&file)) else { + continue; + }; + let mut found: Vec = fixers_for(lang) + .iter() + .filter(|fixer| rule.is_none_or(|name| fixer.rule() == name)) + .flat_map(|fixer| fixer.fixes(&file, &source, index)) + .collect(); + if found.is_empty() { + continue; + } + prune_overlaps(&mut found); + found.sort_by_key(|candidate| candidate.span.start); + plan.files.insert(file, found); + } + plan +} + +// Resolve cross-rule byte conflicts before the engine ever sees them: +// accept candidates by (severity, position), and downgrade any whose edits +// touch an accepted span to a manual report ("skipped") with its edits +// dropped. The apply run is deterministic and the next `fix` sees the +// re-written file, so overlapping fixes converge over consecutive runs +// instead of rejecting the whole plan. Empty spans (insertions) conflict +// only when strictly inside another span, so inserts at a replaced block's +// boundary coexist with the replacement. +fn prune_overlaps(candidates: &mut [FixCandidate]) { + let mut order: Vec = (0..candidates.len()).collect(); + order.sort_by_key(|&i| (candidates[i].severity.rank(), candidates[i].span.start)); + let mut accepted: Vec> = Vec::new(); + for i in order { + let candidate = &mut candidates[i]; + let conflict = candidate.edits.iter().any(|e| { + accepted + .iter() + .any(|a| a.start < e.span.end && e.span.start < a.end) + }); + if conflict { + candidate.edits.clear(); + candidate.auto_fixable = false; + candidate + .message + .push_str(" (skipped: overlaps a higher-priority fix, re-run after applying)"); + } else { + accepted.extend(candidate.edits.iter().map(|e| e.span.clone())); + } + } +} + +#[cfg(test)] +mod tests { + use super::{plan_fix, prune_overlaps}; + use crate::core::Edit; + use crate::core::JmoveResult; + use crate::core::index::Index; + use crate::parser::FixCandidate; + use std::fs; + + fn rule_fixture(source: &str) -> JmoveResult { + let dir = tempfile::TempDir::new()?; + let file = dir.path().join("src/main/java/p/A.java"); + fs::create_dir_all(file.parent().unwrap())?; + fs::write(file, source)?; + Ok(dir) + } + + #[test] + fn plan_groups_candidates_and_filters_by_rule() -> JmoveResult<()> { + let dir = rule_fixture("package p;\n\nimport a.b.User;\n\nclass C {}\n")?; + let index = Index::build(dir.path())?; + let plan = plan_fix(&index, None); + assert_eq!(plan.total(), 1); + assert_eq!(plan.auto_edits().len(), 1); + // Filtering on a rule that exists keeps the candidate... + assert_eq!(plan_fix(&index, Some("java/unused-import")).total(), 1); + // ...and an unknown one yields an empty plan. + assert!(plan_fix(&index, Some("ts/unused-import")).is_empty()); + Ok(()) + } + + #[test] + fn clean_project_yields_empty_plan() -> JmoveResult<()> { + let dir = rule_fixture("package p;\n\nimport a.b.User;\n\nclass C { User u; }\n")?; + let index = Index::build(dir.path())?; + assert!(plan_fix(&index, None).is_empty()); + Ok(()) + } + + fn candidate( + rule: &'static str, + severity: crate::parser::Severity, + edits: Vec, + ) -> FixCandidate { + let span = edits.first().map_or(0..0, |e| e.span.clone()); + FixCandidate { + rule, + message: rule.to_owned(), + severity, + auto_fixable: !edits.is_empty(), + span, + edits, + candidates: Vec::new(), + } + } + + fn edit(start: usize, end: usize) -> Edit { + Edit { + span: start..end, + old_text: String::new(), + new_text: String::new(), + } + } + + #[test] + fn overlapping_lower_severity_candidate_is_deferred() { + use crate::parser::Severity; + let mut found = vec![ + candidate("order", Severity::Info, vec![edit(0, 40)]), + candidate("unused", Severity::Warning, vec![edit(10, 20)]), + ]; + prune_overlaps(&mut found); + // The urgent deletion survives; the whole-block rewrite waits. + assert!(found[1].auto_fixable); + assert!(!found[0].auto_fixable); + assert!(found[0].edits.is_empty()); + assert!(found[0].message.contains("skipped")); + } + + #[test] + fn boundary_touching_and_disjoint_edits_coexist() { + use crate::parser::Severity; + // Insertion exactly at the replaced block's end byte: not a + // conflict (the engine sorts stable and applies back-to-front). + let mut found = vec![ + candidate("order", Severity::Info, vec![edit(0, 40)]), + candidate("missing", Severity::Error, vec![edit(40, 40)]), + ]; + prune_overlaps(&mut found); + assert!(found.iter().all(|c| c.auto_fixable), "no deferral expected"); + } +} diff --git a/src/core/index/mod.rs b/src/core/index/mod.rs index 49dce0d..12965f9 100644 --- a/src/core/index/mod.rs +++ b/src/core/index/mod.rs @@ -45,6 +45,9 @@ pub struct Index { pub imports: HashMap>, /// Declared `package` of each Java file that has one. pub packages: HashMap, + /// FQN → file map for every indexed Java class; the fix rules use it + /// for candidate lookup (empty when the project has no Java sources). + pub java_classes: JavaClassIndex, } impl Index { @@ -57,6 +60,7 @@ impl Index { files: FileSet::default(), imports: HashMap::new(), packages: HashMap::new(), + java_classes: JavaClassIndex::default(), }; index.scan()?; // Resolution needs the complete file set (extension/index guessing) @@ -74,6 +78,7 @@ impl Index { }; } } + index.java_classes = java_classes; Ok(index) } diff --git a/src/core/mod.rs b/src/core/mod.rs index 7f02590..7d8279c 100644 --- a/src/core/mod.rs +++ b/src/core/mod.rs @@ -1,4 +1,4 @@ -//! Core engine: project indexing, dependency graph, move planning and +//! Core engine: project indexing, dependency graph, move/fix planning and //! atomic apply with rollback. //! //! Path convention used across the crate: every `PathBuf` produced by @@ -7,11 +7,13 @@ //! from OS walking. pub mod apply; +pub mod fix; pub mod index; pub mod plan; use std::ffi::OsStr; use std::io; +use std::ops::Range; use std::path::{Component, Path, PathBuf}; use thiserror::Error; @@ -31,11 +33,39 @@ pub enum JmoveError { /// The planned move cannot be applied safely. #[error("plan rejected: {0}")] PlanRejected(String), + /// A git operation (`git mv`) failed. + #[error("git error: {0}")] + Git(String), } /// Result alias used throughout the crate. pub type JmoveResult = Result; +/// One in-file byte-span edit: replaces the contents of `span` with +/// `new_text`, after verifying the bytes there still equal `old_text`. +/// +/// This is the shared currency of every edit generator: `mv` produces +/// specifier replacements, the `fix` command will produce line deletions +/// and insertions. Engine semantics: +/// - **replace**: non-empty `span`, `old_text` is the current span content; +/// - **insert**: empty span (`start == end`) with empty `old_text` — +/// `new_text` lands at `span.start`; +/// - **delete**: empty `new_text`; to drop a whole line the generator +/// extends the span over its trailing `\n` and puts the exact line bytes +/// in `old_text` (the engine itself never grows spans). +/// +/// The `old_text` check is the staleness guard: a plan whose file changed +/// since indexing is rejected before anything is written. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct Edit { + /// Byte range in the original file contents. + pub span: Range, + /// Exact bytes the span must currently hold (empty for insertions). + pub old_text: String, + /// Replacement bytes (empty for deletions). + pub new_text: String, +} + /// Normalize a project-relative path: strip `.` segments, collapse `..` /// where possible and reject paths that escape the project root. /// Returns `None` if the result would be empty, absolute or above the root. @@ -70,9 +100,76 @@ pub fn normalize_rel_path(path: &Path) -> Option { (!stack.is_empty()).then(|| stack.iter().collect::()) } +/// Format a project-relative path for anything that crosses the CLI +/// boundary (human messages, JSON payloads, diff headers, git pathspecs): +/// always `/`-separated, on every platform. `Path::display()` would leak +/// `\\` on Windows into texts where `/` is the contract — and git even +/// reads backslashes in pathspecs as escapes — so nothing user-facing +/// may use it for project-relative paths. +/// +/// # Examples +/// +/// ``` +/// use std::path::Path; +/// use jmove::core::rel_str; +/// +/// assert_eq!(rel_str(Path::new("src/main/java/A.java")), "src/main/java/A.java"); +/// ``` +#[must_use] +pub fn rel_str(path: &Path) -> String { + path.components() + .map(|c| c.as_os_str().to_string_lossy()) + .collect::>() + .join("/") +} + +/// Convert a user-supplied path to a normalized project-relative path. +/// Relative paths are taken against `root`; absolute ones must live +/// underneath it. Shared by CLI commands that accept paths. +/// +/// # Errors +/// +/// [`JmoveError::InvalidArgument`] when the path is outside `root` or +/// normalizes to nothing. +pub fn rel_from_root(root: &Path, path: &Path) -> JmoveResult { + let joined = if path.is_absolute() { + path.into() + } else { + root.join(path) + }; + let outside = || { + JmoveError::InvalidArgument(format!( + "path '{}' is outside the project root", + path.display() + )) + }; + let abs = collapse(&joined); + let rel = abs.strip_prefix(root).map_err(|_| outside())?; + normalize_rel_path(rel).ok_or_else(|| { + JmoveError::InvalidArgument(format!("invalid project path '{}'", path.display())) + }) +} + +/// Lexically normalize a path: drop `.` segments, apply `..` where possible. +fn collapse(path: &Path) -> PathBuf { + let mut stack: Vec> = Vec::new(); + for comp in path.components() { + match comp { + Component::CurDir => {} + Component::ParentDir => { + if stack.last() != Some(&Component::ParentDir) { + stack.pop(); + } + } + other => stack.push(other), + } + } + stack.into_iter().collect() +} + #[cfg(test)] mod tests { - use super::normalize_rel_path; + use super::{normalize_rel_path, rel_from_root, rel_str}; use std::path::{Path, PathBuf}; #[test] @@ -99,4 +196,29 @@ mod tests { fn rejects_absolute_paths() { assert_eq!(normalize_rel_path(Path::new("/etc/passwd")), None); } + + #[test] + fn rel_str_uses_forward_slashes() { + assert_eq!( + rel_str(Path::new("src/main/java/A.java")), + "src/main/java/A.java" + ); + assert_eq!(rel_str(Path::new("a.ts")), "a.ts"); + assert_eq!(rel_str(Path::new("")), ""); + } + + #[test] + fn rel_from_root_resolves_inside_and_rejects_outside() { + let root = Path::new("/tmp/proj"); + assert_eq!( + rel_from_root(root, Path::new("src/a.ts")).unwrap(), + PathBuf::from("src/a.ts") + ); + assert_eq!( + rel_from_root(root, Path::new("/tmp/proj/./src/../src/b.ts")).unwrap(), + PathBuf::from("src/b.ts") + ); + assert!(rel_from_root(root, Path::new("/elsewhere/x.ts")).is_err()); + assert!(rel_from_root(root, Path::new("../x.ts")).is_err()); + } } diff --git a/src/core/plan/java/mod.rs b/src/core/plan/java/mod.rs index 8d2e76c..e2264db 100644 --- a/src/core/plan/java/mod.rs +++ b/src/core/plan/java/mod.rs @@ -14,7 +14,7 @@ use std::path::{Path, PathBuf}; use crate::core::index::Index; use crate::core::plan::Rewrite; -use crate::core::{JmoveError, JmoveResult}; +use crate::core::{JmoveError, JmoveResult, rel_str}; /// Rewrite set for moving `source.java` to `target.java` (both /// project-relative, validated by the caller). @@ -28,7 +28,7 @@ pub(super) fn java_rewrites( if target.extension().is_none_or(|e| e != "java") { return Err(rejected(format!( "'{}' is a .java file, the target must keep the .java extension", - source.display() + rel_str(source) ))); } let sdir = source.parent().unwrap_or(Path::new("")); @@ -40,7 +40,7 @@ pub(super) fn java_rewrites( } else { Err(rejected(format!( "'{}' has no `package` declaration (default package); it can only be renamed inside its directory", - source.display() + rel_str(source) ))) }; }; @@ -50,15 +50,15 @@ pub(super) fn java_rewrites( if !sdir.ends_with(Path::new(&pkg_path)) { return Err(rejected(format!( "package '{pkg}' does not match directory '{}'", - sdir.display() + rel_str(sdir) ))); } let src_root = strip_package_dir(sdir, pkg); let rest = tdir.strip_prefix(&src_root).map_err(|_| { rejected(format!( "target directory '{}' is outside the Java source root '{}'", - tdir.display(), - src_root.display() + rel_str(tdir), + rel_str(&src_root) )) })?; let new_pkg = package_of(rest); @@ -124,7 +124,7 @@ fn file_stem(path: &Path) -> JmoveResult { .and_then(|s| s.to_str()) .map(str::to_owned) .ok_or_else(|| { - JmoveError::InvalidArgument(format!("invalid file name '{}'", path.display())) + JmoveError::InvalidArgument(format!("invalid file name '{}'", rel_str(path))) }) } diff --git a/src/core/plan/java/tests.rs b/src/core/plan/java/tests.rs index 037d0a7..135b954 100644 --- a/src/core/plan/java/tests.rs +++ b/src/core/plan/java/tests.rs @@ -69,7 +69,7 @@ fn move_between_packages_rewrites_package_and_all_importers() { rewrites .iter() .map(|r| ( - r.file.display().to_string(), + crate::core::rel_str(&r.file), r.old_text.clone(), r.new_text.clone() )) diff --git a/src/core/plan/mod.rs b/src/core/plan/mod.rs index 44126e5..4c0e580 100644 --- a/src/core/plan/mod.rs +++ b/src/core/plan/mod.rs @@ -15,7 +15,7 @@ use std::ops::Range; use std::path::{Path, PathBuf}; use crate::core::index::Index; -use crate::core::{JmoveError, JmoveResult, normalize_rel_path}; +use crate::core::{Edit, JmoveError, JmoveResult, normalize_rel_path, rel_str}; use crate::parser::SourceLanguage; /// One in-file replacement of an import specifier. Only the specifier text @@ -32,6 +32,16 @@ pub struct Rewrite { pub new_text: String, } +impl From<&Rewrite> for Edit { + fn from(rewrite: &Rewrite) -> Self { + Edit { + span: rewrite.span.clone(), + old_text: rewrite.old_text.clone(), + new_text: rewrite.new_text.clone(), + } + } +} + /// Complete plan for moving `source` to `target`. #[derive(Debug, Clone, PartialEq, Eq)] pub struct MovePlan { @@ -60,7 +70,7 @@ pub fn plan_move(index: &Index, source: &Path, target: &Path) -> JmoveResult JmoveResult = plan .rewrites .iter() - .map(|r| (r.file.display().to_string(), r.span.start)) + .map(|r| (crate::core::rel_str(&r.file), r.span.start)) .collect(); assert_eq!( keys, diff --git a/src/lib.rs b/src/lib.rs index dc2b79c..c0c3c23 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -1,14 +1,16 @@ -//! `jmove` — a project-aware file mover for TypeScript/JavaScript and Java. +//! `jmove` — a project-aware file mover and import fixer for +//! TypeScript/JavaScript and Java. //! //! Moving a file inside a project invalidates every relative import that //! points at it. `jmove` indexes the project's import graph, computes the //! minimal set of specifier rewrites, and applies everything atomically -//! (with rollback), optionally in dry-run mode. +//! (with rollback), optionally in dry-run mode. The same engine drives +//! `fix`, which auto-repairs small import problems via deterministic rules. //! //! Module map: //! - [`cli`] — argument parsing, command dispatch, user-facing output. -//! - [`core`] — indexing, dependency graph, move planning, atomic apply. -//! - [`parser`] — language frontends (import extraction, path resolution). +//! - [`core`] — indexing, dependency graph, move/fix planning, atomic apply. +//! - [`parser`] — language frontends (import extraction, path resolution, fix rules). //! - [`cache`] — on-disk index cache (Phase 2, intentionally empty for now). pub mod cache; diff --git a/src/parser/java/mod.rs b/src/parser/java/mod.rs index 277d48f..2fc62fc 100644 --- a/src/parser/java/mod.rs +++ b/src/parser/java/mod.rs @@ -12,6 +12,8 @@ use std::path::{Path, PathBuf}; use tree_sitter::{Node, Parser, Tree}; +pub mod rules; + use super::{ImportRecord, Language, PackageDecl, SourceLanguage}; use crate::core::index::FileSet; @@ -79,7 +81,12 @@ impl Language for TreeSitterJava { tree.root_node() .children(&mut cursor) .find(|child| child.kind() == "package_declaration") - .and_then(|decl| find_child_kind(decl, "scoped_identifier")) + // A dotted name parses as `scoped_identifier`; a single-segment + // package (`package p;`) has no dots and is a bare `identifier`. + .and_then(|decl| { + find_child_kind(decl, "scoped_identifier") + .or_else(|| find_child_kind(decl, "identifier")) + }) .map(|name| PackageDecl { name: text(name, source).to_owned(), span: name.start_byte()..name.end_byte(), @@ -105,12 +112,21 @@ fn record(node: Node, source: &str) -> ImportRecord { } } -fn text<'a>(node: Node, source: &'a str) -> &'a str { +fn text<'a>(node: Node<'a>, source: &'a str) -> &'a str { source .get(node.start_byte()..node.end_byte()) .unwrap_or_default() } +// Extend a statement end over exactly one `\n` or `\r\n` line terminator. +pub(super) fn line_end(source: &[u8], end: usize) -> usize { + match (source.get(end), source.get(end + 1)) { + (Some(b'\r'), Some(b'\n')) => end + 2, + (Some(b'\n'), _) => end + 1, + _ => end, + } +} + /// Map from every indexed Java class's declared FQN to its file. A file /// without a `package` declaration lives in the default package, which is /// un-importable, so it gets no entry. Collisions (two files declaring the @@ -118,6 +134,8 @@ fn text<'a>(node: Node, source: &'a str) -> &'a str { #[derive(Debug, Default)] pub struct JavaClassIndex { classes: HashMap, + // simple name -> sorted FQNs declaring it (missing-import lookups). + by_simple: HashMap>, } impl JavaClassIndex { @@ -136,7 +154,18 @@ impl JavaClassIndex { .entry(format!("{}.{stem}", decl.name)) .or_insert(file); } - Self { classes } + let mut by_simple: HashMap> = HashMap::new(); + for fqn in classes.keys() { + let simple = fqn.rsplit('.').next().unwrap_or(fqn); + by_simple + .entry(simple.to_owned()) + .or_default() + .push(fqn.clone()); + } + for fqns in by_simple.values_mut() { + fqns.sort(); + } + Self { classes, by_simple } } /// Resolve a Java import `specifier` to an indexed file. Exact FQN @@ -152,95 +181,14 @@ impl JavaClassIndex { let owner = specifier.rsplit_once('.')?.0; self.classes.get(owner).map(PathBuf::as_path) } + + /// Every indexed FQN whose simple name is `simple`, sorted. Empty for + /// jdk/third-party names, which the index never contains. + #[must_use] + pub fn candidates(&self, simple: &str) -> &[String] { + self.by_simple.get(simple).map_or(&[], Vec::as_slice) + } } #[cfg(test)] -mod tests { - use super::*; - - fn imports(source: &str) -> Vec { - TreeSitterJava::new().extract_imports(source) - } - - #[test] - fn extracts_package_with_exact_span() { - let src = "package com.example.utils;\n\npublic class P {}\n"; - let decl = TreeSitterJava::new().extract_package(src).expect("package"); - assert_eq!(decl.name, "com.example.utils"); - assert_eq!(&src[decl.span.clone()], "com.example.utils"); - } - - #[test] - fn package_in_comment_is_not_captured() { - let src = "// package com.example;\npublic class P {}\n"; - assert!(TreeSitterJava::new().extract_package(src).is_none()); - } - - #[test] - fn extracts_single_type_and_static_imports() { - let src = "package p;\nimport a.b.User;\nimport static a.b.User.create;\n"; - let recs = imports(src); - assert_eq!(recs.len(), 2); - assert_eq!(recs[0].specifier, "a.b.User"); - assert_eq!(&src[recs[0].span.clone()], "a.b.User"); - assert_eq!(recs[1].specifier, "a.b.User.create"); - assert_eq!(&src[recs[1].span.clone()], "a.b.User.create"); - assert!(recs.iter().all(|r| !r.is_dynamic)); - } - - #[test] - fn on_demand_imports_are_not_extracted() { - let src = "package p;\nimport java.util.*;\nimport a.b.User;\n"; - let recs = imports(src); - assert_eq!(recs.len(), 1); - assert_eq!(recs[0].specifier, "a.b.User"); - } - - #[test] - fn imports_inside_nested_types_are_still_top_level() { - let src = "package p;\nclass A { }\nimport q.B;\n"; - assert_eq!(imports(src).len(), 1); - } - - #[test] - fn class_index_maps_package_and_stem() { - let mut files = FileSet::default(); - files.add(PathBuf::from("src/main/java/com/example/utils/Parser.java")); - files.add(PathBuf::from("src/Main.java")); - let mut packages = HashMap::new(); - packages.insert( - PathBuf::from("src/main/java/com/example/utils/Parser.java"), - PackageDecl { - name: "com.example.utils".into(), - span: 0..0, - }, - ); - let classes = JavaClassIndex::new(&files, &packages); - assert_eq!( - classes.resolve("com.example.utils.Parser"), - Some(Path::new("src/main/java/com/example/utils/Parser.java")) - ); - // default package: un-importable, no entry - assert_eq!(classes.classes.len(), 1); - } - - #[test] - fn resolve_handles_exact_and_member_imports() { - let mut classes = HashMap::new(); - classes.insert( - "com.example.Parser".to_owned(), - PathBuf::from("src/com/example/Parser.java"), - ); - let index = JavaClassIndex { classes }; - assert_eq!( - index.resolve("com.example.Parser"), - Some(Path::new("src/com/example/Parser.java")) - ); - assert_eq!( - index.resolve("com.example.Parser.parse"), - Some(Path::new("src/com/example/Parser.java")) - ); - assert_eq!(index.resolve("java.util.List"), None); - assert_eq!(index.resolve("Parser"), None); - } -} +mod tests; diff --git a/src/parser/java/rules/import_order/mod.rs b/src/parser/java/rules/import_order/mod.rs new file mode 100644 index 0000000..9b78169 --- /dev/null +++ b/src/parser/java/rules/import_order/mod.rs @@ -0,0 +1,147 @@ +//! Java rule: normalise the import block to Google order — static imports +//! first, then single-type imports, each group ASCII-sorted, duplicates +//! dropped, one blank line between the groups. +//! +//! The repair is a single replace-span over the whole (contiguous) import +//! block, so `old_text` doubles as the staleness guard: if anything moved +//! between index and apply, the engine rejects before writing. +//! +//! Bail-out rules keep the direction of the other Java rules — never +//! rewrite something the rule cannot fully account for: fewer than two +//! imports is never dirty; a block containing comments or any non-import +//! statement is left alone (attached comments must not be orphaned by a +//! re-sort); an already-normalised block produces no candidate at all. + +use std::path::Path; + +use tree_sitter::Node; + +use super::is_static_line; +use crate::core::Edit; +use crate::core::index::Index; +use crate::parser::java::{TreeSitterJava, line_end, text}; +use crate::parser::{Fix, FixCandidate, Severity}; + +/// Rule id accepted by `jmove fix --rule`. +pub const RULE: &str = "java/import-order"; + +/// Import block orderer. +pub struct JavaImportOrder; + +impl JavaImportOrder { + /// Create the rule. + #[must_use] + pub fn new() -> Self { + Self + } +} + +impl Default for JavaImportOrder { + fn default() -> Self { + Self::new() + } +} + +impl Fix for JavaImportOrder { + fn rule(&self) -> &'static str { + RULE + } + + fn fixes(&self, _path: &Path, source: &str, _index: &Index) -> Vec { + let Some(tree) = TreeSitterJava::parse(source) else { + return Vec::new(); + }; + let root = tree.root_node(); + let imports = import_nodes(root); + if imports.len() < 2 { + return Vec::new(); + } + let (start, end) = ( + imports[0].start_byte(), + line_end(source.as_bytes(), imports[imports.len() - 1].end_byte()), + ); + let block = &source[start..end]; + if dirty_neighbours(root, start, end) { + return Vec::new(); // comments or foreign statements inside: hands off + } + let lines: Vec<&str> = imports.iter().map(|n| text(*n, source)).collect(); + let target = normalised(&lines, newline_of(block)); + if target == block { + return Vec::new(); + } + let total = lines.len(); + vec![FixCandidate { + rule: RULE, + message: format!("{} imports are not in google order", total), + severity: Severity::Info, + auto_fixable: true, + span: start..end, + edits: vec![Edit { + span: start..end, + old_text: block.to_owned(), + new_text: target, + }], + candidates: Vec::new(), + }] + } +} + +// Top-level import statements, in source order. +fn import_nodes(root: Node) -> Vec { + let mut cursor = root.walk(); + root.children(&mut cursor) + .filter(|child| child.kind() == "import_declaration") + .collect() +} + +// Any root child inside the block that is not one of the imports (the +// walk stops at the statement containing the span, so comments — which +// hang off their statement — still register on the enclosing node). +fn dirty_neighbours(root: Node, start: usize, end: usize) -> bool { + let mut cursor = root.walk(); + root.children(&mut cursor).any(|child| { + child.kind() != "import_declaration" + && child.start_byte() < end + && child.end_byte() > start + && overlaps_import_line(child, start, end) + }) +} + +// The package declaration legally precedes the block; statements that +// merely abut it (zero-gap) are not inside it. +fn overlaps_import_line(child: Node, start: usize, end: usize) -> bool { + child.start_byte() >= start && child.end_byte() <= end +} + +// The sortable identity of an import: its path, without the leading +// `import [static] ` boilerplate. A free fn (not a closure) so the +// returned `&str` keeps the input's lifetime. +fn sort_key(line: &str) -> &str { + line.strip_prefix("import static ") + .or_else(|| line.strip_prefix("import ")) + .unwrap_or(line) +} + +// Google order: statics first, blank line, then single-type imports; +// both groups ASCII-sorted. +fn normalised(lines: &[&str], nl: &str) -> String { + let (mut statics, mut types): (Vec<&str>, Vec<&str>) = + lines.iter().copied().partition(|l| is_static_line(l)); + statics.sort_unstable_by_key(|l| sort_key(l)); + types.sort_unstable_by_key(|l| sort_key(l)); + statics.dedup(); + types.dedup(); + let mut out: Vec<&str> = statics; + if !out.is_empty() && !types.is_empty() { + out.push(""); + } + out.extend(types); + format!("{}{nl}", out.join(nl)) +} + +fn newline_of(block: &str) -> &'static str { + if block.contains("\r\n") { "\r\n" } else { "\n" } +} + +#[cfg(test)] +mod tests; diff --git a/src/parser/java/rules/import_order/tests.rs b/src/parser/java/rules/import_order/tests.rs new file mode 100644 index 0000000..2058cb2 --- /dev/null +++ b/src/parser/java/rules/import_order/tests.rs @@ -0,0 +1,77 @@ +use super::JavaImportOrder; +use crate::core::index::Index; +use crate::parser::Fix; +use std::path::Path; + +fn fix(source: &str) -> Vec { + JavaImportOrder::new().fixes(Path::new("A.java"), source, &Index::default()) +} + +#[test] +fn statics_move_first_and_groups_sort_by_ascii() { + let src = "package p;\n\nimport java.util.List;\nimport com.example.Text;\nimport static java.lang.Math.PI;\nimport java.util.Map;\n\nclass C {}\n"; + let found = fix(src); + assert_eq!(found.len(), 1); + assert!(found[0].auto_fixable); + assert_eq!(found[0].severity, crate::parser::Severity::Info); + let edit = &found[0].edits[0]; + assert_eq!( + edit.new_text, + "import static java.lang.Math.PI;\n\nimport com.example.Text;\nimport java.util.List;\nimport java.util.Map;\n" + ); + // The replace span is exactly the old block: the staleness guard. + assert_eq!(&src[edit.span.clone()], edit.old_text); +} + +#[test] +fn sorted_and_single_blocks_produce_nothing() { + let src = "import static a.A;\n\nimport a.B;\nimport b.C;\n\nclass D {}\n"; + assert!(fix(src).is_empty()); + // One import can never be out of order. + assert!(fix("import a.B;\nclass D {}\n").is_empty()); +} + +#[test] +fn duplicates_collapse_and_only_dupes_change_the_block() { + let src = "import a.B;\nimport a.B;\nimport a.C;\n\nclass D {}\n"; + let found = fix(src); + assert_eq!(found.len(), 1, "dedup alone is a change"); + assert_eq!(found[0].edits[0].new_text, "import a.B;\nimport a.C;\n"); +} + +#[test] +fn comments_and_foreign_statements_abort_the_rewrite() { + // Comment attached inside the block: re-sorting would orphan it. + let src = "import b.B;\n// keep first\nimport a.A;\n\nclass D {}\n"; + assert!(fix(src).is_empty()); + // A type declaration wedged between imports: not a clean block. + let src = "import b.B;\nclass Mid {}\nimport a.A;\n"; + assert!(fix(src).is_empty()); +} + +#[test] +fn crlf_blocks_stay_crlf() { + let src = "import b.B;\r\nimport a.A;\r\n\r\nclass D {}\r\n"; + let found = fix(src); + assert_eq!(found.len(), 1); + assert_eq!(found[0].edits[0].new_text, "import a.A;\r\nimport b.B;\r\n"); +} + +#[test] +fn sorting_applies_to_the_whole_statement_after_the_keyword() { + // `java` sorts before `javax` by raw ASCII, matching Checkstyle. + let src = "import javax.swing.JDialog;\nimport java.util.List;\n\nclass D {}\n"; + let found = fix(src); + assert_eq!( + found[0].edits[0].new_text, + "import java.util.List;\nimport javax.swing.JDialog;\n" + ); +} + +#[test] +fn duplicates_far_apart_still_collapse() { + let src = "import a.B;\nimport a.C;\nimport a.B;\n\nclass D {}\n"; + let found = fix(src); + assert_eq!(found.len(), 1); + assert_eq!(found[0].edits[0].new_text, "import a.B;\nimport a.C;\n"); +} diff --git a/src/parser/java/rules/missing_imports/mod.rs b/src/parser/java/rules/missing_imports/mod.rs new file mode 100644 index 0000000..d034ff0 --- /dev/null +++ b/src/parser/java/rules/missing_imports/mod.rs @@ -0,0 +1,242 @@ +//! Java rule: add the missing `import` of a project class that is used by +//! its simple name. +//! +//! Motivation (guava smoke test): after `mv` moves a `.java` file into a +//! new package, its unqualified references to former same-package siblings +//! stop resolving — the compiler needs an explicit import. The rule adds +//! one only when the FQN index proves a single candidate; ambiguity is +//! reported as non-auto `candidates` for an agent to resolve, never +//! guessed at. +//! +//! Safety mirrors [`super::super::unused_imports`] from the other side: adding an +//! import can only be harmless-or-wrong, so the wrong case is fenced off +//! structurally — dotted chains (`a.b.Foo`) and files with on-demand +//! imports never auto-fix, same-package siblings are skipped (they resolve +//! without an import), and names declared in or imported by the file are +//! invisible to the rule. Comments and strings produce no AST nodes, so a +//! mention there cannot trigger an insertion. + +use std::collections::HashSet; +use std::ops::Range; +use std::path::Path; + +use tree_sitter::Node; + +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}; + +/// Rule id accepted by `jmove fix --rule`. +pub const RULE: &str = "java/missing-import"; + +/// Missing-import adder for Java sources. +pub struct JavaMissingImports; + +impl JavaMissingImports { + /// Create the rule. + #[must_use] + pub fn new() -> Self { + Self + } +} + +impl Default for JavaMissingImports { + fn default() -> Self { + Self::new() + } +} + +// What the file already makes resolvable without a new import, plus the +// insertion anchor for the import block. +#[derive(Default)] +struct Scope { + declared: HashSet, + imported: HashSet, + wildcard: bool, + last_import: usize, + package_line: usize, + package_name: Option, +} + +impl Fix for JavaMissingImports { + fn rule(&self) -> &'static str { + RULE + } + + fn fixes(&self, _path: &Path, source: &str, index: &Index) -> Vec { + let Some(tree) = TreeSitterJava::parse(source) else { + return Vec::new(); + }; + let mut scope = Scope::default(); + collect_scope(tree.root_node(), source, &mut scope); + let mut refs: Vec<(String, Range)> = Vec::new(); + collect_refs(tree.root_node(), source, &scope, &mut refs); + // After the last import; else after the package line; else at the + // head of the file. Insertions at one byte keep candidate order. + let anchor = scope.last_import.max(scope.package_line); + refs.iter() + .filter_map(|(name, span)| candidate(name, span, anchor, &scope, index)) + .collect() + } +} + +// One candidate per unresolved name: an auto insertion when the index +// proves a unique importable FQN, else a manual report for the agent. +fn candidate( + name: &str, + span: &Range, + anchor: usize, + scope: &Scope, + index: &Index, +) -> Option { + let cands = index.java_classes.candidates(name); + if cands.is_empty() { + return None; // jdk/third-party name: the index cannot invent an import + } + // The file's own declared package wins over the index: `mv` may have + // rewritten it after the index snapshot, and the source is the truth. + if scope + .package_name + .as_ref() + .is_some_and(|pkg| cands.iter().any(|fqn| fqn == &format!("{pkg}.{name}"))) + { + return None; // same-package sibling resolves without an import + } + if cands.len() == 1 && !scope.wildcard { + let fqn = &cands[0]; + return Some(FixCandidate { + rule: RULE, + message: format!("missing import '{fqn}' for type '{name}'"), + severity: Severity::Error, + auto_fixable: true, + span: span.clone(), + edits: vec![Edit { + span: anchor..anchor, + old_text: String::new(), + new_text: format!("import {fqn};\n"), + }], + candidates: Vec::new(), + }); + } + let why = if scope.wildcard { + "on-demand imports may already resolve it" + } else { + "the index holds several classes with this name" + }; + Some(FixCandidate { + rule: RULE, + message: format!( + "type '{name}' needs an import: {why} ({})", + cands.join(", ") + ), + severity: Severity::Error, + auto_fixable: false, + span: span.clone(), + edits: Vec::new(), + candidates: cands.to_vec(), + }) +} + +// Pass 1: the names the file already resolves, and the import-block end. +fn collect_scope(node: Node, source: &str, scope: &mut Scope) { + match node.kind() { + "import_declaration" => { + if has_child_kind(node, "asterisk") { + scope.wildcard = true; + } else if let Some(spec) = find_child_kind(node, "scoped_identifier") + .or_else(|| find_child_kind(node, "identifier")) + { + let simple = text(spec, source).rsplit('.').next().unwrap_or_default(); + scope.imported.insert(simple.to_owned()); + } + scope.last_import = line_end(source.as_bytes(), node.end_byte()); + } + "package_declaration" => { + scope.package_line = line_end(source.as_bytes(), node.end_byte()); + scope.package_name = find_child_kind(node, "scoped_identifier") + .or_else(|| find_child_kind(node, "identifier")) + .map(|n| text(n, source).to_owned()); + } + "class_declaration" + | "interface_declaration" + | "enum_declaration" + | "record_declaration" + | "annotation_type_declaration" + | "type_parameter" => { + if let Some(name) = declared_name(node, source) { + scope.declared.insert(name); + } + } + _ => {} + } + let mut cursor = node.walk(); + for child in node.children(&mut cursor) { + collect_scope(child, source, scope); + } +} + +// The declaration's own name node (`type_parameter` keeps it unnamed). +fn declared_name(node: Node, source: &str) -> Option { + let name = node + .child_by_field_name("name") + .or_else(|| find_child_kind(node, "type_identifier"))?; + Some(text(name, source).to_owned()) +} + +// Positions whose `name` field is a member/declarator, not a type use. +const MEMBER_NAME: &[&str] = &[ + "method_declaration", + "constructor_declaration", + "variable_declarator", + "method_invocation", + "field_access", + "enum_constant", +]; + +// Pass 2: bare capitalized identifier/type_identifier occurrences. +// Dotted chains, package and import statements resolve on their own and +// are never descended into. +fn collect_refs(node: Node, source: &str, scope: &Scope, refs: &mut Vec<(String, Range)>) { + match node.kind() { + "import_declaration" + | "package_declaration" + | "scoped_identifier" + | "scoped_type_identifier" => return, + "identifier" | "type_identifier" if !is_member_name(node) => { + push_ref(text(node, source), node.byte_range(), scope, refs); + } + _ => {} + } + let mut cursor = node.walk(); + for child in node.children(&mut cursor) { + collect_refs(child, source, scope, refs); + } +} + +fn is_member_name(node: Node) -> bool { + let Some(parent) = node.parent() else { + return false; + }; + if !MEMBER_NAME.contains(&parent.kind()) { + return false; + } + parent + .child_by_field_name("name") + .is_some_and(|n| n.start_byte() == node.start_byte() && n.end_byte() == node.end_byte()) +} + +// Keep the first occurrence of each new capitalized, unresolved name. +fn push_ref(name: &str, span: Range, scope: &Scope, refs: &mut Vec<(String, Range)>) { + if !name.chars().next().is_some_and(char::is_uppercase) + || scope.declared.contains(name) + || scope.imported.contains(name) + || refs.iter().any(|(seen, _)| seen == name) + { + return; + } + refs.push((name.to_owned(), span)); +} + +#[cfg(test)] +mod tests; diff --git a/src/parser/java/rules/missing_imports/tests.rs b/src/parser/java/rules/missing_imports/tests.rs new file mode 100644 index 0000000..4db9bd6 --- /dev/null +++ b/src/parser/java/rules/missing_imports/tests.rs @@ -0,0 +1,121 @@ +//! Tests for the `java/missing-import` rule (child module: sees the +//! rule's private helpers through `super`). + +use super::JavaMissingImports; +use crate::core::index::Index; +use crate::parser::Fix; +use std::fs; +use std::path::Path; + +// Index built from `rel -> body` files; the tempdir guard must stay +// alive as long as the returned index. +fn index_of(files: &[(&str, &str)]) -> (tempfile::TempDir, Index) { + let dir = tempfile::TempDir::new().expect("tempdir"); + for (rel, body) in files { + let path = dir.path().join(rel); + fs::create_dir_all(path.parent().unwrap()).expect("mkdir"); + fs::write(path, body).expect("write"); + } + let index = Index::build(dir.path()).expect("index"); + (dir, index) +} + +fn fix(rel: &str, source: &str, index: &Index) -> Vec { + JavaMissingImports::new().fixes(Path::new(rel), source, index) +} + +const G_CLASS: &str = "package a.b;\n\npublic class G {}\n"; + +#[test] +fn unique_candidate_gets_an_inserted_import_after_the_package() { + let (_dir, index) = index_of(&[("src/a/b/G.java", G_CLASS)]); + let src = "package p;\n\nclass C { G g; }\n"; + let found = fix("src/p/C.java", src, &index); + assert_eq!(found.len(), 1); + assert!(found[0].auto_fixable); + assert_eq!(found[0].edits[0].new_text, "import a.b.G;\n"); + assert_eq!( + found[0].edits[0].span, + src.find("\n\nclass").unwrap() + 1..src.find("\n\nclass").unwrap() + 1 + ); + assert_eq!(found[0].message, "missing import 'a.b.G' for type 'G'"); +} + +#[test] +fn insertion_lands_after_the_last_existing_import() { + let (_dir, index) = index_of(&[("src/a/b/G.java", G_CLASS)]); + let src = "package p;\nimport x.Y;\n\nclass C { G g; Y y; }\n"; + let found = fix("src/p/C.java", src, &index); + assert_eq!(found.len(), 1); + assert_eq!( + &src[..found[0].edits[0].span.start], + "package p;\nimport x.Y;\n" + ); +} + +#[test] +fn same_package_declared_imported_and_jdk_names_never_fix() { + let (_dir, index) = index_of(&[ + ("src/a/b/G.java", G_CLASS), + ( + "src/p/Sibling.java", + "package p;\npublic class Sibling {}\n", + ), + ]); + // Same-package sibling: resolves without an import. + let src = "package p;\nclass C { Sibling s; }\n"; + assert!(fix("src/p/C.java", src, &index).is_empty()); + // Declared in the file itself, already imported, lowercase method: + // none of them is a missing type. + let src = "package p;\nimport a.b.G;\nclass C { G g; H h; void g() {} }\nclass H {}\n"; + assert!(fix("src/p/C.java", src, &index).is_empty()); + // `java.util.List`: not in the class index at all. + let src = "package p;\nimport java.util.List;\nclass C { List l; }\n"; + assert!(fix("src/p/C.java", src, &index).is_empty()); +} + +#[test] +fn ambiguous_and_wildcard_findings_are_manual_with_candidates() { + let (_dir, index) = index_of(&[ + ("src/a/b/G.java", G_CLASS), + ("src/z/G.java", "package z;\npublic class G {}\n"), + ("src/u/U.java", "package u;\npublic class U {}\n"), + ]); + // Two indexed `G`s: the agent must choose. + let src = "package p;\nclass C { G g; }\n"; + let found = fix("src/p/C.java", src, &index); + assert_eq!(found.len(), 1); + assert!(!found[0].auto_fixable); + assert!(found[0].edits.is_empty()); + assert_eq!(found[0].candidates, ["a.b.G".to_owned(), "z.G".to_owned()]); + // A unique name is still manual when a `pkg.*` may shadow it. + let src = "package p;\nimport q.*;\nclass C { U u; }\n"; + let found = fix("src/p/C.java", src, &index); + assert_eq!(found.len(), 1); + assert!(!found[0].auto_fixable); + assert_eq!(found[0].candidates, ["u.U".to_owned()]); +} + +#[test] +fn annotations_static_uses_and_crlf_stay_bare_references() { + let (_dir, index) = index_of(&[("src/a/b/G.java", G_CLASS)]); + let src = "package p;\r\n\r\nclass C {\r\n @G void m() { G.make(); }\r\n}\r\n"; + let found = fix("src/p/C.java", src, &index); + assert_eq!(found.len(), 1, "one candidate per name, not per use"); + assert!(found[0].auto_fixable); + // Qualified uses need no import: the dotted chain is skipped. + let src = "package p;\nclass C { a.b.G g; }\n"; + assert!(fix("src/p/C.java", src, &index).is_empty()); +} + +#[test] +fn moved_file_gets_its_sibling_import_like_guava() { + // The guava failure shape: `H` moved out of `a.b`, still referring + // to sibling `G` bare. Index says G lives in a.b exactly once. + let (_dir, index) = index_of(&[("src/a/b/G.java", G_CLASS)]); + let src = "package a.c;\n\nclass H { G wrap() { return null; } }\n"; + let found = fix("src/a/c/H.java", src, &index); + assert_eq!(found.len(), 1); + assert!(found[0].auto_fixable); + assert_eq!(found[0].edits[0].new_text, "import a.b.G;\n"); +} diff --git a/src/parser/java/rules/mod.rs b/src/parser/java/rules/mod.rs new file mode 100644 index 0000000..fd762f8 --- /dev/null +++ b/src/parser/java/rules/mod.rs @@ -0,0 +1,25 @@ +//! Deterministic Java fix rules (`java/unused-import`, `java/missing-import`, +//! `java/import-order`). Each rule implements +//! [`Fix`](crate::parser::Fix) and reports candidates for the shared +//! dry-run/atomic engine; frontend helpers live in [`super`]. + +pub mod import_order; +pub mod missing_imports; +pub mod unused_imports; + +use tree_sitter::Node; + +use crate::parser::java::text; + +/// `import static ...;` — the grammar keeps `static` as an anonymous token, +/// so the statement text is the cheapest reliable check. +#[must_use] +pub(super) fn is_static(node: Node, source: &str) -> bool { + is_static_line(text(node, source)) +} + +/// The same check on already-extracted statement text. +#[must_use] +pub(super) fn is_static_line(line: &str) -> bool { + line.starts_with("import static ") +} diff --git a/src/parser/java/rules/unused_imports.rs b/src/parser/java/rules/unused_imports.rs new file mode 100644 index 0000000..63e412f --- /dev/null +++ b/src/parser/java/rules/unused_imports.rs @@ -0,0 +1,194 @@ +//! Java rule: drop single-type imports whose simple name is never used. +//! +//! Detection is a raw word-boundary scan over the whole file (comments and +//! strings included), never an AST usage analysis. That asymmetry is the +//! safety property: a false "still used" verdict keeps one dead import +//! (harmless), while a wrong "unused" verdict deletes live code. So a type +//! referenced only in javadoc or a string literal keeps its import, and an +//! import whose name appears in a *sibling import* (e.g. the static member +//! import of the same type) is kept too. +//! +//! The deletion edit covers the whole statement plus its line ending +//! (CRLF-aware); leading indentation is not absorbed — Java imports sit at +//! 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; + +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}; + +/// Rule id accepted by `jmove fix --rule`. +pub const RULE: &str = "java/unused-import"; + +/// Unused single-type import remover. +pub struct JavaUnusedImports; + +impl JavaUnusedImports { + /// Create the rule. + #[must_use] + pub fn new() -> Self { + Self + } +} + +impl Default for JavaUnusedImports { + fn default() -> Self { + Self::new() + } +} + +impl Fix for JavaUnusedImports { + fn rule(&self) -> &'static str { + RULE + } + + fn fixes(&self, _path: &Path, source: &str, _index: &Index) -> Vec { + let Some(tree) = TreeSitterJava::parse(source) else { + return Vec::new(); + }; + let mut cursor = tree.root_node().walk(); + tree.root_node() + .children(&mut cursor) + .filter(|node| node.kind() == "import_declaration") + .filter(|node| !has_child_kind(*node, "asterisk")) // `pkg.*` + .filter_map(|node| unused_import(node, source)) + .collect() + } +} + +// One candidate when the import's used-name(s) occur nowhere else. +fn unused_import(node: Node, source: &str) -> Option { + let path = find_child_kind(node, "scoped_identifier")?; + let specifier = text(path, source); + let names: Vec<&str> = if is_static(node, source) { + // `a.b.User.create` is used via `create(...)` or `User.create(...)`. + let mut segments = specifier.rsplitn(2, '.'); + let member = segments.next()?; + let owner = segments.next()?.rsplit('.').next()?; + vec![member, owner] + } else { + vec![specifier.rsplit('.').next()?] + }; + let skip = node.start_byte()..node.end_byte(); + if names + .iter() + .any(|name| word_occurs(source.as_bytes(), name.as_bytes(), &skip)) + { + return None; + } + let span = skip.start..line_end(source.as_bytes(), skip.end); + let old_text = source[span.clone()].to_owned(); + Some(FixCandidate { + rule: RULE, + message: format!("unused import '{specifier}'"), + severity: Severity::Warning, + auto_fixable: true, + span: span.clone(), + edits: vec![Edit { + span, + old_text, + new_text: String::new(), + }], + candidates: Vec::new(), + }) +} + +// `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}; + use crate::core::index::Index; + use crate::parser::{Fix, Severity}; + use std::path::Path; + + fn candidates(source: &str) -> Vec { + JavaUnusedImports::new().fixes(Path::new("A.java"), source, &Index::default()) + } + + #[test] + fn unused_import_is_deleted_with_its_line() { + let src = "package p;\n\nimport a.b.User;\n\nclass C { int u; }\n"; + let found = candidates(src); + assert_eq!(found.len(), 1); + let candidate = &found[0]; + assert_eq!(candidate.rule, RULE); + assert!(candidate.auto_fixable); + assert_eq!(candidate.severity, Severity::Warning); + assert_eq!(candidate.message, "unused import 'a.b.User'"); + let edit = &candidate.edits[0]; + assert_eq!(edit.new_text, ""); + assert_eq!(&src[edit.span.clone()], "import a.b.User;\n"); + } + + #[test] + fn used_types_annotations_and_words_are_kept() { + let src = "import a.b.User;\nclass C implements User { @User Ann u; }"; + assert!(candidates(src).is_empty()); + // `UserFactory` is a different token: it does not keep `User`. + let src = "import a.b.User;\nclass C { UserFactory f; }"; + assert_eq!(candidates(src).len(), 1); + // A javadoc/string mention keeps the import (safe direction). + let src = "import a.b.User;\n/** see {@link User} */ class C {}"; + assert!(candidates(src).is_empty()); + } + + #[test] + fn static_imports_check_member_and_owner_names() { + // `create` used directly. + let src = "import static a.b.User.create;\nclass C { auto x = create(); }"; + assert!(candidates(src).is_empty()); + // `User` used as the qualifier. + let src = "import static a.b.User.create;\nclass C { auto x = User.create(); }"; + assert!(candidates(src).is_empty()); + // neither appears: the import goes. + let src = "import static a.b.User.create;\nclass C {}"; + assert_eq!(candidates(src).len(), 1); + } + + #[test] + fn sibling_import_mention_keeps_the_type_import() { + let src = "import a.b.User;\nimport static a.b.User.create;\nclass C {}"; + // Each statement sees the other's `User`; neither is provably dead. + assert!(candidates(src).is_empty()); + } + + #[test] + fn wildcards_and_crlf_are_handled() { + let src = "import a.b.*;\nclass C {}"; + assert!(candidates(src).is_empty()); + let src = "import a.b.User;\r\nclass C {}"; + let found = candidates(src); + assert_eq!(found.len(), 1); + assert_eq!(&src[found[0].edits[0].span.clone()], "import a.b.User;\r\n"); + } +} diff --git a/src/parser/java/tests.rs b/src/parser/java/tests.rs new file mode 100644 index 0000000..bd68d70 --- /dev/null +++ b/src/parser/java/tests.rs @@ -0,0 +1,105 @@ +//! Tests for the Java frontend and the FQN class index. + +use super::*; + +fn imports(source: &str) -> Vec { + TreeSitterJava::new().extract_imports(source) +} + +#[test] +fn extracts_package_with_exact_span() { + let src = "package com.example.utils;\n\npublic class P {}\n"; + let decl = TreeSitterJava::new().extract_package(src).expect("package"); + assert_eq!(decl.name, "com.example.utils"); + assert_eq!(&src[decl.span.clone()], "com.example.utils"); +} + +#[test] +fn single_segment_package_is_not_a_scoped_identifier() { + // `package p;` has no dots: the grammar yields a bare `identifier`. + let src = "package p;\n\npublic class P {}\n"; + let decl = TreeSitterJava::new().extract_package(src).expect("package"); + assert_eq!(decl.name, "p"); + assert_eq!(&src[decl.span.clone()], "p"); +} + +#[test] +fn package_in_comment_is_not_captured() { + let src = "// package com.example;\npublic class P {}\n"; + assert!(TreeSitterJava::new().extract_package(src).is_none()); +} + +#[test] +fn extracts_single_type_and_static_imports() { + let src = "package p;\nimport a.b.User;\nimport static a.b.User.create;\n"; + let recs = imports(src); + assert_eq!(recs.len(), 2); + assert_eq!(recs[0].specifier, "a.b.User"); + assert_eq!(&src[recs[0].span.clone()], "a.b.User"); + assert_eq!(recs[1].specifier, "a.b.User.create"); + assert_eq!(&src[recs[1].span.clone()], "a.b.User.create"); + assert!(recs.iter().all(|r| !r.is_dynamic)); +} + +#[test] +fn on_demand_imports_are_not_extracted() { + let src = "package p;\nimport java.util.*;\nimport a.b.User;\n"; + let recs = imports(src); + assert_eq!(recs.len(), 1); + assert_eq!(recs[0].specifier, "a.b.User"); +} + +#[test] +fn imports_inside_nested_types_are_still_top_level() { + let src = "package p;\nclass A { }\nimport q.B;\n"; + assert_eq!(imports(src).len(), 1); +} + +#[test] +fn class_index_maps_package_and_stem() { + let mut files = FileSet::default(); + files.add(PathBuf::from("src/main/java/com/example/utils/Parser.java")); + files.add(PathBuf::from("src/Main.java")); + let mut packages = HashMap::new(); + packages.insert( + PathBuf::from("src/main/java/com/example/utils/Parser.java"), + PackageDecl { + name: "com.example.utils".into(), + span: 0..0, + }, + ); + let classes = JavaClassIndex::new(&files, &packages); + assert_eq!( + classes.resolve("com.example.utils.Parser"), + Some(Path::new("src/main/java/com/example/utils/Parser.java")) + ); + // default package: un-importable, no entry + assert_eq!(classes.classes.len(), 1); +} + +#[test] +fn resolve_handles_exact_and_member_imports() { + let mut classes = HashMap::new(); + classes.insert( + "com.example.Parser".to_owned(), + PathBuf::from("src/com/example/Parser.java"), + ); + let mut by_simple = HashMap::new(); + by_simple.insert("Parser".to_owned(), vec!["com.example.Parser".to_owned()]); + let index = JavaClassIndex { classes, by_simple }; + assert_eq!( + index.resolve("com.example.Parser"), + Some(Path::new("src/com/example/Parser.java")) + ); + assert_eq!( + index.resolve("com.example.Parser.parse"), + Some(Path::new("src/com/example/Parser.java")) + ); + assert_eq!(index.resolve("java.util.List"), None); + assert_eq!(index.resolve("Parser"), None); + assert_eq!( + index.candidates("Parser"), + ["com.example.Parser".to_owned()].as_slice() + ); + assert!(index.candidates("List").is_empty()); +} diff --git a/src/parser/mod.rs b/src/parser/mod.rs index 69cd8e6..5c8e44c 100644 --- a/src/parser/mod.rs +++ b/src/parser/mod.rs @@ -14,10 +14,18 @@ //! - Rewrites must touch **only the specifier string**, never the rest of //! the statement (KISS + no formatter dependency): that is why //! [`ImportRecord::span`] is a byte range into the original source. +//! - Fix rules (Phase 1.6) implement [`Fix`] and propose [`FixCandidate`]s: +//! byte [`Edit`](crate::core::Edit)s for one file, run through the same +//! dry-run/atomic engine as `mv`. Deterministic by contract — ambiguity +//! is reported as `auto_fixable: false` for an agent to resolve, never +//! guessed at. use std::ops::Range; use std::path::Path; +use crate::core::Edit; +use crate::core::index::Index; + pub mod java; pub mod resolve; pub mod ts; @@ -111,3 +119,94 @@ pub fn frontend_for(lang: SourceLanguage) -> Box { } } } + +/// Urgency of a [`FixCandidate`] for the reader. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum Severity { + /// The code does not compile (or resolves) without the fix. + Error, + /// The code works but is dirty: unused or misordered imports. + Warning, + /// Style note only. + Info, +} + +impl Severity { + /// Stable lowercase name used in `--json` output. + #[must_use] + pub const fn as_str(self) -> &'static str { + match self { + Self::Error => "error", + Self::Warning => "warning", + Self::Info => "info", + } + } + + /// Overlap-resolution priority: on a byte conflict the more urgent + /// rule wins and the other candidate is deferred to the next run. + #[must_use] + pub const fn rank(self) -> u8 { + match self { + Self::Error => 0, + Self::Warning => 1, + Self::Info => 2, + } + } +} + +/// One proposed fix for a single file: what is wrong and — when +/// `auto_fixable` — the exact edits that repair it. +#[derive(Debug, Clone)] +pub struct FixCandidate { + /// Stable rule id; also the value accepted by `jmove fix --rule`. + pub rule: &'static str, + /// One-line explanation for humans and agents. + pub message: String, + /// How urgent the fix is. + pub severity: Severity, + /// `false` marks an ambiguous finding an agent or human must resolve; + /// the engine never applies such candidates automatically. + pub auto_fixable: bool, + /// Byte range of the offending construct (issue location for reports, + /// even when `edits` is empty). + pub span: Range, + /// Edits on this file, ascending by span. + pub edits: Vec, + /// Fully-qualified options for an ambiguous finding (`auto_fixable: + /// false`); empty for every auto-fixable or non-lookup candidate. + pub candidates: Vec, +} + +/// A deterministic single-file fix rule (Phase 1.6). +pub trait Fix: Send + Sync { + /// Stable rule id reported in candidates and accepted by `--rule`. + fn rule(&self) -> &'static str; + + /// Candidates for `path` (project-relative) with the given contents. + /// A rule may only return `auto_fixable: true` edits it can prove + /// safe; anything ambiguous goes out as a non-auto candidate. + fn fixes(&self, path: &Path, source: &str, index: &Index) -> Vec; +} + +/// Default rule set for `lang` (empty for languages without rules yet). +#[must_use] +pub fn fixers_for(lang: SourceLanguage) -> Vec> { + match lang { + SourceLanguage::Java => vec![ + Box::new(java::rules::unused_imports::JavaUnusedImports::new()), + Box::new(java::rules::missing_imports::JavaMissingImports::new()), + Box::new(java::rules::import_order::JavaImportOrder::new()), + ], + SourceLanguage::TypeScript | SourceLanguage::Tsx | SourceLanguage::JavaScript => Vec::new(), + } +} + +/// Every rule id `jmove fix` currently knows about. +#[must_use] +pub fn rule_ids() -> &'static [&'static str] { + &[ + java::rules::unused_imports::RULE, + java::rules::missing_imports::RULE, + java::rules::import_order::RULE, + ] +} diff --git a/tests/cli_fix.rs b/tests/cli_fix.rs new file mode 100644 index 0000000..cb7cbe8 --- /dev/null +++ b/tests/cli_fix.rs @@ -0,0 +1,249 @@ +//! End-to-end tests for `jmove fix` against a Java fixture. + +mod common; + +use common::{in_root, jmove, read}; +use predicates::prelude::*; + +fn dirty_project() -> tempfile::TempDir { + common::copy_fixture("java", "fix") +} + +#[test] +fn fix_dry_run_previews_deletions_without_writing() { + let tmp = dirty_project(); + jmove(&tmp, &["fix", "--dry-run"]) + .success() + .stdout(predicate::str::contains( + "-import com.example.unused.Ghost;", + )) + .stdout(predicate::str::contains(" import java.util.List;")); + // Disk untouched: the unused import is still there. + let app = read(&in_root( + tmp.path(), + "src/main/java/com/example/app/App.java", + )); + assert!(app.contains("import com.example.unused.Ghost;"), "{app}"); +} + +#[test] +fn fix_removes_unused_and_keeps_used_and_string_mentions() { + let tmp = dirty_project(); + jmove(&tmp, &["fix"]) + .success() + .stdout(predicate::str::contains("fixed 1 issue in 1 file")); + + let app = read(&in_root( + tmp.path(), + "src/main/java/com/example/app/App.java", + )); + // Unused single-type import is gone, whole line removed. + assert!(!app.contains("com.example.unused.Ghost"), "{app}"); + // Used import stays. + assert!(app.contains("import com.example.Text;"), "{app}"); + // `List` only appears inside a string literal => kept (safe direction). + assert!(app.contains("import java.util.List;"), "{app}"); + assert!(app.contains("\"java.util.List\""), "{app}"); + + // Run 2: convergence — the deferred `java/import-order` fix (it + // overlapped the deletion in run 1) now applies on the clean block. + jmove(&tmp, &["fix"]) + .success() + .stdout(predicate::str::contains("fixed 1 issue in 1 file")); + let app = read(&in_root( + tmp.path(), + "src/main/java/com/example/app/App.java", + )); + assert!( + app.contains( + "import static com.example.Text.shout;\n\nimport com.example.Text;\nimport java.util.List;\n" + ), + "{app}" + ); + // Run 3: fixed point — nothing left to change. + jmove(&tmp, &["fix"]) + .success() + .stdout(predicate::str::contains("nothing to change")); + // The project still checks clean. + jmove(&tmp, &["check"]).success(); +} + +#[test] +fn fix_json_reports_candidates_and_applied_count() { + let tmp = dirty_project(); + jmove(&tmp, &["fix", "--dry-run", "--json"]) + .success() + .stdout( + predicate::str::contains("\"status\": \"dry_run\"") + .and(predicate::str::contains("\"operation\": \"fix\"")) + .and(predicate::str::contains("\"rule\": \"java/unused-import\"")) + .and(predicate::str::contains("\"would_fix\": 1")), + ); + + jmove(&tmp, &["fix", "--json"]).success().stdout( + predicate::str::contains("\"status\": \"ok\"") + .and(predicate::str::contains("\"fixes\": 1")) + .and(predicate::str::contains("\"files_changed\": 1")), + ); + let app = read(&in_root( + tmp.path(), + "src/main/java/com/example/app/App.java", + )); + assert!(!app.contains("Ghost"), "{app}"); +} + +#[test] +fn fix_unknown_rule_is_rejected() { + let tmp = dirty_project(); + jmove(&tmp, &["fix", "--rule", "does/not-exist"]) + .failure() + .code(1) + .stderr(predicate::str::contains( + "unknown fix rule 'does/not-exist'", + )) + .stderr(predicate::str::contains("java/unused-import")); + // Nothing on disk changed. + let app = read(&in_root( + tmp.path(), + "src/main/java/com/example/app/App.java", + )); + assert!(app.contains("Ghost"), "{app}"); +} + +#[test] +fn fix_scoped_to_rule_and_reports_guava_like_sibling() { + // `Text` is referenced only by a static member import of the same type, + // so neither import is provably dead (mirrors the Guava smoke note). + let tmp = dirty_project(); + let app = read(&in_root( + tmp.path(), + "src/main/java/com/example/app/App.java", + )); + assert!( + app.contains("import static com.example.Text.shout;"), + "{app}" + ); + jmove(&tmp, &["fix", "--rule", "java/unused-import"]) + .success() + .stdout(predicate::str::contains("fixed 1 issue in 1 file")); + let app = read(&in_root( + tmp.path(), + "src/main/java/com/example/app/App.java", + )); + assert!(app.contains("import com.example.Text;"), "{app}"); + assert!( + app.contains("import static com.example.Text.shout;"), + "{app}" + ); +} + +fn missing_project() -> tempfile::TempDir { + common::copy_fixture("java", "fix_missing") +} + +#[test] +fn missing_import_adds_unique_and_reports_ambiguous() { + let tmp = missing_project(); + jmove(&tmp, &["fix", "--rule", "java/missing-import", "--json"]) + .success() + .stdout( + predicate::str::contains("\"rule\": \"java/missing-import\"") + .and(predicate::str::contains("\"applied\": false")) + .and(predicate::str::contains("\"com.example.a.Config\"")) + .and(predicate::str::contains("\"com.example.b.Config\"")), + ); + let calc = read(&in_root( + tmp.path(), + "src/main/java/com/example/app/Calc.java", + )); + assert!( + calc.contains("package com.example.app;\nimport com.example.util.Maths;\n"), + "{calc}" + ); + // The ambiguous `Config` is never guessed at. + let refer = read(&in_root( + tmp.path(), + "src/main/java/com/example/c/Refer.java", + )); + assert!(!refer.contains("import com.example."), "{refer}"); + jmove(&tmp, &["check"]).success(); +} + +#[test] +fn missing_import_dry_run_then_stable_reapply() { + let tmp = missing_project(); + jmove(&tmp, &["fix", "--dry-run"]) + .success() + .stdout(predicate::str::contains("+import com.example.util.Maths;")); + jmove(&tmp, &["fix"]) + .success() + .stdout(predicate::str::contains("fixed 3 issues in 2 files")); + // Only manual (ambiguous) findings remain: nothing further applies. + jmove(&tmp, &["fix"]) + .success() + .stdout(predicate::str::contains("nothing to change")); +} + +#[test] +fn unused_delete_and_missing_insert_coexist_in_one_file() { + // Dual.java: the unused import is deleted while the missing `Maths` + // import lands at the very byte of the deleted line's end — adjacent, + // not overlapping, so one atomic plan carries both. + let tmp = missing_project(); + let dual = "src/main/java/com/example/app/Dual.java"; + jmove(&tmp, &["fix"]).success(); + let text = read(&in_root(tmp.path(), dual)); + assert!(!text.contains("Gone"), "{text}"); + assert!(text.contains("import com.example.util.Maths;"), "{text}"); + jmove(&tmp, &["check"]).success(); +} + +#[test] +fn three_rules_converge_on_one_project() { + // App.java: unused import inside an out-of-order block (the order + // rewrite overlaps the deletion, so it defers to run 2). + // C.java: out-of-order block + a bare `Maths` reference (insertion at + // the block boundary coexists with the rewrite in run 1). + let tmp = common::copy_fixture("java", "order"); + jmove(&tmp, &["fix", "--dry-run", "--json"]) + .success() + .stdout( + predicate::str::contains("\"rule\": \"java/import-order\"") + .and(predicate::str::contains("\"applied\": false")) + .and(predicate::str::contains("skipped")), + ); + let app = "src/main/java/com/example/app/App.java"; + let c = "src/main/java/com/example/app/C.java"; + jmove(&tmp, &["fix"]) + .success() + .stdout(predicate::str::contains("fixed 3 issues in 2 files")); + let app_text = read(&in_root(tmp.path(), app)); + // Unused is gone; the deferred order fix has not touched the block. + assert!(!app_text.contains("Unneeded"), "{app_text}"); + assert!( + app_text.contains("import java.util.List;\nimport com.example.util.Maths;"), + "{app_text}" + ); + jmove(&tmp, &["fix"]) + .success() + .stdout(predicate::str::contains("fixed 2 issues in 2 files")); + let c_text = read(&in_root(tmp.path(), c)); + assert!( + c_text.contains( + "import com.example.util.Maths;\nimport java.util.List;\nimport java.util.Map;\n" + ), + "{c_text}" + ); + let app_text = read(&in_root(tmp.path(), app)); + assert!( + app_text.contains( + "import com.example.util.Maths;\nimport java.util.List;\nimport java.util.Map;\n" + ), + "{app_text}" + ); + // Fixed point. + jmove(&tmp, &["fix"]) + .success() + .stdout(predicate::str::contains("nothing to change")); + jmove(&tmp, &["check"]).success(); +} diff --git a/tests/cli_git.rs b/tests/cli_git.rs new file mode 100644 index 0000000..8dc1566 --- /dev/null +++ b/tests/cli_git.rs @@ -0,0 +1,115 @@ +//! End-to-end tests for the git integration of `mv`: tracked files move +//! through `git mv` (staged rename, history kept), `--no-git` opts out. +//! Self-contained: it builds a real git repo from the `typescript/basic` +//! fixture instead of reusing the fake `.git` marker of `copy_fixture`. + +use std::fs; +use std::path::Path; +use std::process::Command as Git; + +use assert_cmd::Command; +use predicates::prelude::*; +use tempfile::{TempDir, tempdir}; + +/// Copy the fixture into a tempdir, `git init` it and commit everything. +fn git_fixture(name: &str) -> TempDir { + let tmp = tempdir().expect("tempdir"); + let src = Path::new(env!("CARGO_MANIFEST_DIR")) + .join("tests") + .join("typescript") + .join(name); + copy_dir(&src, tmp.path()); + git(tmp.path(), &["init", "-q", "-b", "main", "."]); + git(tmp.path(), &["config", "user.name", "jmove-test"]); + git(tmp.path(), &["config", "user.email", "test@test"]); + git(tmp.path(), &["add", "-A"]); + git(tmp.path(), &["commit", "-qm", "init"]); + tmp +} + +fn copy_dir(from: &Path, to: &Path) { + fs::create_dir_all(to).expect("mkdir"); + for entry in fs::read_dir(from).expect("readdir") { + let entry = entry.expect("entry"); + let target = to.join(entry.file_name()); + if entry.file_type().expect("filetype").is_dir() { + copy_dir(&entry.path(), &target); + } else { + fs::copy(entry.path(), target).expect("copy"); + } + } +} + +fn jmove(root: &TempDir, args: &[&str]) -> assert_cmd::assert::Assert { + let mut cmd = Command::cargo_bin("jmove").expect("jmove binary"); + cmd.arg("--root").arg(root.path()).args(args); + cmd.assert() +} + +fn git(root: &Path, args: &[&str]) { + let status = Git::new("git") + .arg("-C") + .arg(root) + .args(args) + .status() + .expect("git"); + assert!(status.success(), "`git {args:?}` failed"); +} + +fn git_output(root: &Path, args: &[&str]) -> String { + let out = Git::new("git") + .arg("-C") + .arg(root) + .args(args) + .output() + .expect("git"); + assert!(out.status.success(), "`git {args:?}` failed"); + String::from_utf8(out.stdout).expect("utf-8") +} + +#[test] +fn mv_in_a_git_repo_moves_via_git_mv_and_stages_it() { + let tmp = git_fixture("basic"); + jmove(&tmp, &["mv", "lib/sum.ts", "utils/sum.ts", "--json"]) + .success() + .stdout(predicate::str::contains("\"moved_via\": \"git\"")); + let staged = git_output(tmp.path(), &["diff", "--cached", "-M", "--name-status"]); + assert!( + staged.contains("lib/sum.ts") && staged.contains("utils/sum.ts"), + "rename must be staged: {staged}" + ); + jmove(&tmp, &["check"]).success(); +} + +#[test] +fn mv_dry_run_reports_would_use_git() { + let tmp = git_fixture("basic"); + jmove( + &tmp, + &["mv", "lib/sum.ts", "utils/sum.ts", "--dry-run", "--json"], + ) + .success() + .stdout(predicate::str::contains("\"would_move_via\": \"git\"")); + assert!( + tmp.path().join("lib/sum.ts").is_file(), + "dry-run writes nothing" + ); +} + +#[test] +fn mv_no_git_keeps_the_plain_rename_unstaged() { + let tmp = git_fixture("basic"); + jmove( + &tmp, + &["mv", "lib/sum.ts", "utils/sum.ts", "--no-git", "--json"], + ) + .success() + .stdout(predicate::str::contains("\"moved_via\": \"fs\"")); + let staged = git_output(tmp.path(), &["diff", "--cached", "--name-only"]); + assert!( + staged.trim().is_empty(), + "--no-git must not stage: {staged}" + ); + let target = git_output(tmp.path(), &["status", "--porcelain", "--", "utils/sum.ts"]); + assert!(target.starts_with("??"), "{target}"); +} diff --git a/tests/java/fix/src/main/java/com/example/Text.java b/tests/java/fix/src/main/java/com/example/Text.java new file mode 100644 index 0000000..c626f85 --- /dev/null +++ b/tests/java/fix/src/main/java/com/example/Text.java @@ -0,0 +1,7 @@ +package com.example; + +public class Text { + public static String shout(String s) { + return s.toUpperCase() + "!"; + } +} diff --git a/tests/java/fix/src/main/java/com/example/app/App.java b/tests/java/fix/src/main/java/com/example/app/App.java new file mode 100644 index 0000000..4346db2 --- /dev/null +++ b/tests/java/fix/src/main/java/com/example/app/App.java @@ -0,0 +1,14 @@ +package com.example.app; + +import com.example.Text; +import com.example.unused.Ghost; +import java.util.List; +import static com.example.Text.shout; + +public class App { + public static void main(String[] args) { + System.out.println(Text.shout("hi")); + String topic = "java.util.List"; + System.out.println(topic); + } +} diff --git a/tests/java/fix_missing/src/main/java/com/example/a/Config.java b/tests/java/fix_missing/src/main/java/com/example/a/Config.java new file mode 100644 index 0000000..dd9b73e --- /dev/null +++ b/tests/java/fix_missing/src/main/java/com/example/a/Config.java @@ -0,0 +1,3 @@ +package com.example.a; + +public class Config {} diff --git a/tests/java/fix_missing/src/main/java/com/example/app/Calc.java b/tests/java/fix_missing/src/main/java/com/example/app/Calc.java new file mode 100644 index 0000000..abb96bd --- /dev/null +++ b/tests/java/fix_missing/src/main/java/com/example/app/Calc.java @@ -0,0 +1,5 @@ +package com.example.app; + +public class Calc { + int v = Maths.compute(); +} diff --git a/tests/java/fix_missing/src/main/java/com/example/app/Dual.java b/tests/java/fix_missing/src/main/java/com/example/app/Dual.java new file mode 100644 index 0000000..19fdcbb --- /dev/null +++ b/tests/java/fix_missing/src/main/java/com/example/app/Dual.java @@ -0,0 +1,7 @@ +package com.example.app; + +import com.example.gone.Gone; + +public class Dual { + int v = Maths.compute(); +} diff --git a/tests/java/fix_missing/src/main/java/com/example/b/Config.java b/tests/java/fix_missing/src/main/java/com/example/b/Config.java new file mode 100644 index 0000000..157a097 --- /dev/null +++ b/tests/java/fix_missing/src/main/java/com/example/b/Config.java @@ -0,0 +1,3 @@ +package com.example.b; + +public class Config {} diff --git a/tests/java/fix_missing/src/main/java/com/example/c/Refer.java b/tests/java/fix_missing/src/main/java/com/example/c/Refer.java new file mode 100644 index 0000000..9309308 --- /dev/null +++ b/tests/java/fix_missing/src/main/java/com/example/c/Refer.java @@ -0,0 +1,5 @@ +package com.example.c; + +public class Refer { + Config field; +} diff --git a/tests/java/fix_missing/src/main/java/com/example/util/Maths.java b/tests/java/fix_missing/src/main/java/com/example/util/Maths.java new file mode 100644 index 0000000..489b9cf --- /dev/null +++ b/tests/java/fix_missing/src/main/java/com/example/util/Maths.java @@ -0,0 +1,7 @@ +package com.example.util; + +public class Maths { + public static int compute() { + return 42; + } +} diff --git a/tests/java/order/src/main/java/com/example/app/App.java b/tests/java/order/src/main/java/com/example/app/App.java new file mode 100644 index 0000000..615d026 --- /dev/null +++ b/tests/java/order/src/main/java/com/example/app/App.java @@ -0,0 +1,12 @@ +package com.example.app; + +import java.util.List; +import com.example.util.Maths; +import com.example.gone.Unneeded; +import java.util.Map; + +public class App { + List a; + Map b; + int v = Maths.compute(); +} diff --git a/tests/java/order/src/main/java/com/example/app/C.java b/tests/java/order/src/main/java/com/example/app/C.java new file mode 100644 index 0000000..69a00e2 --- /dev/null +++ b/tests/java/order/src/main/java/com/example/app/C.java @@ -0,0 +1,10 @@ +package com.example.app; + +import java.util.Map; +import java.util.List; + +public class C { + List a; + Map b; + int v = Maths.compute(); +} diff --git a/tests/java/order/src/main/java/com/example/util/Maths.java b/tests/java/order/src/main/java/com/example/util/Maths.java new file mode 100644 index 0000000..489b9cf --- /dev/null +++ b/tests/java/order/src/main/java/com/example/util/Maths.java @@ -0,0 +1,7 @@ +package com.example.util; + +public class Maths { + public static int compute() { + return 42; + } +}