Auto merge of #13226 - ehuss:fix-empty-span, r=weihanglo

rustfix: Support inserting new lines.

If rustfix received a suggestion which inserts new lines without replacing existing lines, it would ignore the suggestion. This is because `parse_snippet` would immediately return if the `lines` to replace was empty.

The solution here is to just drop the code which messes with the original text line. `cargo fix` (and compiletest) currently do not use this. This was originally added back in the days when rustfix supported an interactive UI which showed color highlighting of what it looks like with the replacement. My feeling is that when we add something like this back in, I would prefer to instead use a real diff library and display instead of trying to do various text manipulation for display. This particular code has generally been buggy, and has been a problem several times.

The included test fails without this fix because the changes do not apply, and the code cannot compile.

This also includes a little bit of cleanup for the `parse_and_replace` test. My feeling is that this test could use further improvements.

Author: bors

Cargo.lock

@@ -77,7 +77,7 @@ pulldown-cmark = { version = "0.9.3", default-features = false }
 rand = "0.8.5"
 regex = "1.10.2"
 rusqlite = { version = "0.30.0", features = ["bundled"] }
-rustfix = { version = "0.7.0", path = "crates/rustfix" }
+rustfix = { version = "0.8.0", path = "crates/rustfix" }
 same-file = "1.0.6"
 security-framework = "2.9.2"
 semver = { version = "1.0.20", features = ["serde"] }

Cargo.toml

@@ -1,6 +1,6 @@
 [package]
 name = "rustfix"
-version = "0.7.0"
+version = "0.8.0"
 authors = [
     "Pascal Hertleif <killercup@gmail.com>",
     "Oliver Schneider <oli-obk@users.noreply.github.com>",

crates/rustfix/Cargo.toml

@@ -104,10 +104,6 @@ pub struct Snippet {
     pub file_name: String,
     pub line_range: LineRange,
     pub range: Range<usize>,
-    /// leading surrounding text, text to replace, trailing surrounding text
-    ///
-    /// This split is useful for highlighting the part that gets replaced
-    pub text: (String, String, String),
 }
 
 /// Represents a replacement of a `snippet`.
@@ -119,58 +115,9 @@ pub struct Replacement {
     pub replacement: String,
 }
 
-/// Parses a [`Snippet`] from a diagnostic span item.
-fn parse_snippet(span: &DiagnosticSpan) -> Option<Snippet> {
-    // unindent the snippet
-    let indent = span
-        .text
-        .iter()
-        .map(|line| {
-            let indent = line
-                .text
-                .chars()
-                .take_while(|&c| char::is_whitespace(c))
-                .count();
-            std::cmp::min(indent, line.highlight_start - 1)
-        })
-        .min()?;
-
-    let text_slice = span.text[0].text.chars().collect::<Vec<char>>();
-
-    // We subtract `1` because these highlights are 1-based
-    // Check the `min` so that it doesn't attempt to index out-of-bounds when
-    // the span points to the "end" of the line. For example, a line of
-    // "foo\n" with a highlight_start of 5 is intended to highlight *after*
-    // the line. This needs to compensate since the newline has been removed
-    // from the text slice.
-    let start = (span.text[0].highlight_start - 1).min(text_slice.len());
-    let end = (span.text[0].highlight_end - 1).min(text_slice.len());
-    let lead = text_slice[indent..start].iter().collect();
-    let mut body: String = text_slice[start..end].iter().collect();
-
-    for line in span.text.iter().take(span.text.len() - 1).skip(1) {
-        body.push('\n');
-        body.push_str(&line.text[indent..]);
-    }
-    let mut tail = String::new();
-    let last = &span.text[span.text.len() - 1];
-
-    // If we get a DiagnosticSpanLine where highlight_end > text.len(), we prevent an 'out of
-    // bounds' access by making sure the index is within the array bounds.
-    // `saturating_sub` is used in case of an empty file
-    let last_tail_index = last.highlight_end.min(last.text.len()).saturating_sub(1);
-    let last_slice = last.text.chars().collect::<Vec<char>>();
-
-    if span.text.len() > 1 {
-        body.push('\n');
-        body.push_str(
-            &last_slice[indent..last_tail_index]
-                .iter()
-                .collect::<String>(),
-        );
-    }
-    tail.push_str(&last_slice[last_tail_index..].iter().collect::<String>());
-    Some(Snippet {
+/// Converts a [`DiagnosticSpan`] to a [`Snippet`].
+fn span_to_snippet(span: &DiagnosticSpan) -> Snippet {
+    Snippet {
         file_name: span.file_name.clone(),
         line_range: LineRange {
             start: LinePosition {
@@ -183,13 +130,12 @@ fn parse_snippet(span: &DiagnosticSpan) -> Option<Snippet> {
             },
         },
         range: (span.byte_start as usize)..(span.byte_end as usize),
-        text: (lead, body, tail),
-    })
+    }
 }
 
 /// Converts a [`DiagnosticSpan`] into a [`Replacement`].
 fn collect_span(span: &DiagnosticSpan) -> Option<Replacement> {
-    let snippet = parse_snippet(span)?;
+    let snippet = span_to_snippet(span);
     let replacement = span.suggested_replacement.clone()?;
     Some(Replacement {
         snippet,
@@ -217,7 +163,7 @@ pub fn collect_suggestions<S: ::std::hash::BuildHasher>(
         }
     }
 
-    let snippets = diagnostic.spans.iter().filter_map(parse_snippet).collect();
+    let snippets = diagnostic.spans.iter().map(span_to_snippet).collect();
 
     let solutions: Vec<_> = diagnostic
         .children

crates/rustfix/src/lib.rs

@@ -0,0 +1,9 @@
+use a::f;
+
+mod a {
+    pub fn f() {}
+}
+
+fn main() {
+    f();
+}

crates/rustfix/tests/everything/use-insert.fixed.rs

@@ -0,0 +1,3 @@
+{"$message_type":"diagnostic","message":"cannot find function `f` in this scope","code":{"code":"E0425","explanation":"An unresolved name was used.\n\nErroneous code examples:\n\n```compile_fail,E0425\nsomething_that_doesnt_exist::foo;\n// error: unresolved name `something_that_doesnt_exist::foo`\n\n// or:\n\ntrait Foo {\n    fn bar() {\n        Self; // error: unresolved name `Self`\n    }\n}\n\n// or:\n\nlet x = unknown_variable;  // error: unresolved name `unknown_variable`\n```\n\nPlease verify that the name wasn't misspelled and ensure that the\nidentifier being referred to is valid for the given situation. Example:\n\n```\nenum something_that_does_exist {\n    Foo,\n}\n```\n\nOr:\n\n```\nmod something_that_does_exist {\n    pub static foo : i32 = 0i32;\n}\n\nsomething_that_does_exist::foo; // ok!\n```\n\nOr:\n\n```\nlet unknown_variable = 12u32;\nlet x = unknown_variable; // ok!\n```\n\nIf the item is not defined in the current module, it must be imported using a\n`use` statement, like so:\n\n```\n# mod foo { pub fn bar() {} }\n# fn main() {\nuse foo::bar;\nbar();\n# }\n```\n\nIf the item you are importing is not defined in some super-module of the\ncurrent module, then it must also be declared as public (e.g., `pub fn`).\n"},"level":"error","spans":[{"file_name":"./tests/everything/use-insert.rs","byte_start":45,"byte_end":46,"line_start":6,"line_end":6,"column_start":5,"column_end":6,"is_primary":true,"text":[{"text":"    f();","highlight_start":5,"highlight_end":6}],"label":"not found in this scope","suggested_replacement":null,"suggestion_applicability":null,"expansion":null}],"children":[{"message":"consider importing this function","code":null,"level":"help","spans":[{"file_name":"./tests/everything/use-insert.rs","byte_start":0,"byte_end":0,"line_start":1,"line_end":1,"column_start":1,"column_end":1,"is_primary":true,"text":[],"label":null,"suggested_replacement":"use a::f;\n\n","suggestion_applicability":"MaybeIncorrect","expansion":null}],"children":[],"rendered":null}],"rendered":"error[E0425]: cannot find function `f` in this scope\n --> ./tests/everything/use-insert.rs:6:5\n  |\n6 |     f();\n  |     ^ not found in this scope\n  |\nhelp: consider importing this function\n  |\n1 + use a::f;\n  |\n\n"}
+{"$message_type":"diagnostic","message":"aborting due to 1 previous error","code":null,"level":"error","spans":[],"children":[],"rendered":"error: aborting due to 1 previous error\n\n"}
+{"$message_type":"diagnostic","message":"For more information about this error, try `rustc --explain E0425`.","code":null,"level":"failure-note","spans":[],"children":[],"rendered":"For more information about this error, try `rustc --explain E0425`.\n"}

crates/rustfix/tests/everything/use-insert.json

@@ -0,0 +1,7 @@
+mod a {
+    pub fn f() {}
+}
+
+fn main() {
+    f();
+}

crates/rustfix/tests/everything/use-insert.rs

@@ -1,3 +1,25 @@
+//! Tests that verify rustfix applies the appropriate changes to a file.
+//!
+//! This test works by reading a series of `*.rs` files in the
+//! `tests/everything` directory. For each `.rs` file, it runs `rustc` to
+//! collect JSON diagnostics from the file. It feeds that JSON data into
+//! rustfix and applies the recommended suggestions to the `.rs` file. It then
+//! compares the result with the corresponding `.fixed.rs` file. If they don't
+//! match, then the test fails.
+//!
+//! There are several debugging environment variables for this test that you can set:
+//!
+//! - `RUST_LOG=parse_and_replace=debug`: Print debug information.
+//! - `RUSTFIX_TEST_BLESS=test-name.rs`: When given the name of a test, this
+//!   will overwrite the `.json` and `.fixed.rs` files with the expected
+//!   values. This can be used when adding a new test.
+//! - `RUSTFIX_TEST_RECORD_JSON=1`:  Records the JSON output to
+//!   `*.recorded.json` files. You can then move that to `.json` or whatever
+//!   you need.
+//! - `RUSTFIX_TEST_RECORD_FIXED_RUST=1`: Records the fixed result to
+//!   `*.recorded.rs` files. You can then move that to `.rs` or whatever you
+//!   need.
+
 #![allow(clippy::disallowed_methods, clippy::print_stdout, clippy::print_stderr)]
 
 use anyhow::{anyhow, ensure, Context, Error};
@@ -20,6 +42,7 @@ mod settings {
     pub const CHECK_JSON: &str = "RUSTFIX_TEST_CHECK_JSON";
     pub const RECORD_JSON: &str = "RUSTFIX_TEST_RECORD_JSON";
     pub const RECORD_FIXED_RUST: &str = "RUSTFIX_TEST_RECORD_FIXED_RUST";
+    pub const BLESS: &str = "RUSTFIX_TEST_BLESS";
 }
 
 fn compile(file: &Path) -> Result<Output, Error> {
@@ -79,15 +102,6 @@ fn compiles_without_errors(file: &Path) -> Result<(), Error> {
     }
 }
 
-fn read_file(path: &Path) -> Result<String, Error> {
-    use std::io::Read;
-
-    let mut buffer = String::new();
-    let mut file = fs::File::open(path)?;
-    file.read_to_string(&mut buffer)?;
-    Ok(buffer)
-}
-
 fn diff(expected: &str, actual: &str) -> String {
     use similar::{ChangeTag, TextDiff};
     use std::fmt::Write;
@@ -104,11 +118,7 @@ fn diff(expected: &str, actual: &str) -> String {
                 ChangeTag::Delete => "-",
             };
             if !different {
-                write!(
-                    &mut res,
-                    "differences found (+ == actual, - == expected):\n"
-                )
-                .unwrap();
+                writeln!(&mut res, "differences found (+ == actual, - == expected):").unwrap();
                 different = true;
             }
             write!(&mut res, "{} {}", prefix, change.value()).unwrap();
@@ -133,23 +143,19 @@ fn test_rustfix_with_file<P: AsRef<Path>>(file: P, mode: &str) -> Result<(), Err
     };
 
     debug!("next up: {:?}", file);
-    let code = read_file(file).context(format!("could not read {}", file.display()))?;
+    let code = fs::read_to_string(file)?;
     let errors =
         compile_and_get_json_errors(file).context(format!("could compile {}", file.display()))?;
     let suggestions =
         rustfix::get_suggestions_from_json(&errors, &HashSet::new(), filter_suggestions)
             .context("could not load suggestions")?;
 
     if std::env::var(settings::RECORD_JSON).is_ok() {
-        use std::io::Write;
-        let mut recorded_json = fs::File::create(&file.with_extension("recorded.json")).context(
-            format!("could not create recorded.json for {}", file.display()),
-        )?;
-        recorded_json.write_all(errors.as_bytes())?;
+        fs::write(file.with_extension("recorded.json"), &errors)?;
     }
 
     if std::env::var(settings::CHECK_JSON).is_ok() {
-        let expected_json = read_file(&json_file).context(format!(
+        let expected_json = fs::read_to_string(&json_file).context(format!(
             "could not load json fixtures for {}",
             file.display()
         ))?;
@@ -168,16 +174,23 @@ fn test_rustfix_with_file<P: AsRef<Path>>(file: P, mode: &str) -> Result<(), Err
     }
 
     let fixed = apply_suggestions(&code, &suggestions)
-        .context(format!("could not apply suggestions to {}", file.display()))?;
+        .context(format!("could not apply suggestions to {}", file.display()))?
+        .replace('\r', "");
 
     if std::env::var(settings::RECORD_FIXED_RUST).is_ok() {
-        use std::io::Write;
-        let mut recorded_rust = fs::File::create(&file.with_extension("recorded.rs"))?;
-        recorded_rust.write_all(fixed.as_bytes())?;
+        fs::write(file.with_extension("recorded.rs"), &fixed)?;
+    }
+
+    if let Some(bless_name) = std::env::var_os(settings::BLESS) {
+        if bless_name == file.file_name().unwrap() {
+            std::fs::write(&json_file, &errors)?;
+            std::fs::write(&fixed_file, &fixed)?;
+        }
     }
 
-    let expected_fixed =
-        read_file(&fixed_file).context(format!("could read fixed file for {}", file.display()))?;
+    let expected_fixed = fs::read_to_string(&fixed_file)
+        .context(format!("could read fixed file for {}", file.display()))?
+        .replace('\r', "");
     ensure!(
         fixed.trim() == expected_fixed.trim(),
         "file {} doesn't look fixed:\n{}",
@@ -191,8 +204,7 @@ fn test_rustfix_with_file<P: AsRef<Path>>(file: P, mode: &str) -> Result<(), Err
 }
 
 fn get_fixture_files(p: &str) -> Result<Vec<PathBuf>, Error> {
-    Ok(fs::read_dir(&p)?
-        .into_iter()
+    Ok(fs::read_dir(p)?
         .map(|e| e.unwrap().path())
         .filter(|p| p.is_file())
         .filter(|p| {
@@ -203,7 +215,7 @@ fn get_fixture_files(p: &str) -> Result<Vec<PathBuf>, Error> {
 }
 
 fn assert_fixtures(dir: &str, mode: &str) {
-    let files = get_fixture_files(&dir)
+    let files = get_fixture_files(dir)
         .context(format!("couldn't load dir `{}`", dir))
         .unwrap();
     let mut failures = 0;