Auto merge of #1385 - Xanewok:rustfmt-modified-lines, r=Xanewok

Return only modified lines in formatting responses

Ticks few boxes in #3. (that's an early issue!)
Closes #334.

This is currently based on rust-lang/rustfmt#3424 and so currently blocked until it merges.

Configures Rustfmt to use `EmitMode::ModifiedLines` and parses the response - this should work with both statically-linked and externally provided Rustfmt.

@alexheretic do you think you could take a look and double-check?

Author: bors

rls/src/actions/format.rs

@@ -6,45 +6,109 @@ use std::fs::File;
 use std::io::Write;
 use std::path::{Path, PathBuf};
 use std::process::{Command, Stdio};
+use std::string::FromUtf8Error;
 
 use log::debug;
+use lsp_types::{Position, Range, TextEdit};
 use rand::{distributions, thread_rng, Rng};
-use rustfmt_nightly::{Config, Input, Session};
+use rustfmt_nightly::{Config, Input, ModifiedLines, NewlineStyle, Session};
 use serde_json;
 
 /// Specifies which `rustfmt` to use.
 #[derive(Clone)]
 pub enum Rustfmt {
-    /// `(path to external `rustfmt`, current working directory to spawn at)`
-    External(PathBuf, PathBuf),
+    /// Externally invoked `rustfmt` process.
+    External { path: PathBuf, cwd: PathBuf },
     /// Statically linked `rustfmt`.
     Internal,
 }
 
