From f611bb1fcb4f0e90487dc5bd0bf29adc1f75c0b1 Mon Sep 17 00:00:00 2001 From: Pascal Hertleif Date: Fri, 4 May 2018 11:07:33 +0200 Subject: [PATCH 1/8] Switch to replace-rs --- Cargo.toml | 2 ++ cargo-fix/src/main.rs | 2 +- src/diagnostics.rs | 6 +++--- src/lib.rs | 21 +++++++++++++++++---- tests/everything.rs | 7 ++++--- 5 files changed, 27 insertions(+), 11 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 426bdbf..fa1db71 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -20,6 +20,8 @@ exclude = [ serde = "1.0" serde_json = "1.0" serde_derive = "1.0" +replace-rs = { git = "https://github.com/killercup/replace-rs" } +failure = "0.1.1" [dev-dependencies] duct = "0.8.2" diff --git a/cargo-fix/src/main.rs b/cargo-fix/src/main.rs index 1447036..23a93b3 100644 --- a/cargo-fix/src/main.rs +++ b/cargo-fix/src/main.rs @@ -177,7 +177,7 @@ fn rustfix_crate(rustc: &Path, filename: &str) -> Result<(), Error> { } debug!("applying {} fixes to {}", suggestions.len(), file); - let new_code = rustfix::apply_suggestions(&code, &suggestions); + let new_code = rustfix::apply_suggestions(&code, &suggestions)?; File::create(&file) .and_then(|mut f| f.write_all(new_code.as_bytes())) .with_context(|_| format!("failed to write file `{}`", file))?; diff --git a/src/diagnostics.rs b/src/diagnostics.rs index d0640b6..a0fe9fe 100644 --- a/src/diagnostics.rs +++ b/src/diagnostics.rs @@ -1,5 +1,5 @@ //! Rustc Diagnostic JSON Output -//! +//! //! The following data types are copied from [rust-lang/rust](https://github.com/rust-lang/rust/blob/de78655bca47cac8e783dbb563e7e5c25c1fae40/src/libsyntax/json.rs) #[derive(Deserialize, Debug)] @@ -21,8 +21,8 @@ pub struct Diagnostic { #[derive(Deserialize, Debug)] pub struct DiagnosticSpan { pub file_name: String, - byte_start: u32, - byte_end: u32, + pub byte_start: u32, + pub byte_end: u32, /// 1-based. pub line_start: usize, pub line_end: usize, diff --git a/src/lib.rs b/src/lib.rs index 4f23459..9974e35 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -1,8 +1,13 @@ #[macro_use] extern crate serde_derive; extern crate serde_json; +extern crate failure; +extern crate replace_rs as replace; use std::collections::HashSet; +use std::ops::Range; + +use failure::Error; pub mod diagnostics; use diagnostics::{Diagnostic, DiagnosticSpan}; @@ -61,6 +66,7 @@ pub struct Solution { pub struct Snippet { pub file_name: String, pub line_range: LineRange, + pub range: Range, /// leading surrounding text, text to replace, trailing surrounding text /// /// This split is useful for higlighting the part that gets replaced @@ -109,6 +115,7 @@ fn parse_snippet(span: &DiagnosticSpan) -> Snippet { column: span.column_end, }, }, + range: (span.byte_start as usize)..(span.byte_end as usize), text: (lead, body, tail), } } @@ -208,16 +215,22 @@ pub fn apply_suggestion(file_content: &mut String, suggestion: &Replacement) -> new_content } -pub fn apply_suggestions(code: &str, suggestions: &[Suggestion]) -> String { - let mut fixed = code.to_string(); +pub fn apply_suggestions(code: &str, suggestions: &[Suggestion]) -> Result { + use replace::Data; + + let mut fixed = Data::new(code.as_bytes()); for sug in suggestions.iter().rev() { for sol in &sug.solutions { for r in &sol.replacements { - fixed = apply_suggestion(&mut fixed, r); + fixed.replace_range( + r.snippet.range.start, + r.snippet.range.end - 1, + r.replacement.as_bytes(), + )?; } } } - fixed + Ok(String::from_utf8(fixed.to_vec())?) } diff --git a/tests/everything.rs b/tests/everything.rs index 5a3b5ab..d0e362f 100644 --- a/tests/everything.rs +++ b/tests/everything.rs @@ -15,7 +15,7 @@ use std::collections::HashSet; use std::process::Output; use tempdir::TempDir; -use rustfix::{Replacement, apply_suggestions}; +use rustfix::apply_suggestions; fn compile(file: &Path) -> Result> { let tmp = TempDir::new("rustfix-tests")?; @@ -23,6 +23,7 @@ fn compile(file: &Path) -> Result> { "rustc", file, "--error-format=pretty-json", "-Zunstable-options", "--emit=metadata", "--crate-name=rustfix_test", + "-Zsuggestion-applicability", "--out-dir", tmp.path() ); let res = better_call_clippy @@ -101,7 +102,7 @@ fn test_rustfix_with_file>(file: P) -> Result<(), Box> { "got unexpected suggestions from clippy", ); - let fixed = apply_suggestions(&code, &suggestions); + let fixed = apply_suggestions(&code, &suggestions)?; if std::env::var("RUSTFIX_TEST_RECORD_FIXED_RUST").is_ok() { use std::io::Write; @@ -110,7 +111,7 @@ fn test_rustfix_with_file>(file: P) -> Result<(), Box> { } let expected_fixed = read_file(&fixed_file)?; - assert_eq!(fixed.trim(), expected_fixed.trim(), "file doesn't look fixed"); + assert_eq!(fixed.trim(), expected_fixed.trim(), "file {} doesn't look fixed", file.display()); compiles_without_errors(&fixed_file)?; From d150af81d18787a4387cac9dc41da79b557ff200 Mon Sep 17 00:00:00 2001 From: Pascal Hertleif Date: Fri, 4 May 2018 23:05:35 +0200 Subject: [PATCH 2/8] Make replace a rustfix module --- Cargo.toml | 1 + src/lib.rs | 6 +- src/replace.rs | 223 +++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 229 insertions(+), 1 deletion(-) create mode 100644 src/replace.rs diff --git a/Cargo.toml b/Cargo.toml index fa1db71..6694b72 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -29,6 +29,7 @@ env_logger = "0.5.0-rc.1" log = "0.4.1" pretty_assertions = "0.4.1" tempdir = "0.3.5" +proptest = "0.7.0" [workspace] members = [ diff --git a/src/lib.rs b/src/lib.rs index 9974e35..9894b00 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -1,8 +1,11 @@ #[macro_use] extern crate serde_derive; extern crate serde_json; +#[macro_use] extern crate failure; -extern crate replace_rs as replace; +#[cfg(test)] +#[macro_use] +extern crate proptest; use std::collections::HashSet; use std::ops::Range; @@ -11,6 +14,7 @@ use failure::Error; pub mod diagnostics; use diagnostics::{Diagnostic, DiagnosticSpan}; +mod replace; pub fn get_suggestions_from_json( input: &str, diff --git a/src/replace.rs b/src/replace.rs new file mode 100644 index 0000000..5f4773f --- /dev/null +++ b/src/replace.rs @@ -0,0 +1,223 @@ +//! A small module giving you a simple container that allows easy and cheap +//! replacement of parts of its content, with the ability to prevent changing +//! the same parts multiple times. + +use std::rc::Rc; +use failure::Error; + +#[derive(Debug, Clone, PartialEq, Eq)] +enum State { + Initial, + Replaced(Rc<[u8]>), +} + +#[derive(Debug, Clone, PartialEq, Eq)] +struct Span { + /// Start of this span in parent data + start: usize, + /// up to end inculding + end: usize, + data: State, +} + +/// A container that allows easily replacing chunks of its data +#[derive(Debug, Clone, Default)] +pub struct Data { + original: Vec, + parts: Vec, +} + +impl Data { + /// Create a new data container from a slice of bytes + pub fn new(data: &[u8]) -> Self { + Data { + original: data.into(), + parts: vec![Span { + data: State::Initial, + start: 0, + end: data.len(), + }], + } + } + + /// Render this data as a vector of bytes + pub fn to_vec(&self) -> Vec { + self.parts.iter().fold(Vec::new(), |mut acc, d| { + match d.data { + State::Initial => acc.extend_from_slice(&self.original[d.start..d.end]), + State::Replaced(ref d) => acc.extend_from_slice(&d), + }; + acc + }) + } + + /// Replace a chunk of data with the given slice, erroring when this part + /// was already changed previously. + pub fn replace_range( + &mut self, + from: usize, + up_to_and_including: usize, + data: &[u8], + ) -> Result<(), Error> { + ensure!( + from <= up_to_and_including, + "Invalid range {}...{}, start is larger than end", + from, + up_to_and_including + ); + ensure!( + up_to_and_including <= self.original.len(), + "Invalid range {}...{} given, original data is only {} byte long", + from, + up_to_and_including, + self.original.len() + ); + + // Since we error out when replacing an already replaced chunk of data, + // we can take some shortcuts here. For example, there can be no + // overlapping replacements -- we _always_ split a chunk of 'initial' + // data into three[^empty] parts, and there can't ever be two 'initial' + // parts touching. + // + // [^empty]: Leading and trailing ones might be empty if we replace + // the whole chunk. As an optimization and without loss of generality we + // don't add empty parts. + let new_parts = { + let index_of_part_to_split = self.parts + .iter() + .position(|p| p.start <= from && p.end >= up_to_and_including) + .ok_or_else(|| { + format_err!( + "Could not find data slice that covers range {}..{}", + from, + up_to_and_including + ) + })?; + + let part_to_split = &self.parts[index_of_part_to_split]; + ensure!( + part_to_split.data == State::Initial, + "Cannot replace slice of data that was already replaced" + ); + + let mut new_parts = Vec::with_capacity(self.parts.len() + 2); + + // Previous parts + if let Some(ps) = self.parts.get(..index_of_part_to_split) { + new_parts.extend_from_slice(&ps); + } + + // Keep initial data on left side of part + if from > part_to_split.start { + new_parts.push(Span { + start: part_to_split.start, + end: from, + data: State::Initial, + }); + } + + // New part + new_parts.push(Span { + start: from, + end: up_to_and_including, + data: State::Replaced(data.into()), + }); + + // Keep initial data on right side of part + if up_to_and_including < part_to_split.end { + new_parts.push(Span { + start: up_to_and_including + 1, + end: part_to_split.end, + data: State::Initial, + }); + } + + // Following parts + if let Some(ps) = self.parts.get(index_of_part_to_split + 1..) { + new_parts.extend_from_slice(&ps); + } + + new_parts + }; + + self.parts = new_parts; + + Ok(()) + } +} + +#[cfg(test)] +mod tests { + use super::*; + use proptest::prelude::*; + + fn str(i: &[u8]) -> &str { + ::std::str::from_utf8(i).unwrap() + } + + #[test] + fn replace_some_stuff() { + let mut d = Data::new(b"foo bar baz"); + d.replace_range(4, 6, b"lol").unwrap(); + assert_eq!("foo lol baz", str(&d.to_vec())); + } + + #[test] + fn replace_a_single_char() { + let mut d = Data::new(b"let y = true;"); + d.replace_range(4, 4, b"mut y").unwrap(); + assert_eq!("let mut y = true;", str(&d.to_vec())); + } + + #[test] + fn replace_multiple_lines() { + let mut d = Data::new(b"lorem\nipsum\ndolor"); + + d.replace_range(6, 10, b"lol").unwrap(); + assert_eq!("lorem\nlol\ndolor", str(&d.to_vec())); + + d.replace_range(12, 17, b"lol").unwrap(); + assert_eq!("lorem\nlol\nlol", str(&d.to_vec())); + } + + #[test] + #[should_panic(expected = "Cannot replace slice of data that was already replaced")] + fn replace_overlapping_stuff_errs() { + let mut d = Data::new(b"foo bar baz"); + + d.replace_range(4, 6, b"lol").unwrap(); + assert_eq!("foo lol baz", str(&d.to_vec())); + + d.replace_range(4, 6, b"lol").unwrap(); + } + + #[test] + #[should_panic(expected = "original data is only 3 byte long")] + fn broken_replacements() { + let mut d = Data::new(b"foo"); + d.replace_range(4, 7, b"lol").unwrap(); + } + + proptest! { + #[test] + #[ignore] + fn new_to_vec_roundtrip(ref s in "\\PC*") { + assert_eq!(s.as_bytes(), Data::new(s.as_bytes()).to_vec().as_slice()); + } + + #[test] + #[ignore] + fn replace_random_chunks( + ref data in "\\PC*", + ref replacements in prop::collection::vec( + (any::<::std::ops::Range>(), any::>()), + 1..1337, + ) + ) { + let mut d = Data::new(data.as_bytes()); + for &(ref range, ref bytes) in replacements { + let _ = d.replace_range(range.start, range.end, bytes); + } + } + } +} From 8aef421d82c3f831db0339e2fa7dfe0b9875a38d Mon Sep 17 00:00:00 2001 From: Pascal Hertleif Date: Fri, 4 May 2018 23:24:25 +0200 Subject: [PATCH 3/8] Add test for preserving line endings Closes #77 --- cargo-fix/tests/all/main.rs | 13 +++++++++++-- cargo-fix/tests/all/smoke.rs | 13 +++++++++++++ 2 files changed, 24 insertions(+), 2 deletions(-) diff --git a/cargo-fix/tests/all/main.rs b/cargo-fix/tests/all/main.rs index 8547cdf..f0933c1 100644 --- a/cargo-fix/tests/all/main.rs +++ b/cargo-fix/tests/all/main.rs @@ -2,8 +2,8 @@ extern crate difference; extern crate url; use std::env; -use std::fs; -use std::io::Write; +use std::fs::{self, File}; +use std::io::{Read, Write}; use std::path::{Path, PathBuf}; use std::process::Command; use std::str; @@ -84,6 +84,15 @@ impl Project { cwd: None, } } + + fn read(&self, path: &str) -> String { + let mut ret = String::new(); + File::open(self.root.join(path)) + .unwrap() + .read_to_string(&mut ret) + .unwrap(); + return ret + } } struct ExpectCmd<'a> { diff --git a/cargo-fix/tests/all/smoke.rs b/cargo-fix/tests/all/smoke.rs index c1c5ca2..0bebd62 100644 --- a/cargo-fix/tests/all/smoke.rs +++ b/cargo-fix/tests/all/smoke.rs @@ -87,3 +87,16 @@ fn tricky_ampersand() { .stderr(stderr) .run(); } + +#[test] +fn preserve_line_endings() { + let p = project() + .file("src/lib.rs", "\ + fn add(a: &u32) -> u32 { a + 1 }\r\n\ + pub fn foo() -> u32 { add(1) }\r\n\ + ") + .build(); + + p.expect_cmd("cargo fix").run(); + assert!(p.read("src/lib.rs").contains("\r\n")); +} From de7714dc117ff5dbaf422607a105824b41a67e87 Mon Sep 17 00:00:00 2001 From: Pascal Hertleif Date: Fri, 4 May 2018 23:40:05 +0200 Subject: [PATCH 4/8] Add multiple suggestions test --- cargo-fix/tests/all/smoke.rs | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/cargo-fix/tests/all/smoke.rs b/cargo-fix/tests/all/smoke.rs index 0bebd62..51cf8e0 100644 --- a/cargo-fix/tests/all/smoke.rs +++ b/cargo-fix/tests/all/smoke.rs @@ -100,3 +100,31 @@ fn preserve_line_endings() { p.expect_cmd("cargo fix").run(); assert!(p.read("src/lib.rs").contains("\r\n")); } + +#[test] +fn multiple_suggestions_for_the_same_thing() { + let p = project() + .file("src/lib.rs", "\ + fn main() { + let xs = vec![String::from(\"foo\")]; + // error: no diplay in scope, and there are two options + // (std::path::Display and std::fmt::Display) + let d: &Display = &xs; + println!(\"{}\", d); + } + ") + .build(); + + let stderr = "\ +[CHECKING] foo v0.1.0 (CWD) +error: Cannot replace slice of data that was already replaced +error: Could not compile `foo`. + +To learn more, run the command again with --verbose. +"; + + p.expect_cmd("cargo fix") + .stderr(stderr) + .status(101) + .run(); +} From 893f29fb382a670be6e50b8f6b247dd25c988b66 Mon Sep 17 00:00:00 2001 From: Pascal Hertleif Date: Fri, 4 May 2018 23:40:15 +0200 Subject: [PATCH 5/8] Omit now-unused crate --- Cargo.toml | 1 - 1 file changed, 1 deletion(-) diff --git a/Cargo.toml b/Cargo.toml index 6694b72..70a6546 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -20,7 +20,6 @@ exclude = [ serde = "1.0" serde_json = "1.0" serde_derive = "1.0" -replace-rs = { git = "https://github.com/killercup/replace-rs" } failure = "0.1.1" [dev-dependencies] From 45395939e7d6b50726f25a1e5b3941232545bc7f Mon Sep 17 00:00:00 2001 From: Pascal Hertleif Date: Fri, 4 May 2018 23:40:24 +0200 Subject: [PATCH 6/8] Prevent underflow --- src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/lib.rs b/src/lib.rs index 9894b00..c412809 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -229,7 +229,7 @@ pub fn apply_suggestions(code: &str, suggestions: &[Suggestion]) -> Result Date: Fri, 4 May 2018 23:42:57 +0200 Subject: [PATCH 7/8] Delete obsolete code --- src/lib.rs | 42 ------------------------------------------ 1 file changed, 42 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index c412809..c8a7e08 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -177,48 +177,6 @@ pub fn collect_suggestions(diagnostic: &Diagnostic, } } -pub fn apply_suggestion(file_content: &mut String, suggestion: &Replacement) -> String { - use std::cmp::max; - - let mut new_content = String::new(); - - // Add the lines before the section we want to replace - new_content.push_str(&file_content.lines() - .take(max(suggestion.snippet.line_range.start.line - 1, 0) as usize) - .collect::>() - .join("\n")); - new_content.push_str("\n"); - - // Parts of line before replacement - new_content.push_str(&file_content.lines() - .nth(suggestion.snippet.line_range.start.line - 1) - .unwrap_or("") - .chars() - .take(suggestion.snippet.line_range.start.column - 1) - .collect::()); - - // Insert new content! Finally! - new_content.push_str(&suggestion.replacement); - - // Parts of line after replacement - new_content.push_str(&file_content.lines() - .nth(suggestion.snippet.line_range.end.line - 1) - .unwrap_or("") - .chars() - .skip(suggestion.snippet.line_range.end.column - 1) - .collect::()); - - // Add the lines after the section we want to replace - new_content.push_str("\n"); - new_content.push_str(&file_content.lines() - .skip(suggestion.snippet.line_range.end.line as usize) - .collect::>() - .join("\n")); - new_content.push_str("\n"); - - new_content -} - pub fn apply_suggestions(code: &str, suggestions: &[Suggestion]) -> Result { use replace::Data; From f9b8801585b7583cdd4e0a9ec96d1a0ae4a1d81c Mon Sep 17 00:00:00 2001 From: Pascal Hertleif Date: Fri, 4 May 2018 23:43:32 +0200 Subject: [PATCH 8/8] Run ignored tests on CI --- .travis.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/.travis.yml b/.travis.yml index 54f7326..297b02d 100644 --- a/.travis.yml +++ b/.travis.yml @@ -3,6 +3,7 @@ rust: - nightly script: - cargo test --all + - cargo test --all -- --ignored notifications: email: on_success: never