From 8ef7ed737522ad50ea7d4ef617d67543927eda0f Mon Sep 17 00:00:00 2001 From: Igor Matuszewski Date: Fri, 1 Mar 2019 12:59:21 +0100 Subject: [PATCH 1/6] Redo errors in format.rs --- rls/src/actions/format.rs | 79 +++++++++++++++++++++++-------------- rls/src/actions/requests.rs | 2 +- rls/src/lib.rs | 3 ++ 3 files changed, 53 insertions(+), 31 deletions(-) diff --git a/rls/src/actions/format.rs b/rls/src/actions/format.rs index 747638fec7b..83f820675f6 100644 --- a/rls/src/actions/format.rs +++ b/rls/src/actions/format.rs @@ -6,6 +6,7 @@ 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 rand::{distributions, thread_rng, Rng}; @@ -15,26 +16,54 @@ 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 for Error { + fn from(err: std::io::Error) -> Error { + Error::Io(err) + } +} + +impl From for Error { + fn from(err: FromUtf8Error) -> Error { + Error::OutputNotUtf8(err) + } +} + impl From> 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 { + pub fn format(&self, input: String, cfg: Config) -> Result { 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), } } } @@ -44,7 +73,7 @@ fn format_external( cwd: &PathBuf, input: String, cfg: Config, -) -> Result { +) -> Result { let (_file_handle, config_path) = gen_config_file(&cfg)?; let args = rustfmt_args(&cfg, &config_path); @@ -54,25 +83,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 { +fn format_internal(input: String, config: Config) -> Result { let mut buf = Vec::::new(); { @@ -85,37 +107,34 @@ fn format_internal(input: String, config: Config) -> Result { 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)) } diff --git a/rls/src/actions/requests.rs b/rls/src/actions/requests.rs index 6e6d0f42222..01bb1946199 100644 --- a/rls/src/actions/requests.rs +++ b/rls/src/actions/requests.rs @@ -725,7 +725,7 @@ fn reformat( let formatted_text = ctx .formatter() .format(input, config) - .map_err(|msg| ResponseError::Message(ErrorCode::InternalError, msg))?; + .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`. diff --git a/rls/src/lib.rs b/rls/src/lib.rs index 0254a2a4c78..13ae562d346 100644 --- a/rls/src/lib.rs +++ b/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; From 927cea9d0e4e47d3804b6dd6bdba93d9c4481951 Mon Sep 17 00:00:00 2001 From: Igor Matuszewski Date: Fri, 1 Mar 2019 20:11:29 +0100 Subject: [PATCH 2/6] Return only partial text edits on formatting request --- rls/src/actions/format.rs | 38 ++++++++++++++++++++++++++++++++++++- rls/src/actions/requests.rs | 15 ++++++--------- tests/client.rs | 2 +- 3 files changed, 44 insertions(+), 11 deletions(-) diff --git a/rls/src/actions/format.rs b/rls/src/actions/format.rs index 83f820675f6..22ad52aeec2 100644 --- a/rls/src/actions/format.rs +++ b/rls/src/actions/format.rs @@ -9,8 +9,9 @@ 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. @@ -66,6 +67,41 @@ impl Rustfmt { Rustfmt::External { path, cwd } => format_external(path, cwd, input, cfg), } } + + pub fn calc_text_edits(&self, input: String, mut cfg: Config) -> Result, 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; + let removed = u64::from(item.lines_removed); + // If there is only one line and we add them, we may underflow. + let removed = if removed == 0 { 0 } else { removed - 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( diff --git a/rls/src/actions/requests.rs b/rls/src/actions/requests.rs index 01bb1946199..5c1f2b0a488 100644 --- a/rls/src/actions/requests.rs +++ b/rls/src/actions/requests.rs @@ -620,7 +620,7 @@ impl RequestAction for CodeAction { } impl RequestAction for Formatting { - type Response = [TextEdit; 1]; + type Response = Vec; fn fallback_response() -> Result { Err(ResponseError::Message( @@ -638,7 +638,7 @@ impl RequestAction for Formatting { } impl RequestAction for RangeFormatting { - type Response = [TextEdit; 1]; + type Response = Vec; fn fallback_response() -> Result { Err(ResponseError::Message( @@ -660,7 +660,7 @@ fn reformat( selection: Option, opts: &FormattingOptions, ctx: &InitActionContext, -) -> Result<[TextEdit; 1], ResponseError> { +) -> Result, 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,9 +721,9 @@ fn reformat( config.set().file_lines(file_lines); }; - let formatted_text = ctx + let text_edits = ctx .formatter() - .format(input, config) + .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 @@ -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 { diff --git a/tests/client.rs b/tests/client.rs index 57ccf8ccbfd..5cf65df84c4 100644 --- a/tests/client.rs +++ b/tests/client.rs @@ -1765,7 +1765,7 @@ fn client_reformat() { start: Position { line: 0, character: 0 }, end: Position { line: 2, character: 0 }, }, - 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(), }); } From 6541e48683fffa6fbaf602c5f1d9e5e93349b2b6 Mon Sep 17 00:00:00 2001 From: Igor Matuszewski Date: Sat, 2 Mar 2019 22:54:30 +0100 Subject: [PATCH 3/6] Prefer saturating_sub --- rls/src/actions/format.rs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/rls/src/actions/format.rs b/rls/src/actions/format.rs index 22ad52aeec2..60b321f1b81 100644 --- a/rls/src/actions/format.rs +++ b/rls/src/actions/format.rs @@ -86,9 +86,8 @@ impl Rustfmt { .map(|item| { // Rustfmt's line indices are 1-based let start_line = u64::from(item.line_number_orig) - 1; - let removed = u64::from(item.lines_removed); - // If there is only one line and we add them, we may underflow. - let removed = if removed == 0 { 0 } else { removed - 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), From 02ee44d7fcedff124516daec4e73e9f893c7d63f Mon Sep 17 00:00:00 2001 From: Igor Matuszewski Date: Sat, 2 Mar 2019 22:54:58 +0100 Subject: [PATCH 4/6] Add unit tests for Rustfmt::calc_text_edits --- rls/src/actions/format.rs | 40 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 40 insertions(+) diff --git a/rls/src/actions/format.rs b/rls/src/actions/format.rs index 60b321f1b81..25acbb6cded 100644 --- a/rls/src/actions/format.rs +++ b/rls/src/actions/format.rs @@ -193,3 +193,43 @@ fn rustfmt_args(config: &Config, config_path: &Path) -> Vec { 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() } + ] + ); + } +} From 7157a3727dd8ab13617b085c06d7679c6090cd99 Mon Sep 17 00:00:00 2001 From: Igor Matuszewski Date: Sat, 2 Mar 2019 23:02:10 +0100 Subject: [PATCH 5/6] Adapt format integration test to use new range --- tests/client.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/client.rs b/tests/client.rs index 5cf65df84c4..27da73a1105 100644 --- a/tests/client.rs +++ b/tests/client.rs @@ -1763,7 +1763,7 @@ 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}".to_string(), }); From e17a9e40d889e1cbc9b813e378d42562f4015773 Mon Sep 17 00:00:00 2001 From: Igor Matuszewski Date: Sun, 7 Apr 2019 00:22:24 +0200 Subject: [PATCH 6/6] Account for changed format range in a test --- tests/client.rs | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/tests/client.rs b/tests/client.rs index 27da73a1105..7628fb70663 100644 --- a/tests/client.rs +++ b/tests/client.rs @@ -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]