Merge pull request #2457 from rbtcollins/clippy

Fix all outstanding lints and document expectations

Author: rbtcollins

CONTRIBUTING.md

@@ -2,9 +2,11 @@
 
 1. Fork it!
 2. Create your feature branch: `git checkout -b my-new-feature`
-3. Commit your changes: `git commit -am 'Add some feature'`
-4. Push to the branch: `git push origin my-new-feature`
-5. Submit a pull request :D
+3. Test it: `cargo test`
+4. Lint it: `cargo +nightly clippy --all-targets -- -D warnings`
+5. Commit your changes: `git commit -am 'Add some feature'`
+6. Push to the branch: `git push origin my-new-feature`
+7. Submit a pull request :D
 
 For developing on `rustup` itself, you may want to install into a temporary
 directory, with a series of commands similar to this:
@@ -60,6 +62,34 @@ The `rustup::currentprocess` module abstracts the global state that is
 `std::env::current_dir` and `std::process::exit` permitting threaded tests of
 the CLI logic; use `process()` rather than those APIs directly.
 
+### Clippy lints
+
+We do not enforce lint status in the checks done by GitHub Actions, because
+clippy is a moving target that can make it hard to merge for little benefit.
+
+We do ask that contributors keep the clippy status clean themselves.
+
+Minimally, run `cargo +nightly clippy --all-targets -- -D warnings` before
+submitting code.
+
+Regular contributors or contributors to particularly OS-specific code should
+also make sure that their clippy checking is done on at least Linux and Windows,
+as OS-conditional code is a common source of unused imports and other small
+lints, which can build up over time.
+
+For developers using BSD/Linux/Mac OS, there are Windows VM's suitable for such
+development tasks for use with virtualbox and other hypervisors are downloadable
+from
+[Microsoft](https://developer.microsoft.com/en-us/windows/downloads/virtual-machines/).
+Similarly, there are many Linux and Unix operating systems images available for
+developers whose usual operating system is Windows. Currently Rustup has no Mac
+OS specific code, so there should be no need to worry about Mac VM images.
+
+Clippy is also run in GitHub Actions, in the 'General Checks / Checks` build
+task, but not currently run per-platform, which means there is no way to find
+out the status of clippy per platform without running it on that platform as a
+developer.
+
 ## Version numbers
 
 If you ever see a released version of rustup which has `::` in its version string

src/cli/common.rs

@@ -183,7 +183,7 @@ fn show_channel_updates(
 ) -> Result<()> {
     let data = toolchains.into_iter().map(|(name, result)| {
         let toolchain = cfg.get_toolchain(&name, false).unwrap();
-        let mut version: String = toolchain.rustc_version().into();
+        let mut version: String = toolchain.rustc_version();
 
         let banner;
         let color;
@@ -486,12 +486,12 @@ pub fn list_toolchains(cfg: &Cfg, verbose: bool) -> Result<utils::ExitCode> {
             String::new()
         };
         for toolchain in toolchains {
-            let if_default = if def_toolchain_name == &*toolchain {
+            let if_default = if def_toolchain_name == *toolchain {
                 " (default)"
             } else {
                 ""
             };
-            let if_override = if ovr_toolchain_name == &*toolchain {
+            let if_override = if ovr_toolchain_name == *toolchain {
                 " (override)"
             } else {
                 ""

src/cli/self_update.rs

@@ -1149,7 +1149,7 @@ info: default host triple is {0}
             vars,
             ..Default::default()
         });
-        currentprocess::with(tp.clone(), || -> anyhow::Result<()> {
+        currentprocess::with(tp, || -> anyhow::Result<()> {
             super::install_bins().unwrap();
             Ok(())
         })

src/cli/self_update/windows.rs

@@ -214,7 +214,7 @@ fn get_windows_path_var() -> Result<Option<String>> {
             } else {
                 warn!("the registry key HKEY_CURRENT_USER\\Environment\\PATH does not contain valid Unicode. \
                        Not modifying the PATH variable");
-                return Ok(None);
+                Ok(None)
             }
         }
         Err(ref e) if e.kind() == io::ErrorKind::NotFound => Ok(Some(String::new())),
@@ -230,7 +230,7 @@ fn _add_to_path(old_path: &str, path_str: String) -> Option<String> {
     } else if old_path.contains(&path_str) {
         None
     } else {
-        let mut new_path = path_str.clone();
+        let mut new_path = path_str;
         new_path.push_str(";");
         new_path.push_str(&old_path);
         Some(new_path)
@@ -471,8 +471,11 @@ mod tests {
                     .unwrap();
                 environment.delete_value("PATH").unwrap();
 
-                assert_eq!((), super::_apply_new_path(Some("foo".into())).unwrap());
-
+                {
+                    // Can't compare the Results as Eq isn't derived; thanks error-chain.
+                    #![allow(clippy::unit_cmp)]
+                    assert_eq!((), super::_apply_new_path(Some("foo".into())).unwrap());
+                }
                 let root = RegKey::predef(HKEY_CURRENT_USER);
                 let environment = root
                     .open_subkey_with_flags("Environment", KEY_READ | KEY_WRITE)
@@ -506,8 +509,11 @@ mod tests {
                     )
                     .unwrap();
 
-                assert_eq!((), super::_apply_new_path(Some("".into())).unwrap());
-
+                {
+                    // Can't compare the Results as Eq isn't derived; thanks error-chain.
+                    #![allow(clippy::unit_cmp)]
+                    assert_eq!((), super::_apply_new_path(Some("".into())).unwrap());
+                }
                 let reg_value = environment.get_raw_value("PATH");
                 match reg_value {
                     Ok(_) => panic!("key not deleted"),

src/cli/setup_mode.rs

@@ -101,7 +101,7 @@ pub fn main() -> Result<utils::ExitCode> {
             writeln!(process().stdout().lock(), "{}", e.message)?;
             return Ok(utils::ExitCode(0));
         }
-        Err(e) => Err(e)?,
+        Err(e) => return Err(e.into()),
     };
     let no_prompt = matches.is_present("no-prompt");
     let verbose = matches.is_present("verbose");

src/cli/term2.rs

@@ -76,26 +76,26 @@ mod termhack {
     /// opened.
     #[allow(unused)]
     pub fn stdout(terminfo: Option<TermInfo>) -> Option<Box<StdoutTerminal>> {
-        make_terminal(terminfo, || io::stdout())
+        make_terminal(terminfo, io::stdout)
     }
 
     /// Return a Terminal wrapping stderr, or None if a terminal couldn't be
     /// opened.
     #[allow(unused)]
     pub fn stderr(terminfo: Option<TermInfo>) -> Option<Box<StderrTerminal>> {
-        make_terminal(terminfo, || io::stderr())
+        make_terminal(terminfo, io::stderr)
     }
 
     /// Return a Terminal wrapping stdout.
     #[allow(unused)]
     pub fn stdout_with_fallback(terminfo: Option<TermInfo>) -> Box<StdoutTerminal> {
-        make_terminal_with_fallback(terminfo, || io::stdout())
+        make_terminal_with_fallback(terminfo, io::stdout)
     }
 
     /// Return a Terminal wrapping stderr.
     #[allow(unused)]
     pub fn stderr_with_fallback(terminfo: Option<TermInfo>) -> Box<StderrTerminal> {
-        make_terminal_with_fallback(terminfo, || io::stderr())
+        make_terminal_with_fallback(terminfo, io::stderr)
     }
 }
 

src/config.rs

@@ -585,7 +585,7 @@ impl Cfg {
         let contents = contents.as_ref();
 
         match contents.lines().count() {
-            0 => return Err(ErrorKind::EmptyOverrideFile.into()),
+            0 => Err(ErrorKind::EmptyOverrideFile.into()),
             1 => {
                 let channel = contents.trim();
 

src/currentprocess.rs

@@ -147,8 +147,8 @@ where
     });
 
     PROCESS.with(|p| {
-        if let Some(_) = *p.borrow() {
-            panic!("current process already set {}");
+        if let Some(old_p) = &*p.borrow() {
+            panic!("current process already set {:?}", old_p);
         }
         *p.borrow_mut() = Some(process);
         let result = f();

src/dist/manifest.rs

@@ -440,7 +440,7 @@ impl TargetedPackage {
         if !extensions.is_empty() {
             result.insert("extensions".to_owned(), toml::Value::Array(extensions));
         }
-        if let Some(bins) = self.bins.clone() {
+        if let Some(bins) = self.bins {
             result.insert("hash".to_owned(), toml::Value::String(bins.hash));
             result.insert("url".to_owned(), toml::Value::String(bins.url));
             if let (Some(xz_hash), Some(xz_url)) = (bins.xz_hash, bins.xz_url) {

src/dist/notifications.rs

@@ -182,7 +182,7 @@ impl<'a> Display for Notification<'a> {
                     .iter()
                     .map(|component| {
                         if component.target.as_ref() != Some(&toolchain.target) {
-                            component.name_in_manifest().to_owned()
+                            component.name_in_manifest()
                         } else {
                             component.short_name_in_manifest().to_owned()
                         }