diff --git a/src/cargo.rs b/src/cargo.rs deleted file mode 100644 index 0986fc7..0000000 --- a/src/cargo.rs +++ /dev/null @@ -1,88 +0,0 @@ -use serde::Deserialize; - -#[derive(Deserialize, PartialEq, Debug, Clone)] -pub struct Parser { - members: Vec, -} - -impl Parser { - pub fn from_manifest_path(p: impl AsRef) -> Result { - Ok(Self::from_manifest(cargo_toml::Manifest::from_path(p)?)) - } - pub fn is_workspace(&self) -> bool { - !self.members.is_empty() - } - - fn new() -> Self { - Self { members: vec![] } - } - - fn from_manifest(m: cargo_toml::Manifest) -> Self { - if let Some(w) = m.workspace { - Self { members: w.members } - } else { - Self::new() - } - } -} - -#[cfg(test)] -mod tests { - #[test] - fn test_not_workspace_manifest() { - use crate::cargo::Parser; - let no_members: Vec = Vec::new(); - let manifest = cargo_toml::Manifest::from_path("Cargo.toml").unwrap(); - // Make sure we actually parsed the manifest - assert_eq!("cargo-scout", manifest.clone().package.unwrap().name); - let parser = Parser::from_manifest(manifest); - assert!(!parser.is_workspace()); - assert_eq!(no_members, parser.members); - } - #[test] - fn test_not_workspace_path() { - use crate::cargo::Parser; - let no_members: Vec = Vec::new(); - let parser = Parser::from_manifest_path("Cargo.toml").unwrap(); - assert!(!parser.is_workspace()); - assert_eq!(no_members, parser.members); - } - #[test] - fn test_neqo_members_manifest() { - use crate::cargo::Parser; - let neqo_toml = r#"[workspace] - members = [ - "neqo-client", - "neqo-common", - "neqo-crypto", - "neqo-http3", - "neqo-http3-server", - "neqo-qpack", - "neqo-server", - "neqo-transport", - "neqo-interop", - "test-fixture", - ]"#; - - let manifest = cargo_toml::Manifest::from_slice(neqo_toml.as_bytes()).unwrap(); - - let parser = Parser::from_manifest(manifest); - - assert!(parser.is_workspace()); - assert_eq!( - vec![ - "neqo-client", - "neqo-common", - "neqo-crypto", - "neqo-http3", - "neqo-http3-server", - "neqo-qpack", - "neqo-server", - "neqo-transport", - "neqo-interop", - "test-fixture" - ], - parser.members - ); - } -} diff --git a/src/error.rs b/src/error.rs index 5aa7475..f6c6c0d 100644 --- a/src/error.rs +++ b/src/error.rs @@ -1,5 +1,6 @@ #[derive(Debug)] pub enum Error { + ScoutBuilder, CargoToml(cargo_toml::Error), Command(String), Utf8(std::string::FromUtf8Error), diff --git a/src/clippy.rs b/src/linter/clippy.rs similarity index 81% rename from src/clippy.rs rename to src/linter/clippy.rs index 71b1a4c..1911926 100644 --- a/src/clippy.rs +++ b/src/linter/clippy.rs @@ -1,56 +1,22 @@ -use serde::Deserialize; +use crate::linter::{Lint, Linter}; use std::io::{self, Write}; +use std::path::PathBuf; use std::process::Command; -#[derive(Deserialize, PartialEq, Debug)] -pub struct LintCode { - pub code: String, - pub explanation: String, -} - -#[derive(Deserialize, PartialEq, Debug)] -pub struct LintSpan { - pub file_name: String, - /// The line where the lint should be reported - /// - /// GitHub provides a line_start and a line_end. - /// We should use the line_start in case of multi-line lints. - /// (Why?) - pub line_start: usize, -} - -#[derive(Deserialize, PartialEq, Debug, Clone)] -pub struct Lint { - /// The lint message - /// - /// Example: - /// - /// unused variable: `count` - pub package_id: String, - pub src_path: Option, - pub message: Option, -} - -#[derive(Deserialize, PartialEq, Debug, Clone)] -pub struct Message { - pub rendered: String, - pub spans: Vec, -} - -#[derive(Deserialize, PartialEq, Debug, Clone)] -pub struct Span { - pub file_name: String, - pub line_start: u32, - pub line_end: u32, -} - -pub struct Linter { +pub struct Clippy { verbose: bool, no_default_features: bool, all_features: bool, } -impl Linter { +impl Linter for Clippy { + fn get_lints(&self, working_dir: PathBuf) -> Result, crate::error::Error> { + self.clippy(working_dir) + .map(|clippy_output| lints(clippy_output.as_ref())) + } +} + +impl Clippy { pub fn new() -> Self { Self { verbose: false, @@ -74,10 +40,6 @@ impl Linter { self } - pub fn get_lints(&self) -> Result, crate::error::Error> { - self.clippy().map(|output| lints(&output)) - } - fn get_command_parameters(&self) -> Vec<&str> { let mut params = vec!["clippy", "--message-format", "json"]; if self.verbose { @@ -101,8 +63,9 @@ impl Linter { envs } - fn clippy(&self) -> Result { + fn clippy(&self, path: impl AsRef) -> Result { let clippy_pedantic_output = Command::new("cargo") + .current_dir(path) .args(self.get_command_parameters()) .envs(self.get_envs()) .output() @@ -157,11 +120,10 @@ pub fn lints(clippy_output: &str) -> Vec { #[cfg(test)] mod tests { + use super::*; #[test] fn test_set_verbose() { - use crate::clippy::Linter; - - let mut linter = Linter::new(); + let mut linter = Clippy::new(); assert_eq!(false, linter.verbose); let l2 = linter.set_verbose(true); @@ -172,9 +134,7 @@ mod tests { } #[test] fn test_get_envs() { - use crate::clippy::Linter; - - let mut linter = Linter::new(); + let mut linter = Clippy::new(); let mut expected_envs = vec![]; assert_eq!(expected_envs, linter.get_envs()); @@ -184,9 +144,7 @@ mod tests { } #[test] fn test_get_command_parameters() { - use crate::clippy::Linter; - - let mut linter = Linter::new(); + let mut linter = Clippy::new(); let expected_command_parameters = vec![ "clippy", "--message-format", @@ -248,7 +206,7 @@ mod tests { } #[test] fn test_lints() { - use crate::clippy::{lints, Lint, Message, Span}; + use crate::linter::{Message, Span}; let expected_lints = vec![Lint { package_id: "cargo-scout".to_string(), src_path: Some("test/foo/bar.rs".to_string()), diff --git a/src/linter/mod.rs b/src/linter/mod.rs new file mode 100644 index 0000000..0188661 --- /dev/null +++ b/src/linter/mod.rs @@ -0,0 +1,50 @@ +use serde::Deserialize; +use std::path::PathBuf; + +pub mod clippy; + +pub trait Linter { + fn get_lints(&self, working_dir: PathBuf) -> Result, crate::error::Error>; +} + +#[derive(Deserialize, PartialEq, Debug)] +pub struct LintCode { + pub code: String, + pub explanation: String, +} + +#[derive(Deserialize, PartialEq, Debug)] +pub struct LintSpan { + pub file_name: String, + /// The line where the lint should be reported + /// + /// GitHub provides a line_start and a line_end. + /// We should use the line_start in case of multi-line lints. + /// (Why?) + pub line_start: usize, +} + +#[derive(Deserialize, PartialEq, Debug, Clone)] +pub struct Lint { + /// The lint message + /// + /// Example: + /// + /// unused variable: `count` + pub package_id: String, + pub src_path: Option, + pub message: Option, +} + +#[derive(Deserialize, PartialEq, Debug, Clone)] +pub struct Message { + pub rendered: String, + pub spans: Vec, +} + +#[derive(Deserialize, PartialEq, Debug, Clone)] +pub struct Span { + pub file_name: String, + pub line_start: u32, + pub line_end: u32, +} diff --git a/src/main.rs b/src/main.rs index b2601c8..c14b20e 100644 --- a/src/main.rs +++ b/src/main.rs @@ -1,11 +1,13 @@ +use crate::linter::clippy::Clippy; +use crate::linter::Lint; use std::env; use structopt::StructOpt; -mod cargo; -mod clippy; mod error; mod git; -mod intersections; +mod linter; +mod project; +mod scout; #[derive(Debug, StructOpt)] #[structopt( @@ -40,7 +42,7 @@ struct Options { without_error: bool, } -fn display_warnings(warnings: &[clippy::Lint]) { +fn display_warnings(warnings: &[Lint]) { for w in warnings { if let Some(m) = &w.message { for l in m.rendered.split('\n') { @@ -56,40 +58,46 @@ fn main() -> Result<(), error::Error> { println!("Getting diff against target {}", opts.branch); let diff_sections = git::get_sections(env::current_dir()?, &opts.branch)?; + println!("Checking Cargo manifest"); - let manifest = cargo::Parser::from_manifest_path(opts.cargo_toml)?; - if manifest.is_workspace() { - println!("Running in workspace, please note feature flags are not supported yet."); - } - println!("Running clippy"); - let clippy_lints = clippy::Linter::new() + let mut project = project::cargo::Project::from_manifest_path(opts.cargo_toml)?; + project.set_all_features(opts.all_features); + project.set_no_default_features(opts.no_default_features); + + let mut clippy_linter = Clippy::new(); + clippy_linter .set_verbose(opts.verbose) .set_no_default_features(opts.no_default_features) - .set_all_features(opts.all_features) - .get_lints()?; + .set_all_features(opts.all_features); - let warnings_caused_by_diff = - intersections::get_lints_from_diff(&clippy_lints, &diff_sections, opts.verbose); + let mut scout_builder = scout::Builder::new(); + scout_builder + .set_project_config(Box::new(project)) + .set_linter(Box::new(clippy_linter)); + println!("Running clippy"); + let scout = scout_builder.build()?; - return_warnings(&warnings_caused_by_diff, opts.without_error) + let relevant_lints = scout.run_for_diff(&diff_sections)?; + return_warnings(&relevant_lints, opts.without_error) } -fn return_warnings(lints: &[clippy::Lint], without_error: bool) -> Result<(), error::Error> { +fn return_warnings(lints: &[linter::Lint], without_error: bool) -> Result<(), error::Error> { if lints.is_empty() { println!("No warnings raised by clippy::pedantic in your diff, you're good to go!"); Ok(()) } else { display_warnings(&lints); if without_error { - return Ok(()); + Ok(()) + } else { + Err(error::Error::NotClean) } - Err(error::Error::NotClean) } } #[cfg(test)] mod tests { - use crate::clippy::Lint; + use crate::linter::Lint; use crate::return_warnings; #[test] fn test_return_status_with_lints() { diff --git a/src/project/cargo.rs b/src/project/cargo.rs new file mode 100644 index 0000000..0f7587e --- /dev/null +++ b/src/project/cargo.rs @@ -0,0 +1,140 @@ +use crate::project::Config; +use serde::Deserialize; + +#[derive(Deserialize, PartialEq, Debug, Clone)] +pub struct Project { + no_default_features: bool, + all_features: bool, + members: Vec, +} + +impl Config for Project { + fn linter_must_iterate(&self) -> bool { + !self.members.is_empty() && (self.no_default_features || self.all_features) + } + fn get_members(&self) -> Vec { + self.members.clone() + } +} + +impl Project { + pub fn from_manifest_path(p: impl AsRef) -> Result { + Ok(Self::from_manifest(cargo_toml::Manifest::from_path(p)?)) + } + + pub fn set_no_default_features(&mut self, no_default_features: bool) -> &mut Self { + self.no_default_features = no_default_features; + self + } + + pub fn set_all_features(&mut self, all_features: bool) -> &mut Self { + self.all_features = all_features; + self + } + + fn from_manifest(m: cargo_toml::Manifest) -> Self { + if let Some(w) = m.workspace { + Self { + members: w.members, + no_default_features: false, + all_features: false, + } + } else { + Self { + members: Vec::new(), + no_default_features: false, + all_features: false, + } + } + } +} + +#[cfg(test)] +mod tests { + #[test] + fn test_not_workspace_manifest() { + use crate::project::cargo::Project; + use crate::project::Config; + let no_members: Vec = Vec::new(); + let manifest = cargo_toml::Manifest::from_path("Cargo.toml").unwrap(); + // Make sure we actually parsed the manifest + assert_eq!("cargo-scout", manifest.clone().package.unwrap().name); + let mut project = Project::from_manifest(manifest); + assert!(!project.linter_must_iterate()); + assert_eq!(no_members, project.get_members()); + + // Project must not iterate if not running in a workspace, + // regardless of the passed flags + project.set_all_features(true); + assert!(!project.linter_must_iterate()); + project.set_no_default_features(true); + assert!(!project.linter_must_iterate()); + project.set_all_features(false); + assert!(!project.linter_must_iterate()); + } + #[test] + fn test_not_workspace_path() { + use crate::project::cargo::Project; + use crate::project::Config; + let no_members: Vec = Vec::new(); + let mut project = Project::from_manifest_path("Cargo.toml").unwrap(); + assert!(!project.linter_must_iterate()); + assert_eq!(no_members, project.get_members()); + + // Project must not iterate if not running in a workspace, + // regardless of the passed flags + project.set_all_features(true); + assert!(!project.linter_must_iterate()); + project.set_no_default_features(true); + assert!(!project.linter_must_iterate()); + project.set_all_features(false); + assert!(!project.linter_must_iterate()); + } + #[test] + fn test_neqo_members_manifest() { + use crate::project::cargo::Project; + use crate::project::Config; + let neqo_toml = r#"[workspace] + members = [ + "neqo-client", + "neqo-common", + "neqo-crypto", + "neqo-http3", + "neqo-http3-server", + "neqo-qpack", + "neqo-server", + "neqo-transport", + "neqo-interop", + "test-fixture", + ]"#; + + let manifest = cargo_toml::Manifest::from_slice(neqo_toml.as_bytes()).unwrap(); + + let mut project = Project::from_manifest(manifest); + + assert!(!project.linter_must_iterate()); + assert_eq!( + vec![ + "neqo-client", + "neqo-common", + "neqo-crypto", + "neqo-http3", + "neqo-http3-server", + "neqo-qpack", + "neqo-server", + "neqo-transport", + "neqo-interop", + "test-fixture" + ], + project.get_members() + ); + // Project must iterate if running in a workspace + // With all features or no default features is enabled + project.set_all_features(true); + assert!(project.linter_must_iterate()); + project.set_no_default_features(true); + assert!(project.linter_must_iterate()); + project.set_all_features(false); + assert!(project.linter_must_iterate()); + } +} diff --git a/src/project/mod.rs b/src/project/mod.rs new file mode 100644 index 0000000..3e7d5c1 --- /dev/null +++ b/src/project/mod.rs @@ -0,0 +1,6 @@ +pub mod cargo; + +pub trait Config { + fn linter_must_iterate(&self) -> bool; + fn get_members(&self) -> Vec; +} diff --git a/src/intersections.rs b/src/scout/mod.rs similarity index 52% rename from src/intersections.rs rename to src/scout/mod.rs index e5fa115..95c7a08 100644 --- a/src/intersections.rs +++ b/src/scout/mod.rs @@ -1,24 +1,84 @@ -use crate::clippy; -use crate::git; +use crate::git::Section; +use crate::linter::Lint; +use crate::linter::Span; + +pub struct Builder { + project: Option>, + linter: Option>, +} + +pub struct Scout { + project: Box, + linter: Box, +} + +impl Scout { + pub fn run_for_diff( + &self, + diff_sections: &[Section], + ) -> Result, crate::error::Error> { + let current_dir = std::fs::canonicalize(".")?; + let mut lints = Vec::new(); + if self.project.linter_must_iterate() { + let members = self.project.get_members(); + for m in members { + println!( + "Running clippy on workspace member {:?}", + current_dir.join(&m) + ); + lints.extend(self.linter.get_lints(current_dir.join(m))?); + } + } else { + lints.extend(self.linter.get_lints(current_dir)?); + } + + Ok(get_lints_from_diff(&lints, &diff_sections)) + } +} + +impl Builder { + pub fn new() -> Self { + Self { + project: None, + linter: None, + } + } + + pub fn set_project_config(&mut self, project: Box) -> &mut Self { + self.project = Some(project); + self + } + + pub fn set_linter(&mut self, linter: Box) -> &mut Self { + self.linter = Some(linter); + self + } + + pub fn build(self) -> Result { + match (self.project, self.linter) { + (Some(p), Some(l)) => Ok(Scout { + project: p, + linter: l, + }), + _ => Err(crate::error::Error::ScoutBuilder), + } + } +} // Check if clippy_lint and git_section have overlapped lines -fn lines_in_range(clippy_lint: &clippy::Span, git_section: &git::Section) -> bool { +fn lines_in_range(clippy_lint: &Span, git_section: &Section) -> bool { // If git_section.line_start is included in the clippy_lint span clippy_lint.line_start <= git_section.line_start && git_section.line_start <= clippy_lint.line_end || // If clippy_lint.line_start is included in the git_section span git_section.line_start <= clippy_lint.line_start && clippy_lint.line_start <= git_section.line_end } -fn files_match(clippy_lint: &clippy::Span, git_section: &git::Section) -> bool { +fn files_match(clippy_lint: &Span, git_section: &Section) -> bool { // Git diff paths and clippy paths don't get along too well on Windows... clippy_lint.file_name.replace("\\", "/") == git_section.file_name.replace("\\", "/") } -pub fn get_lints_from_diff( - lints: &[clippy::Lint], - diffs: &[git::Section], - _verbose: bool, -) -> Vec { +pub fn get_lints_from_diff(lints: &[Lint], diffs: &[Section]) -> Vec { let mut lints_in_diff = Vec::new(); for diff in diffs { let diff_lints = lints.iter().filter(|lint| { @@ -40,11 +100,105 @@ pub fn get_lints_from_diff( lints_in_diff } +#[cfg(test)] +mod scout_tests { + use crate::linter::{Lint, Linter}; + use crate::project::Config; + use std::cell::RefCell; + use std::path::PathBuf; + use std::rc::Rc; + + struct TestLinter { + // Using a RefCell here because get_lints + // takes &self and not &mut self. + // We use usize here because we will compare it to a Vec::len() + lints_times_called: Rc>, + } + impl TestLinter { + pub fn new() -> Self { + Self { + lints_times_called: Rc::new(RefCell::new(0)), + } + } + } + impl Linter for TestLinter { + fn get_lints(&self, _working_dir: PathBuf) -> Result, crate::error::Error> { + *self.lints_times_called.borrow_mut() += 1; + Ok(Vec::new()) + } + } + + struct TestProject { + members: Vec, + } + impl TestProject { + pub fn new(members: Vec) -> Self { + TestProject { members } + } + } + impl Config for TestProject { + fn linter_must_iterate(&self) -> bool { + !self.get_members().is_empty() + } + fn get_members(&self) -> Vec { + self.members.clone() + } + } + + #[test] + fn test_scout_no_workspace() -> Result<(), crate::error::Error> { + let diff = vec![]; + use crate::scout::Builder; + let linter = TestLinter::new(); + // No members so we won't have to iterate + let project = TestProject::new(Vec::new()); + let expected_times_called = 1; + let actual_times_called = Rc::clone(&linter.lints_times_called); + + let mut sb = Builder::new(); + sb.set_linter(Box::new(linter)) + .set_project_config(Box::new(project)); + let scout = sb.build()?; + // We don't check for the lints result here. + // It is already tested in the linter tests + // and in intersection tests + let _ = scout.run_for_diff(&diff)?; + assert_eq!(expected_times_called, *actual_times_called.borrow()); + Ok(()) + } + + #[test] + fn test_scout_in_workspace() -> Result<(), crate::error::Error> { + let diff = vec![]; + use crate::scout::Builder; + let linter = TestLinter::new(); + // The project has members, we will iterate + let project = TestProject::new(vec![ + "member1".to_string(), + "member2".to_string(), + "member3".to_string(), + ]); + let expected_times_called = project.get_members().len(); + let actual_times_called = Rc::clone(&linter.lints_times_called); + + let mut sb = Builder::new(); + sb.set_linter(Box::new(linter)) + .set_project_config(Box::new(project)); + let scout = sb.build()?; + // We don't check for the lints result here. + // It is already tested in the linter tests + // and in intersection tests + let _ = scout.run_for_diff(&diff)?; + + assert_eq!(expected_times_called, *actual_times_called.borrow()); + Ok(()) + } +} + #[cfg(test)] mod intersections_tests { - use crate::clippy::Span; use crate::git::Section; - use crate::intersections::files_match; + use crate::linter::Span; type TestSection = (&'static str, u32, u32); #[test] @@ -98,6 +252,7 @@ mod intersections_tests { } fn assert_all_files_match(ranges: Vec<(TestSection, TestSection)>) { + use crate::scout::files_match; for range in ranges { let lint = range.0; let section = range.1; @@ -119,6 +274,7 @@ mod intersections_tests { } fn assert_no_files_match(ranges: Vec<(TestSection, TestSection)>) { + use crate::scout::files_match; for range in ranges { let lint = range.0; let section = range.1; @@ -168,7 +324,7 @@ mod intersections_tests { } fn in_range(lint: (&str, u32, u32), section: (&str, u32, u32)) -> bool { - use crate::intersections::lines_in_range; + use crate::scout::lines_in_range; let clippy_lint = Span { file_name: String::from(lint.0), line_start: lint.1,