From f89806c9cfb6b87a32cc0b77723adb89d99d81dc Mon Sep 17 00:00:00 2001 From: "Filip Demski (Glamhoth)" Date: Thu, 9 May 2019 10:27:44 +0200 Subject: [PATCH 1/3] clitools.rs: Add expect_not_stderr_ok In order to support testing commands which may return certain messages on `stderr` in a way in which they should *NOT* produce such messages, add a counterpart to `expect_not_stdout_ok`. --- tests/mock/clitools.rs | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/tests/mock/clitools.rs b/tests/mock/clitools.rs index 6bdcca49f6..75978e4dcb 100644 --- a/tests/mock/clitools.rs +++ b/tests/mock/clitools.rs @@ -207,6 +207,16 @@ pub fn expect_not_stdout_ok(config: &Config, args: &[&str], expected: &str) { } } +pub fn expect_not_stderr_ok(config: &Config, args: &[&str], expected: &str) { + let out = run(config, args[0], &args[1..], &[]); + if out.ok || out.stderr.contains(expected) { + print_command(args, &out); + println!("expected.ok: false"); + print_indented("expected.stderr.does_not_contain", expected); + panic!(); + } +} + pub fn expect_stderr_ok(config: &Config, args: &[&str], expected: &str) { let out = run(config, args[0], &args[1..], &[]); if !out.ok || !out.stderr.contains(expected) { From 0384166a0eb23138522d644bdcda2fdc8ae24f63 Mon Sep 17 00:00:00 2001 From: "Filip (Glamhoth) Demski" Date: Wed, 1 May 2019 12:46:54 +0200 Subject: [PATCH 2/3] =?UTF-8?q?Implemented=20suggestion=20based=20on=20Dam?= =?UTF-8?q?erau=E2=80=93Levenshtein=20distance?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- Cargo.lock | 7 +++ Cargo.toml | 1 + src/dist/manifest.rs | 7 +++ src/errors.rs | 4 +- src/toolchain.rs | 122 +++++++++++++++++++++++++++++++++++++++++++ tests/cli-v2.rs | 100 ++++++++++++++++++++++++++++++++++- 6 files changed, 237 insertions(+), 4 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index d3e8f63ed4..8762993cde 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1249,6 +1249,7 @@ dependencies = [ "scopeguard 1.0.0 (registry+https://github.com/rust-lang/crates.io-index)", "semver 0.9.0 (registry+https://github.com/rust-lang/crates.io-index)", "sha2 0.8.0 (registry+https://github.com/rust-lang/crates.io-index)", + "strsim 0.9.2 (registry+https://github.com/rust-lang/crates.io-index)", "tar 0.4.24 (registry+https://github.com/rust-lang/crates.io-index)", "tempdir 0.3.7 (registry+https://github.com/rust-lang/crates.io-index)", "term 0.5.1 (registry+https://github.com/rust-lang/crates.io-index)", @@ -1430,6 +1431,11 @@ name = "strsim" version = "0.8.0" source = "registry+https://github.com/rust-lang/crates.io-index" +[[package]] +name = "strsim" +version = "0.9.2" +source = "registry+https://github.com/rust-lang/crates.io-index" + [[package]] name = "syn" version = "0.15.33" @@ -2044,6 +2050,7 @@ dependencies = [ "checksum stable_deref_trait 1.1.1 (registry+https://github.com/rust-lang/crates.io-index)" = "dba1a27d3efae4351c8051072d619e3ade2820635c3958d826bfea39d59b54c8" "checksum string 0.1.3 (registry+https://github.com/rust-lang/crates.io-index)" = "b639411d0b9c738748b5397d5ceba08e648f4f1992231aa859af1a017f31f60b" "checksum strsim 0.8.0 (registry+https://github.com/rust-lang/crates.io-index)" = "8ea5119cdb4c55b55d432abb513a0429384878c15dde60cc77b1c99de1a95a6a" +"checksum strsim 0.9.2 (registry+https://github.com/rust-lang/crates.io-index)" = "032c03039aae92b350aad2e3779c352e104d919cb192ba2fabbd7b831ce4f0f6" "checksum syn 0.15.33 (registry+https://github.com/rust-lang/crates.io-index)" = "ec52cd796e5f01d0067225a5392e70084acc4c0013fa71d55166d38a8b307836" "checksum synstructure 0.10.1 (registry+https://github.com/rust-lang/crates.io-index)" = "73687139bf99285483c96ac0add482c3776528beac1d97d444f6e91f203a2015" "checksum tar 0.4.24 (registry+https://github.com/rust-lang/crates.io-index)" = "2dd071a2604c8fd8902ca42908856821ed7a06e3cd846f84d75873c978dec7fb" diff --git a/Cargo.toml b/Cargo.toml index 1fbce99b48..89eadec5b8 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -42,6 +42,7 @@ same-file = "1" scopeguard = "1" semver = "0.9" sha2 = "0.8" +strsim = "0.9.1" tar = "0.4" tempdir = "0.3.4" # FIXME(issue #1788) diff --git a/src/dist/manifest.rs b/src/dist/manifest.rs index 9c861ea0e1..0017902695 100644 --- a/src/dist/manifest.rs +++ b/src/dist/manifest.rs @@ -448,4 +448,11 @@ impl Component { pkg.to_string() } } + pub fn target(&self) -> String { + if let Some(ref t) = self.target { + format!("{}", t) + } else { + format!("") + } + } } diff --git a/src/errors.rs b/src/errors.rs index f6eccebc71..541f5c2e0d 100644 --- a/src/errors.rs +++ b/src/errors.rs @@ -303,9 +303,9 @@ error_chain! { description("toolchain does not support components") display("toolchain '{}' does not support components", t) } - UnknownComponent(t: String, c: String) { + UnknownComponent(t: String, c: String, s: String) { description("toolchain does not contain component") - display("toolchain '{}' does not contain component {}", t, c) + display("toolchain '{}' does not contain component {}; did you mean '{}'?", t, c, s) } AddingRequiredComponent(t: String, c: String) { description("required component cannot be added") diff --git a/src/toolchain.rs b/src/toolchain.rs index 237d50f1e6..3abafdc4fd 100644 --- a/src/toolchain.rs +++ b/src/toolchain.rs @@ -2,6 +2,7 @@ use crate::config::Cfg; use crate::dist::dist::ToolchainDesc; use crate::dist::download::DownloadCfg; use crate::dist::manifest::Component; +use crate::dist::manifest::Manifest; use crate::dist::manifestation::{Changes, Manifestation}; use crate::dist::prefix::InstallPrefix; use crate::env_var; @@ -10,6 +11,7 @@ use crate::install::{self, InstallMethod}; use crate::notifications::*; use crate::utils::utils; +use std::collections::HashMap; use std::env; use std::env::consts::EXE_SUFFIX; use std::ffi::OsStr; @@ -17,6 +19,7 @@ use std::ffi::OsString; use std::path::{Path, PathBuf}; use std::process::Command; +use strsim::damerau_levenshtein; use url::Url; /// A fully resolved reference to a toolchain which may or may not exist @@ -556,6 +559,111 @@ impl<'a> Toolchain<'a> { } } + fn find_most_similar_component( + &self, + component: &Component, + manifest: &Manifest, + collection: &Vec, + ) -> String { + let short_name_distances: HashMap = collection + .iter() + .map(|s| { + ( + damerau_levenshtein(&component.short_name(manifest), &s.short_name(&manifest)), + s, + ) + }) + .collect(); + + let long_name_distances: HashMap = collection + .iter() + .map(|s| { + ( + damerau_levenshtein( + &component.short_name(manifest), + &s.short_name_in_manifest(), + ), + s, + ) + }) + .collect(); + + let mut least_distance = std::usize::MAX; + let mut closest_component = component; + let mut closest_match = "".to_string(); + + for (k, v) in &short_name_distances { + if k < &least_distance { + least_distance = *k; + closest_component = *v; + closest_match = v.short_name(manifest).to_string(); + } + } + + for (k, v) in &long_name_distances { + if k < &least_distance { + least_distance = *k; + closest_component = *v; + closest_match = v.short_name_in_manifest().to_string(); + } + } + + if component.short_name(manifest) == closest_component.short_name(manifest) { + let target_distances: HashMap = collection + .iter() + .filter(|s| s.short_name(manifest) == component.short_name(manifest)) + .map(|s| (damerau_levenshtein(&component.target(), &s.target()), s)) + .collect(); + + let long_name_distance = components + .iter() + .filter(|c| !only_instaled || c.installed) + .map(|c| { + ( + damerau_levenshtein( + &c.component.name_in_manifest()[..], + &component.name(manifest)[..], + ), + c, + ) + }) + .min_by_key(|t| t.0) + .expect("There should be always at least one component"); + + let mut closest_distance = short_name_distance; + let mut closest_match = short_name_distance.1.component.short_name(manifest); + + // Find closer suggestion + if short_name_distance.0 > long_name_distance.0 { + closest_distance = long_name_distance; + + // Check if only targets differ + if closest_distance.1.component.short_name_in_manifest() + == component.short_name_in_manifest() + { + closest_match = long_name_distance.1.component.target(); + } else { + closest_match = long_name_distance + .1 + .component + .short_name_in_manifest() + .to_string(); + } + } else { + // Check if only targets differ + if closest_distance.1.component.short_name(manifest) + == component.short_name(manifest) + { + closest_match = short_name_distance.1.component.target().to_string(); + } + } + + return target_closest_match; + } else { + return closest_match; + } + } + pub fn add_component(&self, mut component: Component) -> Result<()> { if !self.exists() { return Err(ErrorKind::ToolchainNotInstalled(self.name.to_owned()).into()); @@ -601,9 +709,16 @@ impl<'a> Toolchain<'a> { if targ_pkg.extensions.contains(&wildcard_component) { component = wildcard_component; } else { + let most_similar_component = self.find_most_similar_component( + &component, + &manifest, + &targ_pkg.extensions, + ); + return Err(ErrorKind::UnknownComponent( self.name.to_string(), component.description(&manifest), + most_similar_component, ) .into()); } @@ -671,9 +786,16 @@ impl<'a> Toolchain<'a> { if dist_config.components.contains(&wildcard_component) { component = wildcard_component; } else { + let most_similar_component = self.find_most_similar_component( + &component, + &manifest, + &dist_config.components, + ); + return Err(ErrorKind::UnknownComponent( self.name.to_string(), component.description(&manifest), + most_similar_component, ) .into()); } diff --git a/tests/cli-v2.rs b/tests/cli-v2.rs index 0fe2a9cbda..fb16bbd189 100644 --- a/tests/cli-v2.rs +++ b/tests/cli-v2.rs @@ -4,8 +4,8 @@ pub mod mock; use crate::mock::clitools::{ - self, expect_err, expect_not_stdout_ok, expect_ok, expect_stderr_ok, expect_stdout_ok, - set_current_dist_date, this_host_triple, Config, Scenario, + self, expect_err, expect_not_stderr_ok, expect_not_stdout_ok, expect_ok, expect_stderr_ok, + expect_stdout_ok, set_current_dist_date, this_host_triple, Config, Scenario, }; use std::fs; use std::io::Write; @@ -920,3 +920,99 @@ fn update_unavailable_force() { ); }); } + +#[test] +fn add_component_suggest_best_match() { + setup(&|config| { + expect_ok(config, &["rustup", "default", "nightly"]); + expect_err( + config, + &["rustup", "component", "add", "rsl"], + "did you mean 'rls'?", + ); + expect_err( + config, + &["rustup", "component", "add", "rsl-preview"], + "did you mean 'rls-preview'?", + ); + expect_not_stderr_ok( + config, + &["rustup", "component", "add", "potato"], + "did you mean", + ); + }); +} + +#[test] +fn remove_component_suggest_best_match() { + setup(&|config| { + expect_ok(config, &["rustup", "default", "nightly"]); + expect_not_stderr_ok( + config, + &["rustup", "component", "remove", "rsl"], + "did you mean 'rls'?", + ); + expect_ok(config, &["rustup", "component", "add", "rls"]); + expect_err( + config, + &["rustup", "component", "remove", "rsl"], + "did you mean 'rls'?", + ); + expect_ok(config, &["rustup", "component", "add", "rls-preview"]); + expect_err( + config, + &["rustup", "component", "add", "rsl-preview"], + "did you mean 'rls-preview'?", + ); + }); +} + +#[test] +fn add_target_suggest_best_match() { + setup(&|config| { + expect_ok(config, &["rustup", "default", "nightly"]); + expect_err( + config, + &[ + "rustup", + "target", + "add", + &clitools::CROSS_ARCH1.replace("x", "y"), + ], + &format!("did you mean '{}'", clitools::CROSS_ARCH1), + ); + expect_not_stderr_ok( + config, + &["rustup", "target", "add", "potato"], + "did you mean", + ); + }); +} + +#[test] +fn remove_target_suggest_best_match() { + setup(&|config| { + expect_ok(config, &["rustup", "default", "nightly"]); + expect_not_stderr_ok( + config, + &[ + "rustup", + "target", + "remove", + &format!("{}a", clitools::CROSS_ARCH1)[..], + ], + &format!("did you mean '{}'", clitools::CROSS_ARCH1), + ); + expect_ok(config, &["rustup", "target", "add", clitools::CROSS_ARCH1]); + expect_err( + config, + &[ + "rustup", + "target", + "remove", + &clitools::CROSS_ARCH1.replace("x", "y"), + ], + &format!("did you mean '{}'", clitools::CROSS_ARCH1), + ); + }); +} From d4c1eb8ad820e5ef2c16b9f51f55170b2227eb53 Mon Sep 17 00:00:00 2001 From: retep007 Date: Mon, 6 May 2019 12:05:28 +0200 Subject: [PATCH 3/3] Merged changes from retep007 --- src/errors.rs | 8 +++- src/toolchain.rs | 100 +++++++++++++++-------------------------------- tests/cli-v2.rs | 14 ++++++- 3 files changed, 50 insertions(+), 72 deletions(-) diff --git a/src/errors.rs b/src/errors.rs index 541f5c2e0d..fabff0a267 100644 --- a/src/errors.rs +++ b/src/errors.rs @@ -303,9 +303,13 @@ error_chain! { description("toolchain does not support components") display("toolchain '{}' does not support components", t) } - UnknownComponent(t: String, c: String, s: String) { + UnknownComponent(t: String, c: String, s: Option) { description("toolchain does not contain component") - display("toolchain '{}' does not contain component {}; did you mean '{}'?", t, c, s) + display("toolchain '{}' does not contain component {}{}", t, c, if let Some(suggestion) = s { + format!("; did you mean '{}'?", suggestion) + } else { + "".to_string() + }) } AddingRequiredComponent(t: String, c: String) { description("required component cannot be added") diff --git a/src/toolchain.rs b/src/toolchain.rs index 3abafdc4fd..f66d306acd 100644 --- a/src/toolchain.rs +++ b/src/toolchain.rs @@ -11,7 +11,6 @@ use crate::install::{self, InstallMethod}; use crate::notifications::*; use crate::utils::utils; -use std::collections::HashMap; use std::env; use std::env::consts::EXE_SUFFIX; use std::ffi::OsStr; @@ -19,7 +18,6 @@ use std::ffi::OsString; use std::path::{Path, PathBuf}; use std::process::Command; -use strsim::damerau_levenshtein; use url::Url; /// A fully resolved reference to a toolchain which may or may not exist @@ -559,65 +557,38 @@ impl<'a> Toolchain<'a> { } } - fn find_most_similar_component( + fn get_component_suggestion( &self, component: &Component, manifest: &Manifest, - collection: &Vec, - ) -> String { - let short_name_distances: HashMap = collection - .iter() - .map(|s| { - ( - damerau_levenshtein(&component.short_name(manifest), &s.short_name(&manifest)), - s, - ) - }) - .collect(); - - let long_name_distances: HashMap = collection - .iter() - .map(|s| { - ( - damerau_levenshtein( - &component.short_name(manifest), - &s.short_name_in_manifest(), - ), - s, - ) - }) - .collect(); - - let mut least_distance = std::usize::MAX; - let mut closest_component = component; - let mut closest_match = "".to_string(); - - for (k, v) in &short_name_distances { - if k < &least_distance { - least_distance = *k; - closest_component = *v; - closest_match = v.short_name(manifest).to_string(); - } - } + only_installed: bool, + ) -> Option { + use strsim::damerau_levenshtein; - for (k, v) in &long_name_distances { - if k < &least_distance { - least_distance = *k; - closest_component = *v; - closest_match = v.short_name_in_manifest().to_string(); - } - } + // Suggest only for very small differences + // High number can result in innacurate suggestions for short queries e.g. `rls` + const MAX_DISTANCE: usize = 3; - if component.short_name(manifest) == closest_component.short_name(manifest) { - let target_distances: HashMap = collection + let components = self.list_components(); + if let Ok(components) = components { + let short_name_distance = components .iter() - .filter(|s| s.short_name(manifest) == component.short_name(manifest)) - .map(|s| (damerau_levenshtein(&component.target(), &s.target()), s)) - .collect(); + .filter(|c| !only_installed || c.installed) + .map(|c| { + ( + damerau_levenshtein( + &c.component.name(manifest)[..], + &component.name(manifest)[..], + ), + c, + ) + }) + .min_by_key(|t| t.0) + .expect("There should be always at least one component"); let long_name_distance = components .iter() - .filter(|c| !only_instaled || c.installed) + .filter(|c| !only_installed || c.installed) .map(|c| { ( damerau_levenshtein( @@ -658,9 +629,14 @@ impl<'a> Toolchain<'a> { } } - return target_closest_match; + // If suggestion is too different don't suggest anything + if closest_distance.0 > MAX_DISTANCE { + return None; + } else { + return Some(closest_match.to_string()); + } } else { - return closest_match; + return None; } } @@ -709,16 +685,10 @@ impl<'a> Toolchain<'a> { if targ_pkg.extensions.contains(&wildcard_component) { component = wildcard_component; } else { - let most_similar_component = self.find_most_similar_component( - &component, - &manifest, - &targ_pkg.extensions, - ); - return Err(ErrorKind::UnknownComponent( self.name.to_string(), component.description(&manifest), - most_similar_component, + self.get_component_suggestion(&component, &manifest, false), ) .into()); } @@ -786,16 +756,10 @@ impl<'a> Toolchain<'a> { if dist_config.components.contains(&wildcard_component) { component = wildcard_component; } else { - let most_similar_component = self.find_most_similar_component( - &component, - &manifest, - &dist_config.components, - ); - return Err(ErrorKind::UnknownComponent( self.name.to_string(), component.description(&manifest), - most_similar_component, + self.get_component_suggestion(&component, &manifest, true), ) .into()); } diff --git a/tests/cli-v2.rs b/tests/cli-v2.rs index fb16bbd189..77e1c38e8d 100644 --- a/tests/cli-v2.rs +++ b/tests/cli-v2.rs @@ -935,6 +935,11 @@ fn add_component_suggest_best_match() { &["rustup", "component", "add", "rsl-preview"], "did you mean 'rls-preview'?", ); + expect_err( + config, + &["rustup", "component", "add", "rustd"], + "did you mean 'rustc'?", + ); expect_not_stderr_ok( config, &["rustup", "component", "add", "potato"], @@ -964,6 +969,11 @@ fn remove_component_suggest_best_match() { &["rustup", "component", "add", "rsl-preview"], "did you mean 'rls-preview'?", ); + expect_err( + config, + &["rustup", "component", "remove", "rustd"], + "did you mean 'rustc'?", + ); }); } @@ -977,7 +987,7 @@ fn add_target_suggest_best_match() { "rustup", "target", "add", - &clitools::CROSS_ARCH1.replace("x", "y"), + &format!("{}a", clitools::CROSS_ARCH1)[..], ], &format!("did you mean '{}'", clitools::CROSS_ARCH1), ); @@ -1010,7 +1020,7 @@ fn remove_target_suggest_best_match() { "rustup", "target", "remove", - &clitools::CROSS_ARCH1.replace("x", "y"), + &format!("{}a", clitools::CROSS_ARCH1)[..], ], &format!("did you mean '{}'", clitools::CROSS_ARCH1), );