From a81b64e8b8cef9f4026ca4bc8d35996cecea1a48 Mon Sep 17 00:00:00 2001 From: Sophie Date: Wed, 19 Jun 2024 12:09:02 -0700 Subject: [PATCH 1/2] chore: Add indexing_slicing lint --- Cargo.toml | 3 + src/download.rs | 92 ++++++++++++++------- src/ops/fuelup_check.rs | 7 +- src/proxy_cli.rs | 6 +- src/toolchain.rs | 164 ++++++++++++++++++++++++++++---------- src/toolchain_override.rs | 1 + 6 files changed, 197 insertions(+), 76 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 4ed0c7706..c95590cf0 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -47,3 +47,6 @@ members = ["component", "ci/build-channel", "ci/compare-versions"] [dev-dependencies] chrono = "0.4.33" strip-ansi-escapes = "0.2.0" + +[lints.clippy] +indexing_slicing = "warn" \ No newline at end of file diff --git a/src/download.rs b/src/download.rs index ddc4bbe2b..d0c9aadb2 100644 --- a/src/download.rs +++ b/src/download.rs @@ -1,26 +1,28 @@ +use crate::{ + channel::{Channel, Package}, + constants::CHANNEL_LATEST_URL, + target_triple::TargetTriple, + toolchain::DistToolchainDescription, +}; use anyhow::{anyhow, bail, Result}; use component::{Component, FUELUP}; use flate2::read::GzDecoder; use indicatif::{FormattedDuration, HumanBytes, HumanDuration, ProgressBar, ProgressStyle}; use semver::Version; use serde::{Deserialize, Serialize}; -use std::env; -use std::fs::{File, OpenOptions}; -use std::io::{Read, Write}; -use std::path::{Path, PathBuf}; -use std::str::FromStr; -use std::time::Duration; -use std::{fs, thread}; +use std::{ + env, + fs::{self, File, OpenOptions}, + io::{Read, Write}, + path::{Path, PathBuf}, + str::FromStr, + thread, + time::Duration, +}; use tar::Archive; use tracing::{debug, error, info, warn}; use ureq::Response; -use crate::channel::Channel; -use crate::channel::Package; -use crate::constants::CHANNEL_LATEST_URL; -use crate::target_triple::TargetTriple; -use crate::toolchain::DistToolchainDescription; - fn github_releases_download_url(repo: &str, tag: &Version, tarball: &str) -> String { format!("https://github.com/FuelLabs/{repo}/releases/download/v{tag}/{tarball}") } @@ -76,15 +78,18 @@ impl DownloadCfg { pub fn from_package(name: &str, package: &Package) -> Result { let target = TargetTriple::from_component(name)?; let tarball_name = tarball_name(name, &package.version, &target); - let tarball_url = package.target[&target.to_string()].url.clone(); - let hash = Some(package.target[&target.to_string()].hash.clone()); + let hashed_binary = package + .target + .get(&target.to_string()) + .ok_or_else(|| anyhow!("No binary for target: {}", target))?; + Ok(Self { name: name.to_string(), target, version: package.version.clone(), tarball_name, - tarball_url, - hash, + tarball_url: hashed_binary.url.clone(), + hash: Some(hashed_binary.hash.clone()), }) } } @@ -121,7 +126,6 @@ pub fn tarball_name(tarball_prefix: &str, version: &Version, target: &TargetTrip pub fn get_latest_version(name: &str) -> Result { let handle = build_agent()?; - let mut data = Vec::new(); if name == FUELUP { const FUELUP_RELEASES_API_URL: &str = @@ -131,7 +135,7 @@ pub fn get_latest_version(name: &str) -> Result { let response: LatestReleaseApiResponse = serde_json::from_str(&String::from_utf8_lossy(&data))?; - let version_str = &response.tag_name["v".len()..]; + let version_str = response.tag_name.trim_start_matches('v'); let version = Version::parse(version_str)?; Ok(version) } else { @@ -183,7 +187,7 @@ pub fn download(url: &str) -> Result> { match handle.get(url).call() { Ok(response) => { let mut data = Vec::new(); - write_response_with_progress_bar(response, &mut data, String::new())?; + response.into_reader().read_to_end(&mut data)?; return Ok(data); } Err(ureq::Error::Status(404, r)) => { @@ -222,7 +226,7 @@ pub fn download_file(url: &str, path: &PathBuf) -> Result<()> { if let Err(e) = write_response_with_progress_bar( response, &mut file, - path.display().to_string(), + &path.display().to_string(), ) { fs::remove_file(path)?; return Err(e); @@ -307,7 +311,7 @@ pub fn unpack_bins(dir: &Path, dst_dir: &Path) -> Result> { fn write_response_with_progress_bar( response: Response, writer: &mut W, - target: String, + target: &str, ) -> Result<()> { let total_size = response .header("Content-Length") @@ -329,7 +333,10 @@ fn write_response_with_progress_bar( if bytes_read == 0 { break; } - if let Err(e) = writer.write_all(&buffer[..bytes_read]) { + let buf = buffer + .get(..bytes_read) + .ok_or_else(|| anyhow!("Failed to read buffer"))?; + if let Err(e) = writer.write_all(buf) { log_progress_bar(&progress_bar); if target.is_empty() { bail!("Something went wrong writing data: {}", e) @@ -370,10 +377,9 @@ fn fuels_version_from_toml(toml: toml_edit::Document) -> Result { if let Some(fuels) = deps.get("fuels") { let version = match fuels.as_value() { Some(toml_edit::Value::String(s)) => s.value().to_string(), - Some(toml_edit::Value::InlineTable(t)) => t.get("version").map_or_else( - || "".to_string(), - |v| v.as_str().unwrap_or_default().to_string(), - ), + Some(toml_edit::Value::InlineTable(t)) => t + .get("version") + .map_or_else(String::new, |v| v.as_str().unwrap_or_default().to_string()), _ => String::default(), }; @@ -525,7 +531,7 @@ fuels = { version = "0.1", features = ["some-feature"] } body, ); let res = s.parse::().unwrap(); - assert!(write_response_with_progress_bar(res, &mut data, String::new()).is_ok()); + assert!(write_response_with_progress_bar(res, &mut data, "").is_ok()); let written_res = String::from_utf8(data)?; assert!(written_res.trim().eq(&body)); Ok(()) @@ -545,7 +551,7 @@ fuels = { version = "0.1", features = ["some-feature"] } ); let res = s.parse::().unwrap(); assert_eq!( - write_response_with_progress_bar(res, &mut mock_writer, String::new()) + write_response_with_progress_bar(res, &mut mock_writer, "") .unwrap_err() .to_string(), "Something went wrong writing data: Mock Interrupted Error" @@ -560,4 +566,32 @@ fuels = { version = "0.1", features = ["some-feature"] } assert!(response.header("Content-Length").is_none()); Ok(()) } + + #[test] + fn test_download_cfg_from_package_missing_target() { + let package = Package { + version: Version::parse("0.1.0").unwrap(), + target: { + let mut map = std::collections::BTreeMap::new(); + map.insert( + "dummy-target-triple".to_string(), + crate::channel::HashedBinary { + url: "https://example.com/forc-0.1.0-x86_64-unknown-linux-gnu.tar.gz" + .to_string(), + hash: "hash".to_string(), + }, + ); + map + }, + fuels_version: None, + }; + let error = DownloadCfg::from_package("forc", &package) + .expect_err("Expected error due to missing target"); + + let expected_target = TargetTriple::from_component("forc").expect("target triple"); + assert_eq!( + error.to_string(), + format!("No binary for target: {}", expected_target) + ); + } } diff --git a/src/ops/fuelup_check.rs b/src/ops/fuelup_check.rs index 00854602c..0a1001878 100644 --- a/src/ops/fuelup_check.rs +++ b/src/ops/fuelup_check.rs @@ -9,7 +9,7 @@ use crate::{ toolchain::{DistToolchainDescription, Toolchain}, }; use ansiterm::Color; -use anyhow::Result; +use anyhow::{anyhow, Result}; use component::{self, Components}; use semver::Version; use std::collections::HashMap; @@ -114,7 +114,10 @@ fn check_toolchain(toolchain: &str, verbose: bool) -> Result<()> { if !plugin.is_main_executable() { print!("{:>2}", ""); - plugin_name = &plugin.executables[index]; + plugin_name = plugin + .executables + .get(index) + .ok_or_else(|| anyhow!("Plugin name not found"))?; } let maybe_latest_version = plugin.publish.map_or_else( diff --git a/src/proxy_cli.rs b/src/proxy_cli.rs index a0c35c5ef..12e93a3b7 100644 --- a/src/proxy_cli.rs +++ b/src/proxy_cli.rs @@ -18,10 +18,10 @@ pub fn proxy_run(arg0: &str) -> Result { let cmd_args: Vec<_> = env::args_os().skip(1).collect(); let toolchain = Toolchain::from_settings()?; - if !cmd_args.is_empty() { - let plugin = format!("{}-{}", arg0, &cmd_args[0].to_string_lossy()); + if let Some(first_arg) = cmd_args.first() { + let plugin = format!("{}-{}", arg0, first_arg.to_string_lossy()); if Components::collect_plugin_executables()?.contains(&plugin) { - direct_proxy(&plugin, &cmd_args[1..], &toolchain)?; + direct_proxy(&plugin, cmd_args.get(1..).unwrap_or_default(), &toolchain)?; } } diff --git a/src/toolchain.rs b/src/toolchain.rs index 743a2a498..ccd9c873e 100644 --- a/src/toolchain.rs +++ b/src/toolchain.rs @@ -104,15 +104,23 @@ fn consume_back(parts: &mut VecDeque, number: usize) { } } -/// Attempts to parse a date from the front of the parts list, returning the date and consuming the -/// date parts if they are available +/// Attempts to parse a date from the end of the parts list, returning the date and consuming the +/// date parts if they are available. fn extract_date(parts: &mut VecDeque<&str>) -> Option { let len = parts.len(); if len < 3 { return None; } - let date_str = format!("{}-{}-{}", parts[len - 3], parts[len - 2], parts[len - 1]); + let date_str = parts + .iter() + .rev() + .take(3) + .cloned() + .rev() + .collect::>() + .join("-"); + match Date::parse(&date_str, DATE_FORMAT) { Ok(d) => { consume_back(parts, 3); @@ -122,41 +130,33 @@ fn extract_date(parts: &mut VecDeque<&str>) -> Option { } } -/// Attemps to parse the target from a vector of parts, returning the target and consuming the -/// target parts if they are available +/// Attempts to parse the target from the end of the parts list, returning the target and consuming the +/// target parts if they are available. fn extract_target(parts: &mut VecDeque<&str>) -> Option { - if parts.len() < 3 { - return None; - } - - let len = parts.len(); - let target_str = format!("{}-{}-{}", parts[len - 3], parts[len - 2], parts[len - 1]); - match TargetTriple::new(&target_str) { - Ok(t) => { - consume_back(parts, 3); - Some(t) + fn try_extract(parts: &mut VecDeque<&str>, count: usize) -> Option { + if parts.len() < count { + return None; } - Err(_) => { - if parts.len() < 4 { - return None; - } - let target_str = format!( - "{}-{}-{}-{}", - parts[len - 4], - parts[len - 3], - parts[len - 2], - parts[len - 1] - ); - match TargetTriple::new(&target_str) { - Ok(t) => { - consume_back(parts, 4); - Some(t) - } - Err(_) => None, + let target_str: String = parts + .iter() + .rev() + .take(count) + .cloned() + .rev() + .collect::>() + .join("-"); + + match TargetTriple::new(&target_str) { + Ok(t) => { + consume_back(parts, count); + Some(t) } + Err(_) => None, } } + + try_extract(parts, 3).or_else(|| try_extract(parts, 4)) } /// Parses a distributable toolchain description from a string. @@ -177,12 +177,16 @@ impl FromStr for DistToolchainDescription { } let mut parts = s.split('-').collect::>(); + match parts.len() { - 1 => Ok(Self { - name: DistToolchainName::from_str(parts[0])?, - target: TargetTriple::from_host().ok(), - date: None, - }), + 1 => { + let first_part = *parts.front().unwrap_or(&""); + Ok(Self { + name: DistToolchainName::from_str(first_part)?, + target: TargetTriple::from_host().ok(), + date: None, + }) + } _ => { let date = extract_date(&mut parts); let target = extract_target(&mut parts); @@ -420,11 +424,13 @@ impl Toolchain { Ok(()) } - fn remove_executables(&self, component: &str) -> Result<()> { - let executables = &Components::collect().unwrap().component[component].executables; - for executable in executables { - remove_file(self.bin_path.join(executable)) - .with_context(|| format!("failed to remove executable '{executable}'"))?; + fn remove_executables(&self, component_name: &str) -> Result<()> { + let components = Components::collect()?; + if let Some(component) = components.component.get(component_name) { + for executable in &component.executables { + remove_file(self.bin_path.join(executable)) + .with_context(|| format!("failed to remove executable '{executable}'"))?; + } } Ok(()) } @@ -673,4 +679,78 @@ mod tests { DistToolchainDescription::from_str(channel).expect_err("invalid channel"); } } + + #[test] + fn test_extract_target_with_three_parts() { + let mut parts: VecDeque<&str> = VecDeque::from(vec!["aarch64", "apple", "darwin"]); + let target = extract_target(&mut parts).expect("target triple"); + assert_eq!(target.to_string(), "aarch64-apple-darwin"); + assert!(parts.is_empty()); // Ensure parts are consumed + } + + #[test] + fn test_extract_target_with_four_parts() { + let mut parts: VecDeque<&str> = VecDeque::from(vec!["x86_64", "unknown", "linux", "gnu"]); + let target = extract_target(&mut parts).expect("target triple"); + assert_eq!(target.to_string(), "x86_64-unknown-linux-gnu"); + assert!(parts.is_empty()); // Ensure parts are consumed + } + + #[test] + fn test_extract_target_with_five_parts() { + let mut parts: VecDeque<&str> = + VecDeque::from(vec!["my", "custom", "aarch64", "apple", "darwin"]); + let target = extract_target(&mut parts).expect("target triple"); + assert_eq!(target.to_string(), "aarch64-apple-darwin"); + assert_eq!(parts.len(), 2); // Ensure 3 parts were consumed + } + + #[test] + fn test_extract_target_with_insufficient_parts() { + let mut parts: VecDeque<&str> = VecDeque::from(vec!["apple", "darwin"]); + assert!(extract_target(&mut parts).is_none()); + assert_eq!(parts.len(), 2); // Ensure parts are not consumed + } + + #[test] + fn test_extract_target_with_invalid_target() { + let mut parts: VecDeque<&str> = VecDeque::from(vec!["invalid", "target", "string"]); + assert!(extract_target(&mut parts).is_none()); + assert_eq!(parts.len(), 3); // Ensure parts are not consumed + + let mut parts: VecDeque<&str> = + VecDeque::from(vec!["still", "invalid", "target", "string"]); + assert!(extract_target(&mut parts).is_none()); + assert_eq!(parts.len(), 4); // Ensure parts are not consumed + } + + #[test] + fn test_extract_date_with_valid_date() { + let mut parts: VecDeque<&str> = VecDeque::from(vec!["2022", "12", "25"]); + let date = extract_date(&mut parts).expect("date"); + assert_eq!(date.to_string(), "2022-12-25"); + assert!(parts.is_empty()); // Ensure all parts are consumed + } + + #[test] + fn test_extract_date_with_insufficient_parts() { + let mut parts: VecDeque<&str> = VecDeque::from(vec!["2022", "12"]); + assert!(extract_date(&mut parts).is_none()); + assert_eq!(parts.len(), 2); // Ensure parts are not consumed + } + + #[test] + fn test_extract_date_with_invalid_date() { + let mut parts: VecDeque<&str> = VecDeque::from(vec!["12", "25", "2022"]); + assert!(extract_date(&mut parts).is_none()); + assert_eq!(parts.len(), 3); // Ensure parts are not consumed + } + + #[test] + fn test_extract_date_with_extra_parts() { + let mut parts: VecDeque<&str> = VecDeque::from(vec!["extra", "2022", "12", "25"]); + let date = extract_date(&mut parts).expect("date"); + assert_eq!(date.to_string(), "2022-12-25"); + assert_eq!(parts.len(), 1); // Ensure only the date parts are consumed + } } diff --git a/src/toolchain_override.rs b/src/toolchain_override.rs index 669453725..2cae3c7c9 100644 --- a/src/toolchain_override.rs +++ b/src/toolchain_override.rs @@ -118,6 +118,7 @@ impl ToolchainOverride { Ok(Self { cfg, path }) } + #[allow(clippy::indexing_slicing)] pub fn to_toml(&self) -> Document { let mut document = toml_edit::Document::new(); From 645d8eb9ad5edd1710bc00683a5f2c27b426358a Mon Sep 17 00:00:00 2001 From: Sophie Date: Wed, 19 Jun 2024 13:15:17 -0700 Subject: [PATCH 2/2] Allow index slicing in tests --- src/lib.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/lib.rs b/src/lib.rs index a00756d8c..e581350a0 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -1,3 +1,5 @@ +#![cfg_attr(test, allow(clippy::indexing_slicing))] + pub mod channel; pub mod commands; pub mod config;