+/// Defines a formatting-related error.
+#[derive(Fail, Debug)]
+pub enum Error {
+    /// Generic variant of `Error::Rustfmt` error.
+    #[fail(display = "Formatting could not be completed.")]
+    Failed,
+    #[fail(display = "Could not format source code: {}", _0)]
+    Rustfmt(rustfmt_nightly::ErrorKind),
+    #[fail(display = "Encountered I/O error: {}", _0)]
+    Io(std::io::Error),
+    #[fail(display = "Config couldn't be converted to TOML for Rustfmt purposes: {}", _0)]
+    ConfigTomlOutput(String),
+    #[fail(display = "Formatted output is not valid UTF-8 source: {}", _0)]
+    OutputNotUtf8(FromUtf8Error),
+}
+
+impl From<std::io::Error> for Error {
+    fn from(err: std::io::Error) -> Error {
+        Error::Io(err)
+    }
+}
+
+impl From<FromUtf8Error> for Error {
+    fn from(err: FromUtf8Error) -> Error {
+        Error::OutputNotUtf8(err)
+    }
+}
+
 impl From<Option<(String, PathBuf)>> for Rustfmt {
     fn from(value: Option<(String, PathBuf)>) -> Rustfmt {
         match value {
-            Some((path, cwd)) => Rustfmt::External(PathBuf::from(path), cwd),
+            Some((path, cwd)) => Rustfmt::External { path: PathBuf::from(path), cwd },
             None => Rustfmt::Internal,
         }
     }
 }
 
 impl Rustfmt {
-    pub fn format(&self, input: String, cfg: Config) -> Result<String, String> {
+    pub fn format(&self, input: String, cfg: Config) -> Result<String, Error> {
         match self {
             Rustfmt::Internal => format_internal(input, cfg),
-            Rustfmt::External(path, cwd) => format_external(path, cwd, input, cfg),
+            Rustfmt::External { path, cwd } => format_external(path, cwd, input, cfg),
         }
     }
+
+    pub fn calc_text_edits(&self, input: String, mut cfg: Config) -> Result<Vec<TextEdit>, Error> {
+        cfg.set().emit_mode(rustfmt_nightly::EmitMode::ModifiedLines);
+
+        let native = if cfg!(windows) { "\r\n" } else { "\n" };
+        let newline = match cfg.newline_style() {
+            NewlineStyle::Windows => "\r\n",
+            NewlineStyle::Unix | NewlineStyle::Auto => "\n",
+            NewlineStyle::Native => native,
+        };
+
+        let output = self.format(input, cfg)?;
+        let ModifiedLines { chunks } = output.parse().map_err(|_| Error::Failed)?;
+
+        Ok(chunks
+            .into_iter()
+            .map(|item| {
+                // Rustfmt's line indices are 1-based
+                let start_line = u64::from(item.line_number_orig) - 1;
+                // Could underflow if we don't remove lines and there's only one
+                let removed = u64::from(item.lines_removed).saturating_sub(1);
+                TextEdit {
+                    range: Range {
+                        start: Position::new(start_line, 0),
+                        // We don't extend the range past the last line because
+                        // sometimes it may not exist, skewing the diff and
+                        // making us add an invalid additional trailing newline.
+                        end: Position::new(start_line + removed, u64::max_value()),
+                    },
+                    new_text: item.lines.join(newline),
+                }
+            })
+            .collect())
+    }
 }
 
 fn format_external(
     path: &PathBuf,
     cwd: &PathBuf,
     input: String,
     cfg: Config,
-) -> Result<String, String> {
+) -> Result<String, Error> {
     let (_file_handle, config_path) = gen_config_file(&cfg)?;
     let args = rustfmt_args(&cfg, &config_path);
 
@@ -54,25 +118,18 @@ fn format_external(
         .stdin(Stdio::piped())
         .stdout(Stdio::piped())
         .spawn()
-        .map_err(|_| format!("Couldn't spawn `{}`", path.display()))?;
+        .map_err(Error::Io)?;
 
     {
-        let stdin =
-            rustfmt.stdin.as_mut().ok_or_else(|| "Failed to open rustfmt stdin".to_string())?;
-        stdin
-            .write_all(input.as_bytes())
-            .map_err(|_| "Failed to pass input to rustfmt".to_string())?;
+        let stdin = rustfmt.stdin.as_mut().unwrap(); // Safe because stdin is piped
+        stdin.write_all(input.as_bytes())?;
     }
 
-    rustfmt.wait_with_output().map_err(|err| format!("Error running rustfmt: {}", err)).and_then(
-        |out| {
-            String::from_utf8(out.stdout)
-                .map_err(|_| "Formatted code is not valid UTF-8".to_string())
-        },
-    )
+    let output = rustfmt.wait_with_output()?;
+    Ok(String::from_utf8(output.stdout)?)
 }
 
-fn format_internal(input: String, config: Config) -> Result<String, String> {
+fn format_internal(input: String, config: Config) -> Result<String, Error> {
     let mut buf = Vec::<u8>::new();
 
     {
@@ -85,37 +142,34 @@ fn format_internal(input: String, config: Config) -> Result<String, String> {
                 if session.has_operational_errors() || session.has_parsing_errors() {
                     debug!("reformat: format_input failed: has errors, report = {}", report);
 
-                    return Err("Reformat failed to complete successfully".into());
+                    return Err(Error::Failed);
                 }
             }
             Err(e) => {
                 debug!("Reformat failed: {:?}", e);
 
-                return Err("Reformat failed to complete successfully".into());
+                return Err(Error::Rustfmt(e));
             }
         }
     }
 
-    String::from_utf8(buf).map_err(|_| "Reformat output is not a valid UTF-8".into())
+    Ok(String::from_utf8(buf)?)
 }
 
-fn random_file() -> Result<(File, PathBuf), String> {
+fn random_file() -> Result<(File, PathBuf), Error> {
     const SUFFIX_LEN: usize = 10;
 
     let suffix: String =
         thread_rng().sample_iter(&distributions::Alphanumeric).take(SUFFIX_LEN).collect();
     let path = temp_dir().join(suffix);
 
-    Ok(File::create(&path)
-        .map(|file| (file, path))
-        .map_err(|_| "Config file could not be created".to_string())?)
+    Ok(File::create(&path).map(|file| (file, path))?)
 }
 
-fn gen_config_file(config: &Config) -> Result<(File, PathBuf), String> {
+fn gen_config_file(config: &Config) -> Result<(File, PathBuf), Error> {
     let (mut file, path) = random_file()?;
-    let toml = config.all_options().to_toml()?;
-    file.write(toml.as_bytes())
-        .map_err(|_| "Could not write config TOML file contents".to_string())?;
+    let toml = config.all_options().to_toml().map_err(Error::ConfigTomlOutput)?;
+    file.write_all(toml.as_bytes())?;
 
     Ok((file, path))
 }
@@ -139,3 +193,43 @@ fn rustfmt_args(config: &Config, config_path: &Path) -> Vec<String> {
 
     args
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+    use crate::config::FmtConfig;
+    use lsp_types::{Position, Range, TextEdit};
+
+    #[test]
+    fn calc_text_edits() {
+        let config = || FmtConfig::default().get_rustfmt_config().clone();
+        let format = |x: &str| Rustfmt::Internal.calc_text_edits(x.to_string(), config()).unwrap();
+        let line_range = |start, end| Range {
+            start: Position { line: start, character: 0 },
+            end: Position { line: end, character: u64::max_value() },
+        };
+        // Handle single-line text wrt. added/removed trailing newline
+        assert_eq!(
+            format("fn main() {} "),
+            vec![TextEdit { range: line_range(0, 0), new_text: "fn main() {}\n".to_owned() }]
+        );
+
+        assert_eq!(
+            format("fn main() {} \n"),
+            vec![TextEdit { range: line_range(0, 0), new_text: "fn main() {}".to_owned() }]
+        );
+
+        assert_eq!(
+            format("\nfn main() {} \n"),
+            vec![TextEdit { range: line_range(0, 1), new_text: "fn main() {}".to_owned() }]
+        );
+        // Check that we send two separate edits
+        assert_eq!(
+            format("  struct Upper ;\n\nstruct Lower ;"),
+            vec![
+                TextEdit { range: line_range(0, 0), new_text: "struct Upper;".to_owned() },
+                TextEdit { range: line_range(2, 2), new_text: "struct Lower;\n".to_owned() }
+            ]
+        );
+    }
+}

rls/src/actions/requests.rs

@@ -620,7 +620,7 @@ impl RequestAction for CodeAction {
 }
 
 impl RequestAction for Formatting {
-    type Response = [TextEdit; 1];
+    type Response = Vec<TextEdit>;
 
     fn fallback_response() -> Result<Self::Response, ResponseError> {
         Err(ResponseError::Message(
@@ -638,7 +638,7 @@ impl RequestAction for Formatting {
 }
 
 impl RequestAction for RangeFormatting {
-    type Response = [TextEdit; 1];
+    type Response = Vec<TextEdit>;
 
     fn fallback_response() -> Result<Self::Response, ResponseError> {
         Err(ResponseError::Message(
@@ -660,7 +660,7 @@ fn reformat(
     selection: Option<Range>,
     opts: &FormattingOptions,
     ctx: &InitActionContext,
-) -> Result<[TextEdit; 1], ResponseError> {
+) -> Result<Vec<TextEdit>, ResponseError> {
     ctx.quiescent.store(true, Ordering::SeqCst);
     trace!("Reformat: {:?} {:?} {} {}", doc, selection, opts.tab_size, opts.insert_spaces);
     let path = parse_file_path!(&doc.uri, "reformat")?;
@@ -683,7 +683,6 @@ fn reformat(
         }
     };
 
-    let range_whole_file = ls_util::range_from_file_string(&input);
     let mut config = ctx.fmt_config().get_rustfmt_config().clone();
     if !config.was_set().hard_tabs() {
         config.set().hard_tabs(!opts.insert_spaces);
@@ -722,10 +721,10 @@ fn reformat(
         config.set().file_lines(file_lines);
     };
 
-    let formatted_text = ctx
+    let text_edits = ctx
         .formatter()
-        .format(input, config)
-        .map_err(|msg| ResponseError::Message(ErrorCode::InternalError, msg))?;
+        .calc_text_edits(input, config)
+        .map_err(|msg| ResponseError::Message(ErrorCode::InternalError, msg.to_string()))?;
 
     // Note that we don't need to update the VFS, the client echos back the
     // change to us when it applies the returned `TextEdit`.
@@ -737,9 +736,7 @@ fn reformat(
         ));
     }
 
-    // If Rustfmt returns range of text that changed,
-    // we will be able to pass only range of changed text to the client.
-    Ok([TextEdit { range: range_whole_file, new_text: formatted_text }])
+    Ok(text_edits)
 }
 
 impl RequestAction for ResolveCompletion {

rls/src/lib.rs

@@ -9,6 +9,9 @@
 #![warn(clippy::all, rust_2018_idioms)]
 #![allow(clippy::cyclomatic_complexity, clippy::too_many_arguments, clippy::redundant_closure)]
 
+#[macro_use]
+extern crate failure;
+
 pub use rls_analysis::{AnalysisHost, Target};
 pub use rls_vfs::Vfs;
 

tests/client.rs

@@ -1763,9 +1763,9 @@ fn client_reformat() {
     assert_eq!(result.unwrap()[0], TextEdit {
         range: Range {
             start: Position { line: 0, character: 0 },
-            end: Position { line: 2, character: 0 },
+            end: Position { line: 1, character: u64::max_value() },
         },
-        new_text: "pub mod foo;\npub fn main() {\n    let world = \"world\";\n    println!(\"Hello, {}!\", world);\n}\n".to_string(),
+        new_text: "pub mod foo;\npub fn main() {\n    let world = \"world\";\n    println!(\"Hello, {}!\", world);\n}".to_string(),
     });
 }
 
@@ -1802,17 +1802,14 @@ fn client_reformat_with_range() {
     let newline = if cfg!(windows) { "\r\n" } else { "\n" };
     let formatted = r#"pub fn main() {
     let world1 = "world";
-    println!("Hello, {}!", world1);
-
-// Work around rustfmt#3494
-let world2 = "world"; println!("Hello, {}!", world2);
-let world3 = "world"; println!("Hello, {}!", world3);
-    }
-"#
+    println!("Hello, {}!", world1);"#
     .replace("\r", "")
     .replace("\n", newline);
 
-    assert_eq!(result.unwrap()[0].new_text, formatted);
+    let edits = result.unwrap();
+    assert_eq!(edits.len(), 2);
+    assert_eq!(edits[0].new_text, formatted);
+    assert_eq!(edits[1].new_text, "");
 }
 
 #[test]