chore: Demonstrate use of hashmap with new result objects for analysi…

…s storage (#130)

* refactor: demonstrate use of hashmap with new result objects for analysis result storage

* fix: comment code

* fix: address clippy warnings

* refactor: remove Box wrapper in HCStoredResult and make AnalysisResults use HCStoredResults

* fix: clippy errors

* refactor: prefer predicate enum over dyn trait

hipcheck/src/analysis/analysis.rs

@@ -180,21 +180,16 @@ impl Display for AnalysisOutcome {
 }
 
 pub fn activity_analysis(db: &dyn AnalysisProvider) -> Arc<HCAnalysisReport> {
-	let results = db.activity_metric();
-	match results {
-		Err(err) => Arc::new(HCAnalysisReport {
-			outcome: HCAnalysisOutcome::Error(HCAnalysisError::Generic(err)),
-			concerns: vec![],
-		}),
-		Ok(results) => {
-			let value = results.time_since_last_commit.num_weeks() as u64;
-			let hc_value = HCBasicValue::from(value);
-			Arc::new(HCAnalysisReport {
-				outcome: HCAnalysisOutcome::Completed(HCAnalysisValue::Basic(hc_value)),
-				concerns: vec![],
-			})
-		}
-	}
+	let results = match db.activity_metric() {
+		Err(err) => return Arc::new(HCAnalysisReport::generic_error(err, vec![])),
+		Ok(results) => results,
+	};
+	let value = results.time_since_last_commit.num_weeks() as u64;
+	let hc_value = HCBasicValue::from(value);
+	Arc::new(HCAnalysisReport {
+		outcome: HCAnalysisOutcome::Completed(HCAnalysisValue::Basic(hc_value)),
+		concerns: vec![],
+	})
 }
 
 pub fn affiliation_analysis(db: &dyn AnalysisProvider) -> Result<Arc<AnalysisReport>> {

hipcheck/src/analysis/report_builder.rs

@@ -1,7 +1,7 @@
 // SPDX-License-Identifier: Apache-2.0
 
 use crate::analysis::analysis::AnalysisReport;
-use crate::analysis::result::HCBasicValue;
+use crate::analysis::result::{HCBasicValue, Predicate};
 use crate::analysis::score::ScoringResults;
 use crate::config::RiskConfigQuery;
 use crate::error::Error;
@@ -28,20 +28,22 @@ pub fn build_report(session: &Session, scoring: &ScoringResults) -> Result<Repor
 
 	// Activity analysis.
 
-	match &scoring.results.activity {
-		Some((Ok(pred), concerns)) => {
-			let HCBasicValue::Unsigned(value) = pred.value else {
-				return Err(hc_error!("activity analysis has a non-u64 value"));
-			};
-			let HCBasicValue::Unsigned(thresh) = pred.threshold else {
-				return Err(hc_error!("activity analysis has a non-u64 value"));
-			};
-			builder.add_analysis(Analysis::activity(value, thresh), concerns.clone())?;
-		}
-		Some((Err(error), _concerns)) => {
-			builder.add_errored_analysis(AnalysisIdent::Activity, error);
+	if let Some(stored) = &scoring.results.activity {
+		match &stored.result {
+			Ok(analysis) => {
+				let Predicate::Threshold(pred) = analysis.as_ref();
+				let HCBasicValue::Unsigned(value) = pred.value else {
+					return Err(hc_error!("activity analysis has a non-u64 value"));
+				};
+				let HCBasicValue::Unsigned(thresh) = pred.threshold else {
+					return Err(hc_error!("activity analysis has a non-u64 value"));
+				};
+				builder.add_analysis(Analysis::activity(value, thresh), stored.concerns.clone())?;
+			}
+			Err(error) => {
+				builder.add_errored_analysis(AnalysisIdent::Activity, error);
+			}
 		}
-		None => (),
 	};
 
 	// Affiliation analysis.

hipcheck/src/analysis/result.rs

@@ -1,11 +1,13 @@
 // SPDX-License-Identifier: Apache-2.0
 
+use crate::analysis::score::HCStoredResult;
 use crate::hc_error;
 use crate::report::Concern;
 use crate::Result;
 use crate::F64;
 use std::cmp::Ordering;
 use std::fmt::{self, Display};
+use std::sync::Arc;
 
 /// Represents the enhanced result of a hipcheck analysis. Contains the actual outcome
 /// of the analysis, plus additional meta-information the analysis wants to provide to
@@ -15,6 +17,14 @@ pub struct HCAnalysisReport {
 	pub outcome: HCAnalysisOutcome,
 	pub concerns: Vec<Concern>,
 }
+impl HCAnalysisReport {
+	pub fn generic_error(error: crate::error::Error, concerns: Vec<Concern>) -> Self {
+		HCAnalysisReport {
+			outcome: HCAnalysisOutcome::Error(HCAnalysisError::Generic(error)),
+			concerns,
+		}
+	}
+}
 
 /// Represents the result of a hipcheck analysis. Either the analysis encountered
 /// an error, or it completed and returned a value.
@@ -102,7 +112,7 @@ pub enum HCCompositeValue {
 }
 
 /// The set of possible predicates for deciding if a source passed an analysis.
-pub trait HCPredicate: Display + Clone + Eq + PartialEq {
+pub trait HCPredicate: Display + std::fmt::Debug + std::any::Any + 'static {
 	fn pass(&self) -> Result<bool>;
 }
 
@@ -131,21 +141,43 @@ impl ThresholdPredicate {
 	}
 }
 
-fn pass_threshold<T: Ord>(a: &T, b: &T, ord: &Ordering) -> bool {
-	a.cmp(b) == *ord
+fn pass_threshold<T: Ord>(a: &T, b: &T, ord: Ordering) -> bool {
+	a.cmp(b) == ord
 }
 
+impl ThresholdPredicate {
+	pub fn from_analysis(
+		report: &HCAnalysisReport,
+		threshold: HCBasicValue,
+		units: Option<String>,
+		order: Ordering,
+	) -> HCStoredResult {
+		let result = match &report.outcome {
+			HCAnalysisOutcome::Error(err) => Err(hc_error!("{:?}", err)),
+			HCAnalysisOutcome::Completed(HCAnalysisValue::Basic(av)) => {
+				Ok(ThresholdPredicate::new(av.clone(), threshold, units, order))
+			}
+			HCAnalysisOutcome::Completed(HCAnalysisValue::Composite(_)) => Err(hc_error!(
+				"activity analysis should return a basic u64 type, not {:?}"
+			)),
+		};
+		HCStoredResult {
+			result: result.map(|r| Arc::new(Predicate::Threshold(r))),
+			concerns: report.concerns.clone(),
+		}
+	}
+}
 impl HCPredicate for ThresholdPredicate {
 	// @FollowUp - would be nice for this match logic to error at compile time if a new
 	//  HCBasicValue type is added, so developer is reminded to add new variant here
 	fn pass(&self) -> Result<bool> {
 		use HCBasicValue::*;
 		match (&self.value, &self.threshold) {
-			(Integer(a), Integer(b)) => Ok(pass_threshold(a, b, &self.ordering)),
-			(Unsigned(a), Unsigned(b)) => Ok(pass_threshold(a, b, &self.ordering)),
-			(Float(a), Float(b)) => Ok(pass_threshold(a, b, &self.ordering)),
-			(Bool(a), Bool(b)) => Ok(pass_threshold(a, b, &self.ordering)),
-			(String(a), String(b)) => Ok(pass_threshold(a, b, &self.ordering)),
+			(Integer(a), Integer(b)) => Ok(pass_threshold(a, b, self.ordering)),
+			(Unsigned(a), Unsigned(b)) => Ok(pass_threshold(a, b, self.ordering)),
+			(Float(a), Float(b)) => Ok(pass_threshold(a, b, self.ordering)),
+			(Bool(a), Bool(b)) => Ok(pass_threshold(a, b, self.ordering)),
+			(String(a), String(b)) => Ok(pass_threshold(a, b, self.ordering)),
 			(a, b) => Err(hc_error!(
 				"threshold and value are of different types: {:?}, {:?}",
 				a,
@@ -168,3 +200,25 @@ impl Display for ThresholdPredicate {
 		write!(f, "{} {} {}", val.trim(), order_str, thr.trim())
 	}
 }
+
+#[derive(Debug)]
+pub enum Predicate {
+	Threshold(ThresholdPredicate),
+}
+
+impl Display for Predicate {
+	fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+		use Predicate::*;
+		match self {
+			Threshold(t) => t.fmt(f),
+		}
+	}
+}
+impl HCPredicate for Predicate {
+	fn pass(&self) -> Result<bool> {
+		use Predicate::*;
+		match self {
+			Threshold(t) => t.pass(),
+		}
+	}
+}

hipcheck/src/analysis/score.rs

@@ -9,6 +9,7 @@ use crate::hc_error;
 use crate::report::Concern;
 use crate::shell::Phase;
 use std::cmp::Ordering;
+use std::collections::HashMap;
 use std::default::Default;
 use std::sync::Arc;
 
@@ -42,10 +43,63 @@ pub struct ScoringResults {
 	pub score: Score,
 }
 
+#[derive(Debug, Clone)]
+pub struct HCStoredResult {
+	pub result: Result<Arc<Predicate>>,
+	pub concerns: Vec<Concern>,
+}
+impl HCStoredResult {
+	// Score the analysis by invoking predicate's impl of `pass()`. Errored
+	// analyses treated as failures.
+	// @FollowUp - remove AnalysisOutcome once scoring refactor complete
+	pub fn score(&self) -> (u64, AnalysisOutcome) {
+		match &self.result {
+			Err(e) => (1, AnalysisOutcome::Error(e.clone())),
+			Ok(pred) => {
+				let passed = match pred.pass() {
+					Err(err) => {
+						return (1, AnalysisOutcome::Error(err));
+					}
+					Ok(p) => p,
+				};
+				let msg = pred.to_string();
+				let outcome = if passed {
+					AnalysisOutcome::Pass(msg)
+				} else {
+					AnalysisOutcome::Fail(msg)
+				};
+				let score = (!passed) as u64;
+				(score, outcome)
+			}
+		}
+	}
+}
+#[derive(Debug, Default)]
+pub struct AltAnalysisResults {
+	pub table: HashMap<String, HCStoredResult>,
+}
+impl AltAnalysisResults {
+	pub fn add(
+		&mut self,
+		key: &str,
+		result: Result<Arc<Predicate>>,
+		concerns: Vec<Concern>,
+	) -> Result<()> {
+		if self.table.contains_key(key) {
+			return Err(hc_error!(
+				"analysis results table already contains key '{key}'"
+			));
+		}
+		let result_struct = HCStoredResult { result, concerns };
+		self.table.insert(key.to_owned(), result_struct);
+		Ok(())
+	}
+}
+
 #[allow(dead_code)]
 #[derive(Debug, Default)]
 pub struct AnalysisResults {
-	pub activity: Option<(Result<Arc<ThresholdPredicate>>, Vec<Concern>)>,
+	pub activity: Option<HCStoredResult>,
 	pub affiliation: Option<Result<Arc<AnalysisReport>>>,
 	pub binary: Option<Result<Arc<AnalysisReport>>>,
 	pub churn: Option<Result<Arc<AnalysisReport>>>,
@@ -540,6 +594,7 @@ pub fn score_results(phase: &mut Phase, db: &dyn ScoringProvider) -> Result<Scor
 	// Values set with -1.0 are reseved for parent nodes whose score comes always from children nodes with a score set by hc_analysis algorithms
 
 	let mut results = AnalysisResults::default();
+	let mut alt_results = AltAnalysisResults::default();
 	let mut score = Score::default();
 	let mut score_tree = ScoreTree { tree: Graph::new() };
 	let root_node = score_tree.tree.add_node(ScoreTreeNode {
@@ -571,47 +626,28 @@ pub fn score_results(phase: &mut Phase, db: &dyn ScoringProvider) -> Result<Scor
 		// Check if this analysis was skipped or generated an error, then format the results accordingly for reporting.
 		if db.activity_active() {
 			let raw_activity = db.activity_analysis();
-			let activity_res = match &raw_activity.as_ref().outcome {
-				HCAnalysisOutcome::Error(err) => Err(hc_error!("{:?}", err)),
-				HCAnalysisOutcome::Completed(HCAnalysisValue::Basic(av)) => {
-					let raw_threshold: u64 = db.activity_week_count_threshold();
-					let threshold = HCBasicValue::from(raw_threshold);
-					let predicate = ThresholdPredicate::new(
-						av.clone(),
-						threshold,
-						Some("weeks inactivity".to_owned()),
-						Ordering::Less,
-					);
-					Ok(Arc::new(predicate))
-				}
-				HCAnalysisOutcome::Completed(HCAnalysisValue::Composite(_)) => Err(hc_error!(
-					"activity analysis should return a basic u64 type, not {:?}"
-				)),
-			};
+			let raw_threshold: u64 = db.activity_week_count_threshold();
+
+			// Process analysis value into a threshold predicate and add to new & old storage
+			// objects
+			let activity_result = ThresholdPredicate::from_analysis(
+				&raw_activity,
+				HCBasicValue::from(raw_threshold),
+				Some("weeks inactivity".to_owned()),
+				Ordering::Less,
+			);
+			results.activity = Some(activity_result.clone());
+			alt_results
+				.table
+				.insert(ACTIVITY_PHASE.to_owned(), activity_result);
+
 			// Scoring based off of predicate
-			let score_result = Arc::new(match activity_res.as_ref() {
-				Err(e) => ScoreResult {
-					count: db.activity_weight(),
-					score: 1,
-					outcome: AnalysisOutcome::Error(e.clone()),
-				},
-				Ok(pred) => {
-					// Derive score from predicate, true --> 0, false --> 1
-					let passed = pred.pass()?;
-					let msg = pred.to_string();
-					let outcome = if passed {
-						AnalysisOutcome::Pass(msg)
-					} else {
-						AnalysisOutcome::Fail(msg)
-					};
-					ScoreResult {
-						count: db.activity_weight(),
-						score: (!passed) as u64,
-						outcome,
-					}
-				}
+			let (act_score, outcome) = alt_results.table.get(ACTIVITY_PHASE).unwrap().score();
+			let score_result = Arc::new(ScoreResult {
+				count: db.activity_weight(),
+				score: act_score,
+				outcome,
 			});
-			results.activity = Some((activity_res, raw_activity.concerns.clone()));
 			score.activity = score_result.outcome.clone();
 			match add_node_and_edge_with_score(
 				score_result,