From a833f1ae1e6ac5ecb71c6d6f7a4a98437fd4cba6 Mon Sep 17 00:00:00 2001 From: Zack Jackson <25274700+ScriptedAlchemy@users.noreply.github.com> Date: Sat, 24 Oct 2026 07:03:53 -0700 Subject: [PATCH] fix(source-edit): insert moved imports at Rust syntax boundaries --- .../tracedecay-source-edit/src/edits/mod.rs | 2 + .../src/edits/move_destination_tests.rs | 268 ++++++++++++++++++ .../src/edits/preview.rs | 11 +- .../src/move_symbol/mod.rs | 3 +- .../src/move_symbol/rendering.rs | 213 +++++++++++--- 5 files changed, 449 insertions(+), 48 deletions(-) create mode 100644 crates/tracedecay-source-edit/src/edits/move_destination_tests.rs diff --git a/crates/tracedecay-source-edit/src/edits/mod.rs b/crates/tracedecay-source-edit/src/edits/mod.rs index 63b885dcb7..aaeea9e24b 100644 --- a/crates/tracedecay-source-edit/src/edits/mod.rs +++ b/crates/tracedecay-source-edit/src/edits/mod.rs @@ -10,6 +10,8 @@ mod symbols; #[cfg(test)] mod execute_tests; #[cfg(test)] +mod move_destination_tests; +#[cfg(test)] mod reconcile_tests; #[cfg(test)] mod test_support; diff --git a/crates/tracedecay-source-edit/src/edits/move_destination_tests.rs b/crates/tracedecay-source-edit/src/edits/move_destination_tests.rs new file mode 100644 index 0000000000..e953cb550e --- /dev/null +++ b/crates/tracedecay-source-edit/src/edits/move_destination_tests.rs @@ -0,0 +1,268 @@ +use std::fs; +use std::path::Path; +use std::process::Command; + +use tempfile::tempdir; +use tracedecay_contracts::{ + EffectTermination, SourceEditKind, SourceEditRequest, source_edit_operation, +}; + +use super::test_support::{ + fixture_authorization, fixture_graph, fixture_request_for_edit, fixture_symbol_code_graph, +}; +use crate::execute::resolve_source_edit_preview; +use crate::{SourceEditOutcome, execute_source_edit, preview_source_edit_expected_state}; + +fn compile_move_destination_fixture(project: &Path) { + let output = Command::new(std::env::var_os("RUSTC").unwrap_or_else(|| "rustc".into())) + .current_dir(project) + .args([ + "--edition=2024", + "--crate-type=lib", + "--emit=metadata", + "--crate-name=move_destination", + "src/lib.rs", + "-o", + "fixture.rmeta", + ]) + .output() + .unwrap(); + assert!( + output.status.success(), + "move destination must compile: {}", + String::from_utf8_lossy(&output.stderr) + ); +} + +async fn move_with_destination_header( + before_import: &str, + after_import: &str, + newline: &str, + destination_exists: bool, +) { + let project = tempdir().unwrap(); + fs::create_dir(project.path().join("src")).unwrap(); + let moved_text = "pub fn moved(value: Widget) -> Widget {\n value\n}"; + let source = format!("use crate::model::Widget;\n\n{moved_text}\n"); + let destination = format!("{before_import}{after_import}"); + fs::write(project.path().join("src/source.rs"), &source).unwrap(); + fs::write(project.path().join("src/model.rs"), "pub struct Widget;\n").unwrap(); + fs::write( + project.path().join("src/lib.rs"), + "#![allow(dead_code, unused_imports)]\nmod model;\nmod source;\nmod relocated;\n", + ) + .unwrap(); + if destination_exists { + fs::write(project.path().join("src/relocated.rs"), &destination).unwrap(); + compile_move_destination_fixture(project.path()); + } + let (graph, _) = fixture_graph(project.path()).await; + let code_graph = fixture_symbol_code_graph( + "src/source.rs", + &source, + moved_text, + "moved", + "source::moved", + ); + let mut request = fixture_request_for_edit( + SourceEditRequest::MoveSymbol { + symbol: "source::moved".to_owned(), + dest_file: "src/relocated.rs".to_owned(), + dry_run: false, + update_references: false, + }, + "source-edit.move-destination", + ); + let preview = resolve_source_edit_preview( + &graph, + &code_graph, + &request.context, + request.observed_at, + tracedecay_graph_query::request_graph_cancellation(&request.context), + request.edit.clone(), + ) + .await + .unwrap(); + let SourceEditOutcome::Move(result) = &preview.outcome else { + panic!("expected a move preview"); + }; + assert!(result.success, "{}", result.message); + assert_eq!(result.applied_imports, ["use crate::model::Widget;"]); + assert_eq!(preview.planned_files.len(), 2); + assert_eq!( + fs::read_to_string(project.path().join("src/source.rs")).unwrap(), + source + ); + if destination_exists { + assert_eq!( + fs::read_to_string(project.path().join("src/relocated.rs")).unwrap(), + destination + ); + } else { + assert!(!project.path().join("src/relocated.rs").exists()); + } + request.expected_state = preview.expected_state.unwrap(); + let authorization = fixture_authorization(&request); + let applied = execute_source_edit( + &graph, + &code_graph, + &source_edit_operation(SourceEditKind::MoveSymbol).unwrap(), + request, + &authorization, + ) + .await + .unwrap(); + assert!(applied.outcome.success(), "{}", applied.outcome.message()); + for planned in preview.planned_files { + assert_eq!( + fs::read_to_string(project.path().join(&planned.relative_path)).unwrap(), + planned.intended, + "apply must publish the exact preview for {}", + planned.relative_path + ); + } + compile_move_destination_fixture(project.path()); + let actual = fs::read_to_string(project.path().join("src/relocated.rs")).unwrap(); + let unchanged_destination = + format!("{before_import}use crate::model::Widget;{newline}{after_import}"); + assert!(actual.starts_with(&unchanged_destination), "{actual:?}"); + assert!(actual.ends_with(&format!("{moved_text}\n"))); + let mut parser = tree_sitter::Parser::new(); + let language = tracedecay_code_extraction::ts_provider::try_language("rust").unwrap(); + parser.set_language(&language).unwrap(); + let tree = parser.parse(&actual, None).unwrap(); + assert!(!tree.root_node().has_error()); + assert!( + tree.root_node() + .named_children(&mut tree.walk()) + .any(|node| { + node.kind() == "use_declaration" + && node.utf8_text(actual.as_bytes()).unwrap() == "use crate::model::Widget;" + }), + "the dependency must be at the destination module root" + ); +} + +#[tokio::test] +async fn move_symbol_imports_preserve_destination_syntax_in_preview_and_apply() { + for (before, after) in [ + ( + "use std::{\n fmt::{self, Debug as DebugTrait},\n collections::HashMap,\n};\n", + "pub fn marker() {}\n", + ), + ( + "//! Destination module.\n#![allow(dead_code)]\n/*! Inner block docs. */\n#![allow(unused_imports)]\nuse std::fmt::Debug;\n", + "pub fn marker() {}\n", + ), + ( + "// Header.\nuse std::fmt::Debug; // Existing import.\n\n", + "/// Marker documentation.\n#[inline]\npub fn marker() {}\n", + ), + ( + "", + "#[cfg(any())]\nuse missing_dependency::{Missing, AlsoMissing};\n/** Marker docs. */\n#[inline]\npub fn marker() {}\n", + ), + ( + "", + "mod nested {\n use std::{fmt::Debug, collections::HashMap};\n}\npub fn marker() {}\n", + ), + ( + "\u{feff}#!/usr/bin/env rust-script\n//! Destination module.\n", + "pub fn marker() {}\n", + ), + ] { + move_with_destination_header(before, after, "\n", true).await; + } +} + +#[tokio::test] +async fn move_symbol_preserves_destination_bytes_when_adding_imports() { + move_with_destination_header( + "//! Module ☃.\r\nuse std::fmt::Debug;\r\n\r\n", + "/// Marker.\r\n#[inline]\r\npub fn marker() {}\r\n \t\r\n\r\n", + "\r\n", + true, + ) + .await; + move_with_destination_header("\r\n \t\r\n", "", "\r\n", true).await; + move_with_destination_header("", "", "\n", true).await; + move_with_destination_header("", "", "\n", false).await; +} + +#[tokio::test] +async fn move_symbol_refuses_unparseable_destination_before_preview_or_apply() { + for (destination, dependency) in [ + ("use std::{\n", true), + ("pub fn marker() {\n", false), + ("/* unclosed destination comment", true), + ("/// Detached documentation.\n#[inline]\n", false), + ] { + let project = tempdir().unwrap(); + fs::create_dir(project.path().join("src")).unwrap(); + let moved_text = if dependency { + "pub fn moved(value: Widget) -> Widget { value }" + } else { + "pub fn moved() {}" + }; + let source = format!("use crate::model::Widget;\n\n{moved_text}\n"); + fs::write(project.path().join("src/source.rs"), &source).unwrap(); + fs::write(project.path().join("src/relocated.rs"), destination).unwrap(); + let (graph, _) = fixture_graph(project.path()).await; + let code_graph = fixture_symbol_code_graph( + "src/source.rs", + &source, + moved_text, + "moved", + "source::moved", + ); + let request = fixture_request_for_edit( + SourceEditRequest::MoveSymbol { + symbol: "source::moved".to_owned(), + dest_file: "src/relocated.rs".to_owned(), + dry_run: false, + update_references: false, + }, + "source-edit.move-invalid-destination", + ); + let preview = preview_source_edit_expected_state( + &graph, + &code_graph, + &request.context, + request.observed_at, + request.edit.clone(), + ) + .await; + assert!( + preview.is_err(), + "malformed destination must not get an accepted preview" + ); + let authorization = fixture_authorization(&request); + let applied = execute_source_edit( + &graph, + &code_graph, + &source_edit_operation(SourceEditKind::MoveSymbol).unwrap(), + request, + &authorization, + ) + .await + .unwrap(); + assert!(!applied.outcome.success()); + assert_eq!( + applied.effect.unwrap().receipt.outcome, + EffectTermination::Failed + ); + assert_eq!( + fs::read_to_string(project.path().join("src/source.rs")).unwrap(), + source + ); + assert_eq!( + fs::read_to_string(project.path().join("src/relocated.rs")).unwrap(), + destination + ); + assert!( + !crate::journal::SourceEditDurability::for_graph(&graph) + .journal_path() + .exists() + ); + } +} diff --git a/crates/tracedecay-source-edit/src/edits/preview.rs b/crates/tracedecay-source-edit/src/edits/preview.rs index ab3c938780..d2227df442 100644 --- a/crates/tracedecay-source-edit/src/edits/preview.rs +++ b/crates/tracedecay-source-edit/src/edits/preview.rs @@ -1,13 +1,12 @@ //! Dry-run preview rendering shared by every edit primitive: a bounded //! single-hunk diff of the changed region, the success-message wrapper that -//! marks dry runs, and the line classifier shared by `move_symbol`'s header -//! scanner and inner-doc skip loop. +//! marks dry runs, and the line classifier used by `move_symbol`'s source +//! inner-doc skip loop. /// What a single source line looks like when scanning a file's leading -/// region. One classifier shared by every place that used to hand-roll its -/// own prefix checks in `move_symbol`'s import-insertion header scanner (which -/// leading lines belong to the header vs. the first item) and its `//!` skip -/// loop (never let a moved span start on an inner module doc). +/// region. MoveSymbol's source-span `//!` skip loop uses this classifier to +/// keep an inner module doc out of the moved item. Destination insertion uses +/// Rust syntax and byte ranges instead. #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub(crate) enum LeadingKind { /// Empty (after trimming leading whitespace). diff --git a/crates/tracedecay-source-edit/src/move_symbol/mod.rs b/crates/tracedecay-source-edit/src/move_symbol/mod.rs index f8aea3b615..9d5b50c0f4 100644 --- a/crates/tracedecay-source-edit/src/move_symbol/mod.rs +++ b/crates/tracedecay-source-edit/src/move_symbol/mod.rs @@ -215,7 +215,8 @@ pub(crate) async fn move_symbol( &dest_original, dest_module.as_deref(), ); - let dest_modified = build_dest_content(&dest_original, &applied_imports, &moved_text); + let dest_modified = + build_dest_content(&dest_rel, &dest_original, &applied_imports, &moved_text)?; let mut impact = analysis.hints; impact.extend( diff --git a/crates/tracedecay-source-edit/src/move_symbol/rendering.rs b/crates/tracedecay-source-edit/src/move_symbol/rendering.rs index b0ccc0854c..099b1c66dd 100644 --- a/crates/tracedecay-source-edit/src/move_symbol/rendering.rs +++ b/crates/tracedecay-source-edit/src/move_symbol/rendering.rs @@ -6,10 +6,10 @@ use std::collections::HashSet; -use crate::edits::{ - LeadingKind, MAX_PREVIEW_DIFF_LINES, PREVIEW_DIFF_CONTEXT, bounded_region_diff, - classify_leading_line, splice_lines, -}; +use tracedecay_code_extraction::ts_provider::parse_extractor_source; +use tracedecay_domain::errors::{Result, TraceDecayError}; + +use crate::edits::{MAX_PREVIEW_DIFF_LINES, PREVIEW_DIFF_CONTEXT, bounded_region_diff}; /// Removes `lines[start..=end]` and collapses the blank-line separator the /// removed item left behind so the source stays tidy. @@ -35,51 +35,115 @@ pub(super) fn remove_span_with_cleanup(lines: &[&str], start: usize, end: usize) /// import region or at the top of a fresh file), then the moved block after a /// blank-line separator. pub(super) fn build_dest_content( + dest_file: &str, dest_original: &str, imports: &[String], moved_text: &str, -) -> String { +) -> Result { let moved_block = moved_text.trim_end_matches('\n'); - if dest_original.trim().is_empty() { - let mut parts: Vec = Vec::new(); - if !imports.is_empty() { - parts.push(imports.join("\n")); - } - parts.push(moved_block.to_string()); - let mut content = parts.join("\n\n"); - content.push('\n'); - content + let mut content = if dest_file.ends_with(".rs") { + // Even a dependency-free move must not append inside malformed syntax + // or adopt dangling outer attributes from the destination. + insert_imports(dest_original, imports)? + } else if imports.is_empty() { + dest_original.to_owned() } else { - let with_imports = insert_imports(dest_original, imports); - format!("{}\n\n{}\n", with_imports.trim_end(), moved_block) + return Err(destination_syntax_error( + "automatic dependency imports require a Rust destination", + )); + }; + let newline = line_ending(dest_original); + if !content.is_empty() { + if !content.ends_with('\n') { + content.push_str(newline); + } + if !content.ends_with(&format!("{newline}{newline}")) { + content.push_str(newline); + } } + content.push_str(moved_block); + content.push('\n'); + Ok(content) } -/// Inserts `imports` into an existing Rust file after its leading module-doc / -/// comment / existing-`use` region. -pub(super) fn insert_imports(dest_source: &str, imports: &[String]) -> String { +/// Inserts imports between complete top-level Rust items, preserving all +/// existing bytes and the attachments of outer docs and attributes. +pub(super) fn insert_imports(dest_source: &str, imports: &[String]) -> Result { + let offset = rust_import_offset(dest_source)?; if imports.is_empty() { - return dest_source.to_string(); + return Ok(dest_source.to_string()); + } + let newline = line_ending(dest_source); + let (before, after) = dest_source.split_at(offset); + let mut content = before.to_owned(); + if !before.is_empty() && !before.ends_with('\n') { + content.push_str(newline); + } + content.push_str(&imports.join(newline)); + content.push_str(newline); + content.push_str(after); + Ok(content) +} + +fn line_ending(source: &str) -> &'static str { + if source + .find('\n') + .is_some_and(|offset| offset > 0 && source.as_bytes()[offset - 1] == b'\r') + { + "\r\n" + } else { + "\n" } - let lines: Vec<&str> = dest_source.lines().collect(); - let mut idx = 0; - while idx < lines.len() { - // Stop before an OUTER doc-comment block (`///` or `/**`): it documents - // the first item, and inserting a `use` between the doc and its item - // detaches the doc. Inner docs (`//!`), plain comments (`//`), blank - // lines, and existing imports stay in the header region. - match classify_leading_line(lines[idx]) { - LeadingKind::OuterDoc => break, - LeadingKind::Blank | LeadingKind::InnerDoc | LeadingKind::LineComment => idx += 1, - LeadingKind::UseImport => idx += 1, - LeadingKind::BlockComment | LeadingKind::Attribute | LeadingKind::Code => break, +} + +fn destination_syntax_error(message: impl Into) -> TraceDecayError { + TraceDecayError::project_route("source-edit-destination-syntax-unsupported", false, message) +} + +fn rust_import_offset(source: &str) -> Result { + let tree = parse_extractor_source("rust", "Rust", source).map_err(|error| { + destination_syntax_error(format!("cannot parse Rust destination: {error}")) + })?; + let root = tree.root_node(); + if root.has_error() { + return Err(destination_syntax_error( + "Rust destination contains incomplete or unsupported syntax", + )); + } + let mut offset = None; + let mut seen_item = false; + let mut pending_outer = false; + for node in root.named_children(&mut root.walk()) { + let comment = matches!(node.kind(), "line_comment" | "block_comment"); + if node.kind() == "inner_attribute_item" + || (comment && node.child_by_field_name("inner").is_some()) + { + if seen_item || pending_outer { + return Err(destination_syntax_error( + "Rust destination has an inner attribute or doc after an item or outer attribute", + )); + } + } else if node.kind() == "attribute_item" + || (comment && node.child_by_field_name("outer").is_some()) + { + // Outer attachments belong to the following item, including when + // that item is itself an import. Never split this group. + offset.get_or_insert(node.start_byte()); + pending_outer = true; + } else if !comment && node.kind() != "shebang" { + if !matches!(node.kind(), "use_declaration" | "extern_crate_declaration") { + offset.get_or_insert(node.start_byte()); + } + seen_item = true; + pending_outer = false; } } - let mut rebuilt: Vec<&str> = Vec::with_capacity(lines.len() + imports.len()); - rebuilt.extend_from_slice(&lines[..idx]); - rebuilt.extend(imports.iter().map(String::as_str)); - rebuilt.extend_from_slice(&lines[idx..]); - splice_lines(&rebuilt, dest_source.ends_with('\n')) + if pending_outer { + return Err(destination_syntax_error( + "Rust destination ends with an outer attribute or doc without its item", + )); + } + Ok(offset.unwrap_or(source.len())) } /// Drops imports already present verbatim in the destination and de-duplicates @@ -193,10 +257,12 @@ mod tests { #[test] fn build_dest_content_new_file_prepends_imports() { let content = build_dest_content( + "src/relocated.rs", "", &["use crate::pricing::LineItem;".to_string()], "/// doc\nfn f() {}\n", - ); + ) + .unwrap(); assert_eq!( content, "use crate::pricing::LineItem;\n\n/// doc\nfn f() {}\n" @@ -205,7 +271,13 @@ mod tests { #[test] fn build_dest_content_existing_file_appends_after_blank() { - let content = build_dest_content("//! mod\n\nfn existing() {}\n", &[], "fn f() {}"); + let content = build_dest_content( + "src/relocated.rs", + "//! mod\n\nfn existing() {}\n", + &[], + "fn f() {}", + ) + .unwrap(); assert_eq!(content, "//! mod\n\nfn existing() {}\n\nfn f() {}\n"); } @@ -214,7 +286,8 @@ mod tests { let out = insert_imports( "//! module doc\n\nfn a() {}\n", &["use crate::X;".to_string()], - ); + ) + .unwrap(); assert_eq!(out, "//! module doc\n\nuse crate::X;\nfn a() {}\n"); } @@ -225,7 +298,8 @@ mod tests { let out = insert_imports( "/// Docs for other.\npub fn other() {}\n", &["use crate::X;".to_string()], - ); + ) + .unwrap(); assert_eq!( out, "use crate::X;\n/// Docs for other.\npub fn other() {}\n" @@ -239,6 +313,63 @@ mod tests { assert_eq!(lines[doc + 1].trim(), "pub fn other() {}"); } + #[test] + fn insert_imports_preserves_comments_and_complete_same_line_items() { + for (original, expected) in [ + ( + "/* License. */\nuse a::{B, C}; /* Import note. */\nfn marker() {}", + "/* License. */\nuse a::{B, C}; /* Import note. */\nuse crate::X;\nfn marker() {}", + ), + ( + "use a::B; fn marker() {}", + "use a::B; \nuse crate::X;\nfn marker() {}", + ), + ("// License.", "// License.\nuse crate::X;\n"), + ("/* License. */", "/* License. */\nuse crate::X;\n"), + ("//! Module.", "//! Module.\nuse crate::X;\n"), + ] { + assert_eq!( + insert_imports(original, &["use crate::X;".to_owned()]).unwrap(), + expected + ); + } + } + + #[test] + fn destination_validation_rejects_incomplete_or_misplaced_attachments() { + for original in [ + "use a::{B, C", + "/// Detached.\n", + "#[allow(unused)]\n// Detached.\n", + "use a::B;\n#![allow(unused)]\n", + "fn keep() {}\n//! Misplaced module doc.\n", + ] { + assert!( + build_dest_content("src/relocated.rs", original, &[], "fn moved() {}").is_err(), + "must refuse without needing new imports: {original}" + ); + } + } + + #[test] + fn non_rust_destination_without_imports_keeps_generic_append_support() { + let original = "export const keep = 1;\n \t"; + assert_eq!( + build_dest_content("src/relocated.ts", original, &[], "export const moved = 2;") + .unwrap(), + "export const keep = 1;\n \t\n\nexport const moved = 2;\n" + ); + assert!( + build_dest_content( + "src/relocated.ts", + original, + &["use crate::X;".to_owned()], + "export const moved = 2;" + ) + .is_err() + ); + } + #[test] fn dedup_preserve_skips_existing() { let out = dedup_preserve